> On Feb. 3, 2015, 8: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 > > Filip Wieladek wrote: > 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 ;)
sorry for the delay - I have Internet connectivity problems at the moment. I'll look into the review as soon as my Internet works properly again. - Martin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122392/#review75260 ----------------------------------------------------------- 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