Review Request 118564: Fix locale-aware reading in KDesktopFile

[John Layt]

> On June 6, 2014, 12:21 p.m., John Layt wrote:
> > src/core/kconfig.cpp, line 98
> > <https://git.reviewboard.kde.org/r/118564/diff/1/?file=279088#file279088line98>
> >
> >     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 wrote:
>     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".

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.


- John


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118564/#review59415
-----------------------------------------------------------


On June 6, 2014, 12:43 p.m., Martin Gräßlin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118564/
> -----------------------------------------------------------
> 
> (Updated June 6, 2014, 12:43 p.m.)
> 
> 
> Review request for KDE Frameworks and John Layt.
> 
> 
> Repository: kconfig
> 
> 
> 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.
> 
> 
> Diffs
> -----
> 
>   autotests/kconfigtest.cpp 2b6de0d7d63df6aee69210aa09418628f0b8110a 
>   autotests/kdesktopfiletest.h d57351fd6edcefd6a0efd9126e454546cfc63b55 
>   autotests/kdesktopfiletest.cpp 494a43c0fb5808a261b9beb56cdc4afd8580ab21 
>   src/core/kconfig.cpp ea9746c001e235529a1cdd5865b9e1b5c129b56a 
> 
> Diff: https://git.reviewboard.kde.org/r/118564/diff/
> 
> 
> Testing
> -------
> 
> added unit test failed before. I'm not 100 % sure whether using bcp47Name is correct.
> 
> 
> Thanks,
> 
> Martin Gräßlin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20140607/ca339207/attachment.html>