-
Notifications
You must be signed in to change notification settings - Fork 743
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
Support additional OSC 52 clipboard types #1104
base: master
Are you sure you want to change the base?
Conversation
008c27c
to
1586486
Compare
1586486
to
e922128
Compare
will it be merged? this MR works fine on my Linux server & Mac client. |
Seems to work okay with tmux copy mode. If you are migrating from #1054, then make sure to rebuild both client & server from this PR, since the wire format changed. |
That is true about the wire format. I believe it is incompatible with the current wire format used by the master branch. I don't quite grok the wire format used by mosh and haven't been able to figure out where in the code fields used by the framebuffer get turned into bits sent on the wire. Since there hasn't been an official mosh release since clipboard support was added, I don't believe it causes any compatibility issues with previous releases. If it does introduce incompatibility, then I believe the original commit adding clipboard support would also have introduced the same sort of incompatibility issues. If this wire format compatibility is a concern, it might be possible to make this backwards compatible, but I might need some help understanding the wire format a little more. |
Confirming this fixes all remaining clipboard issues for me on a local client running macOS Catalina v10.15.6 + iTerm2 v3.3.12 + mosh v1.3.2 [build mosh-1.3.2-92-ge922128 using: It's taken over ten years to get to this point; the holy grail has finally been achieved!!! Well done, @mgulick!! |
+1 to merge |
I saw from other issues that there were a couple of blockers to a new release. Creating a pinned issue and a milestone for the next release would help communicate this. (This is offtopic to this PR though) |
@eminence another ping - would be great to have this/ and sounds like you have offers for more help to support mosh. |
Can anyone familiar with this PR see if 4b240ac resolves the same issues noted in the first message? |
No, this PR builds on that commit to add some additional functionality that is missing. |
This almost works. But I cannot make it work in one scenario What worked
successfully remote copy to local clipboard. But if I put this in ~/.tmux.conf.
(~/bin/yank is copied from this well-known script: https://sunaku.github.io/tmux-yank-osc52.html). However, in a ssh session, this works without any issue. I tweaked it further.
then in another window, if I I dont know what the root cause here. but hunch is related to tmux's pane_tty. |
Hi @delbao, what happens if you change the |
Tried SSH_TTY, nothing changed. This issue is specific to this patch of mosh. SSH works fine. $ tmux -V |
@delbao Here is a copy of my
(disclaimer: the following info may be out of date) Older(?) tmux needs the last line because:
|
thanks for the help. However, sunaku's script already did what you described. it doesn't work for Mosh with this patch. |
bumping this thread b/c this pr saved me hours of time. working flawlessly on a debian vps connected from my ipad using blink shell in zsh, tmux, and neovim using osc52 tooling |
Support additional OSC 52 clipboard types
I have applied the patch but when copying to clipboard in emacs I get this error
But it can be fixed if I start emacs like so unset SSH_TTY; emacs Anyway, thanks for this PR, it works now but not out of the box. |
Scrolled through this thread but failed to figure out: Is there any good reason why this was not merged? If this repo is abandoned, is there another more actively-maintained fork? |
Hi, This PR hasn't been merged for two reasons:
Just because there are PRs open doesn't mean the repository is abandoned. |
I absolutely respect maintainers' right to handle any PRs in any way. Glad to know there's no technical reason against this PR so far. OSC 52 is not working for me (and many others I believe), and this is the only blocker for me to use mosh. I hope maintainers consider prioritizing this PR. After all it's a relatively small code change for such an important feature. To be fair, the second reason doesn't stand to me - the merge conflicts are more of a consequence, not reason, of that this PR hasn't been got to for 3+ years. |
I'm not a maintainer, but if someone (original author or otherwise) can get the PR to a state that builds/patches cleanly, I'm happy to do a code review. I think this feature would be phenomenal to add to mosh. |
@markx fwiw, i build and use this branch in production with no issues (for me). to the maintainers, i'd love to help get this pr's conflicts resolved. if i was to take a look, should i be working against master or a wip release branch? thanx. |
I've been meaning to update this PR. I'll take a look ASAP at rebasing these commits onto master. One concern that I had was whether this change constitutes a backwards-incompatible protocol change, and whether that is a problem that needs to be handled gracefully. |
The OSC 52 escape sequence supports specifying which X selection buffer place to the selection into. The protocol format is: \033]52;C;D\007 The C parameter determines the selection buffer. It consists of zero or more of the following characters: c: CLIPBOARD p: PRIMARY q: SECONDARY s: XTerm-selected (based on XTerm.vt100.selectToClipboard) 0-7: Numbered cut buffer The default if left blank is 's0', representing the configurable primary/clipboard selection and cut buffer 0. [1] D is the base64 encoded text to place in the selection buffer. This patch modifies the transferred clipboard data to include both the selection parameter and the base64 text. I.e. previously the transferred clipboard data only contained 'D', and now it contains 'C;D'. To test this functionality: 1. Open XTerm 2. Ctrl-Right Click and select 'Allow Window Ops' 3. Connect to a server w/ mosh 4. Run the following in the remote connection: $ printf "\033]52;c;$(echo test1234 | base64)\007" $ printf "\033]52;p;$(echo test2345 | base64)\007" $ printf "\033]52;q;$(echo test3456 | base64)\007" 6. Open another terminal on the local machine and run: $ xclip -o -selection clipboard test1234 $ xclip -o -selection primary test2345 $ xclip -o -selection secondary test3456 7. You can also try: $ printf "\033]52;;$(echo testdefault | base64)\007" (This should update either the clipboard or primary selection based on the Xterm settings) 8. To test the cut buffers you can use the same procedure, substituting the cut buffer number in C, and then use 'xcutsel' to transfer the data from the cut buffer to the PRIMARY selection, where it can then be viewed with 'xclip'. Note, I observed that XTerm does not currently (as of XTerm patch 358) support specifying more than one value in C. The specification does support it, and this appears to just be a simple bug in XTerm (a patch has been submitted to the maintainer to fix it). [1] https://github.com/ThomasDickey/xterm-snapshots/blob/master/ctlseqs.txt Fixes mobile-shell#967.
Instead of using the contents of the clipboard to determine if the user has copied any text, use a sequence number that is updated whenever text is copied. Consider the following scenario (as described in mobile-shell#1090): 1. User copies text 'abc' on remote machine via mosh. 2. User copies text 'xyz' on local machine. 3. User copies text 'abc' on remote machine again. The local clipboard will still has 'xyz' because the most recent copy text 'abc' matches the last text copied via mosh, so it does not detect that the user copied new text and does not update the local clipboard. This patch updates detection of newly copied text via a sequence number. This number is an 8-bit unsigned integer that is updated every time new text is copied. This will roll over after 256 clipboard updates, but that is fine as we only care about it being different than the last value. Fixes mobile-shell#1090. Fixes mobile-shell#637.
e922128
to
e8337ef
Compare
I just rebased these patches onto master. There were no "real" conflicts, just some indentation changes. I built these changes again and did a quick test to confirm they still appear to be working. I've been using these patches in my daily workflow for a few years, albeit on an older mosh version based on the original PR patch series. The XTerm bug mentioned at the end of the PR description has been fixed as of xterm version 359. I did a quick test using this patched mosh and an unpatched server. While clipboard syncing does not work (to be expected), mosh itself appears to function normally, so maybe there are no backwards-compatibility issues with this change. |
I'd like to try out this PR, but I cannot easily compile the client. I can easily compile the server, though. Is that enough? |
@joaotavora both the client and server need this change, however you don't need to install them in the usual locations. As long as you have the
This does not require updating the |
Thanks @mgulick. Though somehow, compiling just the server worked. at least for very limited testing. I was able to copy to the clipboard from Emacs and paste in my host's browser. I can see the client binary linking in the terminal subsystem though, but I don't know enough of the architecture to understand why that is or why my test worked. |
@joaotavora I think on recent mosh releases, standard clipboard copying will work without this patch. I'm able to do the standard clipboard updates going from a 1.4.0 client to a 1.4.0 server without this patch. However the other issues, such as primary, secondary, and cutbuffer support, as well as the issue with duplicate clipboard updates not getting properly synchronized (#1090), will still exist. |
Thanks, could have sworn it didn't work before I compiled and installed your version though, maybe I misdiagnosed 👍 |
so i've been using the older version of this patch for a long time with no issues beyond when copying higher level unicode and emojis. strings like this would not be correctly copied to the clipboard instead replaced with a placeholder for each character.
|
@ezzieyguywuf If you have permission to merge, PTAL |
@achernya any news on this PR merge possibility ?? |
@mgulick Clipboard does not work for me when using mosh 1.4.0 on both client and server. It works with SSH. Perhaps something relevant about this, is that I'm using it on a fairly old CentOS 7 server. |
Fixes #967.
Fixes #1090.
Fixes #637.
NOTE: This pull request is similar to #1054, but improves it by preserving the selection buffer into which to place the copied text. #1054 always places the copied text into CLIPBOARD, but this preserves the OSC 52 parameter so that the copied text may go into CLIPBOARD, PRIMARY, SECONDARY, or one of the cut buffers. This matches the behavior of
ssh
and plain XTerm.The OSC 52 escape sequence supports specifying which X selection buffer to place the selection into. The protocol format is:
The C parameter determines the selection buffer. It consists of zero or more of the following characters:
c: CLIPBOARD
p: PRIMARY
q: SECONDARY
s: XTerm-selected (based on XTerm.vt100.selectToClipboard)
0-7: Numbered cut buffer
The default if left blank is 's0', representing the configurable primary/clipboard selection and cut buffer 0. [1]
D is the base64 encoded text to place in the selection buffer.
This patch modifies the transferred clipboard data to include both the selection parameter and the base64 text. I.e. previously the
transferred clipboard data only contained 'D', and now it contains 'C;D'.
To test this functionality:
$ printf "\033]52;c;$(echo test1234 | base64)\007"
$ printf "\033]52;p;$(echo test2345 | base64)\007"
$ printf "\033]52;q;$(echo test3456 | base64)\007"
$ printf "\033]52;;$(echo testdefault | base64)\007"
(This should update either the clipboard or primary selection based on the Xterm settings)
xcutsel
to transfer the data from the cut buffer to the PRIMARY selection, where it can then be viewed withxclip
.Note, I observed that XTerm does not currently (as of XTerm patch 358) support specifying more than one value in C. The specification does support it, and this appears to just be a simple bug in XTerm (a patch has been submitted to the maintainer to fix it).
[1] https://github.com/ThomasDickey/xterm-snapshots/blob/master/ctlseqs.txt
This PR also adds a clipboard sequence number to allow duplicate copied text to update the client's clipboard.