Review Request 122392: Fix Klipper Performance issues

Mon Feb 9 18:23:40 UTC 2015

> 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.
> 
> Filip Wieladek wrote:
>     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

Hello Martin,

What do you think would be the best way to go forward with this review? I really think that background processing of the filter makes sense as it makes things a lot more responsive. Do you want another patch off the current git commit? Could you check out the code and play with it? You would see that the user experience is much better when dealing with large input sets.

I would also like to continue contributing to klipper, so I would like to get this out of the way ;)

- 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/20150209/531af90b/attachment-0001.html>