Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Oct 10, 2017

Conversation

Projects
None yet
4 participants
@doandzhi
Copy link
Contributor

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)

This comment has been minimized.

Copy link
@maggu2810

maggu2810 Sep 13, 2017

Contributor

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 = "";

This comment has been minimized.

Copy link
@maggu2810

maggu2810 Sep 13, 2017

Contributor

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

This comment has been minimized.

Copy link
@maggu2810

maggu2810 Sep 13, 2017

Contributor

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

This comment has been minimized.

Copy link
@maggu2810

maggu2810 Sep 13, 2017

Contributor

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

This comment has been minimized.

Copy link
@maggu2810

maggu2810 Sep 13, 2017

Contributor

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 MusalaSoft:timezone-provider-implementation branch from f0dcd77 to 4da0eab Sep 13, 2017

@doandzhi

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2017

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

@sjka
Copy link
Contributor

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

This comment has been minimized.

Copy link
@sjka

sjka Sep 25, 2017

Contributor

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

This comment has been minimized.

Copy link
@sjka

sjka Sep 25, 2017

Contributor

timezone -> time zone

...to keep it consistent

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

This comment has been minimized.

Copy link
@sjka

sjka Sep 25, 2017

Contributor

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

@sjka

This comment has been minimized.

Copy link
Contributor

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 MusalaSoft:timezone-provider-implementation branch 2 times, most recently from 6528ded to b33299e Sep 26, 2017

@doandzhi

This comment has been minimized.

Copy link
Contributor Author

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.

@sjka

This comment has been minimized.

Copy link
Contributor

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 MusalaSoft:timezone-provider-implementation branch from b33299e to 211b76a Sep 28, 2017

@doandzhi

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
@sjka

sjka Oct 4, 2017

Contributor

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.

This comment has been minimized.

Copy link
@sjka

sjka Oct 4, 2017

Contributor

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

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

@sjka
Copy link
Contributor

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

This comment has been minimized.

Copy link
@sjka

sjka Oct 6, 2017

Contributor

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

This comment has been minimized.

Copy link
@sjka

sjka Oct 6, 2017

Contributor

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";

This comment has been minimized.

Copy link
@sjka

sjka Oct 6, 2017

Contributor

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.

This comment has been minimized.

Copy link
@sjka

sjka Oct 6, 2017

Contributor

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>

This comment has been minimized.

Copy link
@sjka

sjka Oct 6, 2017

Contributor

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>

This comment has been minimized.

Copy link
@sjka

sjka Oct 6, 2017

Contributor

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

This comment has been minimized.

Copy link
@sjka

sjka Oct 6, 2017

Contributor

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

This comment has been minimized.

Copy link
@sjka

sjka Oct 6, 2017

Contributor

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

Added time zone delivery service implementation & User Interface conf…
…iguration of it.

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

@doandzhi doandzhi force-pushed the MusalaSoft:timezone-provider-implementation branch from 79f729d to 10ed563 Oct 6, 2017

@doandzhi

This comment has been minimized.

Copy link
Contributor Author

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. :)

@sjka sjka merged commit 2fdba57 into eclipse:master Oct 10, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
ip-validation
Details
@sjka

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2017

👍
Thanks!

@maggu2810

This comment has been minimized.

Copy link
Contributor

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.

@kaikreuzer kaikreuzer added this to the 0.9.0 milestone Nov 30, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.