Fix locale-aware reading in KDesktopFile

Reviews
Diff

- Download Diff

Information
Submitter:	Martin Flöser
Repository:	kconfig
Branch:	master
Bugs:
Depends On:
Reviewers
Groups:	kdeframeworks
People:	jlayt

Description

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.

Testing Done

added unit test failed before. I'm not 100 % sure whether using bcp47Name is correct.

Issues

1
6
0
All Issues: 7

Description	From	Last Updated
The bcp47Name() is a complicated beast that could add lots of other bits on like script to use, etc. I ...	John Layt	June 6, 2014, 11:33 a.m.
Similarly, can you just change the config object's locale to something (en seems fine here) instead of changing the global?	Matthew Dawson	June 10, 2014, 6:40 a.m.
Just to be safe, can you please save + restore the previous default?	Matthew Dawson	June 10, 2014, 6:40 a.m.
Nitpick: whitespace.	Matthew Dawson	June 12, 2014, 5:10 a.m.
Whitespace	Matthew Dawson	June 12, 2014, 5:10 a.m.
Whitespace	Matthew Dawson	June 12, 2014, 5:10 a.m.
I guess this breaks the locales pt_BR, en_GB, zh_CN and zh_TW, which we do have in KDE. Not sure what ...	David Faure	May 15, 2015, 8:41 a.m.

src/core/kconfig.cpp (Diff revision 1)

Show all issues

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.

Matthew Dawson June 7, 2014, 4:33 a.m.

Not that this would block, but I was trying to understand this when I saw the RR.  I was reading this page: http://standards.freedesktop.org/desktop-entry-spec/latest/ar01s04.html , which would suggest that we should take the full version and search more specifically.  Is my reading of this correct?

I was actually wondering if bcp47Name() was correct, as it appears to be similar to Posix.  It's just that KConfig doesn't search the file correctly.  Am I correct there?  Is there a better QLocale function to get something more Posix correct?  I'm new to dealing with internationalization, so I apologize for stupid questions.

Regardless, this won't block the patch.  I rather have 90% functionality working then 20% that is more "correct".

John Layt June 7, 2014, 8:35 a.m.

I haven't had time to dig further yet, but it did occur to me that just the "de" was wrong, from what I remember of other desktop files from KDE4 we used the full locale code. Your link confirms the standard allows for full POSIX locales in all their variations to work, so we may need a lot more code here (what did the KDE4 code do?). However, that said, the POSIX standard explicitly says locale codes should always have the country included, and if you look at the POSIX locales installed on any modern Linux they all have the country, so when writing we should always be using name(), so the matching read should always be using name(). For example, anything generated within KDE's translation system should be using name() when writing which will have the country included.

The BCP47 name is almost always the wrong format to use, it's mostly useful on Windows, for a start it uses "-" in some places where POSIX uses "_", and more importantly it drops parts of the locale that it considers redundant, i.e. returning "de" rather than "de_DE" as it assumes DE is the default country and so not needed, whereas POSIX always requires the country to be added.

See also https://git.reviewboard.kde.org/r/118584/ where we're incorrectly using bcp47Name() to set the system locale, which may be causing us to write the incomplete locale to the desktop file. I think we need to check the whole code base to confirm the bcp47Name() isn't being used anywhere else incorrectly.

Note that QLocale's name() is not fully POSIX compliant, it never adds encoding or modifiers, but I'm working on that.

Change Summary:

Split locale name

Diff:

Revision 2 (+30 -1)

Show changes

	autotests/kdesktopfiletest.h
	autotests/kdesktopfiletest.cpp
	src/core/kconfig.cpp

This change seems to break the KConfigTest::testMerge()

Martin Flöser June 6, 2014, 11:40 a.m.

well not really, it depends on the locale whether it breaks. If I switch to en it succeeds.

Change Summary:

ensure KConfigTest::testMerge() doesn't fail

Diff:

Revision 3 (+31 -1)

Show changes

	autotests/kconfigtest.cpp
	autotests/kdesktopfiletest.h
	autotests/kdesktopfiletest.cpp
	src/core/kconfig.cpp

Ship it!

Excellent, thanks!  Just a couple minor things, then it looks good to go in.

autotests/kconfigtest.cpp (Diff revision 3)

Show all issues

Similarly, can you just change the config object's locale to something (en seems fine here) instead of changing the global?

Matthew Dawson June 11, 2014, 3:05 p.m.

To avoid creating the duplicated code, why not just change the KConfig object's locale?  I tested that locally, and it seems to resolve the issue.  Otherwise, can you move the DefaultLocale class to a header?  Since it is so simple, I think the functions can all just be left as inline functions so no cpp file required.

autotests/kdesktopfiletest.cpp (Diff revision 3)

Show all issues

Just to be safe, can you please save + restore the previous default?

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)

Show changes

	autotests/kconfigtest.cpp
	autotests/kdesktopfiletest.h
	autotests/kdesktopfiletest.cpp
	src/core/kconfig.cpp

Change Summary:

Moved the duplicated code into a helper.h

Diff:

Revision 5 (+78 -1)

Show changes

	autotests/helper.h
	autotests/kconfigtest.cpp
	autotests/kdesktopfiletest.h
	autotests/kdesktopfiletest.cpp
	src/core/kconfig.cpp

Ship it!

LGTM.  The licence header has some extra whitespace.  Please remove then commit.

autotests/helper.h (Diff revision 5)

Show all issues

Nitpick: whitespace.

autotests/helper.h (Diff revision 5)

Show all issues

Whitespace

autotests/helper.h (Diff revision 5)

Show all issues

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.

Status: Closed (submitted)

Fix it!

src/core/kconfig.cpp (Diff revision 5)

Show all issues

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.

You have a pending review.

Review Board 2.5.15

Screenshots

Files

Issues

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Status: Closed (submitted)