Review Request 109471: Fix Bug 312014 - testing: group by file does not work correctly

[Frank Reininghaus]

> On March 14, 2013, 11:58 p.m., Frank Reininghaus wrote:
> > Thanks Emmanuel for looking into this issue! This has been puzzling me for a while.
> > 
> > It seems that I can still reproduce the issue in at least one folder even with your patch. But maybe I did something wrong? Is the bug really fixed for you if you apply your patch? And if yes, what is the root cause of the bug in the old code, and which of your changes fixes it? At first (and also second) sight, your changes look like they won't change the behaviour much.
> > 
> > My way to reproduce is the following:
> > 
> > 1. Make sure that grouping is enabled and we sort by "Name". 
> > 2. Open Dolphin in a folder with many files with different types.
> > 3. Enable "Sort by type".
> > 
> > What then happens is that the files are apparently still sorted by "Name" (you can also see that in the screenshot in the bug report - the files are sorted by name in reverse order there), but a new "Type group" is started every time the mime type of a file is different from the previous one. It's currently unclear to me why that happens though :-(
> 
> Emmanuel Pescosta wrote:
>     > Is the bug really fixed for you if you apply your patch?
>     Yes ;)
>     
>     > And if yes, what is the root cause of the bug in the old code, and which of your changes fixes it?
>     data.value("type") returns an empty string - so Dolphin uses the fallback case in sortRoleCompare (which is sorting by name)
>     
>     > At first (and also second) sight, your changes look like they won't change the behaviour much.
>     The change which fixed the bug (atleast for me) is in KFileItemModel::setRoles. Instead of overwritting the whole list, it uses the existing list and modifies its values (Doesn't throw away all already determined values)
>     
>     The other changes are only needed, because of the retrieveData parameter and return value changes. 
>     
>     > My way to reproduce is the following:
>     This way worked for me too in Dolphin 2.2 / KDE 4.10.1, but it doesn't work with the patched Dolphin version -> Grouping is correct for all roles
>     
>     > What then happens is that the files are apparently still sorted by "Name"
>     Yes, exactly this happens in unpatched Dolphin 2.2

Thanks, but I just tried again, and I can definitely still reproduce the bug even with your patch applied. Don't know if something is wrong here on my system... Moreover, it seems that your patch breaks some things concerning expanding folders in Details View (unit test fails).

I don't have much time now, but I'll try to have a closer look soon.


- Frank


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


On March 13, 2013, 9:48 p.m., Emmanuel Pescosta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109471/
> -----------------------------------------------------------
> 
> (Updated March 13, 2013, 9:48 p.m.)
> 
> 
> Review request for Dolphin and Frank Reininghaus.
> 
> 
> Description
> -------
> 
> Fix wrong grouping of files (Esp. when sorting by file type)
> 
> Changes needed for master:
> Add "const ItemData* parent = itemData->parent;" to retrieveData and remove the parent parameter from retrieveData.
> 
> Ideas for master:
> Maybe use "void QtConcurrent::blockingMap ( Iterator begin, Iterator end, MapFunction function )" instead of a simple loop in setRoles, slotRefreshItems and createItemDataList.
> 
> 
> This addresses bug 312014.
>     http://bugs.kde.org/show_bug.cgi?id=312014
> 
> 
> Diffs
> -----
> 
>   dolphin/src/kitemviews/kfileitemmodel.h ef9dc98 
>   dolphin/src/kitemviews/kfileitemmodel.cpp 7927245 
> 
> Diff: http://git.reviewboard.kde.org/r/109471/diff/
> 
> 
> Testing
> -------
> 
> Works for me.
> 
> Open a folder -> Sort by Type -> Sort by Rating -> Sort by .... -> Finally: Sort by Type (Files should be in the right file-type group)
> 
> 
> Thanks,
> 
> Emmanuel Pescosta
> 
>

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