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 support for bracketed paste #3871

Closed
wants to merge 2 commits into from
Closed

Conversation

@faho
Copy link
Member

@faho faho commented Feb 25, 2017

This is a terminal feature where pastes will be "bracketed" in
\e[200~ and \e[201~.

It is more of a "security" measure (since particularly copying from a
browser can copy text different from what the user sees, which might
be malicious) than a performance optimization.

Fixes #967.


I'd love to see some testing on this, particularly on weird terminals (especially those on OSs I don't have access to) and older versions of emacs' "ansi-term" (I think macOS ships an ancient pre-GPL3 version?).

TODOs:

  • [N/A] Changes to fish usage are reflected in user documentation/manpages.
    Ideally, support is ubiquitous and documentation is unnecessary. We also want to downplay this as \cv is better.
  • [N/A] Tests have been added for regressions fixed
    No regressions fixed, tests have been neutered as I don't know how to test since it depends on the terminal.
  • User-visible changes noted in CHANGELOG.md
This is a terminal feature where pastes will be "bracketed" in
\e\[200~ and \e\[201~.

It is more of a "security" measure (since particularly copying from a
browser can copy text different from what the user sees, which might
be malicious) than a performance optimization.

Fixes #967.
@faho faho added the enhancement label Feb 25, 2017
@faho faho added this to the 2.6.0 milestone Feb 25, 2017
Copy link
Contributor

@krader1961 krader1961 left a comment

This works for me using iTerm2 and Terminal.app on macOS Sierra.

@@ -232,6 +232,21 @@ function __fish_config_interactive -d "Initializations that should be performed
# Load key bindings
__fish_reload_key_bindings

if not set -q FISH_UNIT_TESTS_RUNNING

This comment has been minimized.

@krader1961

krader1961 Feb 26, 2017
Contributor

LOL! It's nice to see another use for that var so soon after I added it.

if not set -q FISH_UNIT_TESTS_RUNNING
# Enable bracketed paste before every prompt (see __fish_shared_bindings for the bindings).
# Disable it for unit tests so we don't have to add the sequences to bind.expect
function __fish_enable_bracketed_paste --on-event fish_prompt

This comment has been minimized.

@krader1961

krader1961 Feb 26, 2017
Contributor

This is a very fishy way to do this. Very cool. I was thinking we need to also emit the magic sequence to disable bracketed paste mode when exiting the shell, such as in response to a signal, but honestly that is such an unlikely scenario I think we should defer doing so until someone complains.

This comment has been minimized.

@lilyball

lilyball May 23, 2017
Member

I'm complaining 😉 It's pretty weird to me that if I start a bash shell, type fish, and exit the Fish shell, pasting is now broken.

This comment has been minimized.

@lilyball

lilyball May 24, 2017
Member

Fixed in 4ff002b.

This comment has been minimized.

@zanchey

zanchey May 25, 2017
Member

Taking for 2.6.0.

printf "\e[?2004l"
end

__fish_enable_bracketed_paste

This comment has been minimized.

@krader1961

krader1961 Feb 26, 2017
Contributor

I'm surprised this is needed. Which means it should probably have a comment explaining why it is needed.

This comment has been minimized.

@faho

faho Feb 26, 2017
Author Member

Of course it's needed - we need to tell the terminal we support bracketed paste, and since this is in __f_c_i, the first fish_prompt has already fired. Will add a comment to that effect.

This comment has been minimized.

@krader1961

krader1961 Feb 26, 2017
Contributor

It's the fact that this isn't loaded until the first fish_prompt event has fired that always trips me up. It's counterintuitive. I, and probably most people, expect autoloading this script to happen first. So I'm always surprised when I see this pattern. Hence the need for a comment.

# Re-running `bind` multiple times per mode is still faster than trying to make the list unique,
# even without calling `sort -u` or `uniq`, for the vi-bindings.
set -l allmodes default
set allmodes $allmodes (bind -a | string match -r -- '-M \w+' | string replace -- '-M ' '')

This comment has been minimized.

@krader1961

krader1961 Feb 26, 2017
Contributor

This is fragile in that it depends on the format of the output of bind -a which isn't guaranteed to not change. For example, we might decide to replace -M with --mode in the output for clarity. I recommend opening a new issue to enhance bind with a way to list the known modes and add a "TODO" comment here referencing that issue.

This comment has been minimized.

@faho

faho Feb 26, 2017
Author Member

Yes, that would be a much better way of handling this.

@krader1961
Copy link
Contributor

@krader1961 krader1961 commented Feb 28, 2017

LGTM. I forgot to mention that I've tested this on macOS using Terminal.app and iTerm2. If no one reports any problems in another four days (one week after sending this out for review) feel free to merge it. We can always revert or amend it if someone reports a problem.

@zanchey
Copy link
Member

@zanchey zanchey commented Feb 28, 2017

Do you think this needs to go into the documentation at all?

@krader1961
Copy link
Contributor

@krader1961 krader1961 commented Feb 28, 2017

I don't think we need to document this outside of the change log. The new behavior is unlikely to surprise anyone. Having said that it might be worth noting we support it in the FAQ so people searching the docs for "bracketed paste" can find a mention that we do support it and know where to look for implementation details since the latter is pretty cool.

@ridiculousfish
Copy link
Member

@ridiculousfish ridiculousfish commented Feb 28, 2017

This looks sweet. Thanks for doing this.

@floam
Copy link
Member

@floam floam commented Mar 1, 2017

Sorry, I can't test this at the moment. Does this escape the pasted output? i.e. allow someone to type wget, paste a URL, and not worry about the ?s?

@faho
Copy link
Member Author

@faho faho commented Mar 1, 2017

Does this escape the pasted output?

No. What it does is allow pasting input that contains characters bound to something, e.g. \n and such, without triggering those bindings (which is similar to escaping). It could feasibly be adjusted to do simple escaping (e.g. insert \? instead of ?). The main issue with that is that it's not entirely clear when you would want something escaped and when you wouldn't. E.g. when you paste echo *, you want to have the glob there, but when you paste some markdown with **emphasis** you don't.

Anyway, what we took from #967 was "support bracketed-paste", which isn't quite what the discussion focussed on. So we can debate whether this "fixes" it, or whether that issue should be will-not-implement or implemented.

@faho
Copy link
Member Author

@faho faho commented Mar 5, 2017

Merged as db63be7.

# This sequence ends paste-mode and returns to the previous mode we have saved before.
bind -M paste \e\[201~ 'set fish_bind_mode $__fish_last_bind_mode; commandline -f force-repaint'
# In paste-mode, everything self-inserts except for the sequence to get out of it
bind -M paste "" self-insert

This comment has been minimized.

@elliott-beach

elliott-beach Aug 8, 2017
Contributor

Is this paste mode documented anywhere? I was looking to make a plugin similar to https://www.npmjs.com/package/hyperterm-paste with fish, and need this feature to do it.

This comment has been minimized.

@faho

faho Aug 8, 2017
Author Member

@e-beach: That right there is basically the extent of our documentation on it.

For those transformations, implementing it here seems difficult. You'll want to use something like fish_clipboard_paste instead. Look at that function and modify it to your liking, then use ctrl-v instead of ctrl-shift-v to paste.

This comment has been minimized.

@elliott-beach

elliott-beach Aug 8, 2017
Contributor

Right on. Thanks!

@jamesgecko
Copy link

@jamesgecko jamesgecko commented Aug 8, 2017

Is there a way to disable bracketed paste mode? When using fish in tmux I've started getting [200~clipboard content[201~ whenever I try to paste.

@faho
Copy link
Member Author

@faho faho commented Aug 8, 2017

Currently, there is not. Which tmux version are you using in which terminal? I checked it just now in tmux 2.5 (and I had tested it before merging) and I just do not see that.

@jamesgecko
Copy link

@jamesgecko jamesgecko commented Aug 8, 2017

I'm using tmux 2.5 with fish 2.6.0 in iTerm 3.0.15. The problem popped up when I ran brew upgrade for the first time in several months; I'm pretty sure that I upgraded both fish and tmux at the same time.

@krader1961
Copy link
Contributor

@krader1961 krader1961 commented Aug 8, 2017

@jamesgecko, I'm using a similar setup and cannot reproduce your problem. Bracketed paste works for me when running fish inside tmux inside iTerm. I'm guessing you've set the tmux escape-time var to zero or close to it. In general setting that to less than 10 ms is going to cause problems.

@jamesgecko
Copy link

@jamesgecko jamesgecko commented Aug 8, 2017

I did have escape-time set to zero. I also had keybindings for using alt+[ and alt+] to tab between tmux windows. 🤦‍♂️

bind-key -n M-[ previous-window
bind-key -n M-] next-window

Thanks!

@krader1961
Copy link
Contributor

@krader1961 krader1961 commented Sep 6, 2017

@jamesgecko, fish doesn't have a bind-key command so presumably you've got a wrapper that translates what you wrote to bind \e\[ previous-window. And therein lies the problem. Bracketed paste works by having the terminal send \e\[200~ and \e\[201~. Fish implements bindings for those sequences. See type __fish_shared_key_bindings. Your binding, because it's a prefix, takes precedence which interferes with fish's handling of bracketed paste. Don't do that 😄

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2020
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.

8 participants
You can’t perform that action at this time.