fish-shell / fish-shell

The user-friendly command line shell.
https://fishshell.com
Other
25.99k stars 1.9k forks source link

Inserting clipboard using OSC 52 #10296

Closed name-snrl closed 8 months ago

name-snrl commented 8 months ago

Hi, @krobelus and other contributors to #9232. Are there any plans to implement pasting part?

krobelus commented 8 months ago

Here is an experimental implementation. Though I'm curious what's the use case; I believe that some terminals will ask for permission before enabling this feature, so as it stands this might make things worse.

diff --git a/doc_src/cmds/commandline.rst b/doc_src/cmds/commandline.rst
index 3473f42c3..a6f901f06 100644
--- a/doc_src/cmds/commandline.rst
+++ b/doc_src/cmds/commandline.rst
@@ -37,6 +37,9 @@ The following options are available:
     This option cannot be combined with any other option.
     See :doc:`bind <bind>` for a list of input functions.

+**-q** or **--queue**
+    Queue the additional arguments as if they are typed.
+
 **-h** or **--help**
     Displays help about using this command.

diff --git a/share/functions/fish_clipboard_paste.fish b/share/functions/fish_clipboard_paste.fish
index 8ce4991c1..65fc5c3ab 100644
--- a/share/functions/fish_clipboard_paste.fish
+++ b/share/functions/fish_clipboard_paste.fish
@@ -1,15 +1,42 @@
 function fish_clipboard_paste
     set -l data
-    if type -q pbpaste
-        set data (pbpaste 2>/dev/null | string collect -N)
-    else if set -q WAYLAND_DISPLAY; and type -q wl-paste
-        set data (wl-paste -n 2>/dev/null | string collect -N)
-    else if set -q DISPLAY; and type -q xsel
-        set data (xsel --clipboard | string collect -N)
-    else if set -q DISPLAY; and type -q xclip
-        set data (xclip -selection clipboard -o 2>/dev/null | string collect -N)
-    else if type -q powershell.exe
-        set data (powershell.exe Get-Clipboard | string trim -r -c \r | string collect -N)
+    if type -q base64 && isatty stdout
+        printf '\e]52;c;?\a'
+        set -l reply
+        set -l c
+        set -l state 1
+        set -l expected \e ] 5 2 \; c \;
+        while read -n1 c <&0
+            switch $state
+                case 1 2 3 4 5 6 7
+                    if test $c != $expected[$state]
+                        commandline --queue -- $c
+                        break
+                    end
+                    set state (math $state + 1)
+                case 8
+                    if test $c = \a
+                        break
+                    end
+                    set -a reply $c
+            end
+        end
+        if test -n "$reply"
+            set data (string join '' -- $reply | base64 -d | string collect -N)
+        end
+    end
+    if not set -q data[1]
+        if type -q pbpaste
+            set data (pbpaste 2>/dev/null | string collect -N)
+        else if set -q WAYLAND_DISPLAY; and type -q wl-paste
+            set data (wl-paste -n 2>/dev/null | string collect -N)
+        else if set -q DISPLAY; and type -q xsel
+            set data (xsel --clipboard | string collect -N)
+        else if set -q DISPLAY; and type -q xclip
+            set data (xclip -selection clipboard -o 2>/dev/null | string collect -N)
+        else if type -q powershell.exe
+            set data (powershell.exe Get-Clipboard | string trim -r -c \r | string collect -N)
+        end
     end

     # Issue 6254: Handle zero-length clipboard content
diff --git a/src/builtins/commandline.rs b/src/builtins/commandline.rs
index c96b3d2e7..06fb7a06c 100644
--- a/src/builtins/commandline.rs
+++ b/src/builtins/commandline.rs
@@ -12,7 +12,8 @@ use crate::parse_util::{
 };
 use crate::proc::is_interactive_session;
 use crate::reader::{
-    commandline_get_state, commandline_set_buffer, reader_handle_command, reader_queue_ch,
+    commandline_get_state, commandline_set_buffer, reader_handle_command,
+    reader_inject_chars_front, reader_queue_ch,
 };
 use crate::tokenizer::TOK_ACCEPT_UNFINISHED;
 use crate::tokenizer::{TokenType, Tokenizer};
@@ -183,6 +184,7 @@ pub fn commandline(parser: &Parser, streams: &mut IoStreams, args: &mut [&wstr])
     let mut append_mode = None;

     let mut function_mode = false;
+    let mut queue_mode = false;
     let mut selection_mode = false;

     let mut token_mode = None;
@@ -217,6 +219,7 @@ pub fn commandline(parser: &Parser, streams: &mut IoStreams, args: &mut [&wstr])
         wopt(L!("current-token"), woption_argument_t::no_argument, 't'),
         wopt(L!("cut-at-cursor"), woption_argument_t::no_argument, 'c'),
         wopt(L!("function"), woption_argument_t::no_argument, 'f'),
+        wopt(L!("queue"), woption_argument_t::no_argument, 'q'),
         wopt(L!("tokens-expanded"), woption_argument_t::no_argument, 'x'),
         wopt(L!("tokens-raw"), woption_argument_t::no_argument, '\x02'),
         wopt(L!("tokenize"), woption_argument_t::no_argument, 'o'),
@@ -245,6 +248,7 @@ pub fn commandline(parser: &Parser, streams: &mut IoStreams, args: &mut [&wstr])
             'j' => buffer_part = Some(TextScope::Job),
             'p' => buffer_part = Some(TextScope::Process),
             'f' => function_mode = true,
+            'q' => queue_mode = true,
             'x' | '\x02' | 'o' => {
                 if token_mode.is_some() {
                     streams.err.append(wgettext_fmt!(
@@ -293,7 +297,7 @@ pub fn commandline(parser: &Parser, streams: &mut IoStreams, args: &mut [&wstr])

     let positional_args = w.argv.len() - w.woptind;

-    if function_mode {
+    if function_mode || queue_mode {
         // Check for invalid switch combinations.
         if buffer_part.is_some()
             || cut_at_cursor
@@ -312,12 +316,26 @@ pub fn commandline(parser: &Parser, streams: &mut IoStreams, args: &mut [&wstr])
         }

         if positional_args == 0 {
-            builtin_missing_argument(parser, streams, cmd, L!("--function"), true);
+            builtin_missing_argument(
+                parser,
+                streams,
+                cmd,
+                if function_mode {
+                    L!("--function")
+                } else {
+                    L!("--queue")
+                },
+                true,
+            );
             return STATUS_INVALID_ARGS;
         }

         type rl = ReadlineCmd;
         for arg in &w.argv[w.woptind..] {
+            if queue_mode {
+                reader_inject_chars_front(arg);
+                continue;
+            }
             let Some(cmd) = input_function_get_code(arg) else {
                 streams
                     .err
diff --git a/src/builtins/read.rs b/src/builtins/read.rs
index e7467287f..72bde2018 100644
--- a/src/builtins/read.rs
+++ b/src/builtins/read.rs
@@ -606,7 +606,7 @@ pub fn read(parser: &Parser, streams: &mut IoStreams, argv: &mut [&wstr]) -> Opt
     loop {
         buff.clear();

-        if stream_stdin_is_a_tty && !opts.split_null {
+        if stream_stdin_is_a_tty && !streams.stdin_is_directly_redirected && !opts.split_null {
             // Read interactively using reader_readline(). This does not support splitting on null.
             exit_res = read_interactive(
                 parser,
diff --git a/src/reader.rs b/src/reader.rs
index 5acb96cdb..dde83eaf9 100644
--- a/src/reader.rs
+++ b/src/reader.rs
@@ -69,7 +69,7 @@ use crate::history::{
 };
 use crate::input::init_input;
 use crate::input::Inputter;
-use crate::input_common::{CharEvent, CharInputStyle, ReadlineCmd};
+use crate::input_common::{CharEvent, CharInputStyle, InputEventQueuer, ReadlineCmd};
 use crate::io::IoChain;
 use crate::kill::{kill_add, kill_replace, kill_yank, kill_yank_rotate};
 use crate::libc::MB_CUR_MAX;
@@ -877,6 +877,13 @@ pub fn reader_queue_ch(ch: CharEvent) {
     }
 }

+pub fn reader_inject_chars_front(chars: &wstr) {
+    if let Some(data) = current_data() {
+        data.inputter
+            .insert_front(chars.chars().map(CharEvent::from_char));
+    }
+}
+
 /// Return the value of the interrupted flag, which is set by the sigint handler, and clear it if it
 /// was set. If the current reader is interruptible, call \c reader_exit().
 pub fn reader_reading_interrupted() -> i32 {
name-snrl commented 8 months ago

Here is an experimental implementation.

This patch is for a rust rewritten fish, unfortunately I can't easily apply it to the latest nixpkgs to test it. I'll do so on the next release of fish in nixpkgs.

Though I'm curious what's the use case

Oh, it's very simple. I don't understand why every time we work on a remote machine we have to think about it and use an unfamiliar bindings to pasting. I just don't want to think about it. Neovim already provides osc52 and it's so wonderful when you don't have to think about whether you pressed the right button or whether what you have in your clipboard is pasted correctly. You just use your tool as usual.

So why not? Why not bring that magic to fish too?

name-snrl commented 8 months ago

At this point, I just did this simple hack and everything works fine with foot:

function fish_clipboard_paste
    commandline -i -- (osc paste)
end

https://github.com/theimpostor/osc

faho commented 8 months ago

The annoying thing here is that OSC-52 works by us printing a sequence to the terminal and then, hopefully, getting a response back.

But OSC-52 isn't supported everywhere - a cursory search shows xterm doesn't support it (this might be wrong), konsole doesn't, terminal.app doesn't, alacritty only does if you enable it (and this might be common because it might be seen as a security feature). And we can't just try OSC-52 because we could also just get nothing back. This is unlike the "copy" case because there we can just try and if nothing happens nothing happens, here we need to know if nothing happened because we would then have to try other methods.

So we would have to find out if you are running a supporting term, and that's pretty much impossible over ssh because a lot of the environment variables we would use for that won't be sent over. (yet another case of $TERM being useless and unsuited for its purpose in practice)

What we can do is add support specifically for that osc tool. I'm not sure if we should always use it - what happens if it's installed and you're using a terminal where it doesn't work? We might try it only if we're connected over ssh, where the other options aren't useful anyway?

name-snrl commented 8 months ago

But OSC-52 isn't supported everywhere

Can't we add a variable for those who want to use OSC-52 and a 5 second timeout to respond if this method is used?

faho commented 8 months ago

If we add a variable, that's the same amount of configuration as just setting the function like you did. set -g fish_use_osc_paste 1 is the same effort as function fish_clipboard_paste; commandline -i -- (osc paste); end. That means instead of adding an on/off setting we can just do nothing and tell you to make that change yourself.

And timeouts are terrible. 5 seconds means anyone using a terminal that doesn't support OSC52 gets to wait 5 seconds any time they paste something. (plus this is all implemented as a function and we do not currently have anything that reads with a timeout)

The question is what we can do by default, especially since this is most useful on systems you connect to via ssh, where you would have to redo any configuration for every system - but you might be able to get osc installed by default.


I have also found that apparently the "st" terminal mishandles (or at one point mishandled) OSC-52 pasting so badly that it cleared your clipboard instead.

Which shows that this sequence might be harmful if we try it locally.

name-snrl commented 8 months ago

If we add a variable, that's the same amount of configuration as just setting the function like you did

My bad, I didn't answer the question about the osc tool. I don't think it's a good solution to add support of osc.

  1. This is a little known tool, not many distro have it in their package base.
  2. wl-paste, pbpaste and other tools are default packages for their DE, unlike osc.
  3. I'm not sure if osc is intended for this use. Even though I said it works fine. When the paste binding (Ctrl+v) is pressed for a long time, an error appears reporting that osc is being used incorrectly.

Going back to my last reply, I think this should be a built-in feature that people could enable at their discretion (those who are sure their terminal supports OSC-52).

As for the timeout, it should be just a precaution for those people who enabled the feature, but for some reason their terminal did not return anything in response, so as not to get into an infinite wait. In Neovim this is also accompanied by the message "Waiting for OSC 52 response from the terminal. Press Ctrl-C to interrupt..." during the timeout and "Timed out waiting for a clipboard response from the terminal" when it is over. I'm not sure if this can be implemented in fish, but I think mentioning timeout in the documentation will suffice

faho commented 8 months ago

Okay, so:

So we could only add something that tries and requires you to manually abort, and we would have to have it disabled by default.

That does not sound like a great idea to me. This is a feature for the small group of people who find out about it and have a supporting terminal and have a need for it by going over ssh and remember to do it on any server they ssh to.

In practice it turns "oh I need to press ctrl-shift-v to paste, this is an ssh session" (which will work in basically all shells) into "oh I need to set -g fish_use_osc_paste 1, this is an ssh session" (with a potential "oh I also need to enable it in my terminal oh no it is terminal.app i need to install iterm first and turn it on there" on top).

It would be worth it if we could enable some form of it by default, but it appears we cannot.

I would point you to either osc or just telling your terminal to paste (typically ctrl-shift-v) instead. Or make a plugin or python script or something.

Unless @krobelus disagrees, I would be inclined to reject this.

name-snrl commented 8 months ago

Okay, so:

* enabling it by default isn't really possible because it's not widely supported, and there is no way to detect it (remember this is ssh, you only have $TERM and the connection may be bad and slow)

* even for those who enable it it is very easy to accidentally not have it work, because the terminal might not have it enabled, and _all_ terminals that support it on the [osc](https://github.com/theimpostor/osc) list disable it by default behind an option or prompt

* I am unclear what the failure mode for terminals that do support it but disable it is - no response, an empty response, some kind of failure response?

* For terminals that don't support it the failure mode is no response, which is pretty terrible

* enabling it might just cause your terminal to clear your clipboard instead

* Some terminals that support it "prompt for access", which presumably means we would have to wait until you click "yes" or "no" on the "program requested paste, do you wanna?" dialog. This means a timeout isn't really useful.

Most of the points about terminals that have poor or no implementation, but there are terminals like foot in which this works out of the box. I agree that there are not many such terminals, but they do exist.

That does not sound like a great idea to me. This is a feature for the small group of people who find out about it and have a supporting terminal and have a need for it by going over ssh and remember to do it on any server they ssh to.

In practice it turns "oh I need to press ctrl-shift-v to paste, this is an ssh session" (which will work in basically all shells) into "oh I need to set -g fish_use_osc_paste 1, this is an ssh session" (with a potential "oh I also need to enable it in my terminal oh no it is terminal.app i need to install iterm first and turn it on there" on top).

I think most people who often work on remote machines have their own little scripts that quickly deploy their environment.

This may be completely useless for experienced users who are already used to ctrl+shift+v, but there are also beginners who are just starting to explore the world of linux. So why not make life easier for those who happen to know about OSC-52 and use the friendly interactive shell?

So yes, I agree that this is functionality for a small group of people, but it seems @krobelus already has an experimental implementation, there doesn't seem to be much work left. If that's the case, why not?

I would point you to either osc or just telling your terminal to paste (typically ctrl-shift-v) instead. Or make a plugin or python script or something.

It's not about me, it's about making life easier for those who would like to see this functionality, like me. The patch that @krobelus posted is enough for me, for which I thank him very much.

faho commented 8 months ago

So why not make life easier for those who happen to know about OSC-52 and use the friendly interactive shell?

That's basically a theoretical group.

People who intimately know about osc-52, can find the options to enable it and don't know about their terminal's paste (which is also usually accessible in the right click menu).

They simultaneously know enough to sync their config across ssh yet don't know how to right click.

This is a very convenient amount of knowledgable.

there doesn't seem to be much work left. If that's the case, why not?

The work left is the work to clean up, test, document and maintain all of that.

An experimental implementation is often far from "not much work left".


Alright, I'm going to close this. @krobelus: Feel free to disagree and reopen.

I do not believe this feature, in its current state (where it's badly supported and requires opt-in in all supporting terminals), is usable. I do not believe adding a feature that we have to disable by default for users to figure out (and not mess up) is the sort of thing we add.

Sorry.

name-snrl commented 8 months ago

That's basically a theoretical group.

People who intimately know about osc-52, can find the options to enable it and don't know about their terminal's paste (which is also usually accessible in the right click menu).

They simultaneously know enough to sync their config across ssh yet don't know how to right click.

I know how to paste in my term, but time and time again, out of habit, I use ctrl+v first. Good thing I have some solution now.

p.s. Yes, I'm one of a theoretical group. I'm a theoretical man :D

opt-in in all supporting terminals

foot in which this works out of the box

Sorry.

No problem, perhaps it will become more in demand in the future. Thanks for your time and for the cool $SHELL

krobelus commented 8 months ago

This patch is for a rust rewritten fish, unfortunately I can't easily apply it to the latest nixpkgs to test it.

Your system should make it easy to bump the version to latest Git master (or rather, point it to a local source tree), add Rust as dependency and rebuild the package. Or you can always use make && make install.

Unless @krobelus disagrees, I would be inclined to reject this.

I think "rejected" can be pretty misleading. I'm sure we all agree that we should have copying/pasting shortcuts. OSC 52 is best-in-class because it supports SSH and containers. I'm interested in solving this because I frequently use C-v by accident inside SSH.

Sure, we are not yet sure know how to implement it in a way that works out of the box and, more importantly, doesn't add a regression (like my patch).

But if someone does the work to solve these problems, then I'm sure we can incorporate this in some form. Labeling it as rejected sounds like we're not interesting in seeing this problem solved in fish, which is totally wrong and might dissuade potential contributors. It's not like keeping it open implies that anyone of the regular contributors is interested in working on it.

That being said, I don't think it's a big deal because the people who are likely to implement this will probably do it regardless. Erring on the side of closing helps those who go through the issue list so it's a good thing in general.


I think we can improve things here. I'll try it if I get to it.