----------------------------------------------------------- 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 > >
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
