Add KLanguageName
ClosedPublic

Authored by aacid on Feb 11 2018, 5:44 PM.

Details

Summary

KLanguageName is a helper namespace that returns the name of a given language code.

Diff Detail

Repository
R265 KConfigWidgets
Branch
arcpatch-D10446
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 6127
Build 6145: arc lint + arc unit
aacid created this revision.Feb 11 2018, 5:44 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptFeb 11 2018, 5:44 PM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
aacid requested review of this revision.Feb 11 2018, 5:44 PM
aacid added a comment.Feb 11 2018, 5:45 PM

With this i can finally stop using kdelibs4support in KTuberling

apol added a subscriber: apol.Mar 5 2018, 1:12 AM

Makes sense to me anyway.

Is there any other code that could use this API?

src/klanguagename.cpp
46

shouldn't this be equivalent to nameForCodeInLocale(code, QLocale().code())?

66

englishName

markg added a subscriber: markg.Mar 5 2018, 11:30 AM

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.

aacid added a comment.Mar 6 2018, 5:50 PM

Isn't this better suited for KCoreAddons?

The kf5_entry.desktop files are part of kconfigwidgets tarball, so that's why i put it here.

aacid planned changes to this revision.Mar 6 2018, 5:52 PM
sitter added a subscriber: sitter.Dec 13 2018, 3:27 PM

@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.

Restricted Application edited subscribers, added: kde-frameworks-devel; removed: Frameworks. · View Herald TranscriptDec 13 2018, 3:27 PM
aacid added a comment.Dec 13 2018, 8:54 PM

@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.

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.

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.

sitter updated this revision to Diff 47623.Dec 15 2018, 3:53 PM
sitter added a subscriber: hein.

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.

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.

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

aacid added inline comments.Dec 16 2018, 10:11 PM
autotests/kf5_entry_data/locale/ca/kf5_entry.desktop
2

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
33

initTestCase is too late and was failing for me

https://paste.kde.org/p8s9js4r3

makes it work

sitter updated this revision to Diff 47758.Dec 18 2018, 10:35 AM

move env setup to qcorestartup to prevent the env from not getting set up in time and the tests failing as a result

sitter added inline comments.Dec 18 2018, 10:36 AM
autotests/kf5_entry_data/locale/ca/kf5_entry.desktop
2

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
33

LGTM, I've added the change to the diff. Thanks.

aacid added inline comments.Dec 18 2018, 10:47 PM
autotests/kf5_entry_data/locale/ca/kf5_entry.desktop
2

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

sitter added inline comments.Jan 7 2019, 12:36 AM
autotests/kf5_entry_data/locale/ca/kf5_entry.desktop
2

Ah, you are absolutely right.

In this case I agree configure_file is the way to go. I'll get on it tomorrow.

sitter updated this revision to Diff 48853.Jan 7 2019, 11:13 AM

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

sitter updated this revision to Diff 48854.Jan 7 2019, 11:15 AM

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

aacid added inline comments.Jan 7 2019, 11:10 PM
src/klanguagename.cpp
31

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?

sitter added inline comments.Jan 8 2019, 10:30 AM
src/klanguagename.cpp
31

Do you mean both being a code or both being the code?

a code

parts.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 code

Both 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')
aacid added a comment.Jan 8 2019, 9:25 PM

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
81

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

Argggg, ignore me i was reading the code wrong. Sorry for the noise

src/klanguagename.h
35

Needs to be 5.55 i think

apol accepted this revision.Jan 9 2019, 2:56 AM
This revision is now accepted and ready to land.Jan 9 2019, 2:56 AM
sitter added inline comments.Jan 9 2019, 3:14 PM
autotests/klanguagenametest.cpp
81

👍 I'll add it

src/klanguagename.h
35

Going to update

Closed by commit R265:91077def022a: Add KLanguageName (authored by aacid, committed by sitter). · Explain WhyJan 9 2019, 3:16 PM
This revision was automatically updated to reflect the committed changes.
dhaumann added a subscriber: dhaumann.

Btw, wouldn't this be a good candidate to upstream to Qt itself?

aacid added a comment.Jan 9 2019, 6:49 PM

Btw, wouldn't this be a good candidate to upstream to Qt itself?

You'd have to import all our .desktop files containing all the translations of all languages to all langauges. Impossible copyright wise

vkrause added a subscriber: vkrause.Jan 9 2019, 8:52 PM

Btw, wouldn't this be a good candidate to upstream to Qt itself?

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.

dhaumann added a comment.EditedJan 9 2019, 11:03 PM

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.

rikmills added a subscriber: rikmills.EditedJan 11 2019, 8:47 AM

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 *****

You'll have to debug this for us. It passes on build.kde.org 😒

Oh actually. I have a theory. My environment vars were wrong. Try with s/LANG/LANGUAGE and s/LOCALE/LANG please.