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

Basic UI internationalization #4338

Merged
merged 1 commit into from Sep 29, 2017
Merged

Basic UI internationalization #4338

merged 1 commit into from Sep 29, 2017

Conversation

@lolodomo
Copy link
Contributor

@lolodomo lolodomo commented Sep 25, 2017

Fixes #4327

Signed-off-by: Laurent Garnier lg.hc@free.fr

@lolodomo
Copy link
Contributor Author

@lolodomo lolodomo commented Sep 25, 2017

Only the page renderer needs to translate texts but the proposed solution could easily be enhanced to provide translation to any widget renderer if needed.

@lolodomo
Copy link
Contributor Author

@lolodomo lolodomo commented Sep 25, 2017

I chose dynamic policy for referenced services added but I am not sure if I should rather use static policy ?
The existing reference to item registry service is with dynamic policy while I would have rather expected static.

Copy link
Contributor

@sjsf sjsf left a comment

Generally makes sense to me. I left some comments inline.

@resetnow any further thoughts?

@@ -12,6 +12,8 @@
<implementation class="org.eclipse.smarthome.ui.basic.internal.render.PageRenderer"/>

<reference bind="setItemUIRegistry" cardinality="1..1" interface="org.eclipse.smarthome.ui.items.ItemUIRegistry" name="ItemUIRegistry" policy="dynamic" unbind="unsetItemUIRegistry"/>
<reference bind="setLocaleProvider" cardinality="1..1" interface="org.eclipse.smarthome.core.i18n.LocaleProvider" name="LocaleProvider" policy="dynamic" unbind="unsetLocaleProvider"/>
Copy link
Contributor

@sjsf sjsf Sep 26, 2017

As you expected: they should all be static!
Especially as they are "1..1", the "dynamic" makes no sense here.

Copy link
Contributor Author

@lolodomo lolodomo Sep 26, 2017

Ok, I will change that.

Copy link
Contributor Author

@lolodomo lolodomo Sep 26, 2017

@SJKA: why is it defined like that with dynamic policy for ItemUIRegistry ? Is it something I should fix ?

Copy link
Contributor

@sjsf sjsf Sep 26, 2017

Yes, please fix it. It makes no sense.

protected void activate(ComponentContext context) {
this.bundleContext = context.getBundleContext();
Copy link
Contributor

@sjsf sjsf Sep 26, 2017

you can change the signature to activate(BundleContext context) and you will get it directly.

Copy link
Contributor Author

@lolodomo lolodomo Sep 26, 2017

Ok


protected String localizeText(String key) {
String result = "";
if (this.i18nProvider != null && this.locale != null && I18nUtil.isConstant(key)) {
Copy link
Contributor

@sjsf sjsf Sep 26, 2017

How can they be null? You specified a "1..1" relation, so you can rely on them.

Copy link
Contributor Author

@lolodomo lolodomo Sep 26, 2017

It was in case the referenced services were not yet loaded. But ok, I will suppress the null test as they will be there if I use a static policy.

@@ -66,10 +75,28 @@ public ItemUIRegistry getItemUIRegistry() {
return itemUIRegistry;
}

public void setLocaleProvider(LocaleProvider localeProvider) {
this.locale = localeProvider.getLocale();
Copy link
Contributor

@sjsf sjsf Sep 26, 2017

I'd keep the localeProvider and call localeProvider.getLocale() whenever you need it. Otherwise you won't notice if the user changes the locale settings.

Copy link
Contributor Author

@lolodomo lolodomo Sep 26, 2017

Makes ense. I realize we have the same mistake elsewhere, sometimes introduced by myself !

Copy link
Contributor

@resetnow resetnow left a comment

Looks good to me. We are going to need a proper templating system sometime in the future, or at least a simple wrapper around String.replace.

Fixes #4327

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@lolodomo
Copy link
Contributor Author

@lolodomo lolodomo commented Sep 26, 2017

Done. All review comments taken into account.

@sjsf sjsf merged commit 853dda5 into eclipse-archived:master Sep 29, 2017
2 checks passed
@sjsf
Copy link
Contributor

@sjsf sjsf commented Sep 29, 2017

Thanks!

@lolodomo lolodomo deleted the basicui_i18n branch Sep 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