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/#review21351
-----------------------------------------------------------


Thanks for the patch! The context menu is one of the areas that I haven't worked on much until now - I wasn't even aware that an operation as expensive as creating a PlacesItemModel is performed every time a context menu is requested for a directory.

However, I think that the context menu issue can be solved in a much easier way. If we want to determine if a directory is already present in the "Places", we are only interested in the "Places" in kfileplaces/bookmarks.xml, i.e., those Places that the user created or the predefined ones like "Home". The devices that Solid knows about are irrelevant in this context.

Therefore, one could just query the Places in kfileplaces/bookmarks.xml (possibly using a new static method in PlacesItemModel), rather than setting up a full PlacesItemModel, in DolphinContextMenu::placeExists(const KUrl& url).

I see that your proposal to add a PlacesCache might still have the benefit that Dolphin starts up faster (if the Places Panel is enabled) by making the Solid stuff asynchronous. I'm not really familiar with Solid, but I assume that the call that takes so long is Solid::Device::listFromQuery(m_predicate)? Wouldn't it be much better to resolve this problem in Solid and provide some API to get this information asynchronously? That would prevent that every app using Solid has to implement its own 'PlacesCache'.

- Frank Reininghaus


On Nov. 1, 2012, 1:13 p.m., Dan Vrátil wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107168/
> -----------------------------------------------------------
> 
> (Updated Nov. 1, 2012, 1:13 p.m.)
> 
> 
> Review request for Dolphin.
> 
> 
> 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/dolphinmainwindow.h ab79fb0 
>   dolphin/src/dolphinmainwindow.cpp babaf14 
>   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/20121102/351342a0/attachment.htm>