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

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

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

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

Copy link
Contributor

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)

Copy link
Contributor

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

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.

Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Member Author

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.

@jasongrout
Copy link
Contributor

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

All set!

Copy link
Contributor

@jasongrout jasongrout left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@blink1073
Copy link
Member Author

Thanks again, @williamstein!

big_yes

@jasongrout jasongrout merged commit ef71f26 into jupyterlab:master May 23, 2019
@blink1073 blink1073 deleted the terminal-copy-paste branch June 2, 2019 09:51
@lock
Copy link

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
@jasongrout jasongrout added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Aug 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement pkg:terminal status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Examining workarounds for copy-paste behavior in the terminal. Document copy/paste shortcuts in terminal
2 participants