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
Changes from 1 commit
901efa0
1bfae50
456549f
3132d85
c6b1ac2
08a9808
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -237,7 +237,8 @@ export class Terminal extends Widget implements ITerminal.ITerminal { | |||
* Initialize the terminal object. | ||||
*/ | ||||
private _initializeTerm(): void { | ||||
this._term.on('data', (data: string) => { | ||||
const term = this._term; | ||||
term.on('data', (data: string) => { | ||||
if (this.isDisposed) { | ||||
return; | ||||
} | ||||
|
@@ -247,9 +248,32 @@ export class Terminal extends Widget implements ITerminal.ITerminal { | |||
}); | ||||
}); | ||||
|
||||
this._term.on('title', (title: string) => { | ||||
term.on('title', (title: string) => { | ||||
this.title.label = title; | ||||
}); | ||||
|
||||
term.attachCustomKeyEventHandler(event => { | ||||
if ( | ||||
(event.ctrlKey || event.metaKey) && | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the updates. Now we can take out |
||||
event.key === 'c' && | ||||
term.hasSelection() | ||||
) { | ||||
// Return so that the usual OS copy happens | ||||
// instead of interrupt signal. | ||||
return false; | ||||
} | ||||
|
||||
if ( | ||||
(event.ctrlKey || event.metaKey) && | ||||
event.key === 'v' && | ||||
this._options.pasteWithCtrlV | ||||
) { | ||||
// Return so that the usual paste happens. | ||||
return false; | ||||
} | ||||
|
||||
return true; | ||||
}); | ||||
} | ||||
|
||||
/** | ||||
|
@@ -303,7 +327,7 @@ export class Terminal extends Widget implements ITerminal.ITerminal { | |||
} | ||||
} | ||||
|
||||
private _term: Xterm; | ||||
private readonly _term: Xterm; | ||||
private _needsResize = true; | ||||
private _termOpened = false; | ||||
private _offsetWidth = -1; | ||||
|
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.
Standard browser behavior is that Ctrl+V doesn't paste on the mac.
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.