Created attachment 169236 [details] Selection made in KDevelop and the resulting clipboard history SUMMARY Selecting text in the navigation widget would _automatically_ copy the selection to the clipboard, which is nice to have, except that each and every time the selection changes, a new copy of the current selection would be copied to the clipboard **immediately**, even if I'm still dragging (to select more text). This means if I were to select something from the navigation widget, my clipboard history would probably look something like this: ``` The QNetworkRequest The QNetworkReques The QNetworkReque The QNetworkRequ The QNetworkRe The QNetworkR The QNetwork ...... ``` Which is really inconvenient, not only because I'll have many unnecessary entries in my clipboard history, but also because one could easily (and quite quickly!) lost content saved in clipboard history if the clipboard manager has a history size (e.g. Klipper). The "correct" behavior should probably be to copy the selected text _once_ **after** the selection has completed (i.e. mouse button has released), which seems to be the behavior of "Always save in history" in Klipper, but I'm not entirely sure about that. Side note: it is also not clear, from the hints in the navigation widget, that such auto-copy feature exists. STEPS TO REPRODUCE 1. Open KDevelop 2. Create or open any project 3. Move the mouse cursor to any keywords/variables/errors 4. Select any text in the the navigation widget popup 5. Check clipboard history (e.g. with Klipper in Plasma) OBSERVED RESULT One copy is made each time the selection has changed. EXPECTED RESULT Only one copy should be made. SOFTWARE/OS VERSIONS Linux/KDE Plasma: Arch Linux, KDE Plasma w/ Wayland KDE Plasma Version: 6.0.4 KDE Frameworks Version: 6.1.0 Qt Version: 6.7.0 ADDITIONAL INFORMATION I did a quick search and found the following code, which is likely responsible for this behavior: https://invent.kde.org/kdevelop/kdevelop/-/blob/master/kdevplatform/language/duchain/navigation/abstractnavigationwidget.cpp#L99 ``` connect(d->m_browser.data(), &QTextEdit::selectionChanged, this, [this]() { Q_D(AbstractNavigationWidget); d->m_browser->copy(); }); ```
I completely agree with your analysis. And you found the responsible code. A quick search turned up nothing else like this auto-copying feature in KDevelop code. Git blame reveals that it was introduced in https://phabricator.kde.org/D7157. I'll be happy to review a merge request at https://invent.kde.org/kdevelop/kdevelop/-/merge_requests that prevents clipboard history flooding and/or makes the feature more discoverable.
Maybe the issue here is rather that text is copied to the wrong clipboard? I think the expected behaviour on unix-like systems would be copying the text to the `QClipboard::Selection` clipboard, but instead this code uses `QTextEdit::copy()` which apparently copies to `QClipboard::Clipboard`. As far as I'm aware the "selection" clipboard (which is pasted with MMB) doesn't usually have a history, so the flooding also shouldn't matter.
(In reply to Sven Brauch from comment #2) > Maybe the issue here is rather that text is copied to the wrong clipboard? I > think the expected behaviour on unix-like systems would be copying the text > to the `QClipboard::Selection` clipboard, but instead this code uses > `QTextEdit::copy()` which apparently copies to `QClipboard::Clipboard`. Copying to the usual clipboard was probably intended by the author of the code under discussion. The selection clipboard is populated automatically (as in all other applications), even with that connection to QTextEdit::selectionChanged commented out (just tested). Kevin Funk commented on the first review of the feature - https://phabricator.kde.org/D5794: > Also note: Selecting the next, then pressing the middle mouse button already does work. > This is not discoverable on other platforms than Linux though. We don't care much about and barely support non-GNU/Linux platforms nowadays. But selection clipboard may work only on X11, not Wayland (cannot verify because I don't use Wayland). If it turns out that selection *does* more or less work on Wayland, the feature (connection) could be removed or limited to Windows and macOS. Also just tested: with the connection commented out, Ctrl+C hides the navigation widget and copies text that is selected in the editor, not navigation widget. That is, if selection clipboard is unavailable, there is no way (that I can see) to copy text from the navigation widget.
Sven, maybe we should restore the initial implementation with a button at https://phabricator.kde.org/D7157?id=17766, which Kevin Funk had suggested but you rejected?
Selection clipboard also works on Wayland, I tried it here. You are right, that works even without this code for the navigation popup, I wasn't aware. I would much prefer removing the feature completely over adding a button. The navigation widget is shown a lot, is already *very* cluttered, and copying text from it isn't something that happens very often. If copying (beyond the selection clipboard) is needed, IMO it should go into the context menu (which doesn't exist right now, but could be added). (Also, for context: IIRC this change was done by a GSOC student back then as part of a getting-into-the-codebase onboarding process. I wouldn't put too much thought into what the original intention of the change was, since certainly it was in part "make a change".)
(In reply to Sven Brauch from comment #5) > Selection clipboard also works on Wayland, I tried it here. You are right, > that works even without this code for the navigation popup, I wasn't aware. I was pretty sure selection clipboard doesn't work in Wayland. But apparently it does for a while (see Bug 422426) and is even specified in a Wayland protocol (though unstable): https://wayland.app/protocols/primary-selection-unstable-v1. > I would much prefer removing the feature completely over adding a button. > The navigation widget is shown a lot, is already *very* cluttered, and > copying text from it isn't something that happens very often. > > If copying (beyond the selection clipboard) is needed, IMO it should go into > the context menu (which doesn't exist right now, but could be added). I agree that the feature belongs to the context menu and that the original feature can be removed for now to fix this bug. Tianshu Feng, what's your take on this plan?
(In reply to Igor Kushnir from comment #6) > I was pretty sure selection clipboard doesn't work in Wayland. But > apparently it does for a while (see Bug 422426) and is even specified in a > Wayland protocol (though unstable): > https://wayland.app/protocols/primary-selection-unstable-v1. I was reading some code of Kilpper (and KSystemClipboard) and indeed KSystemClipboard was also using selection and primary_selection in wlr-data-control to notify Klipper under Wayland. I was confused about the "selection" and "clipboard" clipboard; reading your comments definitely helped me clearing it up a bit more. > I agree that the feature belongs to the context menu and that the original > feature can be removed for now to fix this bug. Tianshu Feng, what's your > take on this plan? I think it's a good solution (at least for now). Thank you all for your insights on this problem!
So what's left to be done: 1. Revert https://commits.kde.org/kdevelop/dadaf13191e50cc99c3420f274f5e857f3050f06 2. Transfer our conclusions from here into the commit message and reference this bug as `BUG: 486656`. 3. Create a merge request. Should we rush the feature removal into the upcoming release https://community.kde.org/Schedules/KDE_Gear_24.05_Schedule or do it in master and release in August https://community.kde.org/Schedules/KDE_Gear_24.08_Schedule ?
A possibly relevant merge request was started @ https://invent.kde.org/kdevelop/kdevelop/-/merge_requests/564
(In reply to Igor Kushnir from comment #8) > Should we rush the feature removal into the upcoming release > https://community.kde.org/Schedules/KDE_Gear_24.05_Schedule or do it in > master and release in August > https://community.kde.org/Schedules/KDE_Gear_24.08_Schedule ? Too late for the 24.05 release now. Created a merge request based on master. Please review.
Git commit 134fd24d0ec5f983d9da65dfe27dcab1801c3f81 by Igor Kushnir. Committed on 22/05/2024 at 10:01. Pushed by igorkushnir into branch 'master'. Revert "Add copy button in AbstractNavigationWidget" When text is selected in a navigation widget (e.g. editor tooltip, Code Browser tool view, Quick Open or Outline item details), KDevelop automatically copies the selection to clipboard. The copying occurs continuously while the user selects text, so clipboard history ends up containing not only the final selection, but also all intermediate selections - all but one character, all but two characters, ..., the first selected character. This behavior not only clutters clipboard history with useless entries, but can also quickly overwrite useful clipboard entries if the clipboard manager limits its history size. The auto-copy feature is unexpected and undocumented. It is also redundant on most GNU/Linux and *BSD systems, because primary selection works naturally and automatically in X11 and in most modern Wayland compositors: https://wayland.app/protocols/primary-selection-unstable-v1 If someone needs to copy text from a navigation widget on other operating systems, the feature can be reintroduced but in the widget's context menu, where it would be discoverable and would not clutter clipboard history. This reverts commit dadaf13191e50cc99c3420f274f5e857f3050f06. FIXED-IN: 5.15.240800 M +0 -4 kdevplatform/language/duchain/navigation/abstractnavigationwidget.cpp https://invent.kde.org/kdevelop/kdevelop/-/commit/134fd24d0ec5f983d9da65dfe27dcab1801c3f81