KLanguageName is a helper namespace that returns the name of a given language code.
Details
Diff Detail
- Repository
- R265 KConfigWidgets
- Branch
- arcpatch-D10446
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 6049 Build 6067: arc lint + arc unit
Isn't this better suited for KCoreAddons?
On a related note, i did make this bug report for Qt: https://bugreports.qt.io/browse/QTBUG-64942 with regard to ISO 3166 country names. But language names also came up, see this comment.
I don't know of any progress there though.
The kf5_entry.desktop files are part of kconfigwidgets tarball, so that's why i put it here.
@aacid what were the changes you had planned here?
Does anyone else have thoughts on kcoreaddons vs. kconfigwidgets? I suppose we could move the class along with kf5_entry.desktop to kcoreaddons, since kconfigwidgets depends on kcoreaddons anyway it's a compatible change.
@aacid what were the changes you had planned here?
As Aleix mentioned the two methods can probably be made to call eachother, or maybe not, but needs to be investigated and an answer given to why not if not possible.
Stacking the functions seems to work fine
QString KLanguageName::nameForCode(const QString &code) { const QStringList parts = QLocale().name().split(QChar('_')); return nameForCodeInLocale(code, parts.at(0)); }
I do have various refinements and fixes for the logic plus a unit test I can update the diff if you are ok with that.
As for kcoreaddons, I don't think we could go there with kconfig could we? And I am not sure QSettings is up to the task?
I also have some behavior concerns. The auto-fallback to QLocale is super handy, but isn't very flexible: As an application author I might have another way to map a language to a pretty string, if KLanguageName automatically falls back to QLocale, I won't be able to easily determine if I should use another (possibly better) fallback.
At the same time falling back to QLocale::languageToString is not necessarily in the interest of the user either. Who's to say the user will know what a language means in English.
So, I haven't give this a great deal of thought, but it seems that always returning QString(), if we cannot resolve a string internally, is possibly more flexible. Worst case the consumer has to do:
str = KLanguageName::nameForCode("fr") if (str.isEmpty()) { str = QLocale("fr").nativeLanguageName() }
Which isn't that much code TBH.
Sure, tests are nice i remember this was kind of a bit of a hit and miss sometimes. But are you going to depend on having all of l10n installed (which is where kf5_entry.desktop files come from or will you have some fake kf5_entry.destkops for the test? (second would be awesome)
As for kcoreaddons, I don't think we could go there with kconfig could we? And I am not sure QSettings is up to the task?
QSettings doesn't support language entries AFAIK
I also have some behavior concerns. The auto-fallback to QLocale is super handy, but isn't very flexible: As an application author I might have another way to map a language to a pretty string, if KLanguageName automatically falls back to QLocale, I won't be able to easily determine if I should use another (possibly better) fallback.
At the same time falling back to QLocale::languageToString is not necessarily in the interest of the user either. Who's to say the user will know what a language means in English.So, I haven't give this a great deal of thought, but it seems that always returning QString(), if we cannot resolve a string internally, is possibly more flexible. Worst case the consumer has to do:
str = KLanguageName::nameForCode("fr") if (str.isEmpty()) { str = QLocale("fr").nativeLanguageName() }Which isn't that much code TBH.
But you end up repeating that in lots of places (which we should there's lots of places that suffer from trying to guess a language name at this point, and all of them went the bad way one way or another). If you want to give some random potential user more flexibility i'm fine with that, add some flags, but i want the "give me the best you can do" possibility to still work. Maybe we should even never return an empty string and worst case scenario return code back.
iterating the diff a bit as mentioned in a comment.
- new unit test which covers all likely scenarios (I think). this uses fixtures and doesn't require anything to be installed etc.
- the specific func now calls the generic to resolve into current-language. this requires some string mangling as qlocale apparently has no way to get the ISO639-1 string directly
- some refinement on the english-check resolution: now returns the kconfig resolved name iff it is different from english and it is not english (previously this was a bit bugged for english itself)
- some refinement on the qlocale fallback: now returns immediately when a final result is found (simplifies code a bit)
- a failed kconfig resolution (for whatever reason) now always continues onwards into the qlocale fallback. this effectively separates the kconfig portion entirely from the qlocale portion for ease of reading
as mentioned in an earlier comment I am not too sure about giving out languageToString names and would rather have it always give out nativeLanguageNames. I am somewhat indifferent on the issue though, it's just a gut feeling is all.
I should also note that I had a quick look at systemsettings and it also does exhaustive poking of QLocale to come up with reasonable prettynames for locales (and then fails in part). Perhaps @hein could take a look at this diff and see if it could be useful for the language KCM and/or give some thoughts on what may be needed to make that happen. For the record though: since KLanguageName exclusively deals with languages and the KCM AFAIK deals with locales there is no 100% overlap; perhaps there should be though.
I am not too invested in the use case. For all I care we can leave it as it is and should the no-fallback usecase actually get requested we can simply add a three-argument variant of the function without default value for KF5 and reshuffle the functions for KF6.
The diff as it is right now is good IMO
autotests/kf5_entry_data/locale/ca/kf5_entry.desktop | ||
---|---|---|
1 | This is going to be a problem, scripty is going to come and whipe these .desktop files out and then make the translators translate them again. Wonder if we could rename them to .desktop.untransltable or something like that and then use the cmake file copy command to copy them to the build folder and trick the test to find the files there? Am i making any sense? | |
autotests/klanguagenametest.cpp | ||
32 |
move env setup to qcorestartup to prevent the env from not getting set up in time and the tests failing as a result
autotests/kf5_entry_data/locale/ca/kf5_entry.desktop | ||
---|---|---|
1 | Really good point. I've had a look and we only extract src/*. In fact, we only have Messages.sh in src :) This is in line with other frameworks where we have desktop file fixtures. They all only extract src/ and use regular desktop files for test fixtures (e.g. kservice, kpackage, kparts). So, this should be fine. | |
autotests/klanguagenametest.cpp | ||
32 | LGTM, I've added the change to the diff. Thanks. |
autotests/kf5_entry_data/locale/ca/kf5_entry.desktop | ||
---|---|---|
1 | If you look carefully at Messages.sh, you'll see that they never mention .desktop files, that's because .desktop files are extracted automatically for translation, so yes, it will be a problem, because translations will be dropped (until translators translate them), making the test flaky |
autotests/kf5_entry_data/locale/ca/kf5_entry.desktop | ||
---|---|---|
1 | Ah, you are absolutely right. In this case I agree configure_file is the way to go. I'll get on it tomorrow. |
configure_file the fixtures lest they get mangled by scripty on account of being desktop files
lookup still goes through qfindtestdata, so we need the output relative to
the binary now
add an empty es directory to see in the source that es is meant to be empty. serves no purpose other than making things more obvious
src/klanguagename.cpp | ||
---|---|---|
31 ↗ | (On Diff #26943) | should the second param be code too? I mean if we read the docs we say both are code (ISO 639-1), so both should be the same and not only the first part? |
src/klanguagename.cpp | ||
---|---|---|
31 ↗ | (On Diff #26943) | Do you mean both being a code or both being the code? a codeparts.at(0) is the code of the current locale. The only way I found to get the 639 code from qlocale is by splitting name http://doc.qt.io/qt-5/qlocale.html#name Perhaps a comment is needed to explain this? the codeBoth params being the variable code wouldn't be right I think. The documentation for nameForCode says it will ideally give you the localized name of code in the system language. LANGUAGE=en nameForCode('en') => 'English' LANGUAGE=fr nameForCode('en') => 'anglaise' which internally is nameForCodeInLocale('en', 'en') nameForCodeInLocale('en', 'fr') |
Two more small comments from my side, otherwise looks cool :) Thanks for the perseverance!
I can't approve it because the review is on my name ^_^
Maybe @apol can give it the ship it?
autotests/klanguagenametest.cpp | ||
---|---|---|
80 ↗ | (On Diff #48854) | Do you think it makes sense adding this? void testNoString() { // Qt doesn't have za support so no string at all QCOMPARE(KLanguageName::nameForCode("za"), QString()); } |
src/klanguagename.cpp | ||
31 ↗ | (On Diff #26943) | Argggg, ignore me i was reading the code wrong. Sorry for the noise |
src/klanguagename.h | ||
35 ↗ | (On Diff #26943) | Needs to be 5.55 i think |
You'd have to import all our .desktop files containing all the translations of all languages to all langauges. Impossible copyright wise
Getting slightly off-topic here, but CLDR has those translations too, in case someone really would want to add this feature to Qt. That's where most other data inside QLocale is coming from too.
I don't think you'd have to import .desktop files (although I can see this is an elegant solution for us). But you would need some lookup table of course to map the names.
Kubuntu CI builds
2/4 Test #3: klanguagenametest ................***Failed 0.03 sec
Start testing of KLanguageNameTest
Config: Using QtTest library 5.11.3, Qt 5.11.3 (x86_64-little_endian-lp64 shared (dynamic) release build; by GCC 8.2.0)
PASS : KLanguageNameTest::initTestCase()
FAIL! : KLanguageNameTest::testNameForCode() Compared values are not the same
Actual (KLanguageName::nameForCode("en")): "English" Expected ("US English") : US English Loc: [/<<BUILDDIR>>/kconfigwidgets-5.54.0+p19.04+git20190111.0330/autotests/klanguagenametest.cpp(46)]
PASS : KLanguageNameTest::testNameForCodeInLocale()
PASS : KLanguageNameTest::testNoTranslation()
PASS : KLanguageNameTest::testNoEntry()
PASS : KLanguageNameTest::testNoString()
PASS : KLanguageNameTest::cleanupTestCase()
Totals: 6 passed, 1 failed, 0 skipped, 0 blacklisted, 6ms
- Finished testing of KLanguageNameTest *****
Oh actually. I have a theory. My environment vars were wrong. Try with s/LANG/LANGUAGE and s/LOCALE/LANG please.