vim / vim

The official Vim repository
https://www.vim.org
Vim License
36.48k stars 5.45k forks source link

Report an error when using / writing to system clipboard without `+clipboard` compile flag #14768

Open delvh opened 5 months ago

delvh commented 5 months ago

At the moment, I've often enough had the problem that Vim seems to "magically" not be working initially regarding system clipboard copy-pasting: For example, when pressing "*y5y, my expectation is that 5 lines are copied to the system clipboard. However, many distros ship with a minimal vim by default that does not have the +clipboard build flag, most notably Ubuntu. As such, to an end user, it simply looks like Vim is broken.

I often enough used Vim on a new Ubuntu system, and wondered why the command above simply does nothing. As such, I would recommend adding an error when attempting to access the system registers "* and "+ when that compile flag is missing. Preferably, this would also include read-only access to the clipboard, but even write-access only would already be a huge improvement.

chrisbra commented 5 months ago

Preferably, this would also include read-only access to the clipboard, but even write-access only would already be a huge improvement.

I am not sure I understand this comment and I am not sure this is actually possible. clipboard handling is a feature provided by the X Server, so you need a connection to X11 Server in order to be able to make use of the clipboard features here.

delvh commented 5 months ago

Yes, but I mean displaying an error when Vim was not built with +clipboard but trying to access the system clipboard (mostly the commands yank and paste, but also whenever you try to write to a custom register, i.e. "*daw). That is something we can statically determine as we simply need to listen for if the "* and "+ registers should be used. As per my understanding, when the compile flag is missing, these registers simply turn into another Vim-internal register. However, that's not what users expect when using these registers.

I am not talking about checking whether the X Server exists (although it would of course also be nice to display an error if the text could not be copied for any other reason such as the X Server not being available).

chrisbra commented 5 months ago

As per my understanding, when the compile flag is missing, these registers simply turn into another Vim-internal register.

No, Vim falls back to the 0 register in that case.

delvh commented 5 months ago

Yes, that's what I mean. I have nothing against keeping this behavior. Nevertheless, I still recommend displaying an error in that case instead: That seems to be much more "expected behavior" to me than silently falling back to something else than the user requested. As I said, as a user, my intuition in this case is that I'm requesting the system clipboard, not anything else.

In the worst case, there's the possibility of adding a config option that is enabled by default whether to show an error (enabled) or use the old behavior (disabled). However, I doubt there will be many users for it as the purpose of these registers should be known by users already.

chrisbra commented 5 months ago

How about the following patch:

diff --git a/src/clipboard.c b/src/clipboard.c
index 8b9850e44..6c766047e 100644
--- a/src/clipboard.c
+++ b/src/clipboard.c
@@ -2220,10 +2220,12 @@ adjust_clip_reg(int *rp)
            *rp = ((clip_unnamed_saved & CLIP_UNNAMED_PLUS)
                                           && clip_plus.available) ? '+' : '*';
     }
-    if (!clip_star.available && *rp == '*')
-       *rp = 0;
-    if (!clip_plus.available && *rp == '+')
+    if ((!clip_star.available && *rp == '*') ||
+           (!clip_plus.available && *rp == '+'))
+    {
+       msg_warn_missing_clipboard();
        *rp = 0;
+    }
 }

 #endif // FEAT_CLIPBOARD
diff --git a/src/message.c b/src/message.c
index 03c7072a7..216b76192 100644
--- a/src/message.c
+++ b/src/message.c
@@ -55,6 +55,9 @@ static int msg_hist_len = 0;
 static FILE *verbose_fd = NULL;
 static int  verbose_did_open = FALSE;

+static int  did_warn_clipboard = FALSE;
+static char *warn_clipboard = "Clipboard register not available, using register 0";
+
 /*
  * When writing messages to the screen, there are many different situations.
  * A number of variables is used to remember the current state:
@@ -4060,6 +4063,20 @@ msg_advance(int col)
            msg_putchar(' ');
 }

+
+/*
+ * Warn about missing Clipboard Support
+ */
+    void
+msg_warn_missing_clipboard()
+{
+    if (!did_warn_clipboard)
+    {
+       msg(_(warn_clipboard));
+       did_warn_clipboard = TRUE;
+    }
+}
+
 #if defined(FEAT_CON_DIALOG) || defined(PROTO)
 /*
  * Used for "confirm()" function, and the :confirm command prefix.
diff --git a/src/proto/message.pro b/src/proto/message.pro
index 6657a08ec..54a0a7765 100644
--- a/src/proto/message.pro
+++ b/src/proto/message.pro
@@ -73,6 +73,7 @@ void give_warning(char_u *message, int hl);
 void give_warning_with_source(char_u *message, int hl, int with_source);
 void give_warning2(char_u *message, char_u *a1, int hl);
 void msg_advance(int col);
+void msg_warn_missing_clipboard(void);
 int do_dialog(int type, char_u *title, char_u *message, char_u *buttons, int dfltbutton, char_u *textfield, int ex_cmd);
 int vim_dialog_yesno(int type, char_u *title, char_u *message, int dflt);
 int vim_dialog_yesnocancel(int type, char_u *title, char_u *message, int dflt);
diff --git a/src/register.c b/src/register.c
index 47ed21846..5b61e1da0 100644
--- a/src/register.c
+++ b/src/register.c
@@ -199,6 +199,12 @@ valid_yank_reg(
 #endif
                                                        )
        return TRUE;
+    // Warn about missing clipboard support once
+    else if (regname == '*' || regname == '+')
+    {
+       msg_warn_missing_clipboard();
+       return FALSE;
+    }
     return FALSE;
 }

@@ -1164,10 +1170,12 @@ op_yank(oparg_T *oap, int deleting, int mess)
        return OK;

 #ifdef FEAT_CLIPBOARD
-    if (!clip_star.available && oap->regname == '*')
-       oap->regname = 0;
-    else if (!clip_plus.available && oap->regname == '+')
+    if ((!clip_star.available && oap->regname == '*') || (!clip_plus.available
+               && oap->regname == '+'))
+    {
        oap->regname = 0;
+       msg_warn_missing_clipboard();
+    }
 #endif

     if (!deleting)                 // op_delete() already set y_current