<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 15th, 2013, 10:29 a.m. UTC, <b>Frank Reininghaus</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;">Thanks for your cool work on QCollator! It will be interesting to see how much we can gain by using QCollatorSortKey for sorting large sets of QStrings :-)
I'm not really familiar with most of the affected code, and I couldn't test it yet (frameworks currently does not build for me, but it's most likely an issue with my system which can fixed by doing a clean build), but here are some things that I noticed:
(a) Is there a reason why you use a helper class to wrap the QCollator in kfile/kurlnavigatorbutton.cpp, but pass the QCollator directly to the sort function in other places?
(b) I'm wondering how cheap it is to initialize a QCollator. Could existing code outside of kdelibs that uses KStringHandler::naturalCompare() for sorting become slow if that happens N*log(N) times?
(c) Something that was not clear to me at all when I first heard about QCollator is that its behavior will depend on whether ICU headers were installed when Qt was built or not, and that things like setNumericMode(true) will simply be ignored (with a warning printed to the terminal) if ICU was not available then. Even worse: QCollator::numericMode() returns true in that case, but it does not use "numeric mode" for sorting at all!
I just found out about that when I tried to write a simple test program that sorts strings using QCollator. It did not work, and after some research I found out that this is because I only have the ICU libs, but not the headers installed on my system.
Now the Qt 5 packages that end up on our users' systems will probably be compiled with ICU support, but still, I have a very uneasy feeling about using a class that may or may not do what you expect, and that provides no good way to find out if it will do what you expect (as I said, QCollator::numericMode() from qcollator_posix.cpp always returns true). I'm already seeing the bug reports coming from people who built Qt from source and "forgot" (like me) to install the ICU headers before.
I don't see a good solution for that problem. Even if we made the ICU headers a hard dependency for frameworks, it could still be that Qt was built without ICU support.
Probably the best solution would be to try to get something like our KStringHandler::naturalCompare() function into qcollator_posix.cpp, to make sure that the fallback that is used if ICU isn't available actually uses "numeric mode" if it's told to?
</pre>
</blockquote>
<p>On September 16th, 2013, 11:32 a.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;">a) Well, I tried to minimize the number initializations, but I also tried to reduce the code changes.
b) I don't have such data. It's a possibility, that it's slower. Either way, the less we do, the faster it will work.
c) When you configure Qt, if ICU is found it will be used. I think it's ok to assume that Dolphin on linux users will all have ICU available.
I wouldn't hack around the posix backends, personally.</pre>
</blockquote>
<p>On September 16th, 2013, 12:01 p.m. UTC, <b>Frank Reininghaus</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;">"I think it's ok to assume that Dolphin on linux users will all have ICU available."
If you build Qt from source, you have to install ICU headers manually in order to "have ICU available" (at least if the ICU-devel package is not installed by the distro by default). This means that it's very easy to end up with a QCollator that does not support "numeric mode". Considering that many people who contribute to KDE do build Qt from source, it will most likely go wrong for some of them, so I tend to strongly disagree with the "it's ok to assume..." statement. These people will notice that things don't work as expected and either waste time analyzing the problem or file a bug report.
I experienced this myself yesterday: I noticed that QCollator did not work for me, and I was surprised about that because, according to the API docs, "setNumericMode(true)" causes the sorting to be "natural", and it does not mention any conditions that have to be fulfilled. At least I saw the warning message in Konsole, but if a user/developer doesn't even see that (e.g., because it gets lost in .xsession-errors), how is he/she supposed to know what the cause of the problem is?
This is all my personal opinion, and I don't actually maintain any of the affected code, but I tend to think that using a class that may or may not do what it actually pretends to do, depending on things that are out of our control, might not be a good idea.</pre>
</blockquote>
<p>On September 16th, 2013, 12:23 p.m. UTC, <b>John Layt</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;">The plan for Qt is in 5.3 to have ICU is a hard-ish dependency on Linux. QLocale will use ICU for all localization on Linux, and only provide a simple POSIX fallback for embedded platforms that don't want ICU. Distro's will be told that they should always build with ICU support enabled. (We were going to make ICU a hard dependency on all platforms in 5.2, but Windows devs objected too much).
This is not that different from needing the Cups headers installed if you want to do any Qt printing under Linux, Qt will build without them, the API won't change, and there's no way to query if you have Cups support built, but when you try to print all you'll get is the option for PDF.
The problem really comes from Qt configure not giving very good feedback on what's missing and the consequences there-of. On Linux it auto-detects of ICU headers are installed and then enables ICU support, but you can force it on by using the -icu configure flag which will cause it to complain if they are not found. I think we will have to document in the build instructions that KF5 works best with ICU support explicitly enabled with -icu.</pre>
</blockquote>
<p>On September 17th, 2013, 9:31 p.m. UTC, <b>Frank Reininghaus</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;">Thanks for the info!
I fully agree with Aurélien that this should be fixed in Qt (i.e., either the build should fail without ICU, or QCollator should do what its API docs say it does even if ICU is not available). However, I don't see why it is so urgent to start using QCollator in frameworks now, before the problem is fixed. Even if it helps with splitting, one could at least leave KStringHandler::naturalCompare() as it is and only use QCollator in code which should not depend on kcoreaddons any more.
But if I'm the only one who thinks that it can be problematic to throw away <150 lines of working and well-tested code in favor of something that only works if some requirements that we have no control over are met, then I will stop complaining now ;-) I'm not really involved in the frameworks splitting anyway, but I had been added to this review request, so I thought that I could share my opinion. Maybe I'm a bit overcautious because of my frustration that is caused by the endless stream of incoming bug reports which are mostly not really about bugs in Dolphin, but bugs in kdelibs, Qt, various kioslaves, thumbnailers, context menu plugins and quite a few other things (one user even reported a problem with the GTK+ file dialog as a Dolphin bug). This is why I don't really like the idea of adding code which makes us vulnerable to known problems in external libraries.</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;">No, you're not the only one Frank. I would not recommend naturalCompare and let the two live side by side for a while so that it can be thoroughly tested. Your (Aleix) QCollator efforts are greatly appreciated, but removing naturalCompare is a NO-GO!
I personally know how extremely difficult it can be to mimic the naturalCompare function and i would be _very_ surprised if QCollator does it how it should be done right from the start. In other terms, i expect bugs in there simply because it is very complex.</pre>
<br />
<p>- Mark</p>
<br />
<p>On September 13th, 2013, 5:55 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. 13, 2013, 5: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>