Added options for language & regional settings #3755
Conversation
| @@ -0,0 +1,17 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
There was a problem hiding this comment.
How about using the nice DS annotations that you have introduced recently? Adding further XMLs only makes the refactoring harder in future!
There was a problem hiding this comment.
Just caught by my own lazyness to port them all.
I'll adapt this PR!
Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>
|
|
||
| @Override | ||
| public Collection<ParameterOption> getParameterOptions(URI uri, String param, Locale locale) { | ||
| if (uri.toString().equals("system:i18n")) { |
There was a problem hiding this comment.
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))
There was a problem hiding this comment.
uri is a non-null argument (see caller)
There was a problem hiding this comment.
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")) { |
There was a problem hiding this comment.
param can also be null, we can do this -> "language".equals(param).
There was a problem hiding this comment.
a param with a null name? how should that work?
There was a problem hiding this comment.
No clue if param is never null in the callers. Just the contract was not clear about it.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
According to the contract, we need to return empty list.
There was a problem hiding this comment.
right - javadoc and caller implementation don't match
|
|
||
| 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()); |
There was a problem hiding this comment.
Just a syntactic sugar: ParameterOption::getLabel
There was a problem hiding this comment.
I guess that's personal flavor, isn't it?
There was a problem hiding this comment.
definitely. It's upto you.
kaikreuzer
changed the title
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