<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 />
<p>Ship it!</p>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Looks good to me, except for one field which should be renamed. Ship it from me when this is fixed.
Regarding the issue of Qt not being build with ICU, I think this should be fixed in Qt itself, not worked around in kdelibs.</pre>
<br />
<div>
<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/112717/diff/2/?file=189348#file189348line44" style="color: black; font-weight: bold; text-decoration: underline;">kfile/kdirsortfilterproxymodel.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">44</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QCollator</span> <span class="n">c</span><span class="p">;</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This field should be renamed to "m_collator"</pre>
</div>
<br />
<p>- Aurélien</p>
<br />
<p>On September 13th, 2013, 7:55 p.m. CEST, 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. 13, 2013, 7:55 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>