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

Update default translations #15420

Closed

Conversation

florian-h05
Copy link
Contributor

@florian-h05 florian-h05 commented Aug 13, 2023

@openhab/add-ons-maintainers While working on #15378, I noticed that there are many add-ons with out-dated default translations.

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
@wborn
Copy link
Member

wborn commented Aug 14, 2023

I noticed that there are many add-ons with out-dated default translations.

Previously the plug-in was only used to generate files for add-ons that did not have any at all (#11760).
It was still new back then and not all issues had been ironed out, so I didn't run it on everything like you did.
It seems to do a pretty good job now though. 🙂

@florian-h05
Copy link
Contributor Author

@lsiepel

Did you only regenerate the default translation from the xml files? Or did you alse check custom translations on other places in the binding that are not detected bij the i18n generation?

I only ran the i18n plugin, did not check anything manually.

The i18n plugin does not detect all placeholders.
This is removed, but used here: https://github.com/openhab/openhab-addons/blob/main/bundles/org.openhab.binding.livisismarthome/src/main/java/org/openhab/binding/livisismarthome/internal/handler/LivisiBridgeHandler.java#L170

There are certainly more bindings that have similar issues.

Right, haven't thought about this yet. Probably it is better to close this PR and instead ask the binding maintainers from the code owners file to update their bindings? I don't have the time to check all placeholders that are removed ...

Copy link
Contributor

@soenkekueper soenkekueper left a comment

Choose a reason for hiding this comment

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

LGTM deutschebahn

@lsiepel
Copy link
Contributor

lsiepel commented Aug 14, 2023

@lsiepel

Did you only regenerate the default translation from the xml files? Or did you alse check custom translations on other places in the binding that are not detected bij the i18n generation?

I only ran the i18n plugin, did not check anything manually.

The i18n plugin does not detect all placeholders.
This is removed, but used here: https://github.com/openhab/openhab-addons/blob/main/bundles/org.openhab.binding.livisismarthome/src/main/java/org/openhab/binding/livisismarthome/internal/handler/LivisiBridgeHandler.java#L170
There are certainly more bindings that have similar issues.

Right, haven't thought about this yet. Probably it is better to close this PR and instead ask the binding maintainers from the code owners file to update their bindings? I don't have the time to check all placeholders that are removed ...

I have checked 80, might check the rest too. One might allways slip, but that can allways be fixed easily. Creating 147 separate PR's is also a huge effort and is no garantuee of similar issues. So i suggest to go forward with this, but i am not a maintainer :-)

Copy link

@gdolfen gdolfen left a comment

Choose a reason for hiding this comment

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

LGTM for enigma2 binding

@andrewfg
Copy link
Contributor

only ran the i18n plugin, did not check anything manually.

Perhaps do a search for @text/* ??

Copy link
Contributor

@lsiepel lsiepel left a comment

Choose a reason for hiding this comment

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

2nd pass: checked remaining 65 bindings :-/
Did not check each lines, just scanned them and i think there are some minor adjustements needed.

@@ -1,590 +1,72 @@
# add-on
Copy link
Contributor

Choose a reason for hiding this comment

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

@markus7017 is doing an extensive refactoring of this binding, maybe he can confirm that this is ok? Removing ~600 lines and only adding ~75 lines of translation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case I will leave the shelly.properties unchanged and let @markus7017 manage the translations.

Copy link
Contributor

Choose a reason for hiding this comment

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

We are just finalizing the latest PR. Anything open from this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

@florian-h05 I always manage translations incl. using crowdin
There are still some strings in the XML like symbolic thing name, which I didn't move to the resource file. I think it doesn't make sense to have the same string in EN+DE, but of course I could do that.

I didn't got what's required on top, is there anything specific from this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As you always manage translations, I will leave those translations to you.
I guess I will close this PR now as it seems no one is willing to review it, so do your Shelly PR as you want to.

I think it doesn't make sense to have the same string in EN+DE, but of course I could do that.

Agreed. I haven’t checked these details when updating the translations, because doing that for all bindings would be very time-expensive job.


# thing types

thing-type.satel.atd-100.label = ATD-100
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to put device model symbol to translation file?

Copy link
Contributor Author

@florian-h05 florian-h05 Aug 17, 2023

Choose a reason for hiding this comment

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

In this case it makes sense to leave because the descriptions should be translated IMHO.

- Addressed review.
- I have read through all changes files and fixed every bug I noticed.
- I decided to leave out persistence, because I have an extra PR for that.

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Copy link
Contributor Author

@florian-h05 florian-h05 left a comment

Choose a reason for hiding this comment

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

I have read through all changed files, addressed the reviews and fixed my own findings.


# thing types

thing-type.satel.atd-100.label = ATD-100
Copy link
Contributor Author

@florian-h05 florian-h05 Aug 17, 2023

Choose a reason for hiding this comment

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

In this case it makes sense to leave because the descriptions should be translated IMHO.

@@ -1,590 +1,72 @@
# add-on
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case I will leave the shelly.properties unchanged and let @markus7017 manage the translations.

@@ -12,30 +12,6 @@ thing-type.nzwateralerts.wateralert.description = Water Alert Levels for New Zea

thing-type.config.nzwateralerts.wateralert.location.label = Location
thing-type.config.nzwateralerts.wateralert.location.description = The location to get the Water Alert level for.
thing-type.config.nzwateralerts.wateralert.location.option.bewaterwise:whangarei:breambay = Bream Bay
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI: I removed the locations here, no need to have them translated.

@florian-h05
Copy link
Contributor Author

@lsiepel @openhab/add-ons-maintainers IMO this is ready now.

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Copy link
Member

@druciak druciak left a comment

Choose a reason for hiding this comment

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

Satel looks good.

Copy link
Contributor

@lsiepel lsiepel left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks!

addon.mimictts.name = Mimic Text-to-Speech
addon.mimictts.description = The Mimic Text-to-Speech (TTS) service uses the offline open source Text-to-Speech engine designed by Mycroft A.I.

# add-on
Copy link
Contributor

Choose a reason for hiding this comment

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

Hello,
I think this second "# add-on" line is a typo ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is generated by the plugin, and also exists for other files like jsscripting.properties.

Copy link
Contributor

@stefan-hoehn stefan-hoehn left a comment

Choose a reason for hiding this comment

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

LGTM for Nanoleaf

Copy link

@Wolfgang1966 Wolfgang1966 left a comment

Choose a reason for hiding this comment

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

LGTM for aWATTar and tacmi

Copy link
Contributor

@weymann weymann left a comment

Choose a reason for hiding this comment

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

mercedes & e3dc lgtm

Copy link
Contributor

@bern77 bern77 left a comment

Choose a reason for hiding this comment

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

Thanks for your initiative!
I've added some additional translations to completely match the German version of the file.
Also, I believe the file has to be named differently.
(see my 2 commits)

Otherwise - LGTM 😊

Copy link
Contributor

@ne0h ne0h left a comment

Choose a reason for hiding this comment

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

LGTM for KVV

@lsiepel
Copy link
Contributor

lsiepel commented Aug 24, 2023

Thanks for your initiative! I've added some additional translations to completely match the German version of the file. Also, I believe the file has to be named differently. (see my 2 commits)

Otherwise - LGTM 😊

Both of your commits, lack DCO.

Besides that, if the i18n plugin generates wrong filename for 'sub' bindings like Modbus and Bluetooth, shouldn't those bindings also be fixed?

Then i look ad at your other commit where you added some translations, i noticed that here the modbuis without subbinding name is missing: example:
channel-group-type.modbus.co2Control.label

Only difference is the binding name. This looks to be the same problem (wrong binding name) as with the file name, possibly a i18n plugin bug? Or am i mistaken?
Edit: i guess i'm wrong. Anyway, why didn't the i18n tool pick up these additional translations?

Unless this can be fixed easily, i suggest leaving the translations for those affected bindings out of this PR. Just to speed up merging this PR, as the number of bindings/files involved, this can get a merge conflict galore :-)

@bern77
Copy link
Contributor

bern77 commented Aug 25, 2023

Thanks for your initiative! I've added some additional translations to completely match the German version of the file. Also, I believe the file has to be named differently. (see my 2 commits)
Otherwise - LGTM 😊

Both of your commits, lack DCO.

I made the edits directly in GitHub. The graced me with the learning, into which of the two presented text boxes I should copy the DCO 🙄

Besides that, if the i18n plugin generates wrong filename for 'sub' bindings like Modbus and Bluetooth, shouldn't those bindings also be fixed?

Then i look ad at your other commit where you added some translations, i noticed that here the modbuis without subbinding name is missing: example: channel-group-type.modbus.co2Control.label

Only difference is the binding name. This looks to be the same problem (wrong binding name) as with the file name, possibly a i18n plugin bug? Or am i mistaken? Edit: i guess i'm wrong. Anyway, why didn't the i18n tool pick up these additional translations?

TBH I've never used the i18n plugin and created the translation files manually... so I'm not really sure where or how to tackle this.

Unless this can be fixed easily, i suggest leaving the translations for those affected bindings out of this PR. Just to speed up merging this PR, as the number of bindings/files involved, this can get a merge conflict galore :-)

I'm also fine with that and could simply add the translations in an own PR. No intention to make this more complex than necessary!

Copy link
Contributor

@stefan-hoehn stefan-hoehn left a comment

Choose a reason for hiding this comment

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

Nanoleaf LGTM

Copy link
Contributor

@marcelrv marcelrv left a comment

Choose a reason for hiding this comment

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

thanks!
LGTM for miio

@florian-h05
Copy link
Contributor Author

Closing this PR as it I don’t expect it to get reviewed and it meanwhile might be outdated as well. In case someone agrees to review this, I can reopen.

@lsiepel
Copy link
Contributor

lsiepel commented Nov 13, 2023

Maybe it is better to seperate this to smaller chunks. Too bad for the efforts allready made.

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

Successfully merging this pull request may close these issues.