> On Sept. 15, 2013, 10:29 a.m., Frank Reininghaus wrote: > > 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? > > > > Aleix Pol Gonzalez wrote: > 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. > > Frank Reininghaus wrote: > "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. > > John Layt wrote: > 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. > > Frank Reininghaus wrote: > 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.
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. - Mark ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112717/#review40054 ----------------------------------------------------------- On Sept. 13, 2013, 5:55 p.m., Aleix Pol Gonzalez wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/112717/ > ----------------------------------------------------------- > > (Updated Sept. 13, 2013, 5:55 p.m.) > > > Review request for KDE Frameworks, Frank Reininghaus and Aurélien Gâteau. > > > Description > ------- > > 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. > > > Diffs > ----- > > KDE5PORTING.html 1287de7 > kfile/kdirsortfilterproxymodel.cpp 7c7aa5f > kfile/kurlnavigatorbutton.cpp d2c27fd > staging/itemviews/src/CMakeLists.txt 353a413 > staging/itemviews/src/kcategorizedsortfilterproxymodel.h a21e7ca > staging/itemviews/src/kcategorizedsortfilterproxymodel.cpp c8b652d > staging/itemviews/src/kcategorizedsortfilterproxymodel_p.h eb1a67b > staging/kcompletion/src/kcompletion.cpp 5f60a6c > staging/xmlgui/src/kshortcutsdialog_p.h ab102bc > staging/xmlgui/src/kshortcutseditoritem.cpp e89a8aa > tier1/kcoreaddons/autotests/CMakeLists.txt 19227dc > tier1/kcoreaddons/autotests/kstringhandlertest.cpp d12f086 > tier1/kcoreaddons/src/lib/text/kstringhandler.h 1b08f6f > tier1/kcoreaddons/src/lib/text/kstringhandler.cpp 2f192aa > > Diff: http://git.reviewboard.kde.org/r/112717/diff/ > > > Testing > ------- > > Builds, affected tests pass. > > > Thanks, > > Aleix Pol Gonzalez > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
