> 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? > >
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. - Aleix ----------------------------------------------------------- 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
