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

Support bracketed paste mode #1097

Merged
merged 1 commit into from
Nov 9, 2017
Merged

Conversation

jdanbrown
Copy link
Contributor

Repro:

  • Run ipython (version 6.2.1)
  • Copy multiple lines of text and paste into ipython prompt, e.g.
print(1)
print(2)
print(3)
  • Before: first line would paste and run, and subsequent lines are lost
  • After: all lines paste into single prompt

@jdanbrown jdanbrown force-pushed the pr-bracketedpaste branch 2 times, most recently from c7f4855 to 5986c09 Compare October 30, 2017 07:08
- As described in https://cirw.in/blog/bracketed-paste
- Example support in vim:
  - https://github.com/ConradIrwin/vim-bracketed-paste/blob/master/plugin/bracketed-paste.vim
  - http://vimdoc.sourceforge.net/htmldoc/term.html#raw-terminal-mode
- Required for e.g. ipython, via python-prompt-toolkit:
  - https://github.com/ipython/ipython
  - https://github.com/jonathanslenders/python-prompt-toolkit

Repro:
- Run `ipython` (version 6.2.1)
- Copy multiple lines of text and paste into ipython prompt, e.g.
```py
print(1)
print(2)
print(3)
```
- Before: first line would paste and run, and subsequent lines are lost
- After: all lines paste into single prompt
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Really well put together PR! Just some minor comments

@@ -52,6 +63,7 @@ export function pasteHandler(ev: ClipboardEvent, term: ITerminal): void {

let dispatchPaste = function(text: string): void {
text = prepareTextForTerminal(text, term.browser.isMSWindows);
Copy link
Member

Choose a reason for hiding this comment

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

Let's merge bracketTextForPaste into prepareTextForTerminal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Will do, but I might not have time to get to it before the weekend, so feel free to take it over in the meantime.

* Bracket text for paste, if necessary, as per https://cirw.in/blog/bracketed-paste
* @param text The pasted text to bracket
*/
export function bracketTextForPaste(text: string, bracketedPasteMode: boolean): string {
Copy link
Member

Choose a reason for hiding this comment

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

A test in Clipboard.test.ts that tests either this function of pasteHandler would be great

@Tyriar Tyriar self-assigned this Oct 30, 2017
@Tyriar Tyriar added this to the 3.0.0 milestone Oct 30, 2017
@mofux
Copy link
Contributor

mofux commented Oct 30, 2017

Very nice contribution. One general question that comes to my mind: Shouldn't this mode be specific to the buffer (normal / alt) that activated it? Say we open vim and it sends the sequence to enable this mode, If vim is killed from the outside (say killall vim), the bracketed paste mode will still be enabled for the remaining lifetime of the session in both buffers, which could cause weird behaviour... 🤔

@jdanbrown
Copy link
Contributor Author

@mofux Generally yes, but afaik there's not a good solution to that problem. Terminal processes that set bracketed paste mode do make efforts to unset it on exit—so actually killall vim should successfully unset bracketed paste mode—but in other situations like killall -9 vim the process has no chance to do anything on exit, so the terminal will be stuck in bracketed paste mode and there's not really much to do about it except reset / restart the terminal.

@Tyriar
Copy link
Member

Tyriar commented Oct 30, 2017

@mofux you bring up a good point, I'm not sure we unset all the mode-related stuff when switching away from the alt buffer though. Only savedX, savedY and tabs seem to be stored on a per-buffer basis:

https://github.com/sourcelair/xterm.js/blob/8ece306dbe9c7aee13cce9c60fae74202f8660ee/src/Buffer.ts#L31-L33

@mofux
Copy link
Contributor

mofux commented Oct 30, 2017

@Tyriar Do you think the modes (like the mouse modes) should actually depend on the buffer? Didn't find anything meaningful about it on the web, but if they do, we should probably stores these modes with the buffer instead of setting them on the terminal instance...

@Tyriar
Copy link
Member

Tyriar commented Oct 30, 2017

@mofux we should test how other terminals handle this (or how xterm does) before changing anything.

@mofux
Copy link
Contributor

mofux commented Oct 30, 2017

Yeah, so it looks like the only stuff that should be stored on a per-buffer basis is the cursor position (which can be saved & restored).

I've checked the xterm, libvte and iterm2 sources as well as this xterm sequences guide, and couldn't find any hint to store / reset the mode settings with the buffer. Seems that one has to rely on the terminal application to correctly reset modes that have previously been set (as already stated above by @jdanbrown).

@jdanbrown
Copy link
Contributor Author

In case another example terminal is helpful, iTerm2 on OSX has handled bracketed paste mode very well for me in practice.

jdanbrown added a commit to jdanbrown/atom-xterm that referenced this pull request Nov 5, 2017
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Looks great, should land in v3.0.0 😃

@Tyriar Tyriar merged commit bc424b2 into xtermjs:v3 Nov 9, 2017
@jdanbrown jdanbrown deleted the pr-bracketedpaste branch November 9, 2017 20:08
@jdanbrown
Copy link
Contributor Author

Thanks! Looking forward to v3!

@Tyriar
Copy link
Member

Tyriar commented Nov 11, 2017

@jdanbrown you can track progress on the release here #1116

It's our biggest release ever so it's getting delayed a little 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants