Review Request 122392: Fix Klipper Performance issues

[Filip Wieladek]

> On Feb. 3, 2015, 7:23 a.m., Martin Gräßlin wrote:
> > could you please split the review in a per-commit review? I find it hard to review as there are so many changes to different areas. Especially I think there are a few no-brainer which could go in quickly, while the threaded filtering is the part which needs most thought, so having that in a dedicated review would certainly help :-)
> 
> Filip Wieladek wrote:
>     Yes, I can do that once I get home. If you really believe that the popup is going away anyway, then I would however prefer to work on the new way, so that the "legacy" way can be removed.
> 
> Martin Gräßlin wrote:
>     we probably cannot completely remove the legacy way as we still want to support the dedicated klipper mode without plasmoid. So improvements on that are still fine.

I looked at splitting the change, but what hit me is that none of the changes I have make much of a difference unless I have background filtering. Unless background filtering is enabled, the UI still remains to be very sluggish. I have attached a screencast with the current version vs the version with my changes.

Note, that the UI is sluggish when there are a lot of entries (e.g. 2048) which are also long. This happens quite frequently in my workflow where I copy the contents of whole files


- Filip


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


On Feb. 8, 2015, 5:08 p.m., Filip Wieladek wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122392/
> -----------------------------------------------------------
> 
> (Updated Feb. 8, 2015, 5:08 p.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
> -------
> 
> 
> File Attachments
> ----------------
> 
> A screencast with foreground processing vs background processing
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/02/08/1d998fd9-57fd-4997-92f8-11b8e038e795__screencast.mp4
> 
> 
> Thanks,
> 
> Filip Wieladek
> 
>

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