Dashboard internationalization

[lolodomo]

French version included

Fixes #214

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

[lolodomo]

Noone to review this one ?

[martinvw]

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]

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]

Can this be merged ?

[martinvw]

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

[lolodomo]

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

[kaikreuzer]

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]

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 !

[ThomDietrich]

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

[lolodomo]

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]

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

• https://github.com/marketplace/category/localization
• https://github.com/works-with/category/localization

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

[Hilbrand]

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]

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.

[Hilbrand]

PR created: lolodomo#1

[lolodomo]

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

[lolodomo]

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]

@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]

@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]

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]

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

[lolodomo]

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]

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

[lolodomo]

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]

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]

Thinking more about it, maybe that was a wrong idea to consider the server locale to set the UI language.

Can you explain who and how is set req locale ? Is it the WEB browser ?

Your code was using sometimes req locale sometimes null and so the server locale.

I think we have to decide to never use the server locale or to always use it.

[ThomDietrich]

@kaikreuzer 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?

[lolodomo]

@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.

[lolodomo]

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]

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]

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]

So we finally forget the idea of an option ?

[lolodomo]

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

[lolodomo]

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

[martinvw]

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]

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]

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

[kaikreuzer]

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]

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

[kaikreuzer]

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

[ThomDietrich]

@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]

@kaikreuzer friendly ping

[kaikreuzer]

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]

Yes please. :)

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

[kaikreuzer]

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]

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

[kaikreuzer]

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.