Review Request 122392: Fix Klipper Performance issues

Tue Feb 3 07:22:18 UTC 2015

> On Feb. 3, 2015, 7:57 a.m., Martin Gräßlin wrote:
> > As I worked on that just yesterday: you cannot filter in a different thread as Klipper is not thread save. Please see https://git.reviewboard.kde.org/r/122382/ for how I solved the problem. I suggest to rebase the patch on top of it and also lock while it's filtering.
> > 
> > The obvious question would then be: why filter in a thread at all if the main gui thread needs to be blocked?
> > 
> > Personally I think the change is too large for the legacy code base. Especially doing the filtering this way looks strange to me given that the History is nowadays backed by a model. So it would be way easier IMHO to introduce a QSortFilterProxyModel in between. By having the a QSortFilterProxyModel in between the filtering could be done there, maybe it's even possible for the popup to get rid of the usage of the History completly and just build up on the model? That's something I didn't look into as I only wanted to get the dataengine running and not touching the legacy way. If there is no development on the legacy way, it might be better to go that road.
> 
> Filip Wieladek wrote:
>     Hi Martin,
>     
>     I'll make sure to lock while filtering. 
>     
>     To answer your question: When we run in a background thread, we do not lock the main gui thread. We update the results using the signal, which means that the GUI thread is never blocked. We never have issues of the UI being "sluggish", which leads to bad user experience.
>     
>     I would love to use QSortFilterProxyModel, since reusing existing infrastructure is always better than coming up with something new. But as far as I see, QSortFilterProxyModel does not support filtering in the background. In the patch, we filter using a cancellable future. On each new press, we cancel the filtering and it starts a new. This is all to prevent the UI from starting to feel sluggish.
>     
>     What do you mean by legacy way? I understand that klipper now has a plasmoid, but unless I can open the plasmoid at the mouse cursor location, the plasmoid is pretty much useless for my daily workflow. I use a two monitor setup, and going from one screen to the other to access klipper is not the fastest way. At that point I might as well not use the history at all. If you say that there is an alternative to this workflow and is supposed to be replaced, then I will hapilly look into it.

yes, I consider the klipper popup as the legacy way. It's only still used for the popup at location as the other code is not yet written. One could also go for opening the plasmoid when the shortcut is involved and ensure that the plasmoid has a good filtering experience. I assume for such a workflow it should be keyboard driven and the fact that it doesn't open close to the mouse shouldn't matter. Or am I wrong on that?

- Martin

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122392/#review75255
-----------------------------------------------------------

On Feb. 3, 2015, 7:27 a.m., Filip Wieladek wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122392/
> -----------------------------------------------------------
> 
> (Updated Feb. 3, 2015, 7:27 a.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-workspace
> 
> 
> Description
> -------
> 
> This patch fixes multiple klipper issues:
> 
> * Moves filtering logic into a background task. This makes Klipper responsive while the clipboard is being filtered.
>   Previously, Klipper would "hang" while it was filtering the results. This was worse when there were no matches as
>   Klipper had to go through the entire history. THe computation is immediate on small history sets, but significant
>   on larger sets with larger strings in the clipboard
> * Provides a progress bar at the top to indicate the filtering process.
> * Simplifies the code significantly, by:
>    * Moving filtering in a separate class file
>    * Cleaning up Popup proxy to build the menu directly using a QList
>    * Removing the "index" magic in KlipperPopup. Now we maintain a list of history actions which can be easily cleared.
>    * Removed explicit deletion of the "more" submenus, as these are owned by the parent menus and should be removed
>      automatically
> * Avoids flickering of Klipper while removing and inserting actions by forcing the height and width during the update.
> * Fixes a potential memory leak. The QActions for the KlipperPopup were only removed, but never deleted. The API used
>   to add actions addAction(QAction*) was not taking ownership of the action. This is fixed by deleting the actions
>   manually when clearing.
> * Fixes a performance issue during menu rendering when truncating large strings. The method call elidedText() can be
>   slow on large pieces of text. This is worked around by creating a much smaller string of the prefix and suffix of the
>   string. We use the average character width to compute the approximate amount of characters which can be displayed
>   and use twice as much. (this is because in corner cases, such as |||| we might end up with a string which is not
>   long enough).
>   
> This also fixes the bug https://bugs.kde.org/show_bug.cgi?id=238084
> 
> 
> Diffs
> -----
> 
>   klipper/popupproxy.h f33f62c117a08ddbe6b761da4c2e28e51b985044 
>   klipper/popupproxy.cpp 12dd3dd637d0ff9d134fb71237d6f0d3bcc5bd77 
>   klipper/CMakeLists.txt a08f062480b15f32f049e2d0d0e311dbe2964c02 
>   klipper/filterresult.h PRE-CREATION 
>   klipper/filterresult.cpp PRE-CREATION 
>   klipper/history.h 1bfd0424714ff79d93206a74cb7e4214a6c8c652 
>   klipper/history.cpp 9640c23b0cf06dd0135ca573aea0819e2788b852 
>   klipper/historyfilter.h PRE-CREATION 
>   klipper/historyfilter.cpp PRE-CREATION 
>   klipper/klipperpopup.h 6f7b30747562b5e7227504b8c53be2863686072b 
>   klipper/klipperpopup.cpp 0b2f11d6d6d95e96ece0ac7b386fae2492ad0efd 
> 
> Diff: https://git.reviewboard.kde.org/r/122392/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Filip Wieladek
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20150203/6798f2c2/attachment-0001.html>