[Robonect] Use global openHAB timezone for DateTime objects #7903

t2000 · 2020-06-12T08:56:08Z

Use the timezone that has been configured by the user in openHAB settings
for all robonect messages instead of UTC.

Signed-off-by: Stefan Triller [email protected]

Use the timezone that has been configured by the user in openHAB settings for all robonect messages instead of UTC. Fixes openhab#7848 Signed-off-by: Stefan Triller <[email protected]>

TravisBuddy · 2020-06-12T08:59:07Z

Travis tests were successful

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

t2000 · 2020-06-12T09:01:33Z

Jar file for testing (please rename to .jar before placing it in the addons folder).
Also make sure to deactivate the pre-installed (original) binding first!

org.openhab.binding.robonect-2.5.6-SNAPSHOT.zip

Signed-off-by: Stefan Triller <[email protected]>

t2000 · 2020-06-12T14:05:50Z

I have changed to code in a way that the user an configure the timezone from robonect on the Thing and afterwards it will be converted into the time that the user has configured in openHAB.

org.openhab.binding.robonect-2.5.6-SNAPSHOT.zip

TravisBuddy · 2020-06-12T14:06:34Z

Travis tests were successful

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

Signed-off-by: Stefan Triller <[email protected]>

t2000 · 2020-06-12T14:34:38Z

Using the default openHAb timezone now if noting is configured or an error occurs.

org.openhab.binding.robonect-2.5.6-SNAPSHOT.zip

TravisBuddy · 2020-06-12T14:34:58Z

Travis tests were successful

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

me2000 · 2020-06-12T15:17:36Z

Tested it with a fresh OH 2.5.5 install plus this JAR and it works. Times are now corrected inside the binding and set and displayed correctly.

cpmeister · 2020-06-12T22:45:39Z

...ng.robonect/src/main/java/org/openhab/binding/robonect/internal/handler/RobonectHandler.java

+
+        String timeZoneString = robonectConfig.getTimezone();
+        try {
+            timeZone = ZoneId.of(timeZoneString);


This will throw an NPE if timeZoneString is null.

Thanks, good catch!

I have fixed it.

Signed-off-by: Stefan Triller <[email protected]>

TravisBuddy · 2020-06-13T08:54:07Z

Travis tests were successful

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

cpmeister · 2020-06-13T20:30:25Z

...ng.robonect/src/main/java/org/openhab/binding/robonect/internal/handler/RobonectHandler.java

@@ -71,12 +74,16 @@
    private ScheduledFuture<?> pollingJob;

    private HttpClient httpClient;
+    private TimeZoneProvider timeZoneProvider;
+
+    private ZoneId timeZone = ZoneId.of("Europe/Berlin");


I think instead of caching this value in a field, you might consider dynamically creating it every time since its value should fall back to the timeZoneProvider timezone whenever it isn't specified by the config.
The problem I see is that the value returned by timeZoneProvider might change over time if the user changes settings in runtime.
So any value returned by timeZoneProvider should never be cached since it might change at any time, without notification.

My suggestion is to more or less replace uses of timeZone with a new method getTimeZone().

I haven't thought about this. thanks for pointing it out!

I have added a commit to change this.

I thought about this again and noticed that if you change the configuration of the thing, the handler gets restarted anyway, because it doesn't overwrite handleConfigurationUpdate and thus the BaseThingHandler#handleConfigurationUpdate will call dispose() and initialize(), so my previous implementation would also read the changed value within initialize().

I leave it up to you, to include the commit with the change or to drop it during merge.

Edit: I decided to drop my commit because it was difficult to fix the tests (accessing the config ran into an NPE).

I was referring to the return value of timeZoneProvider changing during runtime. A change in the return value wouldn't notify the handler and it wouldn't restart the handler or binding in any way. The binding shouldn't save what timeZoneProvider return into a field.

Hmm, I think we are talking about two different things here:

The timezone of the thing and its fallbacks.

The timezone as it should be presented to the user.

For point 2 I am always using the currently configured time in convertUnitToDateTimeType()

// we provide the time in the format as configured in the openHAB settings ZonedDateTime zdt = adjustedInstant.atZone(timeZoneProvider.getTimeZone());

as its always requested from the timeZoneProvider.

For point 1. the information from the timeZoneProvider acts as a fallback only for the lifetime of the Thinghandler, or until someone sets the correct time on the thing (because then dispose and initialize will be called anyway.

So do you want me to change this, even if its just a fallback and even a warning is printed to the user to make him aware of that he should add the correct zone to his Thing configuration? I am fine with doing so, if you think its better. I just wanted to clarify things and see if I have understood you correctly.

I am fine with doing so, if you think its better. I just wanted to clarify things and see if I have understood you correctly.

Yep you have understood me correctly.

Great :)

i have now changed it in a way that the warning about the not set timezone on the thing will only be printed once on the startup of the handler and if it is not set, I get it from the timezoneProvider at runtime for every call.

t2000 · 2020-06-14T10:09:35Z

Version with getting timezone also if changed on Thing:

org.openhab.binding.robonect-2.5.6-SNAPSHOT.zip

Signed-off-by: Stefan Triller <[email protected]>

TravisBuddy · 2020-06-17T15:54:16Z

Travis tests were successful

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

cpmeister

LGTM

…7903) * [Robonect] Use global openHAB timezone for DateTime objects Use the timezone that has been configured by the user in openHAB settings for all robonect messages instead of UTC. Fixes openhab#7848 Signed-off-by: Stefan Triller <[email protected]>

…7903) * [Robonect] Use global openHAB timezone for DateTime objects Use the timezone that has been configured by the user in openHAB settings for all robonect messages instead of UTC. Fixes openhab#7848 Signed-off-by: Stefan Triller <[email protected]> Signed-off-by: CSchlipp <[email protected]>

…7903) * [Robonect] Use global openHAB timezone for DateTime objects Use the timezone that has been configured by the user in openHAB settings for all robonect messages instead of UTC. Fixes openhab#7848 Signed-off-by: Stefan Triller <[email protected]>

…7903) * [Robonect] Use global openHAB timezone for DateTime objects Use the timezone that has been configured by the user in openHAB settings for all robonect messages instead of UTC. Fixes openhab#7848 Signed-off-by: Stefan Triller <[email protected]> Signed-off-by: Daan Meijer <[email protected]>

…7903) * [Robonect] Use global openHAB timezone for DateTime objects Use the timezone that has been configured by the user in openHAB settings for all robonect messages instead of UTC. Fixes openhab#7848 Signed-off-by: Stefan Triller <[email protected]>

[Robonect] Use global openHAB timezone for DateTime objects

ce778f7

Use the timezone that has been configured by the user in openHAB settings for all robonect messages instead of UTC. Fixes openhab#7848 Signed-off-by: Stefan Triller <[email protected]>

t2000 requested a review from reyem as a code owner June 12, 2020 08:56

[robonect] Provide timezone configuration parameter on thing

3d6be63

Signed-off-by: Stefan Triller <[email protected]>

[robonect] Use openHAB timezone on error

7ced566

Signed-off-by: Stefan Triller <[email protected]>

cpmeister requested changes Jun 12, 2020

View reviewed changes

cpmeister added the enhancement An enhancement or new feature for an existing add-on label Jun 12, 2020

[robonect] Fix NPE

beb09bf

Signed-off-by: Stefan Triller <[email protected]>

cpmeister requested changes Jun 13, 2020

View reviewed changes

t2000 force-pushed the robonectTime branch from 82a950f to beb09bf Compare June 15, 2020 16:03

[robonect] Always get timezone from openHAB settings if not set on Thing

7542175

Signed-off-by: Stefan Triller <[email protected]>

cpmeister approved these changes Jun 17, 2020

View reviewed changes

cpmeister merged commit 00a2cd5 into openhab:2.5.x Jun 17, 2020

cpmeister added this to the 2.5.6 milestone Jun 17, 2020

t2000 deleted the robonectTime branch June 17, 2020 18:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Robonect] Use global openHAB timezone for DateTime objects #7903

[Robonect] Use global openHAB timezone for DateTime objects #7903

t2000 commented Jun 12, 2020

TravisBuddy commented Jun 12, 2020

t2000 commented Jun 12, 2020

t2000 commented Jun 12, 2020

TravisBuddy commented Jun 12, 2020

t2000 commented Jun 12, 2020

TravisBuddy commented Jun 12, 2020

me2000 commented Jun 12, 2020

cpmeister Jun 12, 2020

t2000 Jun 13, 2020

TravisBuddy commented Jun 13, 2020

cpmeister Jun 13, 2020

cpmeister Jun 13, 2020

t2000 Jun 14, 2020

t2000 Jun 15, 2020

t2000 Jun 15, 2020

cpmeister Jun 15, 2020

t2000 Jun 16, 2020

cpmeister Jun 16, 2020

t2000 Jun 17, 2020

t2000 commented Jun 14, 2020

TravisBuddy commented Jun 17, 2020

cpmeister left a comment

[Robonect] Use global openHAB timezone for DateTime objects #7903

[Robonect] Use global openHAB timezone for DateTime objects #7903

Conversation

t2000 commented Jun 12, 2020

TravisBuddy commented Jun 12, 2020

Travis tests were successful

t2000 commented Jun 12, 2020

t2000 commented Jun 12, 2020

TravisBuddy commented Jun 12, 2020

Travis tests were successful

t2000 commented Jun 12, 2020

TravisBuddy commented Jun 12, 2020

Travis tests were successful

me2000 commented Jun 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TravisBuddy commented Jun 13, 2020

Travis tests were successful

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

Choose a reason for hiding this comment

t2000 commented Jun 14, 2020

TravisBuddy commented Jun 17, 2020

Travis tests were successful

cpmeister left a comment

Choose a reason for hiding this comment