----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112717/#review40054 -----------------------------------------------------------
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? - Frank Reininghaus 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
