Review Request 109180: Emmanuel's recent KFileItemModel patch with some small modifications

[Frank Reininghaus]

> On Feb. 26, 2013, 6:26 p.m., Emmanuel Pescosta wrote:
> > After some testing, I haven't found any problems with your modifications.
> > 
> > I agree with all your proposals (Btw.: The QHash idea for filtered items is great!) ;)

Great, thanks for testing it (and for the clarification about bug 311912). Feel free to push it to master - I probably won't be near my development machine today (and I would set you as the author of the patch with git commit's --author option anyway because I only polished your ideas a bit).


- Frank


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


On Feb. 26, 2013, 5:12 p.m., Frank Reininghaus wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109180/
> -----------------------------------------------------------
> 
> (Updated Feb. 26, 2013, 5:12 p.m.)
> 
> 
> Review request for Dolphin and Emmanuel Pescosta.
> 
> 
> Description
> -------
> 
> This is a slightly modified version of https://git.reviewboard.kde.org/r/109045/. Rather than pointing out my suggestions there, I decided to upload a new diff, because some of my suggestions require changes in multiple places, and I wanted to try if it works with my modifications to prevent wasting everyone's time if it doesn't. And then uploading the modified patch seemed more efficient than asking Emmanuel to make the same changes again that I made already.
> 
> My main modifications are:
> 
> 1. Left out everything that is not strictly required to fix the "Network browser" and "timeline" issues, i.e., left out everything that is not related directly to the use of KDirLister's itemsAdded(KUrl,KFileItemList) signal or required to prevent regressions arising from that change.
> 
> 2. Changed the name of the slot "slotNewItems" to "slotItemsAdded" for consistency with the signal.
> 
> 3. Rather than putting the filtered ItemData* into a list, I propose to use a QHash<KFileItem, ItemData*> to store the filtered data. This is needed to keep the O(1) lookup for filtered KFileItems in slotItemsDeleted. The list-based approach would have made that function O(N^2) in the worst case.
> 
> 4. Fixed memory leaks (the filtered ItemData pointers have to be deleted when the model is cleared or destroyed).
> 
> 5. Made the determination of the "expandedParentsCount" slightly simpler - just adding 1 to the parent's level should be fine, I think.
> 
> Emmanuel, do these proposals look OK to you, or did I overlook anything or even introduce a bug with my suggestions? If there is any issue with the new version of the patch, don't hesitate to tell me. If you agree with the suggestions and it works OK for you, feel free to push it.
> 
> What still needs to be done when this patch is in from my point of view:
> 
> a) Adjust slotItemsDeleted to make sure that it respects parent-child relationships correctly for filtered child items (see TODO comment).
> 
> b) Add unit test that documents the bugs that are fixed by this patch (could also be included in the patch, of course, but is not stricly necessary).
> 
> c) Fix the benchmark - after the changes, it is no longer possible to do what I did there, namely, insert children of multiple folders at the same time. I'll take care of that.
> 
> d) Remove everything related to m_expandedParentsCountRoot - the entire concept seems unnecessary to me with the new "determine parents and expansion levels" approach. I would nonetheless prefer to do it in a separate commit to keep the present patch as small as possible and to isolate those things, such that any possible problems that show up later can be detected more easily.
> 
> e) Take care of "Bug 311912 - After erasing a "filter", some thumbnails randomly disappear".
> 
> Many thanks again to Emmanuel for this work! It will make some things simpler in the end and finally enable us to respect all parent-child relationships that KDirLister knows about :-)
> 
> 
> Diffs
> -----
> 
>   dolphin/src/tests/kfileitemmodeltest.cpp 483c5b2 
>   dolphin/src/kitemviews/kfileitemmodel.cpp d16a6c1 
>   dolphin/src/tests/kfileitemmodelbenchmark.cpp b1e777c 
>   dolphin/src/kitemviews/kfileitemmodel.h a05d1f9 
> 
> Diff: http://git.reviewboard.kde.org/r/109180/diff/
> 
> 
> Testing
> -------
> 
> Works for me.
> 
> 
> Thanks,
> 
> Frank Reininghaus
> 
>

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