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

From: Vladimir Panteleev <[email-masked]>
Date: Fri, 4 Feb 2022 03:01:43 +0000

1 - Emacs deletes the MULTIPLE property (with the requested
target/property atom pairs) after reading it; whereas it should be
deleted by the requestor, not the owner.
2 - Emacs should update the MULTIPLE property with the conversion
outcome for each target (i.e. None if the conversion failed)

Patches attached, please see commit messages for details. I've
previously signed the paperwork.
From 4fa927746796c7d0f7dd7bfcf09ecf3af7ad4614 Mon Sep 17 00:00:00 2001
From: Vladimir Panteleev <[email-masked]>
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..8f47aa84cc 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);
 }
 
 /* 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

From b52462885ae6b1710bc691f179f53a6cc52f55ea Mon Sep 17 00:00:00 2001
From: Vladimir Panteleev <[email-masked]>
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 8f47aa84cc..c0fd9e322a 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