Fix locale-aware reading in KDesktopFile
Review Request #118564 - Created June 5, 2014 and submitted
Information | |
---|---|
Martin Flöser | |
kconfig | |
master | |
Reviewers | |
kdeframeworks | |
jlayt |
Fix locale-aware reading in KDesktopFile The underlying KConfig used QLocale::name() for getting the locale aware part. But this returns "de_DE" while the desktop files store "de". In addition it constructs a QLocale object instead of using the system locale. This has the advantage that the usage of QLocale::setDafault() gets honored by KConfig.
added unit test failed before. I'm not 100 % sure whether using bcp47Name is correct.
Issues
- 1
- 6
- 0
- 7
Description | From | Last Updated |
---|---|---|
I guess this breaks the locales pt_BR, en_GB, zh_CN and zh_TW, which we do have in KDE. Not sure what ... |
|
-
src/core/kconfig.cpp (Diff revision 1) -
The bcp47Name() is a complicated beast that could add lots of other bits on like script to use, etc. I would stick with name() as it is documented in Qt to always return language_COUNTRY, so "QLocale().name().split('_').at(0)". I'll remember to add the needed languageCode() api for QT 5.4.
This change seems to break the KConfigTest::testMerge()
Review request changed
Excellent, thanks! Just a couple minor things, then it looks good to go in.
-
autotests/kconfigtest.cpp (Diff revision 3) -
Similarly, can you just change the config object's locale to something (en seems fine here) instead of changing the global?
-
autotests/kdesktopfiletest.cpp (Diff revision 3) -
Just to be safe, can you please save + restore the previous default?
Review request changed
Change Summary:
Storing and resetting the default using a small helper class. It's currently duplicating the code, any preferences on where to put shared code from the tests or whether duplication is OK in tests?
Diff: |
Revision 4 (+63 -1) |
---|
Review request changed
LGTM. The licence header has some extra whitespace. Please remove then commit.
-
autotests/helper.h (Diff revision 5) -
Nitpick: whitespace.
-
autotests/helper.h (Diff revision 5) -
Whitespace
-
autotests/helper.h (Diff revision 5) -
Whitespace
This review has been submitted with commit 988f09bb051dca0437ecec431ee44ed5b4a560d8 by Martin Gräßlin to branch master.
This review has been submitted with commit 69c203156d21385a8c263096cb35600e85a5c604 by Thomas Braxton on behalf of Martin Gräßlin to branch master.
-
src/core/kconfig.cpp (Diff revision 5) -
I guess this breaks the locales pt_BR, en_GB, zh_CN and zh_TW, which we do have in KDE.
Not sure what the solution is, though.