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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added options for language & regional settings #3755

Merged
merged 1 commit into from Jun 29, 2017

Conversation

Projects
None yet
4 participants
@sjka
Copy link
Contributor

commented Jun 27, 2017

I was just wondering why we don't have a config options provider for language/region settings. So please consider this PR as a question 馃槃

Signed-off-by: Simon Kaufmann simon.kfm@googlemail.com

@@ -0,0 +1,17 @@
<?xml version="1.0" encoding="UTF-8"?>

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Jun 27, 2017

Member

How about using the nice DS annotations that you have introduced recently? Adding further XMLs only makes the refactoring harder in future!

This comment has been minimized.

Copy link
@sjka

sjka Jun 27, 2017

Author Contributor

Just caught by my own lazyness to port them all.
I'll adapt this PR!

This comment has been minimized.

Copy link
@sjka

sjka Jun 27, 2017

Author Contributor

done

config option provider for language & region settings
Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

@sjka sjka force-pushed the sjka:configOptionsI18n branch from d6b71d4 to 7256af6 Jun 27, 2017


@Override
public Collection<ParameterOption> getParameterOptions(URI uri, String param, Locale locale) {
if (uri.toString().equals("system:i18n")) {

This comment has been minimized.

Copy link
@amitjoy

amitjoy Jun 27, 2017

Member

As the contract doesn't specify any throws clause, we cannot add any invariants here. Hence we could do this -> "system:i18n".equals(String.valueOf(uri))

This comment has been minimized.

Copy link
@sjka

sjka Jun 28, 2017

Author Contributor

uri is a non-null argument (see caller)

This comment has been minimized.

Copy link
@amitjoy

amitjoy Jun 28, 2017

Member

I had a quick look on the contract and the contract doesn't specify uri to be non null. If you think, the callers will never use null, you can ignore this.

public Collection<ParameterOption> getParameterOptions(URI uri, String param, Locale locale) {
if (uri.toString().equals("system:i18n")) {
Locale translation = locale != null ? locale : Locale.getDefault();
if (param.equals("language")) {

This comment has been minimized.

Copy link
@amitjoy

amitjoy Jun 27, 2017

Member

param can also be null, we can do this -> "language".equals(param).

This comment has been minimized.

Copy link
@sjka

sjka Jun 28, 2017

Author Contributor

a param with a null name? how should that work?

This comment has been minimized.

Copy link
@amitjoy

amitjoy Jun 28, 2017

Member

No clue if param is never null in the callers. Just the contract was not clear about it.

This comment has been minimized.

Copy link
@sjka

sjka Jun 28, 2017

Author Contributor

Oh, the contract in fact is pretty clear:
ConfigDescriptionParameter.java#L204, config-description-1.0.0.xsd#L59
It's just at the wrong place/not replicated everywhere (yet), depending on your perspective.

This comment has been minimized.

Copy link
@amitjoy

amitjoy Jun 28, 2017

Member

You are right. I only looked into ConfigOptionProvider interface but its callers handle the invariants separately. I believe it would also be beneficial if such invariants can also be added to the interfaces.

return getAvailable(locale, l -> new ParameterOption(l.getVariant(), l.getDisplayVariant(translation)));
}
}
return null;

This comment has been minimized.

Copy link
@amitjoy

amitjoy Jun 27, 2017

Member

According to the contract, we need to return empty list.

This comment has been minimized.

Copy link
@sjka

sjka Jun 28, 2017

Author Contributor

right - javadoc and caller implementation don't match

This comment has been minimized.

Copy link
@sjka

sjka Jun 28, 2017

Author Contributor

private Collection<ParameterOption> getAvailable(Locale locale, Function<Locale, ParameterOption> mapFunction) {
return Arrays.stream(Locale.getAvailableLocales()).map(l -> mapFunction.apply(l)).distinct()
.sorted(Comparator.comparing(a -> a.getLabel())).collect(Collectors.toList());

This comment has been minimized.

Copy link
@amitjoy

amitjoy Jun 27, 2017

Member

Just a syntactic sugar: ParameterOption::getLabel

This comment has been minimized.

Copy link
@sjka

sjka Jun 28, 2017

Author Contributor

I guess that's personal flavor, isn't it?

This comment has been minimized.

Copy link
@amitjoy

amitjoy Jun 28, 2017

Member

definitely. It's upto you.

@kaikreuzer kaikreuzer merged commit 68f7388 into eclipse:master Jun 29, 2017

2 checks passed

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

@kaikreuzer kaikreuzer changed the title config options provider for language & region settings Added options for language & regional settings Jun 29, 2017

@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鈥檛 perform that action at this time.