> On Feb. 3, 2015, 6:57 a.m., Martin Gräßlin wrote: > > As I worked on that just yesterday: you cannot filter in a different thread > > as Klipper is not thread save. Please see > > https://git.reviewboard.kde.org/r/122382/ for how I solved the problem. I > > suggest to rebase the patch on top of it and also lock while it's filtering. > > > > The obvious question would then be: why filter in a thread at all if the > > main gui thread needs to be blocked? > > > > Personally I think the change is too large for the legacy code base. > > Especially doing the filtering this way looks strange to me given that the > > History is nowadays backed by a model. So it would be way easier IMHO to > > introduce a QSortFilterProxyModel in between. By having the a > > QSortFilterProxyModel in between the filtering could be done there, maybe > > it's even possible for the popup to get rid of the usage of the History > > completly and just build up on the model? That's something I didn't look > > into as I only wanted to get the dataengine running and not touching the > > legacy way. If there is no development on the legacy way, it might be > > better to go that road. > > Filip Wieladek wrote: > Hi Martin, > > I'll make sure to lock while filtering. > > To answer your question: When we run in a background thread, we do not > lock the main gui thread. We update the results using the signal, which means > that the GUI thread is never blocked. We never have issues of the UI being > "sluggish", which leads to bad user experience. > > I would love to use QSortFilterProxyModel, since reusing existing > infrastructure is always better than coming up with something new. But as far > as I see, QSortFilterProxyModel does not support filtering in the background. > In the patch, we filter using a cancellable future. On each new press, we > cancel the filtering and it starts a new. This is all to prevent the UI from > starting to feel sluggish. > > What do you mean by legacy way? I understand that klipper now has a > plasmoid, but unless I can open the plasmoid at the mouse cursor location, > the plasmoid is pretty much useless for my daily workflow. I use a two > monitor setup, and going from one screen to the other to access klipper is > not the fastest way. At that point I might as well not use the history at > all. If you say that there is an alternative to this workflow and is supposed > to be replaced, then I will hapilly look into it. > > Martin Gräßlin wrote: > yes, I consider the klipper popup as the legacy way. It's only still used > for the popup at location as the other code is not yet written. One could > also go for opening the plasmoid when the shortcut is involved and ensure > that the plasmoid has a good filtering experience. I assume for such a > workflow it should be keyboard driven and the fact that it doesn't open close > to the mouse shouldn't matter. Or am I wrong on that?
Well, for me it would be a usability regression. As previously mentioned, I have a two screen setup (and big screens are getting more common). Usually I focus my eyes close where the mouse is. If the popup now opens on a different screen in a different place, then it would distract my workflow significantly (I need to focus my eyes somewhere else, making a "context switch") - Filip ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122392/#review75255 ----------------------------------------------------------- On Feb. 3, 2015, 6:27 a.m., Filip Wieladek wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/122392/ > ----------------------------------------------------------- > > (Updated Feb. 3, 2015, 6:27 a.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 > ------- > > > Thanks, > > Filip Wieladek > >
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
