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

Config parameter: Change inferred i18n key for add-ons + alternative key #4305

Merged
merged 1 commit into from
Aug 4, 2024

Conversation

lolodomo
Copy link
Contributor

@lolodomo lolodomo commented Jul 10, 2024

Fix openhab/openhab-webui#2641

The i18n key for add-on configuration parameter should now start with addon.config.<param>.
To maintain compatibility with hundreds of existing translations, an alternative key starting with <type>.config.<param> is still accepted for an add-on parameter

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

@lolodomo lolodomo requested a review from a team as a code owner July 10, 2024 07:09
Copy link
Member

@J-N-K J-N-K left a comment

Choose a reason for hiding this comment

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

Why is there a mismatch between the actual scheme name and the required scheme name?

@lolodomo
Copy link
Contributor Author

lolodomo commented Jul 10, 2024

IICR, this was introduced when you introduced the addon.xml file with new features. In OH 4.0 I believe

@lolodomo lolodomo changed the title Fix build of i18n keys for binding parameters Fix infer of i18n keys for binding parameters Jul 10, 2024
@J-N-K
Copy link
Member

J-N-K commented Jul 10, 2024

But why don't we fix the config description scheme naming if it should be addon instead of binding?

@lolodomo
Copy link
Contributor Author

lolodomo commented Jul 10, 2024

But why don't we fix the config description scheme naming if it should be addon instead of binding?

Yes, that looks like a good idea.

It is probably coming from the XML tag named "type" in all addon.xml. To be confirmed.
So you will suggest to replace in all addon.xml the value "binding" by "addon" ?
It should also fix the issue, I believe. It is just more work, around 500 files to change (one file for each binding) !
And in my mind, add-on is a generic word for binding, voice, transformation. Using it for bindings in the addon.xml file is not 100% logic.

@lolodomo
Copy link
Contributor Author

The other option would be to update the i18n keys to use "binding.config.xxx" instead of *addon config.xxx". But that would be even more work and a disaster as it would require to translate again all bindings parameters.
I would really like to avoid that option.

@lolodomo
Copy link
Contributor Author

lolodomo commented Jul 10, 2024

I could also rather fix where the description uri is built, rather than where the i18n key is inferred from it.
I mean we could build addon:netatmo instead of binding:netatmo in the REST addon API. Then the config_description API should return properly translated parameters.

@lolodomo
Copy link
Contributor Author

lolodomo commented Jul 10, 2024

I am counting 85 entries in english properties files having "addon.config.xxx". It concerns 21 bindings.
If I include the other languages, I count 182 entries, meaning 97 translations.

So another solution, probably the cleaner, would be to fix manually the 85 wrong entries and then start again their translations in Crowdin.

Note that I found wrong entries also for two persistence services (entries should probably start with "persistence.config.xxx"):

org.openhab.persistence.inmemory/src/main/resources/OH-INF/i18n/inmemory.properties:addon.config.inmemory.maxEntries.label = Maximum Entries
org.openhab.persistence.inmemory/src/main/resources/OH-INF/i18n/inmemory.properties:addon.config.inmemory.maxEntries.description = The maximum number of values stored for each item (0 = infinite).
org.openhab.persistence.mongodb/src/main/resources/OH-INF/i18n/mongodb.properties:addon.config.mongodb.collection.label = Collection
org.openhab.persistence.mongodb/src/main/resources/OH-INF/i18n/mongodb.properties:addon.config.mongodb.database.label = Database Name
org.openhab.persistence.mongodb/src/main/resources/OH-INF/i18n/mongodb.properties:addon.config.mongodb.url.label = MongoDB connection URL

@J-N-K
Copy link
Member

J-N-K commented Jul 10, 2024

IMO addon.config is the correct prefix for all. Why should it be binding or persistence? Can't we rename the keys on Crowdin?

@lolodomo
Copy link
Contributor Author

IMO addon.config is the correct prefix for all. Why should it be binding or persistence?

If we do that, we have probably less bindings to update, only io/voice/persistence/transform bindings having parameters.

Can't we rename the keys on Crowdin?

We can of course change the keys but then you have to provide a new translation.
There is an option to import the translations but it could override in the case of uncomplete translations.

@lolodomo
Copy link
Contributor Author

IMO addon.config is the correct prefix for all. Why should it be binding or persistence?

If we do that, we have probably less bindings to update, only io/voice/persistence/transform bindings having parameters.

In fact, it is largely more entries to change:
114 for persistence
262 for voice
...

So it would be easier to fix entries for the 19 standard bindings and 2 persistence bindings.

@lolodomo
Copy link
Contributor Author

Or we go with my proposed workaround and we don't have to update bindings. Only 2 lines changed in the core framework.

PS: the 2 persistence services should be fixed anyway.

@lolodomo
Copy link
Contributor Author

lolodomo commented Jul 10, 2024

I also found one binding using "binding.config.xxx":

org.openhab.binding.mqtt.homeassistant/src/main/resources/OH-INF/i18n/mqtt.properties:binding.config.mqtt.homeassistant-status.label = Publish Online Status
org.openhab.binding.mqtt.homeassistant/src/main/resources/OH-INF/i18n/mqtt.properties:binding.config.mqtt.homeassistant-status.description = Publish <tt>online</tt> to <tt>homeassistant/status</tt> when discovering Home Assistant things in order to trigger devices to publish up-to-date discovery information. If you also run Home Assistant <i>and</i> other services that depend on knowing if Home Assistant is not running, then it's possible for those services to be out-of-sync with the actual status of Home Assistant, and you may want to disable this.

@lolodomo
Copy link
Contributor Author

lolodomo commented Jul 10, 2024

If my proposal is accepted, I will have to take a look to failing integration tests.

@lolodomo
Copy link
Contributor Author

I am going to enhance my proposal to support both keys, that is <type>.config.<param> and addon.config.<param>.

@lolodomo lolodomo force-pushed the binding_parameters_i18n branch from 90358aa to 9a2b320 Compare July 10, 2024 20:17
@lolodomo lolodomo changed the title Fix infer of i18n keys for binding parameters Supports alternative key starting with addon for certain config parameters Jul 10, 2024
@lolodomo lolodomo force-pushed the binding_parameters_i18n branch from 9a2b320 to 811e7b0 Compare July 10, 2024 20:21
@lolodomo
Copy link
Contributor Author

lolodomo commented Jul 10, 2024

@J-N-K : please look at the last version, I believe it is a good compromise supporting both i18n keys. No changes in add-ons should then be required.

@lolodomo lolodomo force-pushed the binding_parameters_i18n branch from 811e7b0 to 776dfa3 Compare July 10, 2024 20:31
@lolodomo lolodomo changed the title Supports alternative key starting with addon for certain config parameters Supports alternative i18n key starting with addon for certain config parameters Jul 10, 2024
@lolodomo
Copy link
Contributor Author

Close)re-open to trigger a rebuild

@lolodomo lolodomo closed this Jul 12, 2024
@lolodomo
Copy link
Contributor Author

Go

@lolodomo lolodomo reopened this Jul 12, 2024
@lolodomo
Copy link
Contributor Author

lolodomo commented Jul 15, 2024

I am going to reverse the logic, making addon.config.xxx the normal key for add-ons and keeping the key <type>.config.xxx as alternative.

@lolodomo lolodomo force-pushed the binding_parameters_i18n branch from 776dfa3 to 7bee99d Compare July 15, 2024 14:05
@lolodomo lolodomo changed the title Supports alternative i18n key starting with addon for certain config parameters Config parameter: Change inferred i18n key for add-ons + alternative i18n key Jul 15, 2024
@lolodomo lolodomo force-pushed the binding_parameters_i18n branch from 7bee99d to aa12beb Compare July 15, 2024 14:07
@lolodomo lolodomo changed the title Config parameter: Change inferred i18n key for add-ons + alternative i18n key Config parameter: Change inferred i18n key for add-ons + alternative key Jul 15, 2024
@lolodomo lolodomo force-pushed the binding_parameters_i18n branch 2 times, most recently from e127b01 to af88c55 Compare July 15, 2024 14:13
Fix openhab/openhab-webui#2641

The i18n key for add-on configuration parameter should now start with addon.config.<param>.
To maintain compatibility with hundreds of existing translations, an alternative key starting with <type>.config.<param> is still accepted for an add-on parameter

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@lolodomo lolodomo force-pushed the binding_parameters_i18n branch from af88c55 to 22f8ac7 Compare July 15, 2024 14:17
@lolodomo
Copy link
Contributor Author

Ok, finished. I changed the title and the content of the initial message.

Copy link
Member

@J-N-K J-N-K left a comment

Choose a reason for hiding this comment

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

LGTM.

@J-N-K J-N-K added the bug An unexpected problem or unintended behavior of the Core label Aug 4, 2024
@J-N-K J-N-K added this to the 4.3 milestone Aug 4, 2024
@J-N-K J-N-K merged commit 289f063 into openhab:main Aug 4, 2024
5 checks passed
@lolodomo lolodomo deleted the binding_parameters_i18n branch August 4, 2024 11:15
@lolodomo
Copy link
Contributor Author

lolodomo commented Aug 5, 2024

@J-N-K @kaikreuzer : please backport to branch 4.2.x before building 4.2.1.

kaikreuzer pushed a commit to kaikreuzer/openhab-core that referenced this pull request Aug 5, 2024
…key (openhab#4305)

Fix openhab/openhab-webui#2641

The i18n key for add-on configuration parameter should now start with addon.config.<param>.
To maintain compatibility with hundreds of existing translations, an alternative key starting with <type>.config.<param> is still accepted for an add-on parameter

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@kaikreuzer
Copy link
Member

@lolodomo Done!

@kaikreuzer kaikreuzer added the patch A PR that has been cherry-picked to a patch release branch label Aug 5, 2024
wborn pushed a commit that referenced this pull request Sep 7, 2024
…key (#4305)

Fix openhab/openhab-webui#2641

The i18n key for add-on configuration parameter should now start with addon.config.<param>.
To maintain compatibility with hundreds of existing translations, an alternative key starting with <type>.config.<param> is still accepted for an add-on parameter

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@wborn
Copy link
Member

wborn commented Sep 7, 2024

Now it is cherry-picked to the OH repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An unexpected problem or unintended behavior of the Core patch A PR that has been cherry-picked to a patch release branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Binding settings not translated in Main UI
4 participants