Review Request 122392: Fix Klipper Performance issues

[Martin Gräßlin]

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


sorry for the delay. I was basically without Internet all of last week.

I haven't done a full review yet as it's really large with lots of changes. First a general comment: please check the coding style. We use kdelibs coding style (https://techbase.kde.org/Policies/Kdelibs_Coding_Style) and I have a hard time reviewing code if it's not following the style.


klipper/filterresult.h
<https://git.reviewboard.kde.org/r/122392/#comment52507>

    nitpick: whitespaces in the empty lines of the GPL header



klipper/filterresult.h
<https://git.reviewboard.kde.org/r/122392/#comment52508>

    suggestion: const & the QList



klipper/filterresult.cpp
<https://git.reviewboard.kde.org/r/122392/#comment52509>

    with C++11 it could also be written as:
    FilterResult::~FilterResult() = default;



klipper/filterresult.cpp
<https://git.reviewboard.kde.org/r/122392/#comment52506>

    nitpick: the placing of the opening braces is wrong, please see https://techbase.kde.org/Policies/Kdelibs_Coding_Style#Braces



klipper/filterresult.cpp
<https://git.reviewboard.kde.org/r/122392/#comment52510>

    suggestion: make it an inline method



klipper/historyfilter.h
<https://git.reviewboard.kde.org/r/122392/#comment52513>

    I do not really understand why a QThreadPool is used at all if it's only used with one thread. It somehow looks strange to me.



klipper/historyfilter.cpp
<https://git.reviewboard.kde.org/r/122392/#comment52512>

    maybe move it to a header?



klipper/historyfilter.cpp
<https://git.reviewboard.kde.org/r/122392/#comment52514>

    no need to pass the ", 0", it's the default argument of QThreadPool::start



klipper/historyfilter.cpp
<https://git.reviewboard.kde.org/r/122392/#comment52511>

    why do you delete the threadPool? Both QThreadPool and HistoryFilter are QObjects, and you pass HistoryFilter as parent to threadPool, so it will get deleted automatically.


- Martin Gräßlin


On Feb. 8, 2015, 6: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, 6: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/20150216/585aff22/attachment-0001.html>