<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/112717/">http://git.reviewboard.kde.org/r/112717/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On September 18th, 2013, 3:22 p.m. UTC, <b>Mark Gaiser</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;">How RUDE to just commit this without a addressing the concerns Frank and i have. That is not appreciated!</pre>
</blockquote>
<p>On September 18th, 2013, 4:32 p.m. UTC, <b>Aleix Pol Gonzalez</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;">Alright, maybe I didn't think this through. I'll un-deprecate and bring back the KStringHandler::naturalCompare code as you want.
Said that, I doubt these concerns make that much a difference. QCollator has to be adopted. Not because of performance reasons but localization alone. Performance will happen as well, but for that we need development on the applications side, which I'm afraid cannot be up to me as well.
Either way, if you really shouldn't consider ::naturalCompare as a solution. ICU is much better tested than naturalCompare (yes, even) and has people more qualified than us by making it locale aware (::naturalCompare implementation is naive to localization).</pre>
</blockquote>
<p>On September 18th, 2013, 5:20 p.m. UTC, <b>Mark Gaiser</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;">Idea: restore naturalCompare as it was and add a "collatorNaturalCompare" that is taking the QCollator route. That way it's easily testable as well.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">See the other patch I just sent.
I don't think creating such function would be a good thing because it means using QCollator the wrong way. I much rather prefer working on tools to properly use QCollator, for example by making it possible to sort things with the sort key instead of QCollator::compare.</pre>
<br />
<p>- Aleix</p>
<br />
<p>On September 18th, 2013, 2:58 p.m. UTC, Aleix Pol Gonzalez 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, Frank Reininghaus and Aurélien Gâteau.</div>
<div>By Aleix Pol Gonzalez.</div>
<p style="color: grey;"><i>Updated Sept. 18, 2013, 2:58 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;">Now that QCollator is public API, I wanted to give it a try, so I decided to port all uses KStringHandler::naturalCompare() to QCollator.
All the porting was quite straightforward I'd say, the only weird part is that I removed some tests of the KStringHandler tests. There are 2 kind of tests disabled:
- The ones that say that "A" == "a" in case of Qt::CaseInsensitive. ICU is deterministic and it will decide itself which one goes in, so the test doesn't make sense anymore.
- There's a mention to the 237788 bug where in some cases our former algorithm wouldn't be deterministic. This doesn't apply anymore, but also now ICU takes care about it now, so there's little point of keeping unit testing it.
I decided to leave the unit test because it might be useful eventually (although note that it was not being compiled so far). In any case we probably want it out.
In any case, the rest seems straightforward enough. I didn't concentrate on performance though, in some cases we'll want to use the QCollatorSortKey.</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;">Builds, affected tests pass.</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>KDE5PORTING.html <span style="color: grey">(1287de7)</span></li>
<li>kfile/kdirsortfilterproxymodel.cpp <span style="color: grey">(7c7aa5f)</span></li>
<li>kfile/kurlnavigatorbutton.cpp <span style="color: grey">(d2c27fd)</span></li>
<li>staging/itemviews/src/CMakeLists.txt <span style="color: grey">(353a413)</span></li>
<li>staging/itemviews/src/kcategorizedsortfilterproxymodel.h <span style="color: grey">(a21e7ca)</span></li>
<li>staging/itemviews/src/kcategorizedsortfilterproxymodel.cpp <span style="color: grey">(c8b652d)</span></li>
<li>staging/itemviews/src/kcategorizedsortfilterproxymodel_p.h <span style="color: grey">(eb1a67b)</span></li>
<li>staging/kcompletion/src/kcompletion.cpp <span style="color: grey">(5f60a6c)</span></li>
<li>staging/xmlgui/src/kshortcutsdialog_p.h <span style="color: grey">(ab102bc)</span></li>
<li>staging/xmlgui/src/kshortcutseditoritem.cpp <span style="color: grey">(e89a8aa)</span></li>
<li>tier1/kcoreaddons/autotests/CMakeLists.txt <span style="color: grey">(19227dc)</span></li>
<li>tier1/kcoreaddons/autotests/kstringhandlertest.cpp <span style="color: grey">(d12f086)</span></li>
<li>tier1/kcoreaddons/src/lib/text/kstringhandler.h <span style="color: grey">(1b08f6f)</span></li>
<li>tier1/kcoreaddons/src/lib/text/kstringhandler.cpp <span style="color: grey">(2f192aa)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/112717/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>