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

Basic UI internationalization #4338

Merged
merged 1 commit into from Sep 29, 2017

Conversation

Projects
None yet
4 participants
@lolodomo
Copy link
Contributor

commented Sep 25, 2017

Fixes #4327

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

@lolodomo

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Contributor Author

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.

@lolodomo lolodomo force-pushed the lolodomo:basicui_i18n branch 2 times, most recently from cde4e00 to 48da64b Sep 26, 2017

@sjka
Copy link
Contributor

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

This comment has been minimized.

Copy link
@sjka

sjka Sep 26, 2017

Contributor

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

This comment has been minimized.

Copy link
@lolodomo

lolodomo Sep 26, 2017

Author Contributor

Ok, I will change that.

This comment has been minimized.

Copy link
@lolodomo

lolodomo Sep 26, 2017

Author Contributor

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

This comment has been minimized.

Copy link
@sjka

sjka Sep 26, 2017

Contributor

Yes, please fix it. It makes no sense.

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

This comment has been minimized.

Copy link
@sjka

sjka Sep 26, 2017

Contributor

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

This comment has been minimized.

Copy link
@lolodomo

lolodomo Sep 26, 2017

Author Contributor

Ok


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

This comment has been minimized.

Copy link
@sjka

sjka Sep 26, 2017

Contributor

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

This comment has been minimized.

Copy link
@lolodomo

lolodomo Sep 26, 2017

Author Contributor

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

This comment has been minimized.

Copy link
@sjka

sjka Sep 26, 2017

Contributor

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.

This comment has been minimized.

Copy link
@lolodomo

lolodomo Sep 26, 2017

Author Contributor

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

@sjka sjka added the UI-BasicUI label Sep 26, 2017

@resetnow
Copy link
Contributor

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.

Basic UI internationalization
Fixes #4327

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

@lolodomo lolodomo force-pushed the lolodomo:basicui_i18n branch from 48da64b to 1f004d7 Sep 26, 2017

@lolodomo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2017

Done. All review comments taken into account.

@sjka sjka merged commit 853dda5 into eclipse:master Sep 29, 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 Sep 29, 2017

Thanks!

@lolodomo lolodomo deleted the lolodomo:basicui_i18n branch Sep 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’t perform that action at this time.