Review Request 118063: New Formats KCM

Mon May 12 01:58:15 UTC 2014

> On May 11, 2014, 5:59 p.m., John Layt wrote:
> > kcms/formats/formats.desktop, line 19
> > <https://git.reviewboard.kde.org/r/118063/diff/1/?file=272242#file272242line19>
> >
> >     accessibility?

That's the same as for the locale category. I think this category in the .desktop file only matters for the group, and as it seems consistent this way with the other options, I've left it as that. (There's a bit of discussion going on how to categorize, and I think this can in that case be revisitited.

> On May 11, 2014, 5:59 p.m., John Layt wrote:
> > kcms/formats/kcmformats.cpp, line 113
> > <https://git.reviewboard.kde.org/r/118063/diff/1/?file=272244#file272244line113>
> >
> >     Perhaps "Use Default Locale"?

I've pondered that, but we have "Default (C)" as the next option then, and that seems to be confusing. I've therefore chosen to tell what this setting will effect, and that is "don't change anything". It could also be "System locale", but then, if the LC_* env vars are set "from the outside", before logging in, in .xinitrc, or some crazy stuff like that, it's not the "System locale" anymore.

Better ideas of course welcome...

> On May 11, 2014, 5:59 p.m., John Layt wrote:
> > kcms/formats/kcmformats.cpp, line 204
> > <https://git.reviewboard.kde.org/r/118063/diff/1/?file=272244#file272244line204>
> >
> >     Why are you writing all these entries here?  If all they have set is the global then all we need to export is LANG, so only writing out lcGlobal should be enough. If there's no overrides we should always be deleting them.

Hm, LANG will be set from the Translations KCM, and affects that, no? (I might be off here, that's how I understand it.)

This brings me back a bit to the way this KCM works, it's used to override settings specified elsewhere, if necessary. I think in combination with the Translations KCM, a clean separation makes sense, but I'm not 100% sure that's achievable. Effectively, with the current version of code, we have a few scenarios:

- Language set from distro installer, however. User's happy, doesn't touch the KCM
- User sets Language in the Translations KCModule, which sets LANG (in the same fashion as we do here), LC_* is not set, so everything falls back to LANG -> we're peachy
- User sets Language, but wants something else changed, configures that in the Formats KCM, and it's applied by exporting LC_*, -> we're fine again

The Translations KCM probably the first one we should show when the category in systemsettings is opened, as it allows a very "Global" setting: LANG is changed, affects translations, and additionally all the formatting because LC_* not set means fall back to LANG.

> On May 11, 2014, 5:59 p.m., John Layt wrote:
> > kcms/formats/kcmformats.cpp, line 88
> > <https://git.reviewboard.kde.org/r/118063/diff/1/?file=272244#file272244line88>
> >
> >     While this approach is easier and makes more sense to the user, I don't think it will work in the world of POSIX locales. I believe the LC_MEASUREMENT locale set may also affect the formatting of the numbers i.e. the decimal and grouping separators used, not just the units.  We probably need to check that and if so just use the same list of locales as the others.  If it doesn't, then there's actually 3 different possible values, Metric, US Imperial, and UK Imperial.

I've made it use the list of Locales, just like in the other combos.

> On May 11, 2014, 5:59 p.m., John Layt wrote:
> > kcms/formats/kcmformats.cpp, line 142
> > <https://git.reviewboard.kde.org/r/118063/diff/1/?file=272244#file272244line142>
> >
> >     Hmmmm.  I do prefer listing by Country rather than Language (it's more natural to users I think), and I sort-of like showing the locale code so experts knwo what they are getting, but I think we do need to include the name of the language somehow (just ask your Catalans how they feel :-)

The combo values are "locale.nativeCountryName() - locale.nativeLanguageName() (locale->name())" now, I'm sure our Catalan friends will be happy. :)

> On May 11, 2014, 5:59 p.m., John Layt wrote:
> > kcms/formats/kcmformats.cpp, line 294
> > <https://git.reviewboard.kde.org/r/118063/diff/1/?file=272244#file272244line294>
> >
> >     Shouldn't lcGlobal just be exported as LANG?  The lcCollate and lcCtype should be read form the config and exported in their own right?

lcCtype now checks if it's set from config, and if so, that's used, otherwise the global setting. I don't want to add another combo for this, as this setting is almost impossible to explain to the user, so it follows the global setting.

> On May 11, 2014, 5:59 p.m., John Layt wrote:
> > kcms/formats/kcmformats.cpp, line 363
> > <https://git.reviewboard.kde.org/r/118063/diff/1/?file=272244#file272244line363>
> >
> >     We need to check what it actually affects and use that as an example.  Problem is QLocale doesn't have any methods that use this other than measurementSystem() returning Metric/ImperialUK/ImperialUS. If my comment above about needing to show the locale names in teh combo is correct, then just showing the resultign system name shoudl be enough.  Otherwise we need to find something that will format using LC_MEASUREMENT.  Or maybe we just leave it out for now.  It needs research.

I've made the example just show the measurement system. Good enough for now.

- Sebastian

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

On May 9, 2014, 4:05 p.m., Sebastian Kügler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118063/
> -----------------------------------------------------------
> 
> (Updated May 9, 2014, 4:05 p.m.)
> 
> 
> Review request for Plasma and John Layt.
> 
> 
> Bugs: 331930
>     https://bugs.kde.org/show_bug.cgi?id=331930
> 
> 
> Repository: plasma-desktop
> 
> 
> Description
> -------
> 
> The current "Locale" KCM is almost entirely broken. The way forward is to split it into localization options (this, Formats), and translaticons. This patch implements a new Formats KCM which writes out an environment suitable for QLocale to pick it up.
> 
> It's a rewrite of the current KCM, since:
> - everything under the hood is different (KLocale is gone, QLocale takes over)
> - everything above the hood is different, in QLocale, everything is deduced from the country / region
> 
> Now this includes feature regressions compared to the old KCM, but not all of these features can be done at all on top of QLocale right now, so we have to set up what we can.
> 
> This KCM caches the settings in a config file, but most importantly, it writes out a script with export instructions, which can be picked up by startkde. This is all done according to John's suggestions, and I have a patch for startkde to pick up the settings here. It all works fine (on my machine).
> 
> Many more details about the implementation can be found in the plasma-devel thead "Locale settings in Plasma Next", started by John on February, 23rd 2014.
> 
> 
> Diffs
> -----
> 
>   kcms/formats/kcmformatswidget.ui PRE-CREATION 
>   kcms/formats/kcmformats.cpp PRE-CREATION 
>   kcms/formats/kcmformats.h PRE-CREATION 
>   kcms/formats/Messages.sh PRE-CREATION 
>   kcms/formats/formats.desktop PRE-CREATION 
>   kcms/formats/CMakeLists.txt PRE-CREATION 
>   kcms/CMakeLists.txt ed86efa 
> 
> Diff: https://git.reviewboard.kde.org/r/118063/diff/
> 
> 
> Testing
> -------
> 
> Tried all kinds of different combinations, applied them, made sure they're exported correctly in the script.
> 
> 
> File Attachments
> ----------------
> 
> new Formats KCM in kcmshell5
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/05/09/873980fe-04f2-4d75-9074-2aa676723061__formatskcm.png
> Formats KCM in systemsettings
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/05/09/86a830ed-2d5d-4bdd-ba82-71ec73e6ce2c__formatskcmss.png
> 
> 
> Thanks,
> 
> Sebastian Kügler
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20140512/e128325b/attachment-0001.html>