[netatmo] Null annotations Part 3 of 3 #8057

lolodomo · 2020-07-03T06:33:27Z

Signed-off-by: Laurent Garnier lg.hc@free.fr

Fix openhab#7913 Signed-off-by: Laurent Garnier <lg.hc@free.fr>

lolodomo · 2020-07-03T06:36:16Z

I tagged this PR with "Work in progress" due to the number of changes and the usage of Optional I am not expert in. This requires a good testing before a merge.
By the way, reviews are already welcome.

TravisBuddy · 2020-07-03T06:36:49Z

Travis tests were successful

Hey @lolodomo,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

lolodomo · 2020-07-03T07:09:48Z

After a quick test, everything looks good. I am confident in my changes. But I will do more tests to be very sure.

lolodomo · 2020-07-04T11:19:40Z

Tag "Work in progress" removed, I found only little bug that I immediately fixed and which was already present before this PR, the video status from the last event must be computed only if the event contains a video.

Signed-off-by: Laurent Garnier <lg.hc@free.fr>

TravisBuddy · 2020-07-04T11:56:34Z

Travis tests were successful

Hey @lolodomo,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

clinique

LGTM

TravisBuddy · 2020-07-07T17:46:54Z

Travis tests were successful

Hey @lolodomo,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

Fix openhab#8083 Signed-off-by: Laurent Garnier <lg.hc@free.fr>

TravisBuddy · 2020-07-07T18:01:08Z

Travis tests were successful

Hey @lolodomo,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

lolodomo · 2020-07-07T18:02:24Z

@clinique : please review my last change, I implemented the configuration option for the background discovery. Works perfectly as expected.

cpmeister · 2020-07-07T19:36:09Z

bundles/org.openhab.binding.netatmo/README.md

- Some features like the video surveillance are accessed via the local network, so it may be helpful to set a static IP address
-for the camera within your local network.
+
+- The URL of the live snapshot is a fixed URL so the value of the channel cameraLivePictureUrl / welcomeCameraLivePictureUrl will never be updated once first set by the binding. So to get a refreshed picture, you need to use the refresh parameter in your sitemap image element.


Please put each sentence on its own line.

This was done to avoid a warning in the checks (blank line required after a list item, something like that).
Either 2 lines to help maintenance but a warning, or one line and no warning.
Let me know what you prefer.

Just leave as is then. I've prefer to avoid the warning even if it might be misguided.

cpmeister · 2020-07-07T19:38:02Z

.../src/main/java/org/openhab/binding/netatmo/internal/handler/AbstractNetatmoThingHandler.java

- NetatmoBridgeHandler handler = getBridgeHandler();
- if (handler == null) {
+ if (!getBridgeHandler().isPresent() || device == null) {


Caching the bridge handler result in a local variable is much safer from a concurrency standpoint.

cpmeister · 2020-07-07T19:44:03Z

...tatmo/src/main/java/org/openhab/binding/netatmo/internal/welcome/NAWelcomePersonHandler.java

- if (bridgeHandler != null && avatarURL == null && module != null) {
- NAWelcomeFace face = module.getFace();
+ private @Nullable String getAvatarURL() {
+ if (getBridgeHandler().isPresent() && avatarURL == null && getModule().isPresent()) {


getBridgeHandler() and getModule() should be cached to local variables

cpmeister · 2020-07-07T19:45:39Z

...tatmo/src/main/java/org/openhab/binding/netatmo/internal/welcome/NAWelcomePersonHandler.java

+ if (getBridgeHandler().isPresent() && getLastEvent().isPresent()
+ && getLastEvent().get().getSnapshot() != null) {


getBridgeHandler() and getLastEvent() should be cached to local variables

cpmeister · 2020-07-07T19:47:04Z

...tatmo/src/main/java/org/openhab/binding/netatmo/internal/welcome/NAWelcomePersonHandler.java

+ return (getLastEvent().isPresent() && getLastEvent().get().getMessage() != null)
+ ? toStringType(getLastEvent().get().getMessage().replace("<b>", "").replace("</b>", ""))


you get the idea, fix wherever else it might apply.

Signed-off-by: Laurent Garnier <lg.hc@free.fr>

TravisBuddy · 2020-07-08T16:28:30Z

Travis tests were successful

Hey @lolodomo,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

lolodomo · 2020-07-08T16:31:10Z

Review comments taken into account.
Only remains the one about README. Let me know what of the 2 options you prefer.

lolodomo · 2020-07-09T22:36:15Z

Thank you for the quick review and merge.

* [netatmo] Null annotations Part 3 of 3 Fix openhab#7913 * Video status only set when there is a video in the event * Add a setting to enable/disable the background discovery Fix openhab#8083 * Cache Optional result in local variable Signed-off-by: Laurent Garnier <lg.hc@free.fr>

* [netatmo] Null annotations Part 3 of 3 Fix openhab#7913 * Video status only set when there is a video in the event * Add a setting to enable/disable the background discovery Fix openhab#8083 * Cache Optional result in local variable Signed-off-by: Laurent Garnier <lg.hc@free.fr> Signed-off-by: CSchlipp <christian@schlipp.de>

* [netatmo] Null annotations Part 3 of 3 Fix openhab#7913 * Video status only set when there is a video in the event * Add a setting to enable/disable the background discovery Fix openhab#8083 * Cache Optional result in local variable Signed-off-by: Laurent Garnier <lg.hc@free.fr> Signed-off-by: MPH80 <michael@hazelden.me>

* [netatmo] Null annotations Part 3 of 3 Fix openhab#7913 * Video status only set when there is a video in the event * Add a setting to enable/disable the background discovery Fix openhab#8083 * Cache Optional result in local variable Signed-off-by: Laurent Garnier <lg.hc@free.fr>

* [netatmo] Null annotations Part 3 of 3 Fix openhab#7913 * Video status only set when there is a video in the event * Add a setting to enable/disable the background discovery Fix openhab#8083 * Cache Optional result in local variable Signed-off-by: Laurent Garnier <lg.hc@free.fr> Signed-off-by: Daan Meijer <daan@studioseptember.nl>

* [netatmo] Null annotations Part 3 of 3 Fix openhab#7913 * Video status only set when there is a video in the event * Add a setting to enable/disable the background discovery Fix openhab#8083 * Cache Optional result in local variable Signed-off-by: Laurent Garnier <lg.hc@free.fr>

[netatmo] Null annotations Part 3 of 3

9c03927

Fix openhab#7913 Signed-off-by: Laurent Garnier <lg.hc@free.fr>

lolodomo added the work in progress A PR that is not yet ready to be merged label Jul 3, 2020

lolodomo requested review from clinique and cweitkamp as code owners July 3, 2020 06:33

lolodomo removed the work in progress A PR that is not yet ready to be merged label Jul 4, 2020

Video status only set when there is a video in the event

7df9497

Signed-off-by: Laurent Garnier <lg.hc@free.fr>

lolodomo force-pushed the netatmo_null3 branch from 7b620e9 to 7df9497 Compare July 4, 2020 11:45

openhab deleted a comment from TravisBuddy Jul 4, 2020

lolodomo mentioned this pull request Jul 4, 2020

[infrastructure] enable spotless #8068

Closed

clinique approved these changes Jul 5, 2020

View reviewed changes

Add a setting to enable/disable the background discovery

e0113c4

Fix openhab#8083 Signed-off-by: Laurent Garnier <lg.hc@free.fr>

lolodomo force-pushed the netatmo_null3 branch from 12918c8 to e0113c4 Compare July 7, 2020 17:57

cpmeister requested changes Jul 7, 2020

View reviewed changes

Cache Optional result in local variable

c34133a

Signed-off-by: Laurent Garnier <lg.hc@free.fr>

cpmeister approved these changes Jul 9, 2020

View reviewed changes

cpmeister merged commit 51dacbd into openhab:2.5.x Jul 9, 2020

cpmeister added this to the 2.5.7 milestone Jul 9, 2020

lolodomo deleted the netatmo_null3 branch July 9, 2020 22:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[netatmo] Null annotations Part 3 of 3 #8057

[netatmo] Null annotations Part 3 of 3 #8057

lolodomo commented Jul 3, 2020

lolodomo commented Jul 3, 2020

TravisBuddy commented Jul 3, 2020

lolodomo commented Jul 3, 2020

lolodomo commented Jul 4, 2020

TravisBuddy commented Jul 4, 2020

clinique left a comment

TravisBuddy commented Jul 7, 2020

TravisBuddy commented Jul 7, 2020

lolodomo commented Jul 7, 2020

cpmeister Jul 7, 2020

lolodomo Jul 8, 2020

cpmeister Jul 9, 2020

cpmeister Jul 7, 2020

cpmeister Jul 7, 2020

cpmeister Jul 7, 2020

cpmeister Jul 7, 2020

lolodomo Jul 8, 2020

TravisBuddy commented Jul 8, 2020

lolodomo commented Jul 8, 2020

lolodomo commented Jul 9, 2020

		if (getBridgeHandler().isPresent() && getLastEvent().isPresent()
		&& getLastEvent().get().getSnapshot() != null) {

		return (getLastEvent().isPresent() && getLastEvent().get().getMessage() != null)
		? toStringType(getLastEvent().get().getMessage().replace("<b>", "").replace("</b>", ""))

[netatmo] Null annotations Part 3 of 3 #8057

[netatmo] Null annotations Part 3 of 3 #8057

Conversation

lolodomo commented Jul 3, 2020

lolodomo commented Jul 3, 2020

TravisBuddy commented Jul 3, 2020

Travis tests were successful

lolodomo commented Jul 3, 2020

lolodomo commented Jul 4, 2020

TravisBuddy commented Jul 4, 2020

Travis tests were successful

clinique left a comment

Choose a reason for hiding this comment

TravisBuddy commented Jul 7, 2020

Travis tests were successful

TravisBuddy commented Jul 7, 2020

Travis tests were successful

lolodomo commented Jul 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TravisBuddy commented Jul 8, 2020

Travis tests were successful

lolodomo commented Jul 8, 2020

lolodomo commented Jul 9, 2020