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

Add i18n-maven-plugin to make internationalization easier #2544

Merged
merged 10 commits into from
Dec 1, 2021

Conversation

wborn
Copy link
Member

@wborn wborn commented Oct 28, 2021

This plugin simplifies generating the default translation .properties files from the add-on XML information files.

It reuses the same XStream parsing classes that are used by openhab-core for parsing the binding/config/thing XML files.
It will also keep any existing default translations already present in property files for translations using @text/.
Furthermore it will nicely group and sort the translations.

After building this Maven plugin you can use it on add-ons using:

mvn org.openhab.core.tools:i18n-maven-plugin:3.2.0-SNAPSHOT:generate-default-translations

This commit shows what it generates when run on all add-ons.

@wborn wborn added the i18n/l10n Internationalization/Localization label Oct 28, 2021
@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/language-support-question-regional-settings-language-vs-browser-language/128010/2

@lolodomo
Copy link
Contributor

lolodomo commented Nov 1, 2021

Trying with the vigicrues binding, I got these channels types:

channel-type.vigicrues.alert-icon.label = Pictogram
channel-type.vigicrues.alert-icon.description = Official pictogram associated to alert level.
channel-type.vigicrues.alert-level.label = Alert
channel-type.vigicrues.alert-level.state.option.0 = Green
channel-type.vigicrues.alert-level.state.option.1 = Yellow
channel-type.vigicrues.alert-level.state.option.2 = Orange
channel-type.vigicrues.alert-level.state.option.3 = Red
channel-type.vigicrues.comment.label = Comment
channel-type.vigicrues.flow.label = Current Flow
channel-type.vigicrues.gauge.label = Relative Measure
channel-type.vigicrues.height.label = Height
channel-type.vigicrues.height.description = Water level in the river
channel-type.vigicrues.observation-time.label = Observation Time
channel-type.vigicrues.observation-time.description = Observation date and time

But in the original french properties file created by @clinique, I have other entries like

channel-type.vigicrues.relative-flow.label = Débit relatif
channel-type.vigicrues.relative-flow.description = Débit relatif par rapport aux crues historiques.
channel-type.vigicrues.relative-height.label = Hauteur relative
channel-type.vigicrues.relative-height.description = Hauteur relative par rapport aux crues historiques.
channel-type.vigicrues.short-comment.label = Situation
channel-type.vigicrues.short-comment.description = Bref descriptif de la situation.

Here is how are defined these channel in the thing file:

		<channels>
			<channel id="height" typeId="height"/>
			<channel id="relative-height" typeId="gauge">
				<label>Relative Height</label>
				<description>Current height toward historical floods.</description>
			</channel>
			<channel id="flow" typeId="flow"/>
			<channel id="relative-flow" typeId="gauge">
				<label>Relative Flow</label>
				<description>Current flow toward historic floods.</description>
			</channel>
			<channel id="alert" typeId="alert-level"/>
			<channel id="alert-icon" typeId="alert-icon"/>
			<channel id="short-comment" typeId="comment">
				<label>Short description</label>
			</channel>
			<channel id="comment" typeId="comment"/>
			<channel id="observation-time" typeId="observation-time"/>
		</channels>

I am asking myself if there is not a bug in your tool which should create an entry for each channel, not only for each channel type (except if the channel type is a system channel type) ?
In case the bug is confirmed, we will have to check again all bindings for which we use the tool to build the default properties file.

@lolodomo
Copy link
Contributor

lolodomo commented Nov 1, 2021

Finally, I am not sure there is a bug. This looks more like a missing feature or maybe I don't know what path to use for example to translate the label of the short-comment channel. channel-type.vigicrues.short-comment.label is not working. channel.vigicrues.short-comment.label is not working. @cweitkamp : do you know what path should be used in this case (label or description defined on the channel itself) ?
If this is not implemented, the solution would be to use @text.

@lolodomo
Copy link
Contributor

lolodomo commented Nov 1, 2021

I finally found the right key to use for channels. For example: thing-type.vigicrues.station.channel.short-comment.label and thing-type.vigicrues.station.channel.short-comment.description

Ideally it should be considered by your i18n maven plugin.

@wborn
Copy link
Member Author

wborn commented Nov 1, 2021

I think it should be possible. I remember I had a look at the astro binding where labels/descriptions are overriden for certain channel group types. But I probably forgot to add something similar for channel types. 😊

@wborn
Copy link
Member Author

wborn commented Nov 1, 2021

I think this is the logic that takes care of it for channel group types with overriden labels/descriptions:

https://github.com/openhab/openhab-core/pull/2544/files#diff-b7101042a9b1b8825f910f519872ac389701fb78eaf34c86116933f2b9d30f83R206-R220

@lolodomo
Copy link
Contributor

lolodomo commented Nov 1, 2021

I checked the recent default properties file generated and I think we have to fix those for the following bindings: openuv, weathercompany, kodi and probably awmfritz. So you can test an update of your tool on one of these bindings.

@wborn
Copy link
Member Author

wborn commented Nov 7, 2021

It should now also generate default translations for overridden channels in thing types @lolodomo.

Furthermore it now also generates translations for bundles that only have config descriptions, like automation/io/persistence/transform/voice or core bundles. 🙂

@lolodomo
Copy link
Contributor

lolodomo commented Nov 7, 2021

It should now also generate default translations for overridden channels in thing types

Just tested it with the openuv bidnign and it works.
I am going to fix few bindings.

@lolodomo
Copy link
Contributor

lolodomo commented Nov 7, 2021

One option would be to run your tool on all bindings without any properties file. This will open these bindings to translations in Crowdin. This will not be a full internationalisation of these bindings but probably an internationalisation at around 90% or even 95%. What could be missing (because it requires specific code) is thing status message, discovery result label or hardcoded English string value for channel state. Another risk is to get not optimized properties files with duplicated strings.

For bindings having already properties files, we have to take care and do it manually with precaution.

WDYT @wborn ?

@wborn
Copy link
Member Author

wborn commented Nov 7, 2021

This will not be a full internationalisation of these bindings but probably an internationalisation at around 90% or even 95%.

Yes I also think the generated properties files will be a good start and it allows users to start translating while devs add the last remaining %. Add-ons that no longer have a maintainer would also greatly benefit from this.

My idea was to first get a feeling on how well it works and iron out the bugs before mass generating them all. I think it already works pretty well.

But even with all bugs ironed out it will not remove any duplicate strings. However being able to translate something multiple times is better than not being able to translate strings at all. 😉

@ghys
Copy link
Member

ghys commented Nov 7, 2021

This will not be a full internationalisation of these bindings but probably an internationalisation at around 90% or even 95%.

If it is any incentive, I believe it could also trigger the i18n of the admin part of the UI as well; currently while all the user areas are translatable (and translated), most the admin pages still aren't because I figured a significant part of those are parameter sheets that would still appear in English, so IMO it is not worth the effort at this time (I mean the effort of replacing all these hardcoded strings, a tedious and thankless job... translating strings is another matter but past translation campaign experiences with Crowdin gives much confidence the community will step up).
If the admin needs to understand English to figure out the parameters, then the other parts of the admin UI should be no problem either so there's little point in translating it. But if we can achieve a good i18n "coverage" in most key languages across the most popular bindings then it becomes more appealing to go all the way.

@cweitkamp
Copy link
Contributor

I added a missing feature to translate CommandOptions in wborn#4.

@cweitkamp cweitkamp added rebuild Triggers the Jenkins PR build and removed rebuild Triggers the Jenkins PR build labels Nov 12, 2021
@jimtng
Copy link
Contributor

jimtng commented Nov 17, 2021

mvn org.openhab.core.tools:i18n-maven-plugin:3.2.0-SNAPSHOT:generate-default-translations

I used this handy tool recently. Is there a way to make this command a lot simpler, e.g. mvn tools:generate-translations or something similar?

@wborn
Copy link
Member Author

wborn commented Nov 17, 2021

I think when it is merged and added as plugin dependency you should also be able to use it with:

mvn i18n:generate-default-translations

See also:

Copy link
Contributor

@cweitkamp cweitkamp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your contribution is in a good shape @wborn . Do you consider to change this PRs type from draft to ready to be merged soon?


if (stateDescription.getPattern() != null) {
String pattern = stateDescription.getPattern();
if (pattern != null && pattern.contains("%1$t")) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you add this check (pattern.contains("%1$t")) by intention to filter every other pattern except date time related ones? I was looking into this because I am missing pattern for channel types from feed binding for translation. It uses "%tc" as pattern. I personally would be fine with this and change it for feed binding to something else. Or should we extend it here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

@wborn wborn Nov 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the filter because when it is not applied it will also start adding the patterns for the following states to .properties files:

  • <state pattern="%s">
  • <state pattern="%.2f" readOnly="true"/>
  • <state pattern="%.1f %unit%"/>

I doubt these would often result in other translations.

Although later I realised that any string that has a space in it, is also a potential translation candidate for any language that is right-to-left instead of left-to-right. 🙂

@wborn
Copy link
Member Author

wborn commented Nov 21, 2021

Do you consider to change this PRs type from draft to ready to be merged soon?

Adding some unit tests for this plugin is still on my todo list. Also I want to add a plugin config option to skip generating default translations when the .properties file already exists. That would simplify creating a PR that adds default translations to all add-ons that do not have these already.

This plugin simplifies generating the default translation .properties files from the add-on XML information files.

It reuses the same XStream parsing classes that are used by openhab-core for parsing the binding/config/thing XML files.
It will also keep any existing default translations already present in property files for translations using `@text/`.
Furthermore it will nicely group and sort the translations.

After building this Maven plugin you can use it on add-ons using:

`mvn org.openhab.core.tools:i18n-maven-plugin:3.2.0-SNAPSHOT:generate-default-translations`

Signed-off-by: Wouter Born <[email protected]>
Signed-off-by: Wouter Born <[email protected]>
cweitkamp and others added 5 commits November 27, 2021 09:59
Signed-off-by: Wouter Born <[email protected]>
To only create missing default translations files execute:

mvn org.openhab.core.tools:i18n-maven-plugin:3.2.0-SNAPSHOT:generate-default-translations -Di18n.generation.mode=ADD_MISSING_FILES

Or to replace any existing content and only use the generated content use:

mvn org.openhab.core.tools:i18n-maven-plugin:3.2.0-SNAPSHOT:generate-default-translations -Di18n.generation.mode=REGENERATE_FILES

Signed-off-by: Wouter Born <[email protected]>
Signed-off-by: Wouter Born <[email protected]>
@wborn wborn marked this pull request as ready for review November 27, 2021 20:03
@wborn wborn requested a review from a team as a code owner November 27, 2021 20:03
Copy link
Contributor

@cweitkamp cweitkamp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you very much

This will help developers a lot and safes much time regarding creation of translation properties. I bet you will submit a bunch of documentation later 😉 .

Let's go!

@cweitkamp
Copy link
Contributor

Am I allowed to merge this as I submitted code - not much, but I did - for it?

//CC @openhab/core-maintainers

@kaikreuzer
Copy link
Member

Am I allowed to merge this as I submitted code - not much, but I did - for it?

Fine with me, go ahead!

@cweitkamp cweitkamp merged commit bb3224a into openhab:main Dec 1, 2021
@cweitkamp cweitkamp added this to the 3.2 milestone Dec 1, 2021
@wborn wborn deleted the i18n-plugin branch December 1, 2021 18:46
wborn added a commit to wborn/openhab-core that referenced this pull request Dec 1, 2021
When the plugin dependency is managed you can also use the plugin without adding GAV parameters to commands.

E.g. it allows for using it with:

```
mvn i18n:generate-default-translations
```

Related to openhab#2544

Signed-off-by: Wouter Born <[email protected]>
cweitkamp pushed a commit that referenced this pull request Dec 2, 2021
When the plugin dependency is managed you can also use the plugin without adding GAV parameters to commands.

E.g. it allows for using it with:

```
mvn i18n:generate-default-translations
```

Related to #2544

Signed-off-by: Wouter Born <[email protected]>
splatch pushed a commit to ConnectorIO/copybara-hab-core that referenced this pull request Jul 12, 2023
* Add i18n-maven-plugin to make internationalization easier

This plugin simplifies generating the default translation .properties files from the add-on XML information files.

It reuses the same XStream parsing classes that are used by openhab-core for parsing the binding/config/thing XML files.
It will also keep any existing default translations already present in property files for translations using `@text/`.
Furthermore it will nicely group and sort the translations.

After building this Maven plugin you can use it on add-ons using:

`mvn org.openhab.core.tools:i18n-maven-plugin:3.2.0-SNAPSHOT:generate-default-translations`

Signed-off-by: Wouter Born <[email protected]>
GitOrigin-RevId: bb3224a
@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/openhab-4-translation-fallback-in-main-ui/148331/2

@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/is-there-a-translation-i18n-util-for-general-use/152229/5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
i18n/l10n Internationalization/Localization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants