GNU bug report logs - #55883
[PATCH] Update X Primary Selection with active regions

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 55883 in the body.
You can then email your comments to 55883 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Duncan Findlay <duncf <at> google.com>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Update X Primary Selection with active regions
Date: Thu, 9 Jun 2022 23:18:01 -0700

[Message part 1 (text/plain, inline)]

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.

[0001-Use-display-selections-p-for-selecting-active-region.patch (text/x-patch, attachment)]

[0002-Support-select-active-regions-with-xterm.patch (text/x-patch, attachment)]

Message #8 received at 55883 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Duncan Findlay <duncf <at> google.com>
Cc: 55883 <at> debbugs.gnu.org
Subject: Re: bug#55883: [PATCH] Update X Primary Selection with active regions
Date: Fri, 10 Jun 2022 09:52:01 +0300

> 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 <at> 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?

Message #11 received at 55883 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Duncan Findlay <duncf <at> google.com>
Cc: 55883 <at> debbugs.gnu.org
Subject: Re: bug#55883: [PATCH] Update X Primary Selection with active regions
Date: Sat, 11 Jun 2022 10:53:14 +0300

> From: Duncan Findlay <duncf <at> google.com>
> Date: Fri, 10 Jun 2022 18:59:47 -0700
> Cc: 55883 <at> 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.

Message #14 received at 55883 <at> debbugs.gnu.org (full text, mbox):

From: Duncan Findlay <duncf <at> google.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 55883 <at> debbugs.gnu.org
Subject: Re: bug#55883: [PATCH] Update X Primary Selection with active regions
Date: Fri, 10 Jun 2022 18:59:47 -0700

[Message part 1 (text/plain, inline)]

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 <at> 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 <at> 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?

[0001-Support-select-active-regions-with-xterm.patch (text/x-patch, attachment)]

Message #17 received at 55883 <at> debbugs.gnu.org (full text, mbox):

From: Po Lu <luangruo <at> yahoo.com>
To: Duncan Findlay <duncf <at> google.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 55883 <at> debbugs.gnu.org
Subject: Re: bug#55883: [PATCH] Update X Primary Selection with active regions
Date: Tue, 14 Jun 2022 13:57:33 +0800

>> +	      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.

Message #20 received at 55883 <at> debbugs.gnu.org (full text, mbox):

From: Duncan Findlay <duncf <at> google.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 55883 <at> debbugs.gnu.org
Subject: Re: bug#55883: [PATCH] Update X Primary Selection with active regions
Date: Mon, 13 Jun 2022 23:36:13 -0700

[Message part 1 (text/plain, inline)]

On Sat, Jun 11, 2022 at 12:53 AM Eli Zaretskii <eliz <at> gnu.org> wrote:
>
> > From: Duncan Findlay <duncf <at> google.com>
> > Date: Fri, 10 Jun 2022 18:59:47 -0700
> > Cc: 55883 <at> 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

[0001-Support-select-active-regions-with-xterm.patch (application/x-patch, attachment)]

Message #23 received at 55883 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Duncan Findlay <duncf <at> google.com>
Cc: 55883 <at> debbugs.gnu.org
Subject: Re: bug#55883: [PATCH] Update X Primary Selection with active regions
Date: Tue, 14 Jun 2022 15:15:06 +0300

> From: Duncan Findlay <duncf <at> google.com>
> Date: Mon, 13 Jun 2022 23:36:13 -0700
> Cc: 55883 <at> 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.

Message #26 received at 55883 <at> debbugs.gnu.org (full text, mbox):

From: Duncan Findlay <duncf <at> google.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 55883 <at> debbugs.gnu.org
Subject: Re: bug#55883: [PATCH] Update X Primary Selection with active regions
Date: Tue, 14 Jun 2022 19:01:58 -0700

[Message part 1 (text/plain, inline)]

On Tue, Jun 14, 2022 at 5:15 AM Eli Zaretskii <eliz <at> gnu.org> wrote:
>
> > From: Duncan Findlay <duncf <at> google.com>
> > Date: Mon, 13 Jun 2022 23:36:13 -0700
> > Cc: 55883 <at> 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

[0001-Support-select-active-regions-with-xterm.patch (text/x-patch, attachment)]

Message #29 received at 55883 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Duncan Findlay <duncf <at> google.com>
Cc: 55883 <at> debbugs.gnu.org
Subject: Re: bug#55883: [PATCH] Update X Primary Selection with active regions
Date: Wed, 15 Jun 2022 19:51:10 +0300

> From: Duncan Findlay <duncf <at> google.com>
> Date: Tue, 14 Jun 2022 19:01:58 -0700
> Cc: 55883 <at> 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.

Message #34 received at 55883-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: duncf <at> google.com
Cc: 55883-done <at> debbugs.gnu.org
Subject: Re: bug#55883: [PATCH] Update X Primary Selection with active regions
Date: Sat, 18 Jun 2022 14:13:53 +0300

> Cc: 55883 <at> debbugs.gnu.org
> Date: Wed, 15 Jun 2022 19:51:10 +0300
> From: Eli Zaretskii <eliz <at> gnu.org>
> 
> > From: Duncan Findlay <duncf <at> google.com>
> > Date: Tue, 14 Jun 2022 19:01:58 -0700
> > Cc: 55883 <at> 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.

Message #37 received at 55883 <at> debbugs.gnu.org (full text, mbox):

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: 55883 <at> debbugs.gnu.org
Cc: eliz <at> gnu.org, duncf <at> google.com
Subject: Re: bug#55883: [PATCH] Update X Primary Selection with active regions
Date: Sat, 18 Jun 2022 18:15:28 +0300

[Message part 1 (text/plain, inline)]

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

>> Cc: 55883 <at> debbugs.gnu.org
>> Date: Wed, 15 Jun 2022 19:51:10 +0300
>> From: Eli Zaretskii <eliz <at> gnu.org>
>> 
>> > From: Duncan Findlay <duncf <at> google.com>
>> > Date: Tue, 14 Jun 2022 19:01:58 -0700
>> > Cc: 55883 <at> 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

[errors.log (text/plain, attachment)]

Message #40 received at 55883 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: "Basil L. Contovounesios" <contovob <at> tcd.ie>
Cc: 55883 <at> debbugs.gnu.org, duncf <at> google.com
Subject: Re: bug#55883: [PATCH] Update X Primary Selection with active regions
Date: Sat, 18 Jun 2022 19:09:54 +0300

> From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
> Cc: eliz <at> gnu.org,  duncf <at> 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.

Message #43 received at 55883 <at> debbugs.gnu.org (full text, mbox):

From: Duncan Findlay <duncf <at> google.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: "Basil L. Contovounesios" <contovob <at> tcd.ie>, 55883 <at> debbugs.gnu.org
Subject: Re: bug#55883: [PATCH] Update X Primary Selection with active regions
Date: Tue, 21 Jun 2022 18:58:16 -0700

[Message part 1 (text/plain, inline)]

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 <at> gnu.org> wrote:
>
> > From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
> > Cc: eliz <at> gnu.org,  duncf <at> 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.

[0001-Set-default-value-for-xterm-select-active-regions.patch (application/x-patch, attachment)]

Message #46 received at 55883 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Duncan Findlay <duncf <at> google.com>
Cc: contovob <at> tcd.ie, 55883 <at> debbugs.gnu.org
Subject: Re: bug#55883: [PATCH] Update X Primary Selection with active regions
Date: Wed, 22 Jun 2022 16:02:30 +0300

> From: Duncan Findlay <duncf <at> google.com>
> Date: Tue, 21 Jun 2022 18:58:16 -0700
> Cc: "Basil L. Contovounesios" <contovob <at> tcd.ie>, 55883 <at> 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.

Message #49 received at 55883 <at> debbugs.gnu.org (full text, mbox):

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: Duncan Findlay <duncf <at> google.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 55883 <at> debbugs.gnu.org
Subject: Re: bug#55883: [PATCH] Update X Primary Selection with active regions
Date: Wed, 22 Jun 2022 18:37:24 +0300

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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.