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
Add smart copy/paste in terminal and update docs #6391
Conversation
Thanks for making a pull request to JupyterLab! To try out this branch on binder, follow this link: |
packages/terminal/src/widget.ts
Outdated
|
||
term.attachCustomKeyEventHandler(event => { | ||
if ( | ||
(event.ctrlKey || event.metaKey) && |
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.
This blocks using Ctrl+C on a mac unilaterally. Instead, I think we should do a mac test, like we do in
jupyterlab/packages/filebrowser/src/listing.ts
Line 1241 in ce4c47a
if ((IS_MAC && event.metaKey) || (!IS_MAC && event.ctrlKey)) { |
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.
(note that we can use phosphor's IS_MAC instead of defining our own if we import Platform from phosphor's domutils)
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 the updates. Now we can take out || event.metaKey
@@ -59,6 +62,12 @@ | |||
"description": "Whether to shut down or not the session when closing the terminal.", | |||
"type": "boolean", | |||
"default": false | |||
}, | |||
"pasteWithCtrlV": { |
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.
pasteWithCtrlVNotMac
? Ctrl+V won't paste on a mac, so I think the option should say something about only applying on non-Mac platforms, and the actual code also check to see if it is a mac platform.
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.
As in Ctrl+V doesn't paste on a mac with this PR? If it does, I don't see a reason to make a distinction.
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.
Actually, what we could do is set the default to false if on a mac and add a separate setting for copyWithCtrlC
that is also false on a mac, I think that would be coherent.
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.
As in Ctrl+V doesn't paste on a mac with this PR? If it does, I don't see a reason to make a distinction.
Standard browser behavior is that Ctrl+V doesn't paste on the mac.
Actually, what we could do is set the default to false if on a mac and add a separate setting for
copyWithCtrlC
that is also false on a mac, I think that would be coherent.
Can we change defaults according to platform? I thought the json schema didn't allow changing defaults based on platform.
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.
Ah, shoot, I was thinking of the terminal options themselves, not the schema. I think the setting should be ignored on a Mac and we just don't add the handler at all there.
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.
With documentation to that effect.
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.
Or we could be more explicit: browserDefaultForCtrlVNotMac
or something. Instead of assuming that the browser will be pasting, just mentioning exactly what this option does, which is ignore ctrl+v in xterm.js.
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.
But that doesn't tell you anything about what Ctrl+C does. That name is not read-able at all.
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.
Let me update the PR and see what you think.
Thanks. I like addressing this in the docs. I think take out the |
All set! |
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.
LGTM!
Thanks again, @williamstein! |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related discussion. |
References
Fixes #1146.
Fixes #6385.
Code changes
Adds Ctrl+C/Ctrl+V event handler to
xterm
for clients not on macOS, and a setting for whether to enable pasting with Ctrl+V. PressingCtrl+C
with no selection when using a *nix shell will send SIGINT as usual.User-facing changes
Users can use Ctrl+C on non-macOS platforms to copy selected text. By default, Ctrl+V will paste unless
pasteWithCtrlV
is disabled.kudos to @williamstein for the idea and the sample code!
Backwards-incompatible changes
None