bug#53769: Implementation of X11 MULTIPLE selection is incomplete (with patches)

[Vladimir Panteleev <vladimir.panteleev.md@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 432 bytes --]

Okay, here are the patches with that formatting tweak applied.

- Vladimir

On Fri, 4 Feb 2022 at 04:58, Po Lu <luangruo@yahoo.com> wrote:
>
>
> Your change LGTM, but:
>
> > +  if (! for_multiple && actual_type == dpyinfo->Xatom_INCR)
>
> We typically don't put a space between the "!" and "for_multiple" here.
> The surrounding code does it a lot, but there's no reason to introduce
> any more instances of that style.
>
> Thanks.

[-- Attachment #2: 0001-Do-not-delete-the-MULTIPLE-property-after-reading-it.patch --]
[-- Type: text/x-patch, Size: 3876 bytes --]

From 88b5a0bccc30f459d7ab5b3ff0ca4ac0302d1a8f Mon Sep 17 00:00:00 2001
From: Vladimir Panteleev <git@cy.md>
Date: Fri, 4 Feb 2022 01:54:45 +0000
Subject: [PATCH 1/2] Do not delete the MULTIPLE property after reading it

Per the ICCCM spec:

> The requestor should delete [...] the property specified in the
> MULTIPLE request when it has copied all the data.

We are not the requestor, so we should not be deleting this property
(which is what x_get_window_property_as_lisp_data does).  The property
needs to remain available as the requestor will generally want to read
it back to see which conversions succeeded or not.

* src/xselect.c (x_get_window_property_as_lisp_data): Add flag which
skips deleting the read property, or handling INCR (which does not
make sense for MULTIPLE).
(x_handle_selection_request): Enable the flag.
---
 src/xselect.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/src/xselect.c b/src/xselect.c
index cfe028a169..537be2ddd5 100644
--- a/src/xselect.c
+++ b/src/xselect.c
@@ -52,7 +52,7 @@
 static void wait_for_property_change (struct prop_location *);
 static Lisp_Object x_get_window_property_as_lisp_data (struct x_display_info *,
                                                        Window, Atom,
-                                                       Lisp_Object, Atom);
+                                                       Lisp_Object, Atom, bool);
 static Lisp_Object selection_data_to_lisp_data (struct x_display_info *,
 						const unsigned char *,
 						ptrdiff_t, Atom, int);
@@ -799,7 +799,7 @@ x_handle_selection_request (struct selection_input_event *event)
       if (property == None) goto DONE;
       multprop
 	= x_get_window_property_as_lisp_data (dpyinfo, requestor, property,
-					      QMULTIPLE, selection);
+					      QMULTIPLE, selection, true);
 
       if (!VECTORP (multprop) || ASIZE (multprop) % 2)
 	goto DONE;
@@ -1210,7 +1210,7 @@ x_get_foreign_selection (Lisp_Object selection_symbol, Lisp_Object target_type,
   return
     x_get_window_property_as_lisp_data (dpyinfo, requestor_window,
 					target_property, target_type,
-					selection_atom);
+					selection_atom, false);
 }
 \f
 /* Subroutines of x_get_window_property_as_lisp_data */
@@ -1461,7 +1461,8 @@ receive_incremental_selection (struct x_display_info *dpyinfo,
 x_get_window_property_as_lisp_data (struct x_display_info *dpyinfo,
 				    Window window, Atom property,
 				    Lisp_Object target_type,
-				    Atom selection_atom)
+				    Atom selection_atom,
+				    bool for_multiple)
 {
   Atom actual_type;
   int actual_format;
@@ -1477,6 +1478,8 @@ x_get_window_property_as_lisp_data (struct x_display_info *dpyinfo,
 			 &actual_type, &actual_format, &actual_size);
   if (! data)
     {
+      if (for_multiple)
+	return Qnil;
       block_input ();
       bool there_is_a_selection_owner
 	= XGetSelectionOwner (display, selection_atom) != 0;
@@ -1499,7 +1502,7 @@ x_get_window_property_as_lisp_data (struct x_display_info *dpyinfo,
 	}
     }
 
-  if (actual_type == dpyinfo->Xatom_INCR)
+  if (!for_multiple && actual_type == dpyinfo->Xatom_INCR)
     {
       /* That wasn't really the data, just the beginning.  */
 
@@ -1515,11 +1518,14 @@ x_get_window_property_as_lisp_data (struct x_display_info *dpyinfo,
 				     &actual_size);
     }
 
-  block_input ();
-  TRACE1 ("  Delete property %s", XGetAtomName (display, property));
-  XDeleteProperty (display, window, property);
-  XFlush (display);
-  unblock_input ();
+  if (!for_multiple)
+    {
+      block_input ();
+      TRACE1 ("  Delete property %s", XGetAtomName (display, property));
+      XDeleteProperty (display, window, property);
+      XFlush (display);
+      unblock_input ();
+    }
 
   /* It's been read.  Now convert it to a lisp object in some semi-rational
      manner.  */
-- 
2.34.1


[-- Attachment #3: 0002-Update-the-MULTIPLE-property-with-conversion-outcome.patch --]
[-- Type: text/x-patch, Size: 1782 bytes --]

From 3d80d43d173b164b1c5ade5901b9bb61dd2defe7 Mon Sep 17 00:00:00 2001
From: Vladimir Panteleev <git@cy.md>
Date: Fri, 4 Feb 2022 02:46:50 +0000
Subject: [PATCH 2/2] Update the MULTIPLE property with conversion outcomes

Per the ICCCM spec:

> If the owner fails to convert the target named by an atom in the
> MULTIPLE property, it should replace that atom in the property with
> None.

* src/xselect.c (x_handle_selection_request): Do it.
---
 src/xselect.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/src/xselect.c b/src/xselect.c
index 537be2ddd5..f2a64dd953 100644
--- a/src/xselect.c
+++ b/src/xselect.c
@@ -795,6 +795,7 @@ x_handle_selection_request (struct selection_input_event *event)
       Window requestor = SELECTION_EVENT_REQUESTOR (event);
       Lisp_Object multprop;
       ptrdiff_t j, nselections;
+      struct selection_data cs;
 
       if (property == None) goto DONE;
       multprop
@@ -811,11 +812,19 @@ x_handle_selection_request (struct selection_input_event *event)
 	  Lisp_Object subtarget = AREF (multprop, 2*j);
 	  Atom subproperty = symbol_to_x_atom (dpyinfo,
 					       AREF (multprop, 2*j+1));
+	  bool subsuccess = false;
 
 	  if (subproperty != None)
-	    x_convert_selection (selection_symbol, subtarget,
-				 subproperty, true, dpyinfo);
+	    subsuccess = x_convert_selection (selection_symbol, subtarget,
+					      subproperty, true, dpyinfo);
+	  if (!subsuccess)
+	    ASET (multprop, 2*j+1, Qnil);
 	}
+      /* Save conversion results */
+      lisp_data_to_selection_data (dpyinfo, multprop, &cs);
+      XChangeProperty (dpyinfo->display, requestor, property,
+		       cs.type, cs.format, PropModeReplace,
+		       cs.data, cs.size);
       success = true;
     }
   else
-- 
2.34.1