Review Request 109457: Group files by name correctly also for non-ASCII file names

[Commit Hook]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/109457/#review29892
-----------------------------------------------------------


This review has been submitted with commit 02f4a69f58fef89ec6df4c6775be1e8d4b9d40b2 by Frank Reininghaus to branch KDE/4.10.

- Commit Hook


On March 12, 2013, 6:11 p.m., Frank Reininghaus wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109457/
> -----------------------------------------------------------
> 
> (Updated March 12, 2013, 6:11 p.m.)
> 
> 
> Review request for Dolphin.
> 
> 
> Description
> -------
> 
> The existing "Group by Name" code has some issues:
> 
> 1. In some locales, there are non-ASCII letters which are sorted next to a letter in the range 'A'-'Z' (such as umlauts in the German locale). In the test folder shown in the screenshot, this works fine for A and Ä, which are both shown in the group 'A'. However, Ö is shown in a group 'Others' because there was no file whose name starts with an 'O' before that (there is a TODO comment in the existing code about this issue).
> 
> 2. All other non-ASCII letters are shown in a single group 'Others', see bug 315569.
> 
> I'm proposing a patch to fix these issues.
> 
> If anyone wonders why I used STL, rather than Qt containers: I've read in several places that the Qt container templates expand to more code in the executable. They are of course still preferable if one can make good use of their implicit sharing/copy-on-write feature, e.g., if the container is used in a foreach (...) loop, which always takes a copy of the container.
> 
> I actually checked how much code is generated in this case by copying the code related to the lettersAtoZ container to a single .cpp file, implementing it once with Qt containers and once with STL containers, and found that with -O2, the 'STL' object file is about 25% smaller. Considering that the STL containers are also faster because they save the indirect access through the pointer to the shared data, I think that it makes sense to use STL containers in new code if we are sure that implicit sharing has no benefits there. This is definitely the case if we are dealing with a local variable in a function, and this variable is never copied and not used in a foreach loop.
> 
> 
> This addresses bug 315569.
>     http://bugs.kde.org/show_bug.cgi?id=315569
> 
> 
> Diffs
> -----
> 
>   dolphin/src/kitemviews/kfileitemmodel.cpp 7927245 
> 
> Diff: http://git.reviewboard.kde.org/r/109457/diff/
> 
> 
> Testing
> -------
> 
> Grouping works better now for non-ASCII file names, see screenshot.
> 
> It would definitely be good if this could be tested by people using other locales and file names with language-specific letters to make sure that it does not break anything.
> 
> 
> File Attachments
> ----------------
> 
> Before/after screenshots, made with the de_DE locale
>   http://git.reviewboard.kde.org/media/uploaded/files/2013/03/12/Grouping.png
> 
> 
> Thanks,
> 
> Frank Reininghaus
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20130326/bf39dafb/attachment.htm>