Review Request: Dolphin - optimize use of PlacesItemModel

[Frank Reininghaus]

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


@Alex: could you point me to the discussion where the permission to violate the feature freeze for the udisks2 backend was granted? I could not find anything on kde-core-devel, but maybe I've missed it.

If there is really nothing that we can do to either stop the udisks2 backend from being shipped in KDE 4.10 or to add asynchronous API to Solid, I see that something has to be done. However, I don't see at all how https://git.reviewboard.kde.org/r/107030/ fixes this in KFilePlaces*. It was my understanding that the slow part is the call to Solid::Device::listFromQuery(), which still seems to be in KFilePlacesModel. Could you elaborate on that?

Thanks for the new version of the patch - I've added a few comments on the code below.

One more thing: would you mind to change the license of the new files to LGPL? I'm planning to change the license of the kitemviews/ and panels/places/ directories of Dolphin's source to LGPL (Peter already agreed, I'll contact all others who contributed something to those files) in the hope that the 'File' dialogs and Dolphin can share some code again in the bright frameworks future :-)


dolphin/src/dolphincontextmenu.cpp
<http://git.reviewboard.kde.org/r/107168/#comment16788>

    I prefer to not abbreviate variable names unless they are very long. Moreover, put the space behind ' '.



dolphin/src/panels/places/placescache.h
<http://git.reviewboard.kde.org/r/107168/#comment16784>

    Could we add a comment here saying that the class is designed mostly to make accessing Solid asynchronous and that the issue will be resolved in a different way with libsolid2?



dolphin/src/panels/places/placescache.h
<http://git.reviewboard.kde.org/r/107168/#comment16785>

    If we have only one PlacesItemModel instance in Dolphin, then this instance could have its own PlacesCache, and we would not need the singleton stuff at all, right? We could just make the cache a member of the PlacesItemModel.



dolphin/src/panels/places/placescache.h
<http://git.reviewboard.kde.org/r/107168/#comment16787>

    According to Dolphin's coding style, the space is put behind '&' and '*' (here and in other places).


- Frank Reininghaus


On Nov. 8, 2012, 11:33 a.m., Dan Vrátil wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107168/
> -----------------------------------------------------------
> 
> (Updated Nov. 8, 2012, 11:33 a.m.)
> 
> 
> Review request for Dolphin and Frank Reininghaus.
> 
> 
> Description
> -------
> 
> This patch optimizes PlacesItemModel in two places. The optimizations are needed when using udisks2 Solid backend (which will be shipped in KDE 4.10, but it's already used in KDE 4.9 at least in Fedora). udisks2 is a bit slower because it only can enumerate all devices (unlike udisks where you can ask for specific device) - and that takes time, especially when there are slow devices (cheap USB sticks, slow optical drive).
> 
> 1) Solid devices are stored in a static cache, which takes care that on startup Solid is only queried once (this eliminates CPU spikes by udisks2d daemon and limits DBus traffic). The Solid query is run asynchronously, which has a positive impact on Dolphin startup time.
> 
> 2) Don't create new PlacesItemModel on every context menu popup. This effectively eliminates ~4 seconds delay between right click and the context menu appearing on screen. The persistent model is owned by DolphinMainView.
> 
> 
> Diffs
> -----
> 
>   dolphin/src/CMakeLists.txt 8f7f4db 
>   dolphin/src/dolphincontextmenu.cpp bb26c7a 
>   dolphin/src/panels/places/placescache.h PRE-CREATION 
>   dolphin/src/panels/places/placescache.cpp PRE-CREATION 
>   dolphin/src/panels/places/placesitemmodel.h 463e564 
>   dolphin/src/panels/places/placesitemmodel.cpp 1789f18 
> 
> Diff: http://git.reviewboard.kde.org/r/107168/diff/
> 
> 
> Testing
> -------
> 
> Start up time and opening context menu is faster, no CPU spikes from udisks2d deamon
> 
> 
> Thanks,
> 
> Dan Vrátil
> 
>

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