bug#55883: [PATCH] Update X Primary Selection with active regions

bug#55883: [PATCH] Update X Primary Selection with active regions

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

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

Severity: wishlist
Tags: patch

Attached are two patches to allow users that use xterm's OSC 52
clipboard support to have their primary selection updated with the
active region, to better match the behavior of running Emacs in X
locally.

This behavior is enabled automatically for supported terminals, and is
controlled by the existing `select-active-regions' variable.

Previous discussion:
https://lists.gnu.org/archive/html/emacs-devel/2022-06/msg00126.html

This should be covered by a corporate copyright assignment on file.

[-- Attachment #2: 0001-Use-display-selections-p-for-selecting-active-region.patch --]
[-- Type: text/x-patch, Size: 1457 bytes --]

From 5dee5a44fd0d3ad57ba7e40fd5b0542d70160c14 Mon Sep 17 00:00:00 2001
From: Duncan Findlay <duncf@google.com>
Date: Fri, 20 May 2022 00:17:51 -0700
Subject: [PATCH 1/2] Use `display-selections-p' for selecting active region

`display-selections-p' is documented as the preferred way to determine
capabilities, over `window-system'.  This allows supported text
terminals to indicate support for these selections.

* src/keyboard.c (command_loop_1): Replace call to `window-system'
with `display-selections-p' when deciding whether to update primary
selection.
---
 src/keyboard.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/keyboard.c b/src/keyboard.c
index 55d710ed62..dc3c37e4d8 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -1569,7 +1569,7 @@ command_loop_1 (void)
 	    {
 	      /* Even if not deactivating the mark, set PRIMARY if
 		 `select-active-regions' is non-nil.  */
-	      if (!NILP (Fwindow_system (Qnil))
+	      if (!NILP (call0 (Qdisplay_selections_p))
 		  /* Even if mark_active is non-nil, the actual buffer
 		     marker may not have been set yet (Bug#7044).  */
 		  && XMARKER (BVAR (current_buffer, mark))->buffer
@@ -12162,6 +12162,7 @@ syms_of_keyboard (void)
 
   DEFSYM (Qpolling_period, "polling-period");
 
+  DEFSYM (Qdisplay_selections_p, "display-selections-p");
   DEFSYM (Qgui_set_selection, "gui-set-selection");
 
   /* The primary selection.  */
-- 
2.36.1.476.g0c4daa206d-goog


[-- Attachment #3: 0002-Support-select-active-regions-with-xterm.patch --]
[-- Type: text/x-patch, Size: 2318 bytes --]

From 25bb187911e0444c5e1618650a061f0be317bee7 Mon Sep 17 00:00:00 2001
From: Duncan Findlay <duncf@google.com>
Date: Thu, 2 Jun 2022 20:23:44 -0700
Subject: [PATCH 2/2] Support `select-active-regions' with xterm

This allows Emacs to save the active region to the user's primary
selection on supported terminals.  This behavior is controlled by the
existing `select-active-regions' variable.

* lisp/frame.el (display-selections-p): For text terminals, return
terminal parameter `display-selections-p` to indicate selection
support.
* lisp/term/xterm.el (xterm--init-activate-set-selection): Set the
`display-selections-p` terminal parameter.
---
 etc/NEWS           | 5 +++++
 lisp/frame.el      | 2 ++
 lisp/term/xterm.el | 3 ++-
 3 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/etc/NEWS b/etc/NEWS
index cd4b1b06ec..17c98b772c 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -477,6 +477,11 @@ This is in addition to previously-supported ways of discovering 24-bit
 color support: either via the "RGB" or "setf24" capabilities, or if
 the 'COLORTERM' environment variable is set to the value "truecolor".
 
+*** Primary selection of active regions with xterm setSelection support.
+On supported terminals, the active region may be saved to the X
+primary selection.  This behavior is controlled by the
+'select-active-regions' variable.
+
 ** ERT
 
 +++
diff --git a/lisp/frame.el b/lisp/frame.el
index 27f99fb7d2..e926dff201 100644
--- a/lisp/frame.el
+++ b/lisp/frame.el
@@ -2164,6 +2164,8 @@ display-selections-p
        (not (null dos-windows-version))))
      ((memq frame-type '(x w32 ns pgtk))
       t)
+     ((eq frame-type t)
+      (terminal-parameter display 'display-selections-p))
      (t
       nil))))
 
diff --git a/lisp/term/xterm.el b/lisp/term/xterm.el
index a7e257f41c..f6add0fbf2 100644
--- a/lisp/term/xterm.el
+++ b/lisp/term/xterm.el
@@ -946,7 +946,8 @@ xterm--init-activate-get-selection
 
 (defun xterm--init-activate-set-selection ()
   "Terminal initialization for `gui-set-selection'."
-  (set-terminal-parameter nil 'xterm--set-selection t))
+  (set-terminal-parameter nil 'xterm--set-selection t)
+  (set-terminal-parameter nil 'display-selections-p t))
 
 (defun xterm--init-frame-title ()
   "Terminal initialization for XTerm frame titles."
-- 
2.36.1.476.g0c4daa206d-goog

bug#55883: [PATCH] Update X Primary Selection with active regions

[Eli Zaretskii]

> Date: Thu, 9 Jun 2022 23:18:01 -0700
> From:  Duncan Findlay via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> Attached are two patches to allow users that use xterm's OSC 52
> clipboard support to have their primary selection updated with the
> active region, to better match the behavior of running Emacs in X
> locally.
> 
> This behavior is enabled automatically for supported terminals, and is
> controlled by the existing `select-active-regions' variable.
> 
> Previous discussion:
> https://lists.gnu.org/archive/html/emacs-devel/2022-06/msg00126.html

Thanks.

However, the patches you posted seem to ignore the comments I made in
the above-mentioned discussions, specifically here:

  https://lists.gnu.org/archive/html/emacs-devel/2022-06/msg00134.html

Did you send a wrong version of the patches, perhaps?

bug#55883: [PATCH] Update X Primary Selection with active regions

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

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

Attached is an updated version that I believe addresses all the
concerns raised on the mailing list.

Given the changes don't split so cleanly into two any more, I've
combined them into a single patch.

Thanks!

On Thu, Jun 9, 2022 at 11:52 PM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > Date: Thu, 9 Jun 2022 23:18:01 -0700
> > From:  Duncan Findlay via "Bug reports for GNU Emacs,
> >  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> >
> > Attached are two patches to allow users that use xterm's OSC 52
> > clipboard support to have their primary selection updated with the
> > active region, to better match the behavior of running Emacs in X
> > locally.
> >
> > This behavior is enabled automatically for supported terminals, and is
> > controlled by the existing `select-active-regions' variable.
> >
> > Previous discussion:
> > https://lists.gnu.org/archive/html/emacs-devel/2022-06/msg00126.html
>
> Thanks.
>
> However, the patches you posted seem to ignore the comments I made in
> the above-mentioned discussions, specifically here:
>
>   https://lists.gnu.org/archive/html/emacs-devel/2022-06/msg00134.html
>
> Did you send a wrong version of the patches, perhaps?

[-- Attachment #2: 0001-Support-select-active-regions-with-xterm.patch --]
[-- Type: text/x-patch, Size: 4112 bytes --]

From bc906a8a0c67125880497c4729d7940fa4ba8bf6 Mon Sep 17 00:00:00 2001
From: Duncan Findlay <duncf@google.com>
Date: Fri, 10 Jun 2022 18:46:49 -0700
Subject: [PATCH] Support `select-active-regions' with xterm

This allows Emacs to save the active region to the user's
primary selection on supported terminals.  The behavior follows
the existing `select-active-regions' variable and requires
`xterm-select-active-regions' to be non-nil.

* src/keyboard.c (command_loop_1): Check terminal parameter
`display-selections-p' for text terminals when deciding whether
to update primary selection.
* lisp/frame.el (display-selections-p): Return terminal
parameter `display-selections-p' to indicate selection support.
* lisp/term/xterm.el (xterm-select-active-regions): New
defcustom.  (xterm--init-activate-set-selection): Set
the `display-selections-p' terminal parameter.
---
 etc/NEWS           |  6 ++++++
 lisp/frame.el      |  2 ++
 lisp/term/xterm.el | 11 ++++++++++-
 src/keyboard.c     |  5 ++++-
 4 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 1789d47351..57b5d06e78 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -492,6 +492,12 @@ This is in addition to previously-supported ways of discovering 24-bit
 color support: either via the "RGB" or "setf24" capabilities, or if
 the 'COLORTERM' environment variable is set to the value "truecolor".
 
+*** Select active regions with xterm selection support.
+On terminals with xterm setSelection support, the active region may be
+saved to the X primary selection, following the
+'select-active-regions' variable. This support is enabled with
+'xterm-select-active-regions'.
+
 ** ERT
 
 +++
diff --git a/lisp/frame.el b/lisp/frame.el
index 27f99fb7d2..e530afb387 100644
--- a/lisp/frame.el
+++ b/lisp/frame.el
@@ -2164,6 +2164,8 @@ display-selections-p
        (not (null dos-windows-version))))
      ((memq frame-type '(x w32 ns pgtk))
       t)
+     ((terminal-parameter display 'display-selections-p)
+      t)
      (t
       nil))))
 
diff --git a/lisp/term/xterm.el b/lisp/term/xterm.el
index a7e257f41c..671d757c11 100644
--- a/lisp/term/xterm.el
+++ b/lisp/term/xterm.el
@@ -80,6 +80,13 @@ xterm-store-paste-on-kill-ring
   :version "28.1"
   :type 'boolean)
 
+(defcustom xterm-select-active-regions nil
+  "If non-nil, on a terminal with setSelection support, Emacs will
+also update the primary selection with the active region, based
+on the value of `select-active-regions'."
+  :version "29.1"
+  :type 'boolean)
+
 (defconst xterm-paste-ending-sequence "\e[201~"
   "Characters sent by the terminal to end a bracketed paste.")
 
@@ -946,7 +953,9 @@ xterm--init-activate-get-selection
 
 (defun xterm--init-activate-set-selection ()
   "Terminal initialization for `gui-set-selection'."
-  (set-terminal-parameter nil 'xterm--set-selection t))
+  (set-terminal-parameter nil 'xterm--set-selection t)
+  (when xterm-select-active-regions
+    (set-terminal-parameter nil 'display-selections-p t)))
 
 (defun xterm--init-frame-title ()
   "Terminal initialization for XTerm frame titles."
diff --git a/src/keyboard.c b/src/keyboard.c
index 55d710ed62..8e3738b4c7 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -1,3 +1,4 @@
+
 /* Keyboard and mouse input; editor command loop.
 
 Copyright (C) 1985-1989, 1993-1997, 1999-2022 Free Software Foundation,
@@ -1569,7 +1570,8 @@ command_loop_1 (void)
 	    {
 	      /* Even if not deactivating the mark, set PRIMARY if
 		 `select-active-regions' is non-nil.  */
-	      if (!NILP (Fwindow_system (Qnil))
+	      if ((!NILP (Fwindow_system (Qnil)) ||
+		   !NILP (Fterminal_parameter (Qnil, Qdisplay_selections_p)))
 		  /* Even if mark_active is non-nil, the actual buffer
 		     marker may not have been set yet (Bug#7044).  */
 		  && XMARKER (BVAR (current_buffer, mark))->buffer
@@ -12162,6 +12164,7 @@ syms_of_keyboard (void)
 
   DEFSYM (Qpolling_period, "polling-period");
 
+  DEFSYM (Qdisplay_selections_p, "display-selections-p");
   DEFSYM (Qgui_set_selection, "gui-set-selection");
 
   /* The primary selection.  */
-- 
2.36.1.476.g0c4daa206d-goog

bug#55883: [PATCH] Update X Primary Selection with active regions

[Eli Zaretskii]

> From: Duncan Findlay <duncf@google.com>
> Date: Fri, 10 Jun 2022 18:59:47 -0700
> Cc: 55883@debbugs.gnu.org
> 
> Attached is an updated version that I believe addresses all the
> concerns raised on the mailing list.
> 
> Given the changes don't split so cleanly into two any more, I've
> combined them into a single patch.

Thanks.  A few minor issues with this version:

> * src/keyboard.c (command_loop_1): Check terminal parameter
> `display-selections-p' for text terminals when deciding whether
> to update primary selection.
> * lisp/frame.el (display-selections-p): Return terminal
> parameter `display-selections-p' to indicate selection support.
> * lisp/term/xterm.el (xterm-select-active-regions): New
> defcustom.  (xterm--init-activate-set-selection): Set
> the `display-selections-p' terminal parameter.

Please mention the bug number in the log message.

> +*** Select active regions with xterm selection support.
> +On terminals with xterm setSelection support, the active region may be
> +saved to the X primary selection, following the
> +'select-active-regions' variable. This support is enabled with
> +'xterm-select-active-regions'.  ^^

Our convention is to leave 2 spaces between sentences in
documentation.

> --- a/lisp/frame.el
> +++ b/lisp/frame.el
> @@ -2164,6 +2164,8 @@ display-selections-p
>         (not (null dos-windows-version))))
>       ((memq frame-type '(x w32 ns pgtk))
>        t)
> +     ((terminal-parameter display 'display-selections-p)
> +      t)

This should test xterm--set-selection parameter.

> +(defcustom xterm-select-active-regions nil
> +  "If non-nil, on a terminal with setSelection support, Emacs will
> +also update the primary selection with the active region, based
> +on the value of `select-active-regions'."

The first line of a doc string should be a complete sentence.  So this
doc strings should be reworded to comply with this convention.  For
example:

    "If non-nil, update PRIMARY X selection on text-mode frames.
  On a text-mode terminal that supports setSelection command, if
  this variable is non-nil, Emacs will set the PRIMARY selection
  from the active region, according to `select-active-regions'."

> @@ -946,7 +953,9 @@ xterm--init-activate-get-selection
>  
>  (defun xterm--init-activate-set-selection ()
>    "Terminal initialization for `gui-set-selection'."
> -  (set-terminal-parameter nil 'xterm--set-selection t))
> +  (set-terminal-parameter nil 'xterm--set-selection t)
> +  (when xterm-select-active-regions
> +    (set-terminal-parameter nil 'display-selections-p t)))

I see no reason to introduce a new terminal parameter with non-trivial
semantics.  Moreover, this consults the value of
xterm-select-active-regions only once, so if the user later modifies
the value of the option, the terminal parameter will stay at its stale
value.

I think the code should instead check the value of
xterm-select-active-regions in keyboard.c, where it decides whether to
set PRIMARY.  (Let me know if you need guidance for how to reference a
Lisp variable from C.)

> @@ -1569,7 +1570,8 @@ command_loop_1 (void)
>  	    {
>  	      /* Even if not deactivating the mark, set PRIMARY if
>  		 `select-active-regions' is non-nil.  */
> -	      if (!NILP (Fwindow_system (Qnil))
> +	      if ((!NILP (Fwindow_system (Qnil)) ||
> +		   !NILP (Fterminal_parameter (Qnil, Qdisplay_selections_p)))

This should be looking at the xterm--set-selection parameter instead,
and test the value of xterm-select-active-regions in addition, as
described above.

bug#55883: [PATCH] Update X Primary Selection with active regions

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

>> +	      if ((!NILP (Fwindow_system (Qnil)) ||
>> +		   !NILP (Fterminal_parameter (Qnil, Qdisplay_selections_p)))

In addition to what Eli wrote, this code is formatted incorrectly.

Here and elsewhere, please write:

  if (very_long_condition ()
      || another_very_long_condition ())

as opposed to

  if (very_long_condition () ||
      another_very_long_condition ())

Thanks.

bug#55883: [PATCH] Update X Primary Selection with active regions

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

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

On Sat, Jun 11, 2022 at 12:53 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Duncan Findlay <duncf@google.com>
> > Date: Fri, 10 Jun 2022 18:59:47 -0700
> > Cc: 55883@debbugs.gnu.org
> >
> > Attached is an updated version that I believe addresses all the
> > concerns raised on the mailing list.
> >
> > Given the changes don't split so cleanly into two any more, I've
> > combined them into a single patch.
>
> Thanks.  A few minor issues with this version:
>
> > * src/keyboard.c (command_loop_1): Check terminal parameter
> > `display-selections-p' for text terminals when deciding whether
> > to update primary selection.
> > * lisp/frame.el (display-selections-p): Return terminal
> > parameter `display-selections-p' to indicate selection support.
> > * lisp/term/xterm.el (xterm-select-active-regions): New
> > defcustom.  (xterm--init-activate-set-selection): Set
> > the `display-selections-p' terminal parameter.
>
> Please mention the bug number in the log message.

Done.

> > +*** Select active regions with xterm selection support.
> > +On terminals with xterm setSelection support, the active region may be
> > +saved to the X primary selection, following the
> > +'select-active-regions' variable. This support is enabled with
> > +'xterm-select-active-regions'.  ^^
>
> Our convention is to leave 2 spaces between sentences in
> documentation.

Done.

> > --- a/lisp/frame.el
> > +++ b/lisp/frame.el
> > @@ -2164,6 +2164,8 @@ display-selections-p
> >         (not (null dos-windows-version))))
> >       ((memq frame-type '(x w32 ns pgtk))
> >        t)
> > +     ((terminal-parameter display 'display-selections-p)
> > +      t)
>
> This should test xterm--set-selection parameter.

OK, so the goal is to check the xterm--set-selection terminal
parameter and variable xterm-select-active-regions before selecting
the active region in deactivate-mark in simple.el.

We could do this in display-selections-p, but given that these
variables are not obviously related to display-selections-p, it seems
to me we probably want to do this check in deactivate-mark instead.
Does that seem reasonable to you?

> > +(defcustom xterm-select-active-regions nil
> > +  "If non-nil, on a terminal with setSelection support, Emacs will
> > +also update the primary selection with the active region, based
> > +on the value of `select-active-regions'."
>
> The first line of a doc string should be a complete sentence.  So this
> doc strings should be reworded to comply with this convention.  For
> example:
>
>     "If non-nil, update PRIMARY X selection on text-mode frames.
>   On a text-mode terminal that supports setSelection command, if
>   this variable is non-nil, Emacs will set the PRIMARY selection
>   from the active region, according to `select-active-regions'."

Done, thanks.

> > @@ -946,7 +953,9 @@ xterm--init-activate-get-selection
> >
> >  (defun xterm--init-activate-set-selection ()
> >    "Terminal initialization for `gui-set-selection'."
> > -  (set-terminal-parameter nil 'xterm--set-selection t))
> > +  (set-terminal-parameter nil 'xterm--set-selection t)
> > +  (when xterm-select-active-regions
> > +    (set-terminal-parameter nil 'display-selections-p t)))
>
> I see no reason to introduce a new terminal parameter with non-trivial
> semantics.  Moreover, this consults the value of
> xterm-select-active-regions only once, so if the user later modifies
> the value of the option, the terminal parameter will stay at its stale
> value.
>
> I think the code should instead check the value of
> xterm-select-active-regions in keyboard.c, where it decides whether to
> set PRIMARY.  (Let me know if you need guidance for how to reference a
> Lisp variable from C.)

This seems to work -- is there a better way?

!NILP (SYMBOL_VAL (XSYMBOL (Qxterm_select_active_regions)))

> > @@ -1569,7 +1570,8 @@ command_loop_1 (void)
> >           {
> >             /* Even if not deactivating the mark, set PRIMARY if
> >                `select-active-regions' is non-nil.  */
> > -           if (!NILP (Fwindow_system (Qnil))
> > +           if ((!NILP (Fwindow_system (Qnil)) ||
> > +                !NILP (Fterminal_parameter (Qnil, Qdisplay_selections_p)))
>
> This should be looking at the xterm--set-selection parameter instead,
> and test the value of xterm-select-active-regions in addition, as
> described above.

Done.

I've also addressed Po's comments about long conditionals.

Thanks

Duncan

[-- Attachment #2: 0001-Support-select-active-regions-with-xterm.patch --]
[-- Type: application/x-patch, Size: 4230 bytes --]

bug#55883: [PATCH] Update X Primary Selection with active regions

[Eli Zaretskii]

> From: Duncan Findlay <duncf@google.com>
> Date: Mon, 13 Jun 2022 23:36:13 -0700
> Cc: 55883@debbugs.gnu.org
> 
> > > --- a/lisp/frame.el
> > > +++ b/lisp/frame.el
> > > @@ -2164,6 +2164,8 @@ display-selections-p
> > >         (not (null dos-windows-version))))
> > >       ((memq frame-type '(x w32 ns pgtk))
> > >        t)
> > > +     ((terminal-parameter display 'display-selections-p)
> > > +      t)
> >
> > This should test xterm--set-selection parameter.
> 
> OK, so the goal is to check the xterm--set-selection terminal
> parameter and variable xterm-select-active-regions before selecting
> the active region in deactivate-mark in simple.el.
> 
> We could do this in display-selections-p, but given that these
> variables are not obviously related to display-selections-p, it seems
> to me we probably want to do this check in deactivate-mark instead.
> Does that seem reasonable to you?

I think I'd prefer to have it in display-selections-p, like your last
version did, just using xterm--set-selection terminal parameter, not
the new one you invented.

> > I think the code should instead check the value of
> > xterm-select-active-regions in keyboard.c, where it decides whether to
> > set PRIMARY.  (Let me know if you need guidance for how to reference a
> > Lisp variable from C.)
> 
> This seems to work -- is there a better way?
> 
> !NILP (SYMBOL_VAL (XSYMBOL (Qxterm_select_active_regions)))

This is not safe, IMO.  I think this is better:

  if ((!NILP (Fwindow_system (Qnil))
       || ((symval = find_symbol_value (Qxterm_select_active_regions),
            (!EQ (symval, Qunbound) && !NILP (symval)))
	    && !NILP (Fterminal_parameter (Qnil, Qxterm__set_selection))))

> I've also addressed Po's comments about long conditionals.

Thanks, the patch LGTM, modulo the above 2 minor nits.

bug#55883: [PATCH] Update X Primary Selection with active regions

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

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

On Tue, Jun 14, 2022 at 5:15 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Duncan Findlay <duncf@google.com>
> > Date: Mon, 13 Jun 2022 23:36:13 -0700
> > Cc: 55883@debbugs.gnu.org
> >
> > > > --- a/lisp/frame.el
> > > > +++ b/lisp/frame.el
> > > > @@ -2164,6 +2164,8 @@ display-selections-p
> > > >         (not (null dos-windows-version))))
> > > >       ((memq frame-type '(x w32 ns pgtk))
> > > >        t)
> > > > +     ((terminal-parameter display 'display-selections-p)
> > > > +      t)
> > >
> > > This should test xterm--set-selection parameter.
> >
> > OK, so the goal is to check the xterm--set-selection terminal
> > parameter and variable xterm-select-active-regions before selecting
> > the active region in deactivate-mark in simple.el.
> >
> > We could do this in display-selections-p, but given that these
> > variables are not obviously related to display-selections-p, it seems
> > to me we probably want to do this check in deactivate-mark instead.
> > Does that seem reasonable to you?
>
> I think I'd prefer to have it in display-selections-p, like your last
> version did, just using xterm--set-selection terminal parameter, not
> the new one you invented.

Updated.

> > > I think the code should instead check the value of
> > > xterm-select-active-regions in keyboard.c, where it decides whether to
> > > set PRIMARY.  (Let me know if you need guidance for how to reference a
> > > Lisp variable from C.)
> >
> > This seems to work -- is there a better way?
> >
> > !NILP (SYMBOL_VAL (XSYMBOL (Qxterm_select_active_regions)))
>
> This is not safe, IMO.  I think this is better:
>
>   if ((!NILP (Fwindow_system (Qnil))
>        || ((symval = find_symbol_value (Qxterm_select_active_regions),
>             (!EQ (symval, Qunbound) && !NILP (symval)))
>             && !NILP (Fterminal_parameter (Qnil, Qxterm__set_selection))))

Thanks, I would never have figured that out myself.

> > I've also addressed Po's comments about long conditionals.
>
> Thanks, the patch LGTM, modulo the above 2 minor nits.

Duncan

[-- Attachment #2: 0001-Support-select-active-regions-with-xterm.patch --]
[-- Type: text/x-patch, Size: 4229 bytes --]

From 99337d15f3e0aecd57dd238c9616df83d1ae82a6 Mon Sep 17 00:00:00 2001
From: Duncan Findlay <duncf@google.com>
Date: Fri, 10 Jun 2022 18:46:49 -0700
Subject: [PATCH] Support `select-active-regions' with xterm

This allows Emacs to save the active region to the user's
primary selection on supported terminals.  The behavior follows
the existing `select-active-regions' variable and requires
`xterm-select-active-regions' to be non-nil.

* src/keyboard.c (command_loop_1):
* lisp/frame.el (display-selections-p): On text terminals, check
terminal parameter `xterm--set-selections' and variable
`xterm-select-active-regions' when deciding whether to update
primary selection. (bug#55883)
* lisp/term/xterm.el (xterm-select-active-regions): New
defcustom.
---
 etc/NEWS           |  6 ++++++
 lisp/frame.el      |  6 ++++++
 lisp/term/xterm.el |  8 ++++++++
 src/keyboard.c     | 10 +++++++++-
 4 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/etc/NEWS b/etc/NEWS
index 19ca21f666..ea501b7c69 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -502,6 +502,12 @@ This is in addition to previously-supported ways of discovering 24-bit
 color support: either via the "RGB" or "setf24" capabilities, or if
 the 'COLORTERM' environment variable is set to the value "truecolor".
 
+*** Select active regions with xterm selection support.
+On terminals with xterm setSelection support, the active region may be
+saved to the X primary selection, following the
+'select-active-regions' variable.  This support is enabled with
+'xterm-select-active-regions'.
+
 ** ERT
 
 +++
diff --git a/lisp/frame.el b/lisp/frame.el
index 27f99fb7d2..35863c0135 100644
--- a/lisp/frame.el
+++ b/lisp/frame.el
@@ -2149,6 +2149,9 @@ 'display-blink-cursor-p
 (defalias 'display-multi-frame-p #'display-graphic-p)
 (defalias 'display-multi-font-p #'display-graphic-p)
 
+;; From term/xterm.el
+(defvar xterm-select-active-regions)
+
 (defun display-selections-p (&optional display)
   "Return non-nil if DISPLAY supports selections.
 A selection is a way to transfer text or other data between programs
@@ -2164,6 +2167,9 @@ display-selections-p
        (not (null dos-windows-version))))
      ((memq frame-type '(x w32 ns pgtk))
       t)
+     ((and xterm-select-active-regions
+           (terminal-parameter nil 'xterm--set-selection))
+      t)
      (t
       nil))))
 
diff --git a/lisp/term/xterm.el b/lisp/term/xterm.el
index a7e257f41c..0791780d40 100644
--- a/lisp/term/xterm.el
+++ b/lisp/term/xterm.el
@@ -80,6 +80,14 @@ xterm-store-paste-on-kill-ring
   :version "28.1"
   :type 'boolean)
 
+(defcustom xterm-select-active-regions nil
+  "If non-nil, update PRIMARY X selection on text-mode frames.
+On a text-mode terminal that supports setSelection command, if
+this variable is non-nil, Emacs will set the PRIMARY selection
+from the active region, according to `select-active-regions'."
+  :version "29.1"
+  :type 'boolean)
+
 (defconst xterm-paste-ending-sequence "\e[201~"
   "Characters sent by the terminal to end a bracketed paste.")
 
diff --git a/src/keyboard.c b/src/keyboard.c
index 55d710ed62..9b34a47675 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -1567,9 +1567,15 @@ command_loop_1 (void)
 	    call0 (Qdeactivate_mark);
 	  else
 	    {
+	      Lisp_Object symval;
 	      /* Even if not deactivating the mark, set PRIMARY if
 		 `select-active-regions' is non-nil.  */
-	      if (!NILP (Fwindow_system (Qnil))
+	      if ((!NILP (Fwindow_system (Qnil))
+		   || ((symval =
+			find_symbol_value (Qxterm_select_active_regions),
+			(!EQ (symval, Qunbound) && !NILP (symval)))
+		       && !NILP (Fterminal_parameter (Qnil,
+						      Qxterm__set_selection))))
 		  /* Even if mark_active is non-nil, the actual buffer
 		     marker may not have been set yet (Bug#7044).  */
 		  && XMARKER (BVAR (current_buffer, mark))->buffer
@@ -12163,6 +12169,8 @@ syms_of_keyboard (void)
   DEFSYM (Qpolling_period, "polling-period");
 
   DEFSYM (Qgui_set_selection, "gui-set-selection");
+  DEFSYM (Qxterm__set_selection, "xterm--set-selection");
+  DEFSYM (Qxterm_select_active_regions, "xterm-select-active-regions");
 
   /* The primary selection.  */
   DEFSYM (QPRIMARY, "PRIMARY");
-- 
2.36.1.476.g0c4daa206d-goog

bug#55883: [PATCH] Update X Primary Selection with active regions

[Eli Zaretskii]

> From: Duncan Findlay <duncf@google.com>
> Date: Tue, 14 Jun 2022 19:01:58 -0700
> Cc: 55883@debbugs.gnu.org
> 
> > > !NILP (SYMBOL_VAL (XSYMBOL (Qxterm_select_active_regions)))
> >
> > This is not safe, IMO.  I think this is better:
> >
> >   if ((!NILP (Fwindow_system (Qnil))
> >        || ((symval = find_symbol_value (Qxterm_select_active_regions),
> >             (!EQ (symval, Qunbound) && !NILP (symval)))
> >             && !NILP (Fterminal_parameter (Qnil, Qxterm__set_selection))))
> 
> Thanks, I would never have figured that out myself.

That's what we are here for (one reason, at least).

> > > I've also addressed Po's comments about long conditionals.
> >
> > Thanks, the patch LGTM, modulo the above 2 minor nits.

This version LGTM, I will install soon, unless someone comes up with
more comments.

bug#55883: [PATCH] Update X Primary Selection with active regions

[Eli Zaretskii]

> Cc: 55883@debbugs.gnu.org
> Date: Wed, 15 Jun 2022 19:51:10 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > From: Duncan Findlay <duncf@google.com>
> > Date: Tue, 14 Jun 2022 19:01:58 -0700
> > Cc: 55883@debbugs.gnu.org
> > 
> > > > I've also addressed Po's comments about long conditionals.
> > >
> > > Thanks, the patch LGTM, modulo the above 2 minor nits.
> 
> This version LGTM, I will install soon, unless someone comes up with
> more comments.

No further comments, so I installed this on the master branch now.

Thanks.

bug#55883: [PATCH] Update X Primary Selection with active regions

[Basil L. Contovounesios via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Eli Zaretskii [2022-06-18 14:13 +0300] wrote:

>> Cc: 55883@debbugs.gnu.org
>> Date: Wed, 15 Jun 2022 19:51:10 +0300
>> From: Eli Zaretskii <eliz@gnu.org>
>> 
>> > From: Duncan Findlay <duncf@google.com>
>> > Date: Tue, 14 Jun 2022 19:01:58 -0700
>> > Cc: 55883@debbugs.gnu.org
>> > 
>> > > > I've also addressed Po's comments about long conditionals.
>> > >
>> > > Thanks, the patch LGTM, modulo the above 2 minor nits.
>> 
>> This version LGTM, I will install soon, unless someone comes up with
>> more comments.
>
> No further comments, so I installed this on the master branch now.

Thanks, but please see the resulting 'make check' errors attached.

Perhaps dereferencing xterm-select-active-regions should be guarded by
bound-and-true-p or the like?

-- 
Basil


[-- Attachment #2: errors.log --]
[-- Type: text/plain, Size: 14952 bytes --]

-*- mode: compilation; default-directory: "~/.local/src/emacs/" -*-
Compilation started at Sat Jun 18 18:07:38

make -k EMACS_TEST_VERBOSE=1 test/simple-tests test/lisp-tests
make -C test simple-tests
make[1]: Entering directory '/home/blc/.local/src/emacs/test'
make[2]: Entering directory '/home/blc/.local/src/emacs/test'
  GEN      lisp/simple-tests.log
Running 46 tests (2022-06-18 18:07:39+0300, selector `(not (or (tag :unstable) (tag :nativecomp)))')
Fill column set to 5 (was 70)
   passed   1/46  auto-fill-mode-no-break-before-length-of-fill-prefix (0.000131 sec)
   passed   2/46  command-execute-prune-command-history (0.000085 sec)
   passed   3/46  eval-expression-print-format-large-int (0.000089 sec)
  skipped   4/46  eval-expression-print-format-large-int-echo (0.000096 sec)
   passed   5/46  eval-expression-print-format-small-int (0.000066 sec)
  skipped   6/46  eval-expression-print-format-small-int-echo (0.000091 sec)
   passed   7/46  eval-expression-print-format-sym (0.000050 sec)
  skipped   8/46  eval-expression-print-format-sym-echo (0.000092 sec)
   passed   9/46  line-number-at-pos-in-narrow-buffer (0.000053 sec)
   passed  10/46  line-number-at-pos-in-widen-buffer (0.000048 sec)
   passed  11/46  line-number-at-pos-keeps-point (0.000044 sec)
   passed  12/46  line-number-at-pos-keeps-restriction (0.000051 sec)
   passed  13/46  line-number-at-pos-when-passing-point (0.000057 sec)

Undo
   passed  14/46  missing-record-point-in-undo (0.000246 sec)
   passed  15/46  newline (0.000489 sec)
   passed  16/46  newline-indent (0.000839 sec)
   passed  17/46  open-line (0.000472 sec)
   passed  18/46  open-line-hook (0.000379 sec)
   passed  19/46  open-line-indent (0.000574 sec)
   passed  20/46  open-line-margin-and-prefix (0.000393 sec)
   passed  21/46  simple-delete-indentation-blank-line (0.000121 sec)
   passed  22/46  simple-delete-indentation-boundaries (0.000076 sec)
   passed  23/46  simple-delete-indentation-inactive-region (0.000066 sec)
   passed  24/46  simple-delete-indentation-no-region (0.000091 sec)
   passed  25/46  simple-delete-indentation-prefix (0.000079 sec)
   passed  26/46  simple-delete-indentation-region (0.000121 sec)
   passed  27/46  simple-delete-trailing-whitespace--bug-21766 (0.022844 sec)
   passed  28/46  simple-delete-trailing-whitespace--formfeeds (0.000078 sec)
   passed  29/46  simple-test-count-lines (0.000067 sec)
   passed  30/46  simple-test-count-lines/ignore-invisible-lines (0.000067 sec)
   passed  31/46  simple-test-count-words-bug-41761 (0.000081 sec)
Undo
   passed  32/46  simple-test-undo-extra-boundary-in-tex (0.039261 sec)
Undo
Undo
Redo
Redo
Redo
Redo
Redo
Redo
Redo
Redo
Undo
Undo
Undo
Undo
Redo
Redo
Redo
Redo
   passed  33/46  simple-tests--undo (0.000296 sec)
Undo
Redo
Mark set
Undo in region
Test simple-tests--undo-equiv-table backtrace:
  display-selections-p()
  deactivate-mark(force)
  (let ((ul-hash-table (make-hash-table :test #'equal))) (let ((--doli
  (progn (buffer-enable-undo) (transient-mark-mode) (let ((ul-hash-tab
  (unwind-protect (progn (buffer-enable-undo) (transient-mark-mode) (l
  (save-current-buffer (set-buffer temp-buffer) (unwind-protect (progn
  (let ((temp-buffer (generate-new-buffer " *temp*" t))) (save-current
  (closure (t) nil (let ((temp-buffer (generate-new-buffer " *temp*" t
  ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
  ert-run-test(#s(ert-test :name simple-tests--undo-equiv-table :docum
  ert-run-or-rerun-test(#s(ert--stats :selector ... :tests ... :test-m
  ert-run-tests((not (or (tag :unstable) (tag :nativecomp))) #f(compil
  ert-run-tests-batch((not (or (tag :unstable) (tag :nativecomp))))
  ert-run-tests-batch-and-exit((not (or (tag :unstable) (tag :nativeco
  eval((ert-run-tests-batch-and-exit '(not (or (tag :unstable) (tag :n
  command-line-1(("-L" ":." "-l" "ert" "-l" "lisp/simple-tests.el" "--
  command-line()
  normal-top-level()
Test simple-tests--undo-equiv-table condition:
    (void-variable xterm-select-active-regions)
   FAILED  34/46  simple-tests--undo-equiv-table (0.000231 sec) at lisp/simple-tests.el:511
Mark set
Undo in region
Mark activated
Undo in region
Redo in region
Redo in region
   passed  35/46  simple-tests--undo-in-region (0.000447 sec)
   passed  36/46  simple-tests-async-shell-command-30280 (0.029269 sec)
   passed  37/46  simple-tests-shell-command-39067 (2.665546 sec)
   passed  38/46  simple-tests-shell-command-dont-erase-buffer (1.280669 sec)
   passed  39/46  simple-tests-zap-to-char (0.000376 sec)
   passed  40/46  simple-text-count-lines-non-ascii (0.000156 sec)
   passed  41/46  simple-transpose-subr (0.000744 sec)
   passed  42/46  test-undo-region (0.000201 sec)
Setting up indent for shell type bash
Indentation variables are now local.
Indentation setup for shell type bash
Mark set
Setting up indent for shell type bash
Indentation variables are now local.
Indentation setup for shell type bash
Mark set
Setting up indent for shell type bash
Indentation variables are now local.
Indentation setup for shell type bash
Mark set
   passed  43/46  test-yank-in-context (0.021730 sec)
   passed  44/46  undo-auto--boundaries-added (0.000075 sec)
   passed  45/46  undo-auto-boundary-timer (0.000038 sec)
Undo
Undo
   passed  46/46  undo-point-in-wrong-place (0.000379 sec)

Ran 46 tests, 42 results as expected, 1 unexpected, 3 skipped (2022-06-18 18:07:43+0300, 4.450341 sec)

1 unexpected results:
   FAILED  simple-tests--undo-equiv-table  xterm-select-active-regions

3 skipped results:
  SKIPPED  eval-expression-print-format-large-int-echo  ((skip-unless (not noninteractive)) :form (not t) :value nil)
  SKIPPED  eval-expression-print-format-small-int-echo  ((skip-unless (not noninteractive)) :form (not t) :value nil)
  SKIPPED  eval-expression-print-format-sym-echo  ((skip-unless (not noninteractive)) :form (not t) :value nil)

nilmake[2]: *** [Makefile:174: lisp/simple-tests.log] Error 1
make[2]: Leaving directory '/home/blc/.local/src/emacs/test'
make[1]: *** [Makefile:240: lisp/simple-tests] Error 2
make[1]: Target 'simple-tests' not remade because of errors.
make[1]: Leaving directory '/home/blc/.local/src/emacs/test'
make: *** [Makefile:1022: test/simple-tests] Error 2
make -C test lisp-tests
make[1]: Entering directory '/home/blc/.local/src/emacs/test'
make[2]: Entering directory '/home/blc/.local/src/emacs/test'
  GEN      lisp/emacs-lisp/lisp-tests.log
Running 37 tests (2022-06-18 18:07:44+0300, selector `(not (or (tag :unstable) (tag :nativecomp)))')
   passed   1/37  backward-up-list-basic (0.000117 sec)
   passed   2/37  core-elisp-tests-1-defvar-in-let (0.000057 sec)
Mark set
   passed   3/37  core-elisp-tests-2-window-configurations (0.000064 sec)
   passed   4/37  core-elisp-tests-3-backquote (0.000042 sec)
   passed   5/37  end-of-defun-twice (0.000238 sec)
   passed   6/37  lisp-backward-sexp-1-empty-parens (0.000048 sec)
   passed   7/37  lisp-backward-sexp-1-eobp (0.000047 sec)
   passed   8/37  lisp-backward-sexp-1-error-mismatch (0.000047 sec)
   passed   9/37  lisp-backward-sexp-2-bobp (0.000049 sec)
   passed  10/37  lisp-backward-sexp-2-bobp-and-subsequent (0.000054 sec)
   passed  11/37  lisp-delete-pair-parens (1.001608 sec)
   passed  12/37  lisp-delete-pair-quotation-marks (1.001738 sec)
   passed  13/37  lisp-delete-pair-quotes-in-text-mode (0.000242 sec)
   passed  14/37  lisp-delete-pair-quotes-text-mode-syntax-table (1.001739 sec)
   passed  15/37  lisp-fill-paragraph-colon (0.001325 sec)
   passed  16/37  lisp-forward-sexp-1-empty-parens (0.000162 sec)
   passed  17/37  lisp-forward-sexp-1-eobp (0.000145 sec)
   passed  18/37  lisp-forward-sexp-1-error-mismatch (0.000159 sec)
   passed  19/37  lisp-forward-sexp-2-eobp (0.000158 sec)
   passed  20/37  lisp-forward-sexp-2-eobp-and-subsequent (0.000165 sec)
   passed  21/37  lisp-forward-sexp-elisp-inside-symbol (0.000409 sec)
   passed  22/37  lisp-forward-sexp-elisp-quoted-symbol (0.000376 sec)
   passed  23/37  lisp-forward-sexp-emacs-lisp-quote-char (0.000369 sec)
   passed  24/37  lisp-forward-sexp-emacs-lisp-semi-char-error (0.000339 sec)
   passed  25/37  lisp-forward-sexp-python-triple-quoted-string (0.001036 sec)
   passed  26/37  lisp-forward-sexp-python-triple-quotes-string (0.000929 sec)
   passed  27/37  mark-defun-arg-region-active (0.000594 sec)
Mark set
Mark set
Mark set
Mark set
Mark set
Mark set
Mark set
Mark set
   passed  28/37  mark-defun-bob (0.001626 sec)
Mark set
Mark set
Test mark-defun-neg-arg-region-inactive backtrace:
  display-selections-p()
  deactivate-mark()
  (let ((after-4 216) (inside-4 207) (before-4 174) (after-3 174) (ins
  (progn (emacs-lisp-mode) (insert ";; Comment header\n\n(defun func-1
  (unwind-protect (progn (emacs-lisp-mode) (insert ";; Comment header\
  (save-current-buffer (set-buffer temp-buffer) (unwind-protect (progn
  (let ((temp-buffer (generate-new-buffer " *temp*" t))) (save-current
  (closure (c-e-x t) nil (setq last-command nil) (let ((temp-buffer (g
  ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
  ert-run-test(#s(ert-test :name mark-defun-neg-arg-region-inactive :d
  ert-run-or-rerun-test(#s(ert--stats :selector ... :tests ... :test-m
  ert-run-tests((not (or (tag :unstable) (tag :nativecomp))) #f(compil
  ert-run-tests-batch((not (or (tag :unstable) (tag :nativecomp))))
  ert-run-tests-batch-and-exit((not (or (tag :unstable) (tag :nativeco
  eval((ert-run-tests-batch-and-exit '(not (or (tag :unstable) (tag :n
  command-line-1(("-L" ":." "-l" "ert" "-l" "lisp/emacs-lisp/lisp-test
  command-line()
  normal-top-level()
Test mark-defun-neg-arg-region-inactive condition:
    (void-variable xterm-select-active-regions)
   FAILED  29/37  mark-defun-neg-arg-region-inactive (0.000682 sec) at lisp/emacs-lisp/lisp-tests.el:546
Test mark-defun-no-arg-region-active backtrace:
  display-selections-p()
  deactivate-mark()
  (let ((after-4 216) (inside-4 207) (before-4 174) (after-3 174) (ins
  (progn (emacs-lisp-mode) (insert ";; Comment header\n\n(defun func-1
  (unwind-protect (progn (emacs-lisp-mode) (insert ";; Comment header\
  (save-current-buffer (set-buffer temp-buffer) (unwind-protect (progn
  (let ((temp-buffer (generate-new-buffer " *temp*" t))) (save-current
  (closure (c-e-x t) nil (transient-mark-mode 1) (setq last-command ni
  ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
  ert-run-test(#s(ert-test :name mark-defun-no-arg-region-active :docu
  ert-run-or-rerun-test(#s(ert--stats :selector ... :tests ... :test-m
  ert-run-tests((not (or (tag :unstable) (tag :nativecomp))) #f(compil
  ert-run-tests-batch((not (or (tag :unstable) (tag :nativecomp))))
  ert-run-tests-batch-and-exit((not (or (tag :unstable) (tag :nativeco
  eval((ert-run-tests-batch-and-exit '(not (or (tag :unstable) (tag :n
  command-line-1(("-L" ":." "-l" "ert" "-l" "lisp/emacs-lisp/lisp-test
  command-line()
  normal-top-level()
Test mark-defun-no-arg-region-active condition:
    (void-variable xterm-select-active-regions)
   FAILED  30/37  mark-defun-no-arg-region-active (0.000302 sec) at lisp/emacs-lisp/lisp-tests.el:476
Mark set
Mark set
Test mark-defun-no-arg-region-inactive backtrace:
  display-selections-p()
  deactivate-mark()
  (let ((after-4 216) (inside-4 207) (before-4 174) (after-3 174) (ins
  (progn (emacs-lisp-mode) (insert ";; Comment header\n\n(defun func-1
  (unwind-protect (progn (emacs-lisp-mode) (insert ";; Comment header\
  (save-current-buffer (set-buffer temp-buffer) (unwind-protect (progn
  (let ((temp-buffer (generate-new-buffer " *temp*" t))) (save-current
  (closure (c-e-x t) nil (setq last-command nil) (let ((temp-buffer (g
  ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
  ert-run-test(#s(ert-test :name mark-defun-no-arg-region-inactive :do
  ert-run-or-rerun-test(#s(ert--stats :selector ... :tests ... :test-m
  ert-run-tests((not (or (tag :unstable) (tag :nativecomp))) #f(compil
  ert-run-tests-batch((not (or (tag :unstable) (tag :nativecomp))))
  ert-run-tests-batch-and-exit((not (or (tag :unstable) (tag :nativeco
  eval((ert-run-tests-batch-and-exit '(not (or (tag :unstable) (tag :n
  command-line-1(("-L" ":." "-l" "ert" "-l" "lisp/emacs-lisp/lisp-test
  command-line()
  normal-top-level()
Test mark-defun-no-arg-region-inactive condition:
    (void-variable xterm-select-active-regions)
   FAILED  31/37  mark-defun-no-arg-region-inactive (0.000294 sec) at lisp/emacs-lisp/lisp-tests.el:427
Mark set
Mark set
Test mark-defun-pos-arg-region-inactive backtrace:
  display-selections-p()
  deactivate-mark()
  (let ((after-4 216) (inside-4 207) (before-4 174) (after-3 174) (ins
  (progn (emacs-lisp-mode) (insert ";; Comment header\n\n(defun func-1
  (unwind-protect (progn (emacs-lisp-mode) (insert ";; Comment header\
  (save-current-buffer (set-buffer temp-buffer) (unwind-protect (progn
  (let ((temp-buffer (generate-new-buffer " *temp*" t))) (save-current
  (closure (c-e-x t) nil (setq last-command nil) (let ((temp-buffer (g
  ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
  ert-run-test(#s(ert-test :name mark-defun-pos-arg-region-inactive :d
  ert-run-or-rerun-test(#s(ert--stats :selector ... :tests ... :test-m
  ert-run-tests((not (or (tag :unstable) (tag :nativecomp))) #f(compil
  ert-run-tests-batch((not (or (tag :unstable) (tag :nativecomp))))
  ert-run-tests-batch-and-exit((not (or (tag :unstable) (tag :nativeco
  eval((ert-run-tests-batch-and-exit '(not (or (tag :unstable) (tag :n
  command-line-1(("-L" ":." "-l" "ert" "-l" "lisp/emacs-lisp/lisp-test
  command-line()
  normal-top-level()
Test mark-defun-pos-arg-region-inactive condition:
    (void-variable xterm-select-active-regions)
   FAILED  32/37  mark-defun-pos-arg-region-inactive (0.000331 sec) at lisp/emacs-lisp/lisp-tests.el:522
   passed  33/37  up-list-basic (0.000104 sec)
   passed  34/37  up-list-cross-string (0.000087 sec)
   passed  35/37  up-list-no-cross-string (0.000083 sec)
   passed  36/37  up-list-out-of-string (0.000078 sec)
   passed  37/37  up-list-with-forward-sexp-function (0.000077 sec)

Ran 37 tests, 33 results as expected, 4 unexpected (2022-06-18 18:07:48+0300, 4.020046 sec)

4 unexpected results:
   FAILED  mark-defun-neg-arg-region-inactive  xterm-select-active-regions
   FAILED  mark-defun-no-arg-region-active  xterm-select-active-regions
   FAILED  mark-defun-no-arg-region-inactive  xterm-select-active-regions
   FAILED  mark-defun-pos-arg-region-inactive  xterm-select-active-regions

make[2]: *** [Makefile:174: lisp/emacs-lisp/lisp-tests.log] Error 1
make[2]: Leaving directory '/home/blc/.local/src/emacs/test'
make[1]: *** [Makefile:240: lisp/emacs-lisp/lisp-tests] Error 2
make[1]: Target 'lisp-tests' not remade because of errors.
make[1]: Leaving directory '/home/blc/.local/src/emacs/test'
make: *** [Makefile:1022: test/lisp-tests] Error 2

Compilation exited abnormally with code 2 at Sat Jun 18 18:07:48

bug#55883: [PATCH] Update X Primary Selection with active regions

[Eli Zaretskii]

> From: "Basil L. Contovounesios" <contovob@tcd.ie>
> Cc: eliz@gnu.org,  duncf@google.com
> Date: Sat, 18 Jun 2022 18:15:28 +0300
> 
> > No further comments, so I installed this on the master branch now.
> 
> Thanks, but please see the resulting 'make check' errors attached.
> 
> Perhaps dereferencing xterm-select-active-regions should be guarded by
> bound-and-true-p or the like?

No, that's ugly.  I fixed it in a different way, thanks.

bug#55883: [PATCH] Update X Primary Selection with active regions

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

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

Oops. I'm sorry I missed that, and thank you for the quick fixes! It
looks like you renamed the variable, and put it in frame.el with
7e1f84fa3bc7dfd84415813889c91070c0759da2 and
4e68166d77cdd0f3b84c9bf5681f6a95e51ad238. I assume that means that
frame.el is always loaded in tests, and that xterm.el is sometimes not
loaded?

I don't have a problem with the current fix, though I'm wondering if
the attached patch would have worked too?

Thanks

Duncan


On Sat, Jun 18, 2022 at 9:10 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: "Basil L. Contovounesios" <contovob@tcd.ie>
> > Cc: eliz@gnu.org,  duncf@google.com
> > Date: Sat, 18 Jun 2022 18:15:28 +0300
> >
> > > No further comments, so I installed this on the master branch now.
> >
> > Thanks, but please see the resulting 'make check' errors attached.
> >
> > Perhaps dereferencing xterm-select-active-regions should be guarded by
> > bound-and-true-p or the like?
>
> No, that's ugly.  I fixed it in a different way, thanks.

[-- Attachment #2: 0001-Set-default-value-for-xterm-select-active-regions.patch --]
[-- Type: application/x-patch, Size: 853 bytes --]

bug#55883: [PATCH] Update X Primary Selection with active regions

[Eli Zaretskii]

> From: Duncan Findlay <duncf@google.com>
> Date: Tue, 21 Jun 2022 18:58:16 -0700
> Cc: "Basil L. Contovounesios" <contovob@tcd.ie>, 55883@debbugs.gnu.org
> 
> Oops. I'm sorry I missed that, and thank you for the quick fixes! It
> looks like you renamed the variable, and put it in frame.el with
> 7e1f84fa3bc7dfd84415813889c91070c0759da2 and
> 4e68166d77cdd0f3b84c9bf5681f6a95e51ad238. I assume that means that
> frame.el is always loaded in tests, and that xterm.el is sometimes not
> loaded?

frame.el is preloaded, so it is present in every Emacs session, yes.
xterm.el is only loaded when Emacs needs to display some frame on a
TTY terminal emulated by xterm.

> I don't have a problem with the current fix, though I'm wondering if
> the attached patch would have worked too?

I considered that, but I don't like to have variables related to xterm
(or any other particular terminal type) in a general-purpose
infrastructure code.  I apologize for not realizing this adverse
effect earlier.

bug#55883: [PATCH] Update X Primary Selection with active regions

[Basil L. Contovounesios via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Duncan Findlay [2022-06-21 18:58 -0700] wrote:

> I'm wondering if the attached patch would have worked too?

Its downside is that the variable can then be defined in more than one
place, with potentially different/out-of-sync values, depending on load
order.  This makes debugging and maintenance a bit harder.  Using
bound-and-true-p would have achieved the same effect while keeping the
user option squarely defined in a single place.  But Eli's patch
generalises even further.

Thanks,

-- 
Basil