D10446: Add KLanguageName

sitter added a comment. Oh actually. I have a theory. My environment vars were wrong. Try with s/LANG/LANGUAGE and s/LOCALE/LANG please. REPOSITORY R265 KConfigWidgets REVISION DETAIL https://phabricator.kde.org/D10446 To: aacid, apol Cc: rikmills, vkrause, dhaumann, hein,

D10446: Add KLanguageName

sitter added a comment. You'll have to debug this for us. It passes on build.kde.org  REPOSITORY R265 KConfigWidgets REVISION DETAIL https://phabricator.kde.org/D10446 To: aacid, apol Cc: rikmills, vkrause, dhaumann, hein, kde-frameworks-devel, sitter, markg, apol, michaelh, ngraham,

D10446: Add KLanguageName

rikmills added a comment. Kubuntu CI builds 2/4 Test #3: klanguagenametest ***Failed0.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

D10446: Add KLanguageName

dhaumann added a comment. I don't think you'd have to import .desktop files. But you would need some lookup table of course to map the names. REPOSITORY R265 KConfigWidgets REVISION DETAIL https://phabricator.kde.org/D10446 To: aacid, apol Cc: vkrause, dhaumann, hein,

D10446: Add KLanguageName

vkrause added a comment. In D10446#390025 , @aacid wrote: > In D10446#389972 , @dhaumann wrote: > > > Btw, wouldn't this be a good candidate to upstream to Qt itself? > > > You'd have to

D10446: Add KLanguageName

aacid added a comment. In D10446#389972 , @dhaumann wrote: > 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.

D10446: Add KLanguageName

dhaumann added a comment. Btw, wouldn't this be a good candidate to upstream to Qt itself? REPOSITORY R265 KConfigWidgets REVISION DETAIL https://phabricator.kde.org/D10446 To: aacid, apol Cc: dhaumann, hein, kde-frameworks-devel, sitter, markg, apol, michaelh, ngraham, bruns

D10446: Add KLanguageName

This revision was automatically updated to reflect the committed changes. Closed by commit R265:91077def022a: Add KLanguageName (authored by aacid, committed by sitter). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D10446?vs=48854=49079#toc REPOSITORY R265 KConfigWidgets CHANGES

D10446: Add KLanguageName

sitter added inline comments. INLINE COMMENTS > aacid wrote in klanguagenametest.cpp:80 > 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()); > } 

D10446: Add KLanguageName

apol accepted this revision. This revision is now accepted and ready to land. REPOSITORY R265 KConfigWidgets BRANCH arcpatch-D10446 REVISION DETAIL https://phabricator.kde.org/D10446 To: aacid, apol Cc: hein, kde-frameworks-devel, sitter, markg, apol, michaelh, ngraham, bruns

D10446: Add KLanguageName

aacid added a comment. 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? INLINE COMMENTS > klanguagenametest.cpp:80 > +} > +}; > + Do you

D10446: Add KLanguageName

sitter added inline comments. INLINE COMMENTS > aacid wrote in klanguagename.cpp:30 > 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? Do you mean both being **a** code or both being

D10446: Add KLanguageName

aacid added inline comments. INLINE COMMENTS > klanguagename.cpp:30 > +const QStringList parts = QLocale().name().split(QChar('_')); > +return nameForCodeInLocale(code, parts.at(0)); > +} should the second param be code too? I mean if we read the docs we say both are code (ISO 639-1),

D10446: Add KLanguageName

sitter updated this revision to Diff 48853. sitter added a comment. 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 REPOSITORY R265 KConfigWidgets

D10446: Add KLanguageName

sitter added inline comments. INLINE COMMENTS > aacid wrote in 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

D10446: Add KLanguageName

aacid added inline comments. INLINE COMMENTS > sitter wrote in 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

D10446: Add KLanguageName

sitter added inline comments. INLINE COMMENTS > aacid wrote in 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

D10446: Add KLanguageName

sitter updated this revision to Diff 47758. sitter added a comment. move env setup to qcorestartup to prevent the env from not getting set up in time and the tests failing as a result REPOSITORY R265 KConfigWidgets CHANGES SINCE LAST UPDATE

D10446: Add KLanguageName

aacid added inline comments. INLINE COMMENTS > kf5_entry.desktop:1 > +[KCM Locale] > +Name=Catalan 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

D10446: Add KLanguageName

sitter added a comment. In D10446#377355 , @aacid wrote: > 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

D10446: Add KLanguageName

sitter updated this revision to Diff 47623. sitter added a subscriber: hein. sitter added a comment. 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. -

D10446: Add KLanguageName

aacid added a comment. In D10446#376825 , @sitter wrote: > Stacking the functions seems to work fine > > QString KLanguageName::nameForCode(const QString ) > { > const QStringList parts = QLocale().name().split(QChar('_')); >

D10446: Add KLanguageName

sitter added a comment. Stacking the functions seems to work fine QString KLanguageName::nameForCode(const QString ) { const QStringList parts = QLocale().name().split(QChar('_')); return nameForCodeInLocale(code, parts.at(0)); } I do have various

D10446: Add KLanguageName

aacid added a comment. > @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. REPOSITORY R265 KConfigWidgets REVISION

D10446: Add KLanguageName

sitter added a comment. Herald edited subscribers, added: kde-frameworks-devel; removed: Frameworks. @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

D10446: Add KLanguageName

aacid planned changes to this revision. REPOSITORY R265 KConfigWidgets REVISION DETAIL https://phabricator.kde.org/D10446 To: aacid Cc: markg, apol, #frameworks, michaelh

D10446: Add KLanguageName

aacid added a comment. In D10446#218954 , @markg wrote: > 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. REPOSITORY R265 KConfigWidgets REVISION

D10446: Add KLanguageName

markg added a comment. 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

D10446: Add KLanguageName

apol added a comment. Makes sense to me anyway. Is there any other code that could use this API? INLINE COMMENTS > klanguagename.cpp:45 > + > +return QString(); > +} shouldn't this be equivalent to nameForCodeInLocale(code, QLocale().code())? > klanguagename.cpp:65 > +

D10446: Add KLanguageName

aacid added a comment. ping? REPOSITORY R265 KConfigWidgets REVISION DETAIL https://phabricator.kde.org/D10446 To: aacid Cc: #frameworks, michaelh

D10446: Add KLanguageName

aacid added a comment. With this i can finally stop using kdelibs4support in KTuberling REPOSITORY R265 KConfigWidgets REVISION DETAIL https://phabricator.kde.org/D10446 To: aacid Cc: #frameworks, michaelh

D10446: Add KLanguageName

aacid created this revision. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: Frameworks. aacid requested review of this revision. REVISION SUMMARY KLanguageName is a helper namespace that returns the name of a given language code. REPOSITORY