Re: Review Request 112717: Start adopting QCollator
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112717/#review40273 --- This review has been submitted with commit e7c9d9720635131bef2255a47c6c557d0f406cf0 by Aleix Pol to branch frameworks. - Commit Hook 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
Re: Review Request 112717: Start adopting QCollator
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112717/ --- (Updated Sept. 18, 2013, 2:58 p.m.) Status -- This change has been marked as submitted. 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
Re: Review Request 112717: Start adopting QCollator
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112717/#review40277 --- How RUDE to just commit this without a addressing the concerns Frank and i have. That is not appreciated! - Mark Gaiser On Sept. 18, 2013, 2:58 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. 18, 2013, 2:58 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
Re: Review Request 112717: Start adopting QCollator
On Sept. 18, 2013, 3:22 p.m., Mark Gaiser wrote: How RUDE to just commit this without a addressing the concerns Frank and i have. That is not appreciated! Aleix Pol Gonzalez wrote: Alright, maybe I didn't think this through. I'll un-deprecate and bring back the KStringHandler::naturalCompare code as you want. Said that, I doubt these concerns make that much a difference. QCollator has to be adopted. Not because of performance reasons but localization alone. Performance will happen as well, but for that we need development on the applications side, which I'm afraid cannot be up to me as well. Either way, if you really shouldn't consider ::naturalCompare as a solution. ICU is much better tested than naturalCompare (yes, even) and has people more qualified than us by making it locale aware (::naturalCompare implementation is naive to localization). Mark Gaiser wrote: Idea: restore naturalCompare as it was and add a collatorNaturalCompare that is taking the QCollator route. That way it's easily testable as well. See the other patch I just sent. I don't think creating such function would be a good thing because it means using QCollator the wrong way. I much rather prefer working on tools to properly use QCollator, for example by making it possible to sort things with the sort key instead of QCollator::compare. - Aleix --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112717/#review40277 --- On Sept. 18, 2013, 2:58 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. 18, 2013, 2:58 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
Re: Review Request 112717: Start adopting QCollator
On Sept. 18, 2013, 3:22 p.m., Mark Gaiser wrote: How RUDE to just commit this without a addressing the concerns Frank and i have. That is not appreciated! Aleix Pol Gonzalez wrote: Alright, maybe I didn't think this through. I'll un-deprecate and bring back the KStringHandler::naturalCompare code as you want. Said that, I doubt these concerns make that much a difference. QCollator has to be adopted. Not because of performance reasons but localization alone. Performance will happen as well, but for that we need development on the applications side, which I'm afraid cannot be up to me as well. Either way, if you really shouldn't consider ::naturalCompare as a solution. ICU is much better tested than naturalCompare (yes, even) and has people more qualified than us by making it locale aware (::naturalCompare implementation is naive to localization). Idea: restore naturalCompare as it was and add a collatorNaturalCompare that is taking the QCollator route. That way it's easily testable as well. - Mark --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112717/#review40277 --- On Sept. 18, 2013, 2:58 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. 18, 2013, 2:58 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
Re: Review Request 112717: Start adopting QCollator
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112717/#review40209 --- Ship it! 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. kfile/kdirsortfilterproxymodel.cpp http://git.reviewboard.kde.org/r/112717/#comment29711 This field should be renamed to m_collator - Aurélien Gâteau On Sept. 13, 2013, 7: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, 7: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
Re: Review Request 112717: Start adopting QCollator
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).
Re: Review Request 112717: Start adopting QCollator
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).
Re: Review Request 112717: Start adopting QCollator
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
Re: Review Request 112717: Start adopting QCollator
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. 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. - Frank --- 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/
Re: Review Request 112717: Start adopting QCollator
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112717/#review40174 --- QCollator is available for everyone to use now, if there's any concerns in a specific use of QCollator in this patch I can leave it out, but I think this should go in. Thoughts? - Aleix Pol Gonzalez 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
Re: Review Request 112717: Start adopting QCollator
--- 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:
Review Request 112717: Start adopting QCollator
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112717/ --- 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
Re: Review Request 112717: Start adopting QCollator
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112717/#review39986 --- That's a very impressive part on the porting side! Thank you very much for taking care here. I have some experience in optimizing this function (understatement ^_^) and know very well that even a tiny change in performace (negative or positive) can have a big impact on sorting large folders in Dolphin. You really should measure the performance and see if it beats the current stuff, if it does then it should deffinately replace naturalCompare :) - Mark Gaiser On Sept. 13, 2013, 5:12 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:12 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
Re: Review Request 112717: Start adopting QCollator
On Sept. 13, 2013, 5:32 p.m., Mark Gaiser wrote: That's a very impressive part on the porting side! Thank you very much for taking care here. I have some experience in optimizing this function (understatement ^_^) and know very well that even a tiny change in performace (negative or positive) can have a big impact on sorting large folders in Dolphin. You really should measure the performance and see if it beats the current stuff, if it does then it should deffinately replace naturalCompare :) Note that the change to QCollator is not about performance, but it's about properly listing things in different locales. In any case, QCollatorSortKey should really help. If you have some stress test I can run, I'll be glad to try it. - Aleix --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112717/#review39986 --- On Sept. 13, 2013, 5:12 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:12 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
