Review Request 112288: fillApplicationLanguages: Do not add the system locale

[Kevin Ottens]

> On Aug. 26, 2013, 1:35 p.m., Kevin Ottens wrote:
> > staging/xmlgui/src/kswitchlanguagedialog_p.cpp, line 333
> > <http://git.reviewboard.kde.org/r/112288/diff/1/?file=184765#file184765line333>
> >
> >     From QLocale documentation if the ctor didn't find the language in the database it then uses ::default(). It just happens in your case that ::system() == ::default(), but I think we should test for default() not system().
> >     
> >     Now that means that your default language will never get in the combo box... Don't you end up with an empty combo box with that patch? That would be a problem.
> >     
> >     I wonder if we shouldn't store the default locale before the loop, set C to be the new default and test on that instead. And last restore the previous default language after the loop.
> >     
> >     It's jumping through hoops a bit but we're out of the usual QLocale uses it seems.
> 
> Albert Astals Cid wrote:
>     To be honest I've been thinking about this code and I don't like it (Aleix and me did it together). Sure apply this patch, but a better way for us to find the instaled languages other than
>     
>         for ( int i = 2; i <= QLocale::LastLanguage; ++i )
>     
>     which doesn't work for us anyway since there's pt and pt_BR that is not covered here, basically my idea is to just iterate over
>     
>        QStandardPaths::standardLocations(QStandardPaths::GenericDataLocation) + QString::fromLatin1("locale/"); <-- note this does not compile :D
>     
>     (i.e. /usr/share/locale) to get the languages and then pass that to isApplicationTranslatedInto
>     
>     That should be muuuch better than the current loop we have, but don't prevent my suggestion for a better solution accept this if it is an improvement to the current one
> 
> Vishesh Handa wrote:
>     @Kevin: One could also just do -
>     
>     bool addedDefault = false;
>     for ( ... ) {
>     
>         if (l == QLocale())
>            if (!addedDefault) {
>               addedDefault = true;
>            }
>            else
>               continue;
>         
>         ...
>     }
>     
>     Or we could just add the default one in the beginning/end. Let me know which you prefer. I've already implemented the default swapping one.

@Albert: Sure, if we have a better way to do it go for it. This patch at least unbreaks the status quo though.

@Vishesh: I don't really have a preference, the one you prefer is fine by me.


- Kevin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112288/#review38639
-----------------------------------------------------------


On Aug. 26, 2013, 2:34 p.m., Vishesh Handa wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112288/
> -----------------------------------------------------------
> 
> (Updated Aug. 26, 2013, 2:34 p.m.)
> 
> 
> Review request for KDE Frameworks and Aleix Pol Gonzalez.
> 
> 
> Description
> -------
> 
>     If QLocale cannot find the appropriate language, it then defaults to the
>     system locale. When adding all possible languages it is possible that
>     QLocale::system() is added multiple times. This results in a huge list
>     for the default language being added.
>     
>     For me, "US English" gets added over 50 times.
> 
> 
> Diffs
> -----
> 
>   staging/xmlgui/src/kswitchlanguagedialog_p.cpp 894f2f4 
> 
> Diff: http://git.reviewboard.kde.org/r/112288/diff/
> 
> 
> Testing
> -------
> 
> Ran kwindowtest and compared the list of languages shown in the switch languages dialog.
> 
> 
> Thanks,
> 
> Vishesh Handa
> 
>

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