Improve translation loading #3826

Holzhaus · 2021-05-04T13:14:38Z

While using developer mode, I saw some suspicious log messages regarding failed translations when using the en locale. When looking at the code I realized that mixxx doesn't even attempt to load them (which makes sense) but the error message is misleading.

I cleaned up the translation handling a bit, added some error messages and made them more meaningful. I based this on 2.3 but If you want to postpone this for 2.4 we can do that as well.

daschuer

2.3 is fine for me. I have left some comments.

src/util/translations.h

daschuer · 2021-05-04T17:02:41Z

src/util/translations.h

-            QLocale::setDefault(QLocale(systemLocale));
-        } else {
-            QLocale::setDefault(QLocale(userLocale));
+        QLocale locale = QLocale();


This assignment is redundant.
Please add a comment that we investigate the default value.

Please add a comment that we investigate the default value.

What do you mean?

From the Qt docs:

Constructs a QLocale object initialized with the default locale. If no default locale was set using setDefault(), this locale will be the same as the one returned by system().

I added that as a comment.

Holzhaus · 2021-05-04T21:06:11Z

src/util/translations.h

        // Load Qt translations for this locale from the Mixxx translations
        // folder.


Is this even relevant anymore? I don't think there exist...

$ ls -l ../res/translations/ | grep qt | wc -l 0

Wait, is this only relevant on Windows? IIRC we copy translations there, right?

For these translations, I changed to loglevel to Debug instead of warning.

daschuer · 2021-05-07T07:52:33Z

Warning [Main]: Unsupported locale "ceb"

When I use "System" I get confusing messages on Ubuntu Focal:

Debug [Main]: Found and will use default keyboard preset "/home/daniel/workspace/2.3/res/keyboard/en_US.kbd.cfg"
Debug [Main]: Loading resources from  "/home/daniel/workspace/2.3/res/"
Debug [Main]: Loaded "qt" translations for locale "de_DE" from "/usr/share/qt5/translations"
Debug [Main]: Failed to load "qt" translations for locale "de_DE" from "/home/daniel/workspace/2.3/res/translations/"
Debug [Main]: Loaded "mixxx" translations for locale "de_DE" from "/home/daniel/workspace/2.3/res/translations/"

Holzhaus · 2021-05-07T09:37:10Z

Warning [Main]: Unsupported locale "ceb"

I cannot reproduce this. I guess your config was messed up and contained an invalid string for the locale setting.

When I use "System" I get confusing messages on Ubuntu Focal:

Debug [Main]: Found and will use default keyboard preset "/home/daniel/workspace/2.3/res/keyboard/en_US.kbd.cfg"
Debug [Main]: Loading resources from  "/home/daniel/workspace/2.3/res/"
Debug [Main]: Loaded "qt" translations for locale "de_DE" from "/usr/share/qt5/translations"
Debug [Main]: Failed to load "qt" translations for locale "de_DE" from "/home/daniel/workspace/2.3/res/translations/"
Debug [Main]: Loaded "mixxx" translations for locale "de_DE" from "/home/daniel/workspace/2.3/res/translations/"

What is confusing about it? This looks correct to me.

It's normal that loading qt translations from the mixxx translation directory fails, i think that is only used on windows (an macOS maybe?). See e847f58.

esbrandt · 2021-05-07T12:32:18Z

Warning [Main]: Unsupported locale "ceb"

Delete
https://github.com/mixxxdj/mixxx/blob/2.3/res/translations/mixxx_ceb.qm
https://github.com/mixxxdj/mixxx/blob/2.3/res/translations/mixxx_ceb.ts
Both files have never been touched since ages, and the language is not even supported on Transifex.

See mixxxdj#3826 (comment).

Holzhaus · 2021-05-07T19:06:49Z

Warning [Main]: Unsupported locale "ceb"

Delete
https://github.com/mixxxdj/mixxx/blob/2.3/res/translations/mixxx_ceb.qm
https://github.com/mixxxdj/mixxx/blob/2.3/res/translations/mixxx_ceb.ts
Both files have never been touched since ages, and the language is not even supported on Transifex.

Done.

Holzhaus · 2021-05-09T12:44:00Z

Anything else to do here?

esbrandt · 2021-05-10T16:27:00Z

LGTM

Holzhaus · 2021-05-11T16:59:08Z

ping @daschuer

uklotzde

Very helpful comments. LGTM

@daschuer Ping

daschuer · 2021-05-11T22:21:56Z

What is confusing about it? This looks correct to me.

After reading the code again I understand the messages.

Why do we tray to load the qt translations from the res path when we already have loaded them from the system? Can we make the local lookup conditionally? Or load directly from the correct location target depending.

On windows we install also qtbase and qthelp translations among others in the translations folder but the corresponding install statement is missing. I am afraid they are not loaded, are they? Are they loaded under Windows.

Only the en-GB translation in the translation folder used an underscore instead of a dash like the others. Does this work as well?
In the /usr/share/qt5/translations folder the country postfix is always separated by _

Holzhaus · 2021-05-12T13:28:44Z

Why do we tray to load the qt translations from the res path when we already have loaded them from the system? Can we make the local lookup conditionally? Or load directly from the correct location target depending.

On windows we install also qtbase and qthelp translations among others in the translations folder but the corresponding install statement is missing. I am afraid they are not loaded, are they? Are they loaded under Windows.

I didn't change this. If it worked before, I see no reason why it should not work now.

We could check if the system install translations call succeeded and skip the other one, but I don't want to introduce a bug so shortly before release (is it possible that there incomplete system translations for example?).

Only the en-GB translation in the translation folder used an underscore instead of a dash like the others. Does this work as well?
In the /usr/share/qt5/translations folder the country postfix is always separated by _

I think so. Just try with one of these:

$ LANG=de_DE ./mixxx
$ ./mixxx --locale de_DE

I didn't notice any issues when testing this.

uklotzde · 2021-05-12T22:12:30Z

Why do we tray to load the qt translations from the res path when we already have loaded them from the system? Can we make the local lookup conditionally? Or load directly from the correct location target depending.

On windows we install also qtbase and qthelp translations among others in the translations folder but the corresponding install statement is missing. I am afraid they are not loaded, are they? Are they loaded under Windows.

Only the en-GB translation in the translation folder used an underscore instead of a dash like the others. Does this work as well?
In the /usr/share/qt5/translations folder the country postfix is always separated by _

https://doc.qt.io/qt-5/qlocale.html#QLocale-1

The separator can be either underscore or a minus sign.

daschuer · 2021-05-12T22:20:11Z

I am afraid really have a regression here. I am sorry that this PR is such hard.

I have selected "Spanish (Mexico)" from the interface preferences in Mixxx 2.3. After restart, the Auto DJ tree knot shows the correct "DJ Automatico" from the mixxx_es-MX.ts file. After merging this branch, the Auto DJ knot is renamed to "Auto DJ" which is part of the mixxx_es.ts

By using and intermediate QLocale, Qt turns the - to _ and the translation is no longer found.

Holzhaus · 2021-05-12T23:44:23Z

You're right, but I don't think this is a regression. Before, it also didn't work properly because it was required to use the correct separator (underscore vs. dash) to get the correct locale. That's why I didn't notice. And for System language, this also didn't work I guess.

There is no way to use the dash as delimiter without building the file name yourself:
https://doc.qt.io/qt-5/qtranslator.html#load-1

Instead, we should use the standard underscore for our translations IMHO. If you agree I'll rename the translation files, but we also need to figure out to configure tx correctly.

daschuer · 2021-05-13T09:00:26Z

Renaming the files, works for me as a 2.3.0 band aid.
The loading of the additional qt translation can be postponed.

Switch to using an underscore to separate ISO-639-1 language and ISO-3166-1 country codes (instead of a dash). Also removes the `lang_map` setting from the Transifex client configuration that replaces the underscore with a dash: https://docs.transifex.com/client/client-configuration#language-mappings This fixes the broken loading of translations that used a dash in the filename, because this is not supported by `QTranslator::load()`. See https://doc.qt.io/qt-5/qtranslator.html#load-1 for details.

Holzhaus · 2021-05-13T14:56:11Z

@daschuer Please check again, should be fixed now.

daschuer · 2021-05-14T14:22:55Z

.tx/config

@@ -1,6 +1,5 @@
 [main]
 host = https://www.transifex.com
-lang_map = af_ZA: af-ZA, am_ET: am-ET, ar_AA: ar-AA, ar_AE: ar-AE, ar_BH: ar-BH, ar_DZ: ar-DZ, ar_EG: ar-EG, ar_IQ: ar-IQ, ar_JO: ar-JO, ar_KW: ar-KW, ar_LB: ar-LB, ar_LY: ar-LY, ar_MA: ar-MA, ar_OM: ar-OM, ar_QA: ar-QA, ar_SA: ar-SA, ar_SY: sy-IQ, ar_TN: ar-TN, ar_YE: ar-YE, arn_CL: arn-CL, as_IN: as-IN, ast_ES: ast-ES, az_AZ: az-AZ, ba_RU: ba-RU, be_BY: be-BY, bg_BG: bg-BG, bn_BD: bn-BD, bn_IN: bn-IN, bo_CN: bo-CN, br_FR: br-FR, bs_BA: bs-BA, ca_ES: ca-ES, co_FR: co-FR, cs_CZ: cs-CZ, cy_GB: cy-GB, da_DK: da-DK, de_AT: de-AT, de_CH: de-CH, de_DE: de-DE, de_LI: de-LI, de_LU: de-LU, dsb_DE: dsb-DE, dv_MV: dv-MV, el_GR: el-GR, en_AU: en-AU, en_BZ: en-BZ, en_CA: en-CA, en_GB: en-GB, en_IE: en-IE, en_IN: en-IN, en_JM: en-JM, en_MY: en-MY, en_NZ: en-NZ, en_PH: en-PH, en_SG: en-SG, en_TT: en-TT, en_US: en-US, en_ZA: en-ZA, en_ZW: en-ZW, eo: eo-XX, es_AR: es-AR, es_BO: es-BO, es_CL: es-CL, es_CO: es-CO, es_CR: es-CR, es_DO: es-DO, es_EC: es-EC, es_ES: es-ES, es_GT: es-GT, es_HN: es-HN, es_MX: es-MX, es_NI: es-NI, es_PA: es-PA, es_PE: es-PE, es_PR: es-PR, es_PY: es-PY, es_SV: es-SV, es_US: es-US, es_UY: es-UY, es_VE: es-VE, et_EE: et-EE, eu_ES: eu-ES, fa_IR: fa-IR, fi_FI: fi-FI, fil: fil-PH, fo_FO: fo-FO, fr_BE: fr-BE, fr_CA: fr-CA, fr_CH: fr-CH, fr_FR: fr-FR, fr_LU: fr-LU, fr_MC: fr-MC, fy_NL: fy-NL, ga_IE: ga-IE, gd: gd-GB, gl_ES: gl-ES, gu_IN: gu-IN, ha: ha-NG, he_IL: he-IL, hi_IN: hi-IN, hr_HR: hr-HR, hsb: hsb-DE, ht_HT: ht-HT, hu_HU: hu-HU, hy_AM: hy-AM, id_ID: id-ID, ig_NG: ig-NG, is_IS: is-IS, it_CH: it-CH, it_IT: it-IT, ja_JP: ja-JP, ka_GE: ka-GE, kk_KZ: kk-KZ, kl: kl-GL, km_KH: km-KH, kn_IN: kn-IN, ko_KR: ko-KR, ku_IQ: ckb-IQ, ky: ky-KG, lb: lb-LU, lo_LA: lo-LA, lt_LT: lt-LT, lv_LV: lv-LV, mi: mi-NZ, mk_MK: mk-MK, ml_IN: ml-IN, mn_MN: mn-MN, mr_IN: mr-IN, ms_MY: ms-MY, mt_MT: mt-MT, my_MM : my-MM, nb_NO: nb-NO, ne_NP: ne-NP, nl_BE: nl-BE, nl_NL: nl-NL, nn_NO: nn-NO, nso: nso-ZA, oc: oc-FR, or_IN: or-IN, pa_IN: pa-IN, pl_PL: pl-PL, ps: ps-AF, pt_BR: pt-BR, pt_PT: pt-PT, ro_RO: ro-RO, ru_RU: ru-RU, rw: rw-RW, sah: sah-RU, sd: sd-PK, se: se-FI, se_NO: se-NO, se_SE: se-SE, si_LK: si-LK, sk_SK: sk-SK, sl_SI: sl-SI, sma: sma-SE, sq_AL: sq-AL, sr@latin: sr-YU, sr_BA: sr-BA, sr_CS: sr-CS, sr_ME: sr-ME, sr_RS: sr-RS, sv_FI: sv-FI, sv_SE: sv-SE, sw_KE: sw-KE, ta_IN: ta-IN, ta_LK: ta-LK, te_IN: te-IN, tet: tet-TL, tg_TJ: tg-TJ, th_TH: th-TH, tk_TM: tk-TM, tl_PH: tl-PH, tn: tn-ZA, tr_TR: tr-TR, tt: tt-RU, tzm: tzm-DZ, ug: ug-CN, uk_UA: uk-UA, ur_PK: ur-PK, uz@Latn: uz-UZ, uz@Cyrl: uzb-UZ, vi_VN: vi-VN, wo_SN: wo-SN, xh: xh-ZA, yo: yo-NG, zh_CN: zh-CN, zh_HK: zh-HK, zh_MO: zh-MO, zh_SG: zh-SG, zh_TW: zh-TW, zu_ZA: zu-ZA


Thank you for digging down to the root cause.

daschuer · 2021-05-14T14:29:23Z

I can confirm it is working now.
As a side effect the selection falls back to system, because the dash version in the preferences is no longer found.
But IMHO we can effort this, because the old state was kind of broken anyway.

Is this issue worth a changelog entry?

See mixxxdj#3826 (comment).

Holzhaus added 3 commits May 4, 2021 15:09

Translations: Print warning if translations failed to load

5dfff8a

Translations: Improve locale override logic

a25c388

Translations: Use helper method for unified error handling

fe35905

Holzhaus added the code quality label May 4, 2021

Holzhaus changed the title ~~Load translations~~ Improve translation loading May 4, 2021

daschuer requested changes May 4, 2021

View reviewed changes

Holzhaus added 2 commits May 4, 2021 19:09

Translations: Simplify code (remove scope and redundant default constrs)

8c45a70

Translations: Use default value when reading from config

2babe83

Holzhaus commented May 4, 2021

View reviewed changes

Holzhaus requested a review from daschuer May 4, 2021 23:16

Translations: Use Debug instead of Warning loglevel for mixxx qt strings

e847f58

uklotzde added this to the 2.3.0 milestone May 6, 2021

res/translations: Delete outdates/unused "ceb" translations

300f916

See mixxxdj#3826 (comment).

uklotzde approved these changes May 11, 2021

View reviewed changes

daschuer reviewed May 14, 2021

View reviewed changes

daschuer approved these changes May 14, 2021

View reviewed changes

daschuer merged commit 0406cc0 into mixxxdj:2.3 May 14, 2021

ywwg pushed a commit to ywwg/mixxx that referenced this pull request May 14, 2021

res/translations: Delete outdates/unused "ceb" translations

22a0b2b

See mixxxdj#3826 (comment).

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve translation loading #3826

Improve translation loading #3826

Holzhaus commented May 4, 2021

daschuer left a comment

daschuer May 4, 2021

Holzhaus May 4, 2021

Holzhaus May 4, 2021

Holzhaus May 4, 2021

Holzhaus May 4, 2021 •

edited

Loading

Holzhaus May 6, 2021

daschuer commented May 7, 2021

Holzhaus commented May 7, 2021

esbrandt commented May 7, 2021

Holzhaus commented May 7, 2021

Holzhaus commented May 9, 2021

esbrandt commented May 10, 2021

Holzhaus commented May 11, 2021

uklotzde left a comment

daschuer commented May 11, 2021

Holzhaus commented May 12, 2021

uklotzde commented May 12, 2021

daschuer commented May 12, 2021

Holzhaus commented May 12, 2021

daschuer commented May 13, 2021

Holzhaus commented May 13, 2021

daschuer May 14, 2021

daschuer commented May 14, 2021

		// Load Qt translations for this locale from the Mixxx translations
		// folder.

Improve translation loading #3826

Improve translation loading #3826

Conversation

Holzhaus commented May 4, 2021

daschuer left a comment

Choose a reason for hiding this comment

daschuer May 4, 2021

Choose a reason for hiding this comment

Holzhaus May 4, 2021

Choose a reason for hiding this comment

Holzhaus May 4, 2021

Choose a reason for hiding this comment

Holzhaus May 4, 2021

Choose a reason for hiding this comment

Holzhaus May 4, 2021 • edited Loading

Choose a reason for hiding this comment

Holzhaus May 6, 2021

Choose a reason for hiding this comment

daschuer commented May 7, 2021

Holzhaus commented May 7, 2021

esbrandt commented May 7, 2021

Holzhaus commented May 7, 2021

Holzhaus commented May 9, 2021

esbrandt commented May 10, 2021

Holzhaus commented May 11, 2021

uklotzde left a comment

Choose a reason for hiding this comment

daschuer commented May 11, 2021

Holzhaus commented May 12, 2021

uklotzde commented May 12, 2021

daschuer commented May 12, 2021

Holzhaus commented May 12, 2021

daschuer commented May 13, 2021

Holzhaus commented May 13, 2021

daschuer May 14, 2021

Choose a reason for hiding this comment

daschuer commented May 14, 2021

Holzhaus May 4, 2021 •

edited

Loading