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

Dashboard internationalization #217

Merged
merged 6 commits into from Nov 18, 2017

Conversation

Projects
None yet
5 participants
@lolodomo
Copy link
Contributor

commented Sep 30, 2017

French version included

Fixes #214

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

Dashboard internationalization
French version included

Fixes #214

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

This comment has been minimized.

Copy link
Contributor Author

commented Oct 7, 2017

Noone to review this one ?

@martinvw

This comment has been minimized.

Copy link
Member

commented Oct 7, 2017

Maybe you can find a French person for the texts, I will review the code tonight but I'm not so familiar in this part ;-)

@martinvw

This comment has been minimized.

Copy link
Member

commented Oct 7, 2017

It makes all sense to me, but I normally use some kind of translation where the default language is still in the templates, I would maybe favor that.

@kaikreuzer wdyt?

@lolodomo

This comment has been minimized.

Copy link
Contributor Author

commented Oct 23, 2017

Can this be merged ?

@martinvw martinvw requested a review from kaikreuzer Oct 23, 2017

@martinvw

This comment has been minimized.

Copy link
Member

commented Oct 23, 2017

@lolodomo did you find a French person to review the texts?

@lolodomo

This comment has been minimized.

Copy link
Contributor Author

commented Oct 23, 2017

Honestly, that is not necessary, I think I am able to write few sentences in my natural language lol

@kaikreuzer

This comment has been minimized.

Copy link
Member

commented Oct 24, 2017

It makes all sense to me, but I normally use some kind of translation where the default language is still in the templates, I would maybe favor that.

Honestly, seeing code pieces like this, I am also a bit reluctant and wonder whether this is really where we want to head.

We briefly touched the topic of translations yesterday with @ThomDietrich, so maybe this is a good time to pick up this discussion.
I would want to challenge whether we want full localization support throughout the product. As we can see in this PR, it makes the code more lengthy, harder to read, more error-prone and much harder to maintain as all languages need to be addressed upon any refactoring, extension, etc. Is it really worth it, considering that our documentation is only English as well and so are log entries, etc.?

Maybe we should separate the roles of "admins" and "users" - my take would be that everything related to administrative tasks is fine to be kept in English only, while "user facing" UIs should be localisable (which is mainly achieved through the sitemap definitions).
The dashboard is a bit of a fuzzy middle part, but I would consider it to be rather on the administrative side of things - at least the rest of my family members (whom I consider the "users") have never ever accessed the dashboard themselves; in the daily operation, they only access the "normal" UIs.

While I agree that in an ideal world we should localise everything, I also think that we need to wisely decide where to put our efforts - and a feature that is not only additional work by itself, but which actually makes it harder to maintain the other code as well, is something not to be taken too lightly.

@lolodomo

This comment has been minimized.

Copy link
Contributor Author

commented Oct 24, 2017

What's a pity. Rejecting the internationalization is I think the best way to avoid a large public.
The dashboard is I think the entry point for most users using a WEB browser.
And there are only 2x strings to maintain !

*
* @param key the key starting with &#64;text/ of to the localization string, e.g &#64;text/index.welcome
*
* @return the localized text if the key is found, or an empty string if not

This comment has been minimized.

Copy link
@ThomDietrich

ThomDietrich Oct 24, 2017

Member

"or an empty string if not" 😲
This can't be the solution. Strings will be added over time and if the Japanese language file is not updated right away, please present the english string as a fallback.

This comment has been minimized.

Copy link
@lolodomo

lolodomo Oct 24, 2017

Author Contributor

Yes or course the English text is returned for a key that is not present in the Japanese file.
Empty string is returned in the case the entry is not defined in the default English file too. This case should never happened except if there is a bug in the code.

@ThomDietrich

This comment has been minimized.

Copy link
Member

commented Oct 24, 2017

Hey guys!
Happy to talk about the issue.
The first thing that came to my mind is, that you (@lolodomo) have implemented the whole logic yourself. I am not an experienced developer in the area but surely there are preexisting solutions? I dislike the repetition of string-specific "replace" calls and the html comments in the document. There has to be a better way!

If done right, I have to disagree with (1) it will make development harder and (2) english is enough.

Rationale for 1: If the identifiers are speaking names, this should be fine. A first look at the english file suggests they are.

Rationale for 2: This should be normal nowadays. I don't want to exaggerate but internationalization of frontends can somewhat be compared to the topic of gendering in the social sector.
If a user prefers the english translation he/she may switch over. (Yes, docs.openhab.org is in english. I would change that if it would be in any way feasible)

Last but not least. Language resource will change over time. Strings in the main english file will change, be added or deleted. Translated files need to follow these modifications and translators need to be informed about needed actions. I therefore suggest to utilize an external localization service (integrated with GitHub).

At first glance Crowdin seems to offer great features in that regard, with a free open source plan and full GitHub integration.

@Hilbrand

This comment has been minimized.

Copy link
Contributor

commented Nov 4, 2017

There are several existing methods to do translation. But most would require to include a bunch of libraries. For example with jsp it's fairly simple, but then the libraries to compile jsp's must be included. With some smart replacement (the ResourceBundle already present does already a lot of work) I build up-on the work of Laurent and used a jsp similar notation in the html and some simple replacements. I've put it in the following commit: Hilbrand@6b7f3b0 Let me know if you all think this is worth it or otherwise I'll leave it.

@kaikreuzer

This comment has been minimized.

Copy link
Member

commented Nov 5, 2017

If you all think that it is worthwhile to have the dashboard translated, I won't stop you.
@Hilbrand makes the code a bit nicer, so I think that is a fair compromise and I would suggest to include his work. @lolodomo, if you agree, @Hilbrand could create a PR against your branch so that this PR here is automatically updated accordingly.

Updated localization by using map of keys.
Also added Dutch translation.

Signed-off-by: Hilbrand Bouwkamp <hilbrand@h72.nl>
@Hilbrand

This comment has been minimized.

Copy link
Contributor

commented Nov 6, 2017

PR created: lolodomo#1

Merge pull request #1 from Hilbrand/dashboard_i18n_update
Updated dashboard internationalization
@lolodomo

This comment has been minimized.

Copy link
Contributor Author

commented Nov 11, 2017

I merged the PR from @Hilbrand .
Code is cleaner like that.
Let me just the time to test it now.

@lolodomo

This comment has been minimized.

Copy link
Contributor Author

commented Nov 11, 2017

I was forced to build a new branch locally from master branch and merge this PR into it to test it.
After a very quick test, it looks like I still get French stuff translation.

@lolodomo

This comment has been minimized.

Copy link
Contributor Author

commented Nov 11, 2017

@Hilbrand : it looks like your new code is not taking into account language change on server side (done with Paper UI). I get French whatever my language setting in Paper UI.

@lolodomo

This comment has been minimized.

Copy link
Contributor Author

commented Nov 11, 2017

@Hilbrand : I fixed the problem. It was due to the fact that you were using req.getLocale() which in my case was always returning fr. I don't know where it is set. I replaced req.getLocale() by null to be sure that locale is retrieved from locale provider.
Now it works, switching from French to Dutch or other is correctly taken into account when reopening the dashboard.

@lolodomo

This comment has been minimized.

Copy link
Contributor Author

commented Nov 11, 2017

One another thing, you changed something else, the setting of the lang attribute in the HTML page. I was using locale.getLanguage() while you are using locale.toString(). That means with your code the HTML lang attribute can be set to "de" or "de_DE" for example. Is it what is expected ? Or should we have only "de" ?

@lolodomo

This comment has been minimized.

Copy link
Contributor Author

commented Nov 11, 2017

It looks like it should be only a 2 characters code: https://www.w3schools.com/tags/att_global_lang.asp

@lolodomo

This comment has been minimized.

Copy link
Contributor Author

commented Nov 11, 2017

Problem with ISO code fixed.

Last think is that the page language can be set to a wrong language in case the translation for this language is not provided. For example, if I set the server language to German, the dashboard page is set language "de" while all the text in the page is finally in English.
In my original code, I was handling this case by checking if a translation was provided the server language. But you suppressed this part of the code.

@lolodomo

This comment has been minimized.

Copy link
Contributor Author

commented Nov 11, 2017

I also fixed this problem. The HTML page language is set according to the available translation.

@lolodomo

This comment has been minimized.

Copy link
Contributor Author

commented Nov 11, 2017

I have done a final change to suppress the "locale" parameter which was no more used anywhere (always set to null).

I think we have now ready for a merge.

@Hilbrand

This comment has been minimized.

Copy link
Contributor

commented Nov 11, 2017

It looks like you didn't do a push force , so the code isn't correctly merged.

If you look through the website you linked to you'll see the language code can consist of a language and country code. The way it works if the language+country code isn't present it falls back to the language code. Although using the language+country code will not be compatible or at least make it more complex with your check to determine if the configured language is supported.

Regarding the request.locale. I didn't know the server locale was related to the language set in PaperUI. My approach was that the browser should determine the locale. Remember the user sees this page before ever having seen the paper UI, let alone know how to change the language.

I removed the code to determine the language code if an unsupported locale was given, because I don't know a nice way to determine the locale supported. I found the implementation you used hard to maintain because if that file would move there is no check that will go off to change the string containing the path. I just found it to be too much of a hack to get the supported language for something that isn't a really (big) problem. But if this is generally accepted way, then let it in.

By removing the req locale the need for the second parameter on the BiFunction is gone and it should just be a Function.

@lolodomo

This comment has been minimized.

Copy link
Contributor Author

commented Nov 16, 2017

@Hilbrand : @kaikreuzer does not like the idea to consider by default the browser locale in place of the server locale and he has a valid argument. Please take a look to the issue I opened in ESH repo for Basic UI. As we have no config parameters for the OH dashboard, we have to make a choice: either use the browser locale or the server locale.
Technically, it is only req.getLocale() to replace by null in 2 places. Very easy to change.

Check that a translation is available for the server language before …
…setting the HTML page language with it

Set the language HTML page with an ISO code

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

@lolodomo lolodomo force-pushed the lolodomo:dashboard_i18n branch from 8ddc4e8 to b0bfc37 Nov 16, 2017

@lolodomo

This comment has been minimized.

Copy link
Contributor Author

commented Nov 17, 2017

I was a little wrong, a config file exists for the dashboard, used to define rxternal links, so I could add a config setting for our language stuff to choose between browser or server language.

@kaikreuzer

This comment has been minimized.

Copy link
Member

commented Nov 17, 2017

But the current config is not exposed to the UI (and it's content is afaik not even compatible to it), so adding it there, might not be really good.

And thinking about it a bit longer, maybe it would be better to have this choice (browser or system locale) only once in the regional settings and not individually for every UI. Wdyt?

@kaikreuzer

This comment has been minimized.

Copy link
Member

commented Nov 17, 2017

While it's not the optimal way to determine the language (the optimal way is that the user can set the language on a page) it's commonly used to determine the user language. Since this is a start page I think it's an appropriate approach here.

Some additional comments on that: This is my statement that @lolodomo mentioned above. So I think we are on the same page that defining the locale on the server is the better option (if it was up to me, I would even say the only option we need).
Your point about "starting page" is a good one. Actually, my intension was to set the locale in the runtime to the browser locale on the very first start, just as it is done with the location. This way, the user would be welcomed in the language of his browser, while then being able to (statically) reconfigure his system to some other locale, if he prefers.

@lolodomo

This comment has been minimized.

Copy link
Contributor Author

commented Nov 17, 2017

So we finally forget the idea of an option ?

@lolodomo

This comment has been minimized.

Copy link
Contributor Author

commented Nov 17, 2017

And I drop the use of the browser language keeping "locale" as parameter in case we change our mind in the future ?

Switch back to server locale to get the UI language
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@lolodomo

This comment has been minimized.

Copy link
Contributor Author

commented Nov 18, 2017

Ok, change done to use the server locale for the UI language

@martinvw

This comment has been minimized.

Copy link
Member

commented Nov 18, 2017

Sorry for jumping in univited, but I'm doubting whether @kaikreuzer was correctly interpreted:

Some additional comments on that: This is my statement that @lolodomo mentioned above. So I think we are on the same page that defining the locale on the server is the better option (if it was up to me, I would even say the only option we need).
Your point about "starting page" is a good one. Actually, my intension was to set the locale in the runtime to the browser locale on the very first start, just as it is done with the location. This way, the user would be welcomed in the language of his browser, while then being able to (statically) reconfigure his system to some other locale, if he prefers.

I don't think that 'defining the locale on the server' is the same as 'use the server locale', especially since he also talks about 'my intension was to set the locale in the runtime to the browser locale on the very first start'. So I think that @kaikreuzer suggests to use some method which lives somewhere near/in the I18nProviderImpl. Ideally that should then be set based on the uesrs browser language but that should be done one step earlier just as the location and is not part of this PR.

@kaikreuzer can you validate that I'm not putting any words in your mouth here :-)

@lolodomo

This comment has been minimized.

Copy link
Contributor Author

commented Nov 18, 2017

I have the same understanding.
So we are OK with this PR but a next enhancement would be to have in the initial setup page an optional way to set the server locale from the browser language. That is not the purpose of the current PR which was only here to translate the current dashboard in different languages.

@kaikreuzer

This comment has been minimized.

Copy link
Member

commented Nov 18, 2017

Good to see that we now seem to have reached some common understanding - thanks for your patience, @lolodomo!

@kaikreuzer kaikreuzer merged commit 3fa30c1 into openhab:master Nov 18, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kaikreuzer

This comment has been minimized.

Copy link
Member

commented Nov 18, 2017

After the merge I'd like to set up crowdin for this repository. Do I have your permission / do you want to do that yourself / other opinions?

@ThomDietrich I have created https://crowdin.com/project/openhab and requested an open source sponsorship from them (point 5 says this has to be done by the project lead). Feel free to join this project and I will grant you all available rights to it :-)

@kaikreuzer

This comment has been minimized.

Copy link
Member

commented Nov 18, 2017

Reminder to self: Never blindly merge stuff, always do the testing first:

screen shot 2017-11-19 at 00 06 07

@kaikreuzer

This comment has been minimized.

Copy link
Member

commented Nov 18, 2017

Was luckily just a very minor thing: The property files were not added in the build.properties.
Fixed with #233.

@lolodomo lolodomo deleted the lolodomo:dashboard_i18n branch Nov 19, 2017

@ThomDietrich

This comment has been minimized.

Copy link
Member

commented Nov 21, 2017

@kaikreuzer I've just joined the project.
One question though. Shouldn't the project have a more unique name, e.g. the identical name as the repository it is supposed to reflect? (GitHub integration is limited to one GitHub repo <-> one Crowdin project)

See: https://crowdin.com/projects?q=openhab#advanced-search

@ThomDietrich

This comment has been minimized.

Copy link
Member

commented Dec 4, 2017

@kaikreuzer friendly ping

@kaikreuzer

This comment has been minimized.

Copy link
Member

commented Dec 4, 2017

GitHub integration is limited to one GitHub repo <-> one Crowdin project

Oops, I wasn't aware of that - that's not very convenient :-(
So shall I rename it to openhab-core then?

@ThomDietrich

This comment has been minimized.

Copy link
Member

commented Dec 4, 2017

Yes please. :)

Edit: When changing the name in settings it only effects the presentation, not the internal name. Better recreate it.

@kaikreuzer

This comment has been minimized.

Copy link
Member

commented Dec 4, 2017

There you go: https://crowdin.com/project/openhab
Btw, you are now admin on the project, feel free to do with it whatever you think is necessary :-)

@ThomDietrich

This comment has been minimized.

Copy link
Member

commented Dec 4, 2017

Hmm I guess my edit came too late. You wanna stay with "openhab" in the URL?

@kaikreuzer

This comment has been minimized.

Copy link
Member

commented Dec 4, 2017

I didn't find a way to change the url - but the project name is correctly updated, see https://crowdin.com/projects?q=openhab#advanced-search.
The only possibility is probably to delete the project and create a new one. But imho we can also keep the url, if that's all.

pfink added a commit to pfink/openhab-core that referenced this pull request Dec 9, 2017

Dashboard internationalization (openhab#217)
French version included
Fixes openhab#214

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

@kaikreuzer kaikreuzer added this to the 2.2 milestone Dec 15, 2017

@martinvw martinvw referenced this pull request Mar 13, 2018

Merged

New Crowdin translations #280

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.