Added time zone in the PaperUI and in the I18ProviderImpl #4256

doandzhi · 2017-09-13T07:56:23Z

Querying the underlying OS for a timezone and setting this as a default in the LocationProvider. A user could then choose a different one via a drop down list in Paper UI.

Fixes #3936

Signed-off-by: Erdoan Hadzhiyusein 3rdoan@gmail.com

maggu2810 · 2017-09-13T12:03:19Z

...src/main/java/org/eclipse/smarthome/config/core/internal/i18n/I18nConfigOptionsProvider.java

+                case "timezone":
+                    Collection<ParameterOption> collection = new LinkedList<ParameterOption>();
+                    String[] ids = TimeZone.getAvailableIDs();
+                    List<TimeZone> timeZones = Arrays.asList(ids).stream().map(TimeZone::getTimeZone)


Instead of Arrays.asList(array).stream() you could use Arrays.stream(array)

maggu2810 · 2017-09-13T12:04:25Z

...src/main/java/org/eclipse/smarthome/config/core/internal/i18n/I18nConfigOptionsProvider.java

+        long hours = TimeUnit.MILLISECONDS.toHours(tz.getRawOffset());
+        long minutes = TimeUnit.MILLISECONDS.toMinutes(tz.getRawOffset()) - TimeUnit.HOURS.toMinutes(hours);
+        minutes = Math.abs(minutes);
+        String result = "";


Use final String result; so you ensure that result needs to be assigned in one of the branches and you don't need to assign it twice.

maggu2810 · 2017-09-13T12:05:34Z

...g.eclipse.smarthome.core/src/main/java/org/eclipse/smarthome/core/i18n/LocationProvider.java

+     *
+     * @return the time zone set in the Paper UI and if there isn't one, the default time zone of the underlying system
+     */
+    TimeZone getTimeZone();


Shouldn't there be a TimeZoneProvider interface that is implemented by I18nProviderImpl similar to LocaleProvider and LocationProvider?

maggu2810 · 2017-09-13T12:06:29Z

....smarthome.core/src/main/java/org/eclipse/smarthome/core/internal/i18n/I18nProviderImpl.java

    }

    @Override
    public PointType getLocation() {
        return location;
    }

+    public TimeZone getTimeZone() {


Your IDE should add @Override.

maggu2810 · 2017-09-13T12:30:21Z

...src/main/java/org/eclipse/smarthome/config/core/internal/i18n/I18nConfigOptionsProvider.java

+                            l -> new ParameterOption(l.getVariant(), l.getDisplayVariant(translation)));
+                case "timezone":
+                    Collection<ParameterOption> collection = new LinkedList<ParameterOption>();
+                    String[] ids = TimeZone.getAvailableIDs();


I assume you are more familar with the topic of Java 8's new time API because you implemented #4259

But shouldn't we use ZoneId instead of TimeZone?

There is an existing time zone class in Java—java.util.TimeZone—but it isn’t used by Java SE 8 be-cause all JSR 310 classes are immutable and time zone is mutable.

There is also ZoneId.getAvailableZoneIds(), ...

doandzhi · 2017-09-13T12:51:04Z

Thanks for the quick reaction. I agree with the corrections you proposed.

maggu2810 · 2017-09-21T14:25:29Z

@doandzhi Hi, does it make more sense to provide a time zone or a zone id in the java code?

sjsf

Cool you tackled this! Looks pretty good already.
I just added a few inline comments where I think the implementation could even be further simplified.

sjsf · 2017-09-25T15:38:57Z

...src/main/java/org/eclipse/smarthome/config/core/internal/i18n/I18nConfigOptionsProvider.java

+                    return getAvailable(locale,
+                            l -> new ParameterOption(l.getVariant(), l.getDisplayVariant(translation)));
+                case "timezone":
+                    Collection<ParameterOption> collection = new LinkedList<ParameterOption>();


please factor out this logic into its own method so it stays easy to keep an overview of this switch statement (good idea, by the way!).

And while you are at it: why didn't you keep using the stream? e.g. like

return ZoneId.getAvailableZoneIds().stream().map(TimeZone::getTimeZone).sorted((tz1, tz2) -> { return tz2.getRawOffset() - tz2.getRawOffset(); }).map(tz -> { return new ParameterOption(tz.getID(), getTimeZoneRepresentation(tz)); }).collect(Collectors.toList());

sjsf · 2017-09-25T15:41:19Z

....smarthome.core/src/main/java/org/eclipse/smarthome/core/internal/i18n/I18nProviderImpl.java

+                this.timeZone = TimeZone.getTimeZone(timeZone);
+            } catch (IllegalArgumentException e) {
+                this.timeZone = TimeZone.getDefault();
+                logger.warn("Could not set a new timezone, the system's default time zone will be used ", e);


~~timezone~~ -> time zone

...to keep it consistent

sjsf · 2017-09-25T15:43:07Z

....smarthome.core/src/main/java/org/eclipse/smarthome/core/internal/i18n/I18nProviderImpl.java

-            }
-        }
+        setTimeZone(timeZone);
+        setLocation(location);


why did you factor out these, but not the language?

sjsf · 2017-09-25T15:47:25Z

PS: What I initially wanted to ask you but forgot: Could you please rebase your branch onto the latest master? Thanks!

doandzhi · 2017-09-26T15:01:43Z

@SJKA The language logic in I18nProviderImpl you proposed extracting into a new method is not applicable at this stage since some of the tests of the core bundle doesn't pass. Do you think that this is obligatory for now? :) On the other hand I updated the PR rebased and adressed the other comments.

sjsf · 2017-09-26T16:32:55Z

Cool, thanks. No, that wasn't a requirement - it's easier to read right now, all good!

I just realized that you didn't answer @maggu2810's question above. Considering that in #4259 you always need ZoneIDs and that it makes sense to stay within the new java.time scope, this is a very valid question to consider.

doandzhi · 2017-10-03T14:06:31Z

Hello, @maggu2810! Regarding the changes you proposed in I18nProviderImpl.java (using java.time object instead of String to store the time zone) I made the needed corrections. I would be very glad if you can take a look at some time so I can integrate the changes directly in #4259 soon.

sjsf · 2017-10-04T15:54:39Z

...g.eclipse.smarthome.core/src/main/java/org/eclipse/smarthome/core/i18n/TimeZoneProvider.java

+     *
+     * @return the time zone set in the Paper UI and if there isn't one, the default time zone of the underlying system
+     */
+    TimeZone getTimeZone();


If I understood @maggu2810's question right, he was asking wether it might make more sense to return a java.time.ZoneId here instead of a java.util.TimeZone. Considering that with the java.time.* classes that you are using in #4259 you always have to call timeZone.toZoneId() on it anyway, IMHO this would make sense. Or did I miss anything?

sjsf · 2017-10-04T15:55:03Z

...g.eclipse.smarthome.core/src/main/java/org/eclipse/smarthome/core/i18n/TimeZoneProvider.java

+import java.util.TimeZone;
+
+/**
+ * This interface describes a provider for a location and time zone.


This interface describes a provider for a ~~location and~~ time zone.

sjsf

Much nicer, thanks!
Now just some final things I noticed when I did the last review (my apologies for having missed them before). Then I think this finally is good to go!

sjsf · 2017-10-06T15:26:47Z

....smarthome.core/src/main/java/org/eclipse/smarthome/core/internal/i18n/I18nProviderImpl.java

+                this.timeZone = TimeZone.getTimeZone(timeZone).toZoneId();
+            } catch (IllegalArgumentException e) {
+                this.timeZone = TimeZone.getDefault().toZoneId();
+                logger.warn("Could not set a new time zone, the system's default time zone will be used ", e);


see above wrt logging

sjsf · 2017-10-06T15:26:48Z

....smarthome.core/src/main/java/org/eclipse/smarthome/core/internal/i18n/I18nProviderImpl.java

+                this.location = PointType.valueOf(location);
+            } catch (IllegalArgumentException e) {
+                // preserve old location or null if none was set before
+                logger.warn("Could not set new location, keeping old one: ", e);


The stacktrace here is pretty boring. Therefore it was left out intentionally (although due to the missing placeholder still wrong). It would have been nice though to let the user know what exactly was the value that wasn't so great:

logger.warn("Could not set new location {}, keeping old one: {}", location, e.getMessage());

sjsf · 2017-10-06T15:28:55Z

....smarthome.core/src/main/java/org/eclipse/smarthome/core/internal/i18n/I18nProviderImpl.java

@@ -57,6 +62,7 @@
    private static final String SCRIPT = "script";
    private static final String REGION = "region";
    private static final String VARIANT = "variant";
+    private static final String TIMEZONE = "timezone";


please sort the two new fields (TIMEZONE and timeZone) under their own // TimeZoneProvider comment in order to keep the structure as clean as it was before.

sjsf · 2017-10-06T15:29:52Z

...g.eclipse.smarthome.core/src/main/java/org/eclipse/smarthome/core/i18n/TimeZoneProvider.java

+import java.time.ZoneId;
+
+/**
+ * This interface describes a provider for a location and time zone.


This interface describes a provider for a ~~location and~~ time zone.

sjsf · 2017-10-06T15:31:13Z

bundles/core/org.eclipse.smarthome.core/ESH-INF/config/i18n.xml

+		<parameter name="timezone" type="text" required="false">
+			<label>Time zone</label>
+			<description><![CDATA[
+				<p>A time zone can be set from the dropdown menu.</p>


Please don't refer to a "dropdown menu" here - it's up to the UI how it gets rendered.

Also: It would be nice to give an example just like done for the other parameters above.

sjsf · 2017-10-06T15:31:43Z

bundles/core/org.eclipse.smarthome.core/ESH-INF/config/i18n.xml

@@ -43,6 +43,14 @@
 			</description>
 			<advanced>true</advanced>
 		</parameter>
+		<parameter name="timezone" type="text" required="false">
+			<label>Time zone</label>


sjsf · 2017-10-06T15:32:49Z

...g.eclipse.smarthome.core/src/main/java/org/eclipse/smarthome/core/i18n/TimeZoneProvider.java

+    /**
+     * Provides access to the time zone property.
+     *
+     * @return the time zone set in the Paper UI and if there isn't one, the default time zone of the underlying system


Please don't refer to the PaperUI here. It's just a sample UI and the core should not even know it exists.

sjsf · 2017-10-06T15:33:22Z

...g.eclipse.smarthome.core/src/main/java/org/eclipse/smarthome/core/i18n/TimeZoneProvider.java

+    /**
+     * Provides access to the time zone property.
+     *
+     * @return the time zone set in the Paper UI and if there isn't one, the default time zone of the underlying system


With "underlying system" you mean the operating system, right? Why don't you name it then?

…iguration of it. Signed-off-by: Erdoan Hadzhiyusein <3rdoan@gmail.com>

doandzhi · 2017-10-06T17:31:41Z

Thanks for the appropriate comments. I have to be sorry for not considering these actually. Had a closer look and I think it looks better now. :)

sjsf · 2017-10-10T08:57:36Z

👍
Thanks!

maggu2810 · 2017-10-10T09:36:57Z

Hello, @maggu2810! Regarding the changes you proposed in I18nProviderImpl.java (using java.time object instead of String to store the time zone) I made the needed corrections. I would be very glad if you can take a look at some time so I can integrate the changes directly in #4259 soon.

Sorry, has been not available the last week.
Thanks @SJKA for having a look at (you are right with your assumption what has been my meaning).
And thanks @doandzhi for your work.

maggu2810 reviewed Sep 13, 2017

View reviewed changes

doandzhi force-pushed the timezone-provider-implementation branch from f0dcd77 to 4da0eab Compare September 13, 2017 12:48

doandzhi changed the title ~~Added time zone in the PaperUI and in the LocationProvider service~~ Added time zone in the PaperUI and in the I18ProviderImpl Sep 13, 2017

sjsf reviewed Sep 25, 2017

View reviewed changes

sjsf mentioned this pull request Sep 26, 2017

Refactoring DateTimeType and impacts on ESH #4259

Merged

doandzhi force-pushed the timezone-provider-implementation branch 2 times, most recently from 6528ded to b33299e Compare September 26, 2017 13:11

doandzhi force-pushed the timezone-provider-implementation branch from b33299e to 211b76a Compare September 28, 2017 07:00

sjsf reviewed Oct 4, 2017

View reviewed changes

doandzhi force-pushed the timezone-provider-implementation branch 4 times, most recently from f339064 to 79f729d Compare October 6, 2017 13:59

sjsf reviewed Oct 6, 2017

View reviewed changes

Added time zone delivery service implementation & User Interface conf…

10ed563

…iguration of it. Signed-off-by: Erdoan Hadzhiyusein <3rdoan@gmail.com>

doandzhi force-pushed the timezone-provider-implementation branch from 79f729d to 10ed563 Compare October 6, 2017 15:54

sjsf merged commit 2fdba57 into eclipse-archived:master Oct 10, 2017

kaikreuzer added this to the 0.9.0 milestone Nov 30, 2017

kaikreuzer added the enhancement label Dec 15, 2017

triller-telekom mentioned this pull request Apr 6, 2018

add nullness annotations for i18n interfaces and impl #5293

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added time zone in the PaperUI and in the I18ProviderImpl #4256

Added time zone in the PaperUI and in the I18ProviderImpl #4256

doandzhi commented Sep 13, 2017 •

edited

maggu2810 Sep 13, 2017

maggu2810 Sep 13, 2017

maggu2810 Sep 13, 2017

maggu2810 Sep 13, 2017

maggu2810 Sep 13, 2017

doandzhi commented Sep 13, 2017

maggu2810 commented Sep 21, 2017

sjsf left a comment

sjsf Sep 25, 2017

sjsf Sep 25, 2017

sjsf Sep 25, 2017

sjsf commented Sep 25, 2017

doandzhi commented Sep 26, 2017 •

edited

sjsf commented Sep 26, 2017

doandzhi commented Oct 3, 2017 •

edited

sjsf Oct 4, 2017

sjsf Oct 4, 2017

sjsf left a comment

sjsf Oct 6, 2017

sjsf Oct 6, 2017

sjsf Oct 6, 2017

sjsf Oct 6, 2017

sjsf Oct 6, 2017

sjsf Oct 6, 2017

sjsf Oct 6, 2017

sjsf Oct 6, 2017

doandzhi commented Oct 6, 2017

sjsf commented Oct 10, 2017

maggu2810 commented Oct 10, 2017

Added time zone in the PaperUI and in the I18ProviderImpl #4256

Added time zone in the PaperUI and in the I18ProviderImpl #4256

Conversation

doandzhi commented Sep 13, 2017 • edited

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

doandzhi commented Sep 13, 2017

maggu2810 commented Sep 21, 2017

sjsf left a 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

sjsf commented Sep 25, 2017

doandzhi commented Sep 26, 2017 • edited

sjsf commented Sep 26, 2017

doandzhi commented Oct 3, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjsf left a 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

Choose a reason for hiding this comment

doandzhi commented Oct 6, 2017

sjsf commented Oct 10, 2017

maggu2810 commented Oct 10, 2017

doandzhi commented Sep 13, 2017 •

edited

doandzhi commented Sep 26, 2017 •

edited

doandzhi commented Oct 3, 2017 •

edited