<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/112288/">http://git.reviewboard.kde.org/r/112288/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 26th, 2013, 1:35 p.m. UTC, <b>Kevin Ottens</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/112288/diff/1/?file=184765#file184765line333" style="color: black; font-weight: bold; text-decoration: underline;">staging/xmlgui/src/kswitchlanguagedialog_p.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void KSwitchLanguageDialogPrivate::fillApplicationLanguages(KLanguageButton *button)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">333</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="n">KLocalizedString</span><span class="o">::</span><span class="n">isApplicationTranslatedInto</span><span class="p">(</span><span class="n">languageCode</span><span class="p">))</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">333</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="n"><span class="hl">l</span></span><span class="hl"> </span><span class="o"><span class="hl">!=</span></span><span class="hl"> </span><span class="n"><span class="hl">QLocale</span></span><span class="o"><span class="hl">::</span></span><span class="n"><span class="hl">system</span></span><span class="p"><span class="hl">()</span></span><span class="hl"> </span><span class="o"><span class="hl">&&</span></span><span class="hl"> </span><span class="n">KLocalizedString</span><span class="o">::</span><span class="n">isApplicationTranslatedInto</span><span class="p">(</span><span class="n">languageCode</span><span class="p">))</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On August 26th, 2013, 1:48 p.m. UTC, <b>Albert Astals Cid</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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</pre>
</blockquote>
<p>On August 26th, 2013, 2:30 p.m. UTC, <b>Vishesh Handa</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">@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.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">@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.</pre>
<br />
<p>- Kevin</p>
<br />
<p>On August 26th, 2013, 2:34 p.m. UTC, Vishesh Handa wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDE Frameworks and Aleix Pol Gonzalez.</div>
<div>By Vishesh Handa.</div>
<p style="color: grey;"><i>Updated Aug. 26, 2013, 2:34 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"> 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.
</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Ran kwindowtest and compared the list of languages shown in the switch languages dialog.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>staging/xmlgui/src/kswitchlanguagedialog_p.cpp <span style="color: grey">(894f2f4)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/112288/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>