Skip to content
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

Merged
merged 6 commits into from May 23, 2019

Conversation

@blink1073
Copy link
Member

@blink1073 blink1073 commented May 23, 2019

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. Pressing Ctrl+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.

image

kudos to @williamstein for the idea and the sample code!

Backwards-incompatible changes

None

@jupyterlab-dev-mode
Copy link

@jupyterlab-dev-mode jupyterlab-dev-mode bot commented May 23, 2019

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@blink1073 blink1073 changed the title Add smart copy/paste and update docs Add smart copy/paste in terminal and update docs May 23, 2019

term.attachCustomKeyEventHandler(event => {
if (
(event.ctrlKey || event.metaKey) &&
Copy link
Contributor

@jasongrout jasongrout May 23, 2019

This blocks using Ctrl+C on a mac unilaterally. Instead, I think we should do a mac test, like we do in

if ((IS_MAC && event.metaKey) || (!IS_MAC && event.ctrlKey)) {

Copy link
Contributor

@jasongrout jasongrout May 23, 2019

(note that we can use phosphor's IS_MAC instead of defining our own if we import Platform from phosphor's domutils)

Copy link
Contributor

@jasongrout jasongrout May 23, 2019

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": {
Copy link
Contributor

@jasongrout jasongrout May 23, 2019

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.

Copy link
Member Author

@blink1073 blink1073 May 23, 2019

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.

Copy link
Member Author

@blink1073 blink1073 May 23, 2019

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.

Copy link
Contributor

@jasongrout jasongrout May 23, 2019

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.

Copy link
Member Author

@blink1073 blink1073 May 23, 2019

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.

Copy link
Member Author

@blink1073 blink1073 May 23, 2019

With documentation to that effect.

Copy link
Contributor

@jasongrout jasongrout May 23, 2019

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.

Copy link
Member Author

@blink1073 blink1073 May 23, 2019

But that doesn't tell you anything about what Ctrl+C does. That name is not read-able at all.

Copy link
Member Author

@blink1073 blink1073 May 23, 2019

Let me update the PR and see what you think.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented May 23, 2019

Thanks. I like addressing this in the docs. I think take out the || event.metaKey in each of the tests and then we are good to go.

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented May 23, 2019

All set!

Copy link
Contributor

@jasongrout jasongrout left a comment

LGTM!

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented May 23, 2019

Thanks again, @williamstein!

big_yes

@jasongrout jasongrout merged commit ef71f26 into jupyterlab:master May 23, 2019
9 checks passed
@blink1073 blink1073 deleted the terminal-copy-paste branch Jun 2, 2019
@lock
Copy link

@lock lock bot commented Aug 6, 2019

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.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants