-
-
Notifications
You must be signed in to change notification settings - Fork 6k
feat: support for wayland clipboard #17097
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you say a bit about why this is necessary? It seems to be 2-way, both set by the code and set by the user. When would each be appropriate? Also, is there a reason this shouldn't have a value relating to
winclip.c?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clipmethod is only really relevant to X11 and Wayland. On Windows there is just one implementation of the clipboard so we don't have to worry about that. Could you clarify what you mean by 2-way? Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I finally had a chance to test this out and I think I can better explain the disconnect I see between this setting and the rest of the implementation. I am running a wayland session with Xwayland running. Both
WAYLAND_DISPLAYandDISPLAYare set to legit values. I runvim --clean -u NONE -N.Why would a user not report this as a bug? It is not a bug per your implementation notes because this is a GNOME session. But it sure seems to be a bug to anyone just reading the documentation. I think the information being expected from the user and the information being provided to them assumes they know how their compositor works. That might be interesting to some users but I feel like it is a undue burden for most users.
On the other hand, if I run this under a sway session, I get the more logical output of
Yet, if run with
-X -Ythen I get:This also seems like a bug in a way that I think hints at a simpler approach that would be easier for users to understand. You can technically run x11 embedded in wayland or wayland embedded in x11. In either case both
DISPLAYandWAYLAND_DISPLAYcould be set simultaneously. However, users can be split into two groups:-Xand-Yoptions.wayland,x11-- hence the default you chose here.So I think this would satisfy both sets of users in a way that is simpler for each of them:
clipemethodas a user setting. Keepv:clipmethodas a feedback mechanism and for scripting/test purposes.clipmethodis used in the code now, use the implied order based on whether we are connected or willing to connect to each display.WAYLAND_DISPLAYis set, include and priortizewayland. Unless-Yis specified.DISPLAYis set, includex11. Unless-Xis specified.wayland,x11.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what you are saying is to enable wlsteal by default? Problem is that also introduces the fact that wlsteal has issues too. Since autoselect is default for clipboard option, users may be confused why the terminal loses focus seemingly randomly every time they go into visual mode. The "randomness" is actually just when Vim isn't the owner of the selection.
Not sure what you are trying to say, isn't that expected?
Correct me if I'm wrong, but I think the problem you are describing only applies to GNOME users, and enabling wlsteal by default would fix things. But that brings it's own problems (see above). Additionally, I don't see any harm in keeping the clip method option, using the -X and -Y options as a replacement I can guess could be annoying to specify it every time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm definitely not suggesting to enable
wlstealby default, even if just heuristically chosen for GNOME users. I would ratherwlstealbe removed and this wayland support being limited to places wherewlstealis not required. Alas, I'm trying to not suggest removing it either because I don't want to kill the momentum here. It is off by default, so live and let live. I'm glad to hear you also thinkwlstealis awkward and likely confusing for users :)What I am suggesting is that the awkwardness of
wlstealstems from a fundamental asymmetry between what "wayland" means as a category versus the various protocols. I think this asymmetry is being papered over byclipmethodin a very confusing way. As a user, I would expect vim to disable functionality I've told it to disable.viminvocationvim -X -Yclipmethod=nonevim -Yclipmethod=x11vim -Xclipmethod=waylandvimclipmethod=wayland,x11In essence, I've told vim I want to stay disconnected from one or both of those servers, so it should know that
clipmethodwould not need to include them.My suggestion to rely solely on
-Xand-Yinstead of having aclipmethodat all is because there's very, very few users who would setclipmethod=x11,waylandwhen running in dual setup with both display servers. On the other hand, there are a lot of users who run GNOME, will know vaguely that it is a "wayland" session (many distros brand it as such), and be very confused why settingclipmethod=wayland,x11does not yieldv:clipmethod=wayland.I guess one alternative way to say the same thing is that
wlstealshould maybe go away in favor of a non-default value ofwayland-focus-stealforclipmethod. That would at least put this contradictory asymmetric meaning in front of users when they are inspecting theclipmethodoption.Part of the confusion stems from the symmetric naming between
clipmethodandv:clipmethod. If nothing else, I'd suggest renamingclipmethodtocliporderto break this asymmetry.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for clarifying
I see, I suppose that would make sense. So you are saying to remove
clipmethodbut still keepv:clipmethod, and instead rely on heuristics and manual user intervention to handle when to use X11 or Wayland?Overall it seems like
clipmethodis the main source of confusion, maybe an alternative would be to update the documentation for it and specify whyv:clipmethodmay not be the value thatclipmethodshould imply?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that is a good synopsis.
Updating the documentation would definitely help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, so which way should we go? I'm not sure