| Summary: | Selecting text in the navigation widget demolishes clipboard history | ||
|---|---|---|---|
| Product: | [Applications] kdevelop | Reporter: | Tianshu Feng <me> |
| Component: | general | Assignee: | kdevelop-bugs-null |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | igorkuo, mail |
| Priority: | NOR | ||
| Version First Reported In: | 5.13.240202 | ||
| Target Milestone: | --- | ||
| Platform: | Arch Linux | ||
| OS: | Linux | ||
| Latest Commit: | https://invent.kde.org/kdevelop/kdevelop/-/commit/134fd24d0ec5f983d9da65dfe27dcab1801c3f81 | Version Fixed In: | 5.15.240800 |
| Sentry Crash Report: | |||
| Attachments: | Selection made in KDevelop and the resulting clipboard history | ||
|
Description
Tianshu Feng
2024-05-06 06:38:15 UTC
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 |