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

Added options for language & regional settings #3755

Merged
merged 1 commit into from Jun 29, 2017

Conversation

@sjsf
Copy link
Contributor

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

@kaikreuzer kaikreuzer Jun 27, 2017

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

Copy link
Contributor Author

@sjsf sjsf Jun 27, 2017

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

Copy link
Contributor Author

@sjsf sjsf Jun 27, 2017

done

Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>
@sjsf sjsf force-pushed the 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")) {
Copy link
Contributor

@amitjoy amitjoy Jun 27, 2017

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

Copy link
Contributor Author

@sjsf sjsf Jun 28, 2017

uri is a non-null argument (see caller)

Copy link
Contributor

@amitjoy amitjoy Jun 28, 2017

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

@amitjoy amitjoy Jun 27, 2017

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

Copy link
Contributor Author

@sjsf sjsf Jun 28, 2017

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

Copy link
Contributor

@amitjoy amitjoy Jun 28, 2017

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

Copy link
Contributor Author

@sjsf sjsf Jun 28, 2017

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.

Copy link
Contributor

@amitjoy amitjoy Jun 28, 2017

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

@amitjoy amitjoy Jun 27, 2017

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

Copy link
Contributor Author

@sjsf sjsf Jun 28, 2017

right - javadoc and caller implementation don't match

Copy link
Contributor Author

@sjsf sjsf Jun 28, 2017


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

@amitjoy amitjoy Jun 27, 2017

Just a syntactic sugar: ParameterOption::getLabel

Copy link
Contributor Author

@sjsf sjsf Jun 28, 2017

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

Copy link
Contributor

@amitjoy amitjoy Jun 28, 2017

definitely. It's upto you.

@kaikreuzer kaikreuzer merged commit 68f7388 into eclipse-archived:master Jun 29, 2017
2 checks passed
@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 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