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

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

[Vladimir Panteleev]

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

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.

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

From 4fa927746796c7d0f7dd7bfcf09ecf3af7ad4614 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..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);
 }
 \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: 1779 bytes --]

From b52462885ae6b1710bc691f179f53a6cc52f55ea 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 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

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

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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.

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

[Vladimir Panteleev]

[-- 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

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

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Vladimir Panteleev <vladimir.panteleev.md@gmail.com> writes:

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

Thanks, if your copyright papers are in order this can be installed.

Eli, is the copyright paperwork complete for this change?  Thanks.

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

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: 53769@debbugs.gnu.org, Eli Zaretskii <eliz@gnu.org>
> Date: Fri, 04 Feb 2022 22:03:58 +0800
> 
> Vladimir Panteleev <vladimir.panteleev.md@gmail.com> writes:
> 
> > Okay, here are the patches with that formatting tweak applied.
> 
> Thanks, if your copyright papers are in order this can be installed.
> 
> Eli, is the copyright paperwork complete for this change?  Thanks.

Yes, Vladimir's copyright assignment for Emacs is on file.

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

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

> Yes, Vladimir's copyright assignment for Emacs is on file.

Great, I'll install this.  Thanks.

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

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Po Lu <luangruo@yahoo.com> writes:

> Great, I'll install this.  Thanks.

Now done, so I'm closing this bug.

Thanks for working on Emacs.