Skip to content
This repository has been archived by the owner. It is now read-only.

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

Merged

Conversation

@doandzhi
Copy link
Contributor

@doandzhi doandzhi commented Sep 13, 2017

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.
timezonesnimka1
yee

Fixes #3936

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

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

@maggu2810 maggu2810 Sep 13, 2017

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

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

@maggu2810 maggu2810 Sep 13, 2017

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.

*
* @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();
Copy link
Contributor

@maggu2810 maggu2810 Sep 13, 2017

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

}

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

public TimeZone getTimeZone() {
Copy link
Contributor

@maggu2810 maggu2810 Sep 13, 2017

Your IDE should add @Override.

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

@maggu2810 maggu2810 Sep 13, 2017

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 doandzhi force-pushed the timezone-provider-implementation branch from f0dcd77 to 4da0eab Sep 13, 2017
@doandzhi
Copy link
Contributor Author

@doandzhi doandzhi commented Sep 13, 2017

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

@doandzhi 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
@maggu2810
Copy link
Contributor

@maggu2810 maggu2810 commented Sep 21, 2017

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

Copy link
Contributor

@sjsf sjsf left a comment

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.

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

@sjsf sjsf Sep 25, 2017

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());

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);
Copy link
Contributor

@sjsf sjsf Sep 25, 2017

timezone -> time zone

...to keep it consistent

}
}
setTimeZone(timeZone);
setLocation(location);
Copy link
Contributor

@sjsf sjsf Sep 25, 2017

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

@sjsf
Copy link
Contributor

@sjsf sjsf commented Sep 25, 2017

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

@doandzhi doandzhi force-pushed the timezone-provider-implementation branch 2 times, most recently from 6528ded to b33299e Sep 26, 2017
@doandzhi
Copy link
Contributor Author

@doandzhi doandzhi commented Sep 26, 2017

@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
Copy link
Contributor

@sjsf sjsf commented Sep 26, 2017

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 doandzhi force-pushed the timezone-provider-implementation branch from b33299e to 211b76a Sep 28, 2017
@doandzhi
Copy link
Contributor Author

@doandzhi doandzhi commented Oct 3, 2017

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.

*
* @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();
Copy link
Contributor

@sjsf sjsf Oct 4, 2017

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?

import java.util.TimeZone;

/**
* This interface describes a provider for a location and time zone.
Copy link
Contributor

@sjsf sjsf Oct 4, 2017

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

@doandzhi doandzhi force-pushed the timezone-provider-implementation branch 4 times, most recently from f339064 to 79f729d Oct 6, 2017
Copy link
Contributor

@sjsf sjsf left a comment

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!

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);
Copy link
Contributor

@sjsf sjsf Oct 6, 2017

see above wrt logging

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);
Copy link
Contributor

@sjsf sjsf Oct 6, 2017

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());

@@ -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";
Copy link
Contributor

@sjsf sjsf Oct 6, 2017

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.

import java.time.ZoneId;

/**
* This interface describes a provider for a location and time zone.
Copy link
Contributor

@sjsf sjsf Oct 6, 2017

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

<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>
Copy link
Contributor

@sjsf sjsf Oct 6, 2017

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.

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

@sjsf sjsf Oct 6, 2017

Time Zone

/**
* 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
Copy link
Contributor

@sjsf sjsf Oct 6, 2017

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

/**
* 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
Copy link
Contributor

@sjsf sjsf Oct 6, 2017

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 doandzhi force-pushed the timezone-provider-implementation branch from 79f729d to 10ed563 Oct 6, 2017
@doandzhi
Copy link
Contributor Author

@doandzhi doandzhi commented Oct 6, 2017

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 sjsf merged commit 2fdba57 into eclipse-archived:master Oct 10, 2017
2 checks passed
@sjsf
Copy link
Contributor

@sjsf sjsf commented Oct 10, 2017

👍
Thanks!

@maggu2810
Copy link
Contributor

@maggu2810 maggu2810 commented Oct 10, 2017

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants