> 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 > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
