bug#60841: 30.0.50; kill-ring-save pauses despite region being highlighted

bug#60841: 30.0.50; kill-ring-save pauses despite region being highlighted

[Kévin Le Gouguec]

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

Hello Emacs,

kill-ring-save calls indicate-copied-region, which does a quick
point-and-mark swap (for copy-region-blink-delay seconds) to give the
user an idea of the extent of text that has been saved.

The docstring says that this is done "if there is currently no active
region highlighting"; in practice, this translates to:

	;; Swap point-and-mark quickly so as to show the region that
	;; was selected.  Don't do it if the region is highlighted.
	(when (and (numberp copy-region-blink-delay)
		   (> copy-region-blink-delay 0)
		   (or (not (region-active-p))                  ; (a)
		       (not (face-background 'region nil t))))  ; (b)

IOW "active region highlighting" means "(a) an active region, and (b)
any background for the region face".  face-background, however, is not
enough to asses (b); consider, from emacs -Q:

M-: (custom-set-faces '(region ((t (:foreground "blue" :inverse-video t)))))
C-x h
M-w

In this situation, the region face has a clearly visible background, yet
indicate-copied-region still behaves as if the region is not
"highlighted", and triggers the (harmless and entirely interruptible)
point-and-mark swap followed by a pause.

I did not know about this feature, and only found out about it by
accident after customizing the region face.  Only after (1) assuming
something was off between Emacs and the system clipboard (2) failing to
use debug-on-quit to figure out where Emacs was "stuck" (it wasn't, of
course) (3) diving into kill-ring-save, did I realize what was going on.

The attached patch handles this foreground + inverse-video switcheroo.
Not sure how many themes actually do that in the wild, so I'd understand
if there wasn't much enthusiasm for applying.  That face-background
check has been with us since 2004; haven't found any hint in the BTS
that anyone else has been bothered by this false negative.


(FWIW, I stumbled on this while working on a dark theme with a limited
 palette; having exhausted all backgrounds on other faces and struggling
 to find one to dedicate to the region, I figured I could combine one of
 my bright foreground colors with :inverse-video, and leave :background
 unspecified.

 Of course, instead of:

   :foreground SOMETHING-BRIGHT
   :inverse-video t

 I could specify:

   :background SOMETHING-BRIGHT
   :foreground SOMETHING-DARK

 … but my initial idea was to let the chips fall where they may and
 allow any text background to become a foreground within the region,
 since all of the theme's backgrounds ought to be dark enough to
 contrast with SOMETHING-BRIGHT)


Thanks for your time.


 GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version
 3.24.35, cairo version 1.17.6) of 2023-01-02 built on amdahl30
Repository revision: c209802f7b3721a1b95113290934a23fee88f678
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12101006
System Description: openSUSE Tumbleweed

Configured using:
 'configure --with-cairo --with-gconf --with-sqlite3 --with-xinput2'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES NOTIFY
INOTIFY PDUMPER PNG RSVG SECCOMP SOUND SQLITE3 THREADS TIFF
TOOLKIT_SCROLL_BARS TREE_SITTER WEBP X11 XDBE XIM XINPUT2 XPM GTK3 ZLIB

Important settings:
  value of $LANG: en_US.UTF-8
  value of $XMODIFIERS: @im=local
  locale-coding-system: utf-8-unix


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Avoid-spurious-pause-in-kill-ring-save.patch --]
[-- Type: text/x-patch, Size: 2327 bytes --]

From 7bfb6883fb39fe44282a547a4562f9f4d60058ae Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?K=C3=A9vin=20Le=20Gouguec?= <kevin.legouguec@gmail.com>
Date: Sun, 15 Jan 2023 19:23:17 +0100
Subject: [PATCH] Avoid spurious pause in kill-ring-save

'indicate-copied-region' checks whether the region is highlighted and
if not, briefly moves point to mark to give a visual cue of the extent
of text that was saved to the kill ring.

The region is assumed to be "highlighted" if (a) it is active and (b)
its face specifies a :background.  That latter condition does not
account for this somewhat adversarial face definition:

(custom-set-faces
  '(region ((t (:foreground "blue" :inverse-video t)))))

With this customization, the check will fail and users will experience
an (interruptible) pause, despite the region being clearly
highlighted.

* lisp/simple.el (region-highlighted-p): New function to detect "if
the region is highlighted".  If :background is nil, double-check
:foreground and :inverse-video.
(indicate-copied-region): Use it.
---
 lisp/simple.el | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/lisp/simple.el b/lisp/simple.el
index 4b09f41de5..2029584a94 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -5853,6 +5853,15 @@ copy-region-blink-delay
   :group 'killing
   :version "28.1")
 
+(defun region-highlighted-p ()
+  "Say whether the region is visibly highlighted.
+This takes into account whether the region is active, and whether
+the `region' face has a colored background."
+  (and (region-active-p)
+       (or (face-background 'region nil t)
+           (and (face-foreground 'region nil t)
+                (face-inverse-video-p 'region nil t)))))
+
 (defun indicate-copied-region (&optional message-len)
   "Indicate that the region text has been copied interactively.
 If the mark is visible in the selected window, blink the cursor between
@@ -5873,8 +5882,7 @@ indicate-copied-region
 	;; was selected.  Don't do it if the region is highlighted.
 	(when (and (numberp copy-region-blink-delay)
 		   (> copy-region-blink-delay 0)
-		   (or (not (region-active-p))
-		       (not (face-background 'region nil t))))
+		   (not (region-highlighted-p)))
 	  ;; Swap point and mark.
 	  (set-marker (mark-marker) (point) (current-buffer))
 	  (goto-char mark)
-- 
2.39.0

bug#60841: 30.0.50; kill-ring-save pauses despite region being highlighted

[Eli Zaretskii]

> From: Kévin Le Gouguec <kevin.legouguec@gmail.com>
> Date: Mon, 16 Jan 2023 00:38:02 +0100
> 
> The docstring says that this is done "if there is currently no active
> region highlighting"; in practice, this translates to:
> 
> 	;; Swap point-and-mark quickly so as to show the region that
> 	;; was selected.  Don't do it if the region is highlighted.
> 	(when (and (numberp copy-region-blink-delay)
> 		   (> copy-region-blink-delay 0)
> 		   (or (not (region-active-p))                  ; (a)
> 		       (not (face-background 'region nil t))))  ; (b)
> 
> IOW "active region highlighting" means "(a) an active region, and (b)
> any background for the region face".  face-background, however, is not
> enough to asses (b); consider, from emacs -Q:
> 
> M-: (custom-set-faces '(region ((t (:foreground "blue" :inverse-video t)))))
> C-x h
> M-w
> 
> In this situation, the region face has a clearly visible background, yet
> indicate-copied-region still behaves as if the region is not
> "highlighted", and triggers the (harmless and entirely interruptible)
> point-and-mark swap followed by a pause.

Yes, the existing conditions are a bit naïve.

> The attached patch handles this foreground + inverse-video switcheroo.
> Not sure how many themes actually do that in the wild, so I'd understand
> if there wasn't much enthusiasm for applying.  That face-background
> check has been with us since 2004; haven't found any hint in the BTS
> that anyone else has been bothered by this false negative.

IMNSHO, this just makes another mini-step (albeit in the right
direction), instead of going all the way.  There are other face
attributes that can make the region stand out on display: what about
underline or italics, for example?  And vice versa: the face could
have a background that is identical or almost identical to the default
face.

We could keep adding conditions for more and more face attributes, one
by one.  But it would be better to have there a test which would tell
us whether the region face is "visually different" from the default
face.  Can we do something like that?

Alternatively, we could add a user option to make the swap
unconditional, because maybe some users would prefer that to splitting
hair in this case.  Then we could stop worrying about all those fine
differences.

Thanks.

bug#60841: 30.0.50; kill-ring-save pauses despite region being highlighted

[Kévin Le Gouguec]

Eli Zaretskii <eliz@gnu.org> writes:

>> The attached patch handles this foreground + inverse-video switcheroo.
>> Not sure how many themes actually do that in the wild, so I'd understand
>> if there wasn't much enthusiasm for applying.  That face-background
>> check has been with us since 2004; haven't found any hint in the BTS
>> that anyone else has been bothered by this false negative.
>
> IMNSHO, this just makes another mini-step (albeit in the right
> direction), instead of going all the way.  There are other face
> attributes that can make the region stand out on display: what about
> underline or italics, for example?  And vice versa: the face could
> have a background that is identical or almost identical to the default
> face.

Indeed.  I wasn't sure how much brain-racking we would feel like
devoting to this issue, hence the naive patch; thank you for prompting
more discussion.

> We could keep adding conditions for more and more face attributes, one
> by one.  But it would be better to have there a test which would tell
> us whether the region face is "visually different" from the default
> face.  Can we do something like that?

I struggle to find a heuristic that I'd dare call "smart", so here's a
dumb one: if _any_ attribute of the region face differs from that of the
default face, then odds are whoever customized the region face (theme
author or end user) intended for it to "stand out" to some extent?

(defun dumb/stands-out-from-default-p ()
  (seq-some
   (lambda (attribute)
     (unless (memq attribute '(:extend :inherit))
       (let ((default-value (face-attribute 'default attribute nil t))
             (region-value (face-attribute 'region attribute nil t)))
         (and (not (eq region-value 'unspecified))
              (not (equal region-value default-value))))))
   (mapcar 'car (face-all-attributes 'default))))

> Alternatively, we could add a user option to make the swap
> unconditional, because maybe some users would prefer that to splitting
> hair in this case.  Then we could stop worrying about all those fine
> differences.

Right, after discovering the feature swap I was surprised to find no way
to enable it regardless of whether the region is highlighted.  Sure,
OT1H I thought Emacs was bugging out on me; OTOH now knowing what's
happening (and in particular, knowing that we're in sit-for so not
frozen), considering that the region is deactivated when M-w ends, I
could imagine some people finding this swap helpful and preferring to
have it on always.


Wrapping up…

(a) Would it make sense to introduce a defcustom
    (copy-region-indication-predicate?) that allows nil, t, or a
    predicate function?  The default value being:

    (and (region-active-p)
         (smart/stands-out-from-default-p))

    (Random considerations:
     * nil would be redundant with copy-region-blink-delay 0 or nil,
       not sure how much we care,
     * we could require this to always be a function instead, since
       'always is a thing)

(b) No flash of genius on how to determine whether the region face
    "stands out", yet.

bug#60841: 30.0.50; kill-ring-save pauses despite region being highlighted

[Gregory Heytings]

>
> But it would be better to have there a test which would tell us whether 
> the region face is "visually different" from the default face.  Can we 
> do something like that?
>

face-differs-from-default-p?

bug#60841: 30.0.50; kill-ring-save pauses despite region being highlighted

[Kévin Le Gouguec]

Gregory Heytings <gregory@heytings.org> writes:

>> But it would be better to have there a test which would tell us whether the
>> region face is "visually different" from the default face.  Can we do
>> something like that?
>>
>
> face-differs-from-default-p?

Nice catch; IIUC display-supports-face-attributes-p is what is doing the
heavy work.

Might need to be made smarter wrt :extend though?

(set-face-background 'region (face-background 'default nil t))
(face-differs-from-default-p 'region nil)
; ⇒ :extend

AFAICT (from messing around without any kind of scripted recipe, so
caveat lector),

* :extend nil on 'default does not prevent its background/underline from
  spanning the whole window width,

* but :extend t on 'default does not mean that other faces will be
  extended, even if they have :extend 'unspecified.

(This last point might have gone without saying for the audience; I
admit to not knowing enough about face internals to understand which
attributes are implicitly "inherited" from 'default)

So,

* :extend nil for both: they display differently (region will not be
  extended, default will be),
* :extend t for both: they display the same,
* default has nil, region has t: they display the same,
* default has t, region has nil: they display differently.

Ergo, assuming (a) I didn't mess something up (b) this is the expected
behaviour, it seems that when considering :extend,
(face-differs-from-default-p FACE FRAME) should check

(1) whether FACE's :extend is nil (regardless of default's :extend),
    and
(2) whether FACE's :underline or :background are "supported", as
    reported by display-supports-face-attributes-p.

Hope the coffee kicked in and the above made sense.

bug#60841: 30.0.50; kill-ring-save pauses despite region being highlighted

[Gregory Heytings]

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


>>> But it would be better to have there a test which would tell us 
>>> whether the region face is "visually different" from the default face. 
>>> Can we do something like that?
>>
>> face-differs-from-default-p?
>
> Nice catch; IIUC display-supports-face-attributes-p is what is doing the 
> heavy work.
>
> Might need to be made smarter wrt :extend though?
>

Indeed...

>
> (set-face-background 'region (face-background 'default nil t))
> (face-differs-from-default-p 'region nil)
> ; ⇒ :extend
>

... although that's a really contrived example.

I'm not sure it's TRT to add too much complexity there, so I would suggest 
to add an additional optional argument to face-differs-from-default-p, to 
exclude one or more attributes from the comparison (in this case :extend, 
but someone else might be interested in excluding other attributes in the 
future).

bug#60841: 30.0.50; kill-ring-save pauses despite region being highlighted

[Kévin Le Gouguec]

Gregory Heytings <gregory@heytings.org> writes:

>>>> But it would be better to have there a test which would tell us whether the
>>>> region face is "visually different" from the default face. Can we do
>>>> something like that?
>>>
>>> face-differs-from-default-p?
>>
>> Nice catch; IIUC display-supports-face-attributes-p is what is doing the heavy
>> work.
>>
>> Might need to be made smarter wrt :extend though?
>>
>
> Indeed...
>
>>
>> (set-face-background 'region (face-background 'default nil t))
>> (face-differs-from-default-p 'region nil)
>> ; ⇒ :extend
>>
>
> ... although that's a really contrived example.

OK, how about this example:

>   (defun mtmm-reset-background ()
>     (set-face-attribute 'region nil :background 'unspecified))

Straight from <87vfjhre9z.fsf@zamazal.org> (emacs-devel), i.e. the user
report which resulted in the current code in indicate-copied-region.

Same result: :extend differs, so (face-differs-from-default-p 'region)
returns t, which for this user would cause a regression compared to
(face-background 'region nil t).

> I'm not sure it's TRT to add too much complexity there, so I would suggest to
> add an additional optional argument to face-differs-from-default-p, to exclude
> one or more attributes from the comparison (in this case :extend, but someone
> else might be interested in excluding other attributes in the future).

I really think how face-differs-from-default-p examines attributes…

              (and
               (not (eq attr-val 'unspecified))
               (display-supports-face-attributes-p (list attr attr-val)
                                                   frame))

… makes no sense for :extend.  Here's an example (and this one is
contrived alright):

  (set-face-attribute 'default nil :underline t)
  (set-face-attribute 'region nil :underline nil :extend VALUE)

* VALUE is 'unspecified: the condition becomes nil (meaning ":extend
  does not make this face look different than default"), whereas that
  actually _does_ make a difference (:underline nil too, obviously, and
  luckily face-differs-from-default-p picks up on this),

* VALUE is nil: (display-supports-face-attributes-p '(:extend nil))
  returns nil (because see gui_supports_face_attributes_p), despite
  :extend nil procuding different results for :underline.

* VALUE is t: for the same reason, display-supports-face-attributes-p
  returns t, despite :extend t making :underline indistinguishable from
  the default face.

Granted, yeah, contrived, since that calls for :underline t on the
default face and who does that.

But since, as we've seen, :extend causes a false positive (face reported
as "displaying differently" despite displaying identically) when a face
has a :background equal to the default's, I don't see how the current
behaviour helps anyone.

bug#60841: 30.0.50; kill-ring-save pauses despite region being highlighted

[Eli Zaretskii]

> From: Kévin Le Gouguec <kevin.legouguec@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  60841@debbugs.gnu.org
> Date: Tue, 17 Jan 2023 23:03:32 +0100
> 
> Gregory Heytings <gregory@heytings.org> writes:
> 
> >>>> But it would be better to have there a test which would tell us whether the
> >>>> region face is "visually different" from the default face. Can we do
> >>>> something like that?
> >>>
> >>> face-differs-from-default-p?
> >>
> >> Nice catch; IIUC display-supports-face-attributes-p is what is doing the heavy
> >> work.
> >>
> >> Might need to be made smarter wrt :extend though?
> >>
> >
> > Indeed...
> >
> >>
> >> (set-face-background 'region (face-background 'default nil t))
> >> (face-differs-from-default-p 'region nil)
> >> ; ⇒ :extend
> >>
> >
> > ... although that's a really contrived example.
> 
> OK, how about this example:
> 
> >   (defun mtmm-reset-background ()
> >     (set-face-attribute 'region nil :background 'unspecified))
> 
> Straight from <87vfjhre9z.fsf@zamazal.org> (emacs-devel), i.e. the user
> report which resulted in the current code in indicate-copied-region.
> 
> Same result: :extend differs, so (face-differs-from-default-p 'region)
> returns t, which for this user would cause a regression compared to
> (face-background 'region nil t).
> 
> > I'm not sure it's TRT to add too much complexity there, so I would suggest to
> > add an additional optional argument to face-differs-from-default-p, to exclude
> > one or more attributes from the comparison (in this case :extend, but someone
> > else might be interested in excluding other attributes in the future).
> 
> I really think how face-differs-from-default-p examines attributes…
> 
>               (and
>                (not (eq attr-val 'unspecified))
>                (display-supports-face-attributes-p (list attr attr-val)
>                                                    frame))
> 
> … makes no sense for :extend.  Here's an example (and this one is
> contrived alright):
> 
>   (set-face-attribute 'default nil :underline t)
>   (set-face-attribute 'region nil :underline nil :extend VALUE)
> 
> * VALUE is 'unspecified: the condition becomes nil (meaning ":extend
>   does not make this face look different than default"), whereas that
>   actually _does_ make a difference (:underline nil too, obviously, and
>   luckily face-differs-from-default-p picks up on this),
> 
> * VALUE is nil: (display-supports-face-attributes-p '(:extend nil))
>   returns nil (because see gui_supports_face_attributes_p), despite
>   :extend nil procuding different results for :underline.
> 
> * VALUE is t: for the same reason, display-supports-face-attributes-p
>   returns t, despite :extend t making :underline indistinguishable from
>   the default face.
> 
> Granted, yeah, contrived, since that calls for :underline t on the
> default face and who does that.
> 
> But since, as we've seen, :extend causes a false positive (face reported
> as "displaying differently" despite displaying identically) when a face
> has a :background equal to the default's, I don't see how the current
> behaviour helps anyone.

Sorry, but I don't think I understand what you are proposing.

bug#60841: 30.0.50; kill-ring-save pauses despite region being highlighted

[Kévin Le Gouguec]

Eli Zaretskii <eliz@gnu.org> writes:

> Sorry, but I don't think I understand what you are proposing.

In the message you reply to, not much (and it was quite ingrate of me to
inflict that wall of text on this list, without the shadow of a
conclusion; apologies, will do better next time).

The message before that had a proposal, FWIW:

> So,
> 
> * :extend nil for both: they display differently (region will not be
>   extended, default will be),
> * :extend t for both: they display the same,
> * default has nil, region has t: they display the same,
> * default has t, region has nil: they display differently.
> 
> Ergo, assuming (a) I didn't mess something up (b) this is the expected
> behaviour, it seems that when considering :extend,
> (face-differs-from-default-p FACE FRAME) should check
> 
> (1) whether FACE's :extend is nil (regardless of default's :extend),
>     and
> (2) whether FACE's :underline or :background are "supported", as
>     reported by display-supports-face-attributes-p.

(
 Gregory then suggested that this example, from that same message…

   (set-face-background 'region (face-background 'default nil t))

 … is contrived, and shouldn't justify special-casing :extend in
 face-differs-from-default-p (well, shouldn't justify "adding too much
 complexity").

 To which I replied that the current code in indicate-copied-region was
 in answer to an actual user doing…

   (set-face-attribute 'region nil :background 'unspecified)

 … which runs afoul of the same problem as my contrived example: 'region
 remains "visually different" from 'default according to
 face-differs-from-default-p because of :extent, but for the purposes of
 indicate-copied-region, it _shouldn't_ be.
)

Now Gregory also suggested just adding an optional list of ignored
attributes to face-differs-from-default-p's parameters, so that
indicate-copied-region can set it to '(:extend) (IIUC; Gregory, let me
know if I've misunderstood).


And TBH I don't have a problem with this suggestion: it punts to the
caller, who can do TRT; it keeps face-differs-from-default-p relatively
simple, we can go with that.  My only objection is that I can't imagine
a caller ever wanting to _not_ pass '(:extend), but I admit that my own
"proposition" sounds convoluted.


(Hm, and against my better judgement I went ahead and compared
gui_supports_face_attributes_p vs tty_supports_face_attributes_p, and I
see that they handle :extend differently and *mashes C-c C-c with
forehead before fingers can type another wall of text*)

bug#60841: 30.0.50; kill-ring-save pauses despite region being highlighted

[Eli Zaretskii]

> From: Kévin Le Gouguec <kevin.legouguec@gmail.com>
> Cc: gregory@heytings.org,  60841@debbugs.gnu.org
> Date: Wed, 18 Jan 2023 23:16:34 +0100
> 
> > * :extend nil for both: they display differently (region will not be
> >   extended, default will be),
> > * :extend t for both: they display the same,
> > * default has nil, region has t: they display the same,
> > * default has t, region has nil: they display differently.

The default face is always extended, so it should be treated as
implicitly having the :extend attribute set.

I think face-differs-from-default-p should be fixed to either ignore
the :extend attribute like it ignores :inherit (since it could be
argued that :extend doesn't really control how the face affects
characters on display), or it should treat the default face as having
the :extend attribute with the value t.

Can you see if any of these changes cause any trouble in our own use
of face-differs-from-default-p?  AFAICT, it will actually fix a subtle
problem in diff-mode.el: if diff-changed face doesn't define
non-default colors, it will be still taken as different from
'default', which I think is contrary to what diff-mode expects.

If we make the above change in face-differs-from-default-p, would
using it for the purpose of this issue fix the problem?

> (Hm, and against my better judgement I went ahead and compared
> gui_supports_face_attributes_p vs tty_supports_face_attributes_p, and I
> see that they handle :extend differently and *mashes C-c C-c with
> forehead before fingers can type another wall of text*)

TTY frames always extend the color, that's the reason for the
difference.  But does this affect my proposal above?

bug#60841: 30.0.50; kill-ring-save pauses despite region being highlighted

[Kévin Le Gouguec]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Kévin Le Gouguec <kevin.legouguec@gmail.com>
>> Cc: gregory@heytings.org,  60841@debbugs.gnu.org
>> Date: Wed, 18 Jan 2023 23:16:34 +0100
>> 
>> > * :extend nil for both: they display differently (region will not be
>> >   extended, default will be),
>> > * :extend t for both: they display the same,
>> > * default has nil, region has t: they display the same,
>> > * default has t, region has nil: they display differently.
>
> The default face is always extended, so it should be treated as
> implicitly having the :extend attribute set.
>
> I think face-differs-from-default-p should be fixed to either ignore
> the :extend attribute like it ignores :inherit (since it could be
> argued that :extend doesn't really control how the face affects
> characters on display), or it should treat the default face as having
> the :extend attribute with the value t.

I have been slowly converging toward "ignore :extend" being TRT.

I cannot find a situation where :extend matters.  AFAICT "does this face
stand out vs 'default?" always comes down to whether the face's
:underline or :background _also_ differs, so :extend seems redundant.
Breakdown in footnote[1].

(Have been going back and forth over this; apologies if I've missed
something and that conclusion is wrong)

> Can you see if any of these changes cause any trouble in our own use
> of face-differs-from-default-p?  AFAICT, it will actually fix a subtle
> problem in diff-mode.el: if diff-changed face doesn't define
> non-default colors, it will be still taken as different from
> 'default', which I think is contrary to what diff-mode expects.

Will do; that was on my checklist[2] whichever solution we eventually
pick.

> If we make the above change in face-differs-from-default-p, would
> using it for the purpose of this issue fix the problem?

I think so.  Scribbled the attached patch; will report once I've tested
it more thoroughly against

(a) the :inverse-video use-case from my OP,
(b) the "region with unspecified :background" use-case from emacs-devel,
(c) other in-tree users of face-differs-from-default-p.

In the meantime, let me know if it looks good in principle; there are
still details I'd like to tweak FWIW.  E.g.

* face-differs-from-default-p probably deserves a comment explaining
  what's going on wrt these attributes,

* that seq-difference call makes me inexplicably nervous; I thought I
  vaguely remembered debates on whether preloaded libraries {c,sh}ould
  use seq.el functions, but then I see that "emacs-lisp/seq" is already
  in preloaded-file-list, and e.g. rmc.el calls some of its functions.
  Am I misremembering?

>> (Hm, and against my better judgement I went ahead and compared
>> gui_supports_face_attributes_p vs tty_supports_face_attributes_p, and I
>> see that they handle :extend differently and *mashes C-c C-c with
>> forehead before fingers can type another wall of text*)
>
> TTY frames always extend the color, that's the reason for the
> difference.

(Not sure I get your meaning here; on the Linux TTY I have on hand,
(set-face-extend 'region nil) does disable color extension)

>              But does this affect my proposal above?

Not AFAICT.  Good thing I hit message-send-and-exit in time 🤕


Bottomline: let me know if the attached seems to go in the right
direction; meanwhile, will check that it does TRT for other in-tree
users of face-differs-from-default-p.

Thanks for your patience.


[1]

Face under test has…                 |  Does text with that face stand out?
:background = default   :extend nil  |  no
:background = default   :extend t    |  no
:background ≠ default   :extend nil  |  yes
:background ≠ default   :extend t    |  yes

⇒ face-differs-from-default-p can just check :background ≠ default,
which it already does via display-supports-face-attributes-p.
                                
Face under test has…                 |  Does text with that face stand out?
:underline = default    :extend nil  |  no
:underline = default    :extend t    |  no
:underline ≠ default    :extend nil  |  yes
:underline ≠ default    :extend t    |  yes

⇒ face-differs-from-default-p can just check :underline ≠ default,
which it already does via display-supports-face-attributes-p.

NB: as far as I tested, this is is true for :underline's boolean values
as well as for its other forms.

[2] Another thing on that checklist would be adding ERT testcases for
face-differs-from-default-p, but I see the specter of --batch beaming
with mischievous glee at the thought of me invoking display-related
functions with a "headless" Emacs.  Is it…? - yep, yep, it *is* putting
popcorn in the oven.  😮‍💨

Maybe this time I can convince it to just eat the popcorn and not throw
it at me.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Avoid-spurious-pause-in-kill-ring-save.patch --]
[-- Type: text/x-patch, Size: 2996 bytes --]

From 033dec727e6eb381cae288aaacbce63eba76a7bf Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?K=C3=A9vin=20Le=20Gouguec?= <kevin.legouguec@gmail.com>
Date: Sun, 15 Jan 2023 19:23:17 +0100
Subject: [PATCH] Avoid spurious pause in kill-ring-save

'indicate-copied-region' checks whether the region is "highlighted"
and if not, briefly moves point to mark to give a visual cue of the
extent of text that was saved to the kill ring.

The region is considered "highlighted" if (a) it is active and (b) its
face specifies a :background.  That latter condition does not account
for the multiple ways in which the face can make the region "visually
distinct" from the default face, so switch to a more extensive
predicate.

* lisp/simple.el (region-highlighted-p): New function to detect "if
the region is highlighted", leveraging
display-supports-face-attributes-p.
(indicate-copied-region): Use it.
* lisp/faces.el (face-differs-from-default-p): Also ignore :extend,
since the answers display-supports-face-attributes-p gives for that
attribute do not help determine whether FACE is visually distinct from
default.
---
 lisp/faces.el  |  4 ++--
 lisp/simple.el | 10 ++++++++--
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/lisp/faces.el b/lisp/faces.el
index 3323eab205..53a4ee5e09 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -303,8 +303,8 @@ face-differs-from-default-p
 If the optional argument FRAME is given, report on face FACE in that frame.
 If FRAME is t, report on the defaults for face FACE (for new frames).
 If FRAME is omitted or nil, use the selected frame."
-  (let ((attrs
-	 (delq :inherit (mapcar 'car face-attribute-name-alist)))
+  (let ((attrs (seq-difference (mapcar 'car face-attribute-name-alist)
+                               '(:extend :inherit)))
 	(differs nil))
     (while (and attrs (not differs))
       (let* ((attr (pop attrs))
diff --git a/lisp/simple.el b/lisp/simple.el
index 844cfa68b0..e2828caf39 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -5865,6 +5865,13 @@ copy-region-blink-delay
   :group 'killing
   :version "28.1")
 
+(defun region-highlighted-p ()
+  "Say whether the region is visibly highlighted.
+This takes into account whether the region is active, and whether
+the `region' face displays differently from the default face."
+  (and (region-active-p)
+       (face-differs-from-default-p 'region)))
+
 (defun indicate-copied-region (&optional message-len)
   "Indicate that the region text has been copied interactively.
 If the mark is visible in the selected window, blink the cursor between
@@ -5885,8 +5892,7 @@ indicate-copied-region
 	;; was selected.  Don't do it if the region is highlighted.
 	(when (and (numberp copy-region-blink-delay)
 		   (> copy-region-blink-delay 0)
-		   (or (not (region-active-p))
-		       (not (face-background 'region nil t))))
+		   (not (region-highlighted-p)))
 	  ;; Swap point and mark.
 	  (set-marker (mark-marker) (point) (current-buffer))
 	  (goto-char mark)
-- 
2.39.0

bug#60841: 30.0.50; kill-ring-save pauses despite region being highlighted

[Eli Zaretskii]

> From: Kévin Le Gouguec <kevin.legouguec@gmail.com>
> Cc: gregory@heytings.org,  60841@debbugs.gnu.org
> Date: Sun, 22 Jan 2023 23:45:23 +0100
> 
> > The default face is always extended, so it should be treated as
> > implicitly having the :extend attribute set.
> >
> > I think face-differs-from-default-p should be fixed to either ignore
> > the :extend attribute like it ignores :inherit (since it could be
> > argued that :extend doesn't really control how the face affects
> > characters on display), or it should treat the default face as having
> > the :extend attribute with the value t.
> 
> I have been slowly converging toward "ignore :extend" being TRT.

Fine by me.

> * that seq-difference call makes me inexplicably nervous; I thought I
>   vaguely remembered debates on whether preloaded libraries {c,sh}ould
>   use seq.el functions, but then I see that "emacs-lisp/seq" is already
>   in preloaded-file-list, and e.g. rmc.el calls some of its functions.
>   Am I misremembering?

seq.el is indeed preloaded, so that ship has sailed.  But you still
need to make sure seq is loaded _before_ any preloaded file which uses
it, and in this case faces is loaded before seq, so you cannot use
seq-difference.

And, btw, I cannot say I understand why using seq-difference here is
justified.  Why not just call delq twice and be done?

> >> (Hm, and against my better judgement I went ahead and compared
> >> gui_supports_face_attributes_p vs tty_supports_face_attributes_p, and I
> >> see that they handle :extend differently and *mashes C-c C-c with
> >> forehead before fingers can type another wall of text*)
> >
> > TTY frames always extend the color, that's the reason for the
> > difference.
> 
> (Not sure I get your meaning here; on the Linux TTY I have on hand,
> (set-face-extend 'region nil) does disable color extension)

I'm sorry, you will have to look up the discussion that led to the
development of the :extend attribute; I cannot afford searching for
it.  The differences between TTY and GUI frames were one of the main
reasons why we introduced this attribute.  Maybe what I remember
happens only on some terminals.  Or maybe I'm misremembering and it
was because of underline and not the color.  But there is definitely a
difference.

> +(defun region-highlighted-p ()
> +  "Say whether the region is visibly highlighted.

Please drop the "Say" part, it's not our style.

And I'd use some other word instead of "highlighted", because that
could be misinterpreted (the region is supposed to be highlighted).
Something like "stands-out" perhaps?

bug#60841: 30.0.50; kill-ring-save pauses despite region being highlighted

[Kévin Le Gouguec]

Eli Zaretskii <eliz@gnu.org> writes:

>> * that seq-difference call makes me inexplicably nervous; I thought I
>>   vaguely remembered debates on whether preloaded libraries {c,sh}ould
>>   use seq.el functions, but then I see that "emacs-lisp/seq" is already
>>   in preloaded-file-list, and e.g. rmc.el calls some of its functions.
>>   Am I misremembering?
>
> seq.el is indeed preloaded, so that ship has sailed.  But you still
> need to make sure seq is loaded _before_ any preloaded file which uses
> it, and in this case faces is loaded before seq, so you cannot use
> seq-difference.

(Thanks for spelling this out.  Do we have any documentation that calls
out the precautions one must take when writing Elisp that will be
preloaded, or any tooling that can detect whether some of those
precautions were forgotten?  FWIW I saw no compiler warnings nor runtime
errors with that patch)

> And, btw, I cannot say I understand why using seq-difference here is
> justified.  Why not just call delq twice and be done?

That would work, of course; will go with that for the next revision.

(No justification beyond lack of practice at reading or writing
"preloaded Elisp", so my brain takes a couple more cycles to translate
(delq 'a (delq 'b x)) into "x \ {a,b}")

>> >> (Hm, and against my better judgement I went ahead and compared
>> >> gui_supports_face_attributes_p vs tty_supports_face_attributes_p, and I
>> >> see that they handle :extend differently and *mashes C-c C-c with
>> >> forehead before fingers can type another wall of text*)
>> >
>> > TTY frames always extend the color, that's the reason for the
>> > difference.
>> 
>> (Not sure I get your meaning here; on the Linux TTY I have on hand,
>> (set-face-extend 'region nil) does disable color extension)
>
> I'm sorry, you will have to look up the discussion that led to the
> development of the :extend attribute; I cannot afford searching for
> it.

(Did I mention I'm grateful for the time you and Gregory did afford for
this obscure issue, considering how busy this rc phase is?)

>      The differences between TTY and GUI frames were one of the main
> reasons why we introduced this attribute.  Maybe what I remember
> happens only on some terminals.  Or maybe I'm misremembering and it
> was because of underline and not the color.  But there is definitely a
> difference.

ACK, will look into this.

>> +(defun region-highlighted-p ()
>> +  "Say whether the region is visibly highlighted.
>
> Please drop the "Say" part, it's not our style.

ACK.  I see a few matches for "Return whether…" in-tree; would…

  Return whether the region stands out visually.

… be OK, or should I just go for…

  Return non-nil if the region stands out visually.

?

(Asking because I have a (tiny, subduable) preference for the former;
predicate docstrings that call out nil/t/non-nil force me to pause and
figure out whether I need to negate the rest of the sentence, whereas
"whether" generally precedes a description of the "true" case.

"(elisp) Documentation Tips" recommends "Return t if", but merely as a
way to "avoid starting the sentence with “t”", not because we have a
preference for literally starting with "Return t if")

> And I'd use some other word instead of "highlighted", because that
> could be misinterpreted (the region is supposed to be highlighted).
> Something like "stands-out" perhaps?

Agreed.  stands-out works for me (other candidates that come to mind:
noticeable, conspicuous).

bug#60841: 30.0.50; kill-ring-save pauses despite region being highlighted

[Eli Zaretskii]

> From: Kévin Le Gouguec <kevin.legouguec@gmail.com>
> Cc: gregory@heytings.org,  60841@debbugs.gnu.org
> Date: Mon, 23 Jan 2023 23:29:00 +0100
> 
> > seq.el is indeed preloaded, so that ship has sailed.  But you still
> > need to make sure seq is loaded _before_ any preloaded file which uses
> > it, and in this case faces is loaded before seq, so you cannot use
> > seq-difference.
> 
> (Thanks for spelling this out.  Do we have any documentation that calls
> out the precautions one must take when writing Elisp that will be
> preloaded, or any tooling that can detect whether some of those
> precautions were forgotten?  FWIW I saw no compiler warnings nor runtime
> errors with that patch)

Did you "make bootstrap"?  If not, some errors might not happen,
because the build will use previously compiled foo.elc files.

As for documentation: there's any number of such factoids related to
do's and dont's of Emacs development, and we lack a full-time
documentation fellow to keep all of them documented and up to date...

> >> +(defun region-highlighted-p ()
> >> +  "Say whether the region is visibly highlighted.
> >
> > Please drop the "Say" part, it's not our style.
> 
> ACK.  I see a few matches for "Return whether…" in-tree; would…
> 
>   Return whether the region stands out visually.
> 
> … be OK, or should I just go for…

It's OK, but IMO the "Return" part is almost redundant here.  But I
won't object to having it.

> "(elisp) Documentation Tips" recommends "Return t if", but merely as a
> way to "avoid starting the sentence with “t”", not because we have a
> preference for literally starting with "Return t if")

The point here is that this is a predicate, so it is known up front
that it will "return t" or nil.  The only non-trivial part is the
condition under which it will return non-nil.

Thanks.

bug#60841: 30.0.50; kill-ring-save pauses despite region being highlighted

[Kévin Le Gouguec]

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

Revised patch attached; intended as "good for master".


Answers to open discussion points:

> Can you see if any of these changes cause any trouble in our own use
> of face-differs-from-default-p?  AFAICT, it will actually fix a subtle
> problem in diff-mode.el: if diff-changed face doesn't define
> non-default colors, it will be still taken as different from
> 'default', which I think is contrary to what diff-mode expects.

diff-mode.el (re. smerge-mode.el) can indeed be fooled into thinking
diff-changed (re. smerge-refined-changed) differs-from-default, if one
"shoots their own foot", for example, setting…

* :extend t:         fixed by this patch                    ✔️
* :stipple nil:      foot blown with or without the patch   🤷
* :inherit 'default: foot blown with or without the patch   🤷

Problem with :stipple nil and :inherit 'default explained in [1].
indicate-copied-region will become affected if the current patch goes
in.

replace.el:occur-1 befuddled me for a moment[2], but the tl;dr is that
it will be none the worse for wear.

> > >> (Hm, and against my better judgement I went ahead and compared
> > >> gui_supports_face_attributes_p vs tty_supports_face_attributes_p, and I
> > >> see that they handle :extend differently and *mashes C-c C-c with
> > >> forehead before fingers can type another wall of text*)
> > >
> > > TTY frames always extend the color, that's the reason for the
> > > difference.
> > 
> > (Not sure I get your meaning here; on the Linux TTY I have on hand,
> > (set-face-extend 'region nil) does disable color extension)
> 
> I'm sorry, you will have to look up the discussion that led to the
> development of the :extend attribute; I cannot afford searching for
> it.  The differences between TTY and GUI frames were one of the main
> reasons why we introduced this attribute.

Playing around with more terminals did not yield more insights[3], so it
seems I'll need to dig into the archives, indeed.

Currently squinting at emacs-devel:<83r2fbg5bq.fsf@gnu.org>, which
singles out NS versus X, Windows & TTYs; could this be what you had in
mind?

> Alternatively, we could add a user option to make the swap
> unconditional, because maybe some users would prefer that to splitting
> hair in this case.  Then we could stop worrying about all those fine
> differences.

Should I cook up a user option to unconditionally do the swap before we
apply the attached?  Otherwise we may disgruntle trunk users who
actually liked the behaviour I reported in the OP (swapping regardless
of whether region stands out).


                                   ⁂
                               footnotes
                                   ⁂

[1]

  (face-differs-from-default-p 'default)
  ↦ :stipple
  (display-supports-face-attributes-p '(:stipple nil))
  ↦ t

So a face that explicitly inherits from 'default, or sets :stipple to
nil (rather than 'unspecified) differs-from-default-p.

This seems inappropriate, based on display-supports-face-attributes-p's
docstring:

> The definition of ‘supported’ is somewhat heuristic, but basically means
> that a face containing all the attributes in ATTRIBUTES, when merged
> with the default face for display, can be represented in a way that’s
> 
>  (1) different in appearance from the default face, and
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>  (2) ‘close in spirit’ to what the attributes specify, if not exact.

I can debug gui_supports_face_attributes_p if we agree that there is
something to investigate here?

[2]

  (if (face-differs-from-default-p list-matching-lines-prefix-face)
      list-matching-lines-prefix-face)

> If this face will display the same as the default face, the prefix
> column will not be highlighted specially.
— C-h v list-matching-lines-prefix-face

Why bother with this `if' then?  Isn't this gratuituous complexity?
Just passing the face regardless would have the same effect (the prefix
column will look like it has the default face).

[3]

>                                            Maybe what I remember
> happens only on some terminals.

AFAICT, :extend {nil,t} successfully make the background {stop at,extend
past} EOL on these terminals:

* Linux TTYs (openSUSE Tumbleweed: Linux 6.1.7-1-default x86_64,
              Debian 11:           Linux 5.10.0-19-amd64 x86_64)
* VTE-based pseudo terminals (terminator, xfce4-terminal)
* Plasma's konsole
* xterm

>                                  Or maybe I'm misremembering and it
> was because of underline and not the color.

* Linux TTYs don't support :underline AFAICT,
* all pseudo terminals listed above do, and :extend nil/t both do TRT.

>                                              But there is definitely a
> difference.

ACK.  Based on vc-region-history alone it feels like
tty_supports_face_attributes_p just got left out of the :extend party,
but I'll see what turns up in the lists.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Avoid-spurious-pause-in-kill-ring-save-Bug-60841.patch --]
[-- Type: text/x-patch, Size: 3407 bytes --]

From 9ec48654977d7c8b7a26e99fce6693d67d023599 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?K=C3=A9vin=20Le=20Gouguec?= <kevin.legouguec@gmail.com>
Date: Sun, 15 Jan 2023 19:23:17 +0100
Subject: [PATCH] Avoid spurious pause in kill-ring-save (Bug#60841)

'indicate-copied-region' checks whether the region is "highlighted"
and if not, briefly moves point to mark to give a visual cue of the
extent of text that was saved to the kill ring.

The region is considered "highlighted" if (a) it is active and (b) its
face specifies a :background.  That latter condition does not account
for the multiple ways in which the face can make the region "visually
distinct" from the default face, so switch to a more extensive
predicate.

* lisp/simple.el (region-stands-out-p): New function to detect "if the
region is highlighted", leveraging face-differs-from-default-p.
(indicate-copied-region): Use it.
* lisp/faces.el (face-differs-from-default-p): Also ignore :extend,
since the answers display-supports-face-attributes-p gives for that
attribute do not help determine whether FACE is visually distinct from
default.
---
 lisp/faces.el  | 11 ++++++++++-
 lisp/simple.el | 10 ++++++++--
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/lisp/faces.el b/lisp/faces.el
index 3323eab205a..4933b495a6c 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -304,7 +304,16 @@ face-differs-from-default-p
 If FRAME is t, report on the defaults for face FACE (for new frames).
 If FRAME is omitted or nil, use the selected frame."
   (let ((attrs
-	 (delq :inherit (mapcar 'car face-attribute-name-alist)))
+         ;; The _value_ of :inherit teaches us nothing about how FACE
+         ;; looks compared to the default face.  Instead, we will ask
+         ;; `face-attribute' to take inheritance into account when
+         ;; examining other attributes.
+         (delq :inherit
+               ;; A difference in extension past EOL only matters when
+               ;; relevant attributes (such as :background) also
+               ;; differ from the default; otherwise this difference
+               ;; is a false positive.
+               (delq :extend (mapcar 'car face-attribute-name-alist))))
 	(differs nil))
     (while (and attrs (not differs))
       (let* ((attr (pop attrs))
diff --git a/lisp/simple.el b/lisp/simple.el
index 7bda368d85d..ef816949d36 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -5871,6 +5871,13 @@ copy-region-blink-delay
   :group 'killing
   :version "28.1")
 
+(defun region-stands-out-p ()
+  "Whether the region can be distinguished visually.
+This takes into account whether the region is active, and whether
+the `region' face displays differently from the default face."
+  (and (region-active-p)
+       (face-differs-from-default-p 'region)))
+
 (defun indicate-copied-region (&optional message-len)
   "Indicate that the region text has been copied interactively.
 If the mark is visible in the selected window, blink the cursor between
@@ -5891,8 +5898,7 @@ indicate-copied-region
 	;; was selected.  Don't do it if the region is highlighted.
 	(when (and (numberp copy-region-blink-delay)
 		   (> copy-region-blink-delay 0)
-		   (or (not (region-active-p))
-		       (not (face-background 'region nil t))))
+		   (not (region-stands-out-p)))
 	  ;; Swap point and mark.
 	  (set-marker (mark-marker) (point) (current-buffer))
 	  (goto-char mark)
-- 
2.39.0

bug#60841: 30.0.50; kill-ring-save pauses despite region being highlighted

[Eli Zaretskii]

> From: Kévin Le Gouguec <kevin.legouguec@gmail.com>
> Cc: gregory@heytings.org,  60841@debbugs.gnu.org
> Date: Sat, 28 Jan 2023 18:45:07 +0100
> 
> diff-mode.el (re. smerge-mode.el) can indeed be fooled into thinking
> diff-changed (re. smerge-refined-changed) differs-from-default, if one
> "shoots their own foot", for example, setting…
> 
> * :extend t:         fixed by this patch                    ✔️
> * :stipple nil:      foot blown with or without the patch   🤷
> * :inherit 'default: foot blown with or without the patch   🤷
> 
> Problem with :stipple nil and :inherit 'default explained in [1].
> indicate-copied-region will become affected if the current patch goes
> in.

I wouldn't be bothered by :stipple whose value is nil.  Why would
someone do such a thing, except when the face is meant to be merged
with other faces (which are expected to have non-nil :stipple
attributes)?

> > Alternatively, we could add a user option to make the swap
> > unconditional, because maybe some users would prefer that to splitting
> > hair in this case.  Then we could stop worrying about all those fine
> > differences.
> 
> Should I cook up a user option to unconditionally do the swap before we
> apply the attached?  Otherwise we may disgruntle trunk users who
> actually liked the behaviour I reported in the OP (swapping regardless
> of whether region stands out).

I guess adding such an option would be a good precaution, indeed.

bug#60841: 30.0.50; kill-ring-save pauses despite region being highlighted

[Kévin Le Gouguec]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Kévin Le Gouguec <kevin.legouguec@gmail.com>
>> Cc: gregory@heytings.org,  60841@debbugs.gnu.org
>> Date: Sat, 28 Jan 2023 18:45:07 +0100
>> 
>> diff-mode.el (re. smerge-mode.el) can indeed be fooled into thinking
>> diff-changed (re. smerge-refined-changed) differs-from-default, if one
>> "shoots their own foot", for example, setting…
>> 
>> * :extend t:         fixed by this patch                    ✔️
>> * :stipple nil:      foot blown with or without the patch   🤷
>> * :inherit 'default: foot blown with or without the patch   🤷
>> 
>> Problem with :stipple nil and :inherit 'default explained in [1].
>> indicate-copied-region will become affected if the current patch goes
>> in.
>
> I wouldn't be bothered by :stipple whose value is nil.  Why would
> someone do such a thing, except when the face is meant to be merged
> with other faces (which are expected to have non-nil :stipple
> attributes)?

The only place where this "matters" is in replace.el, AFAICT:

1. Customize list-matching-lines-prefix-face to 'default,

2. (face-differs-from-default-p list-matching-lines-prefix-face)
   ↦ :stipple

3. Luckily this does not impact replace-el:occur-1, since this check…

    (if (face-differs-from-default-p list-matching-lines-prefix-face)
        list-matching-lines-prefix-face)

   … is redundant AFAIU: the goal is to _not_ apply
   list-matching-lines-prefix-face if it is close to 'default, and
   instead… leave the default face?  So why bother checking?

CC'ing Juri, who installed this check in bug#14017, for comment in case
I missed something.

(Hi Juri 👋  For context, the current report is about refining
indicate-copied-region to avoid spuriously considering the region as
"unhighlighted" and triggering the point-mark swap; see attached
patches.  I believe this aspect of the report might also be of interest
to you, given your participation in bug#42865)

>> > Alternatively, we could add a user option to make the swap
>> > unconditional, because maybe some users would prefer that to splitting
>> > hair in this case.  Then we could stop worrying about all those fine
>> > differences.
>> 
>> Should I cook up a user option to unconditionally do the swap before we
>> apply the attached?  Otherwise we may disgruntle trunk users who
>> actually liked the behaviour I reported in the OP (swapping regardless
>> of whether region stands out).
>
> I guess adding such an option would be a good precaution, indeed.

OK, two tentative patches attached, because I don't know which makes for
the better UX:

    ▼ Copy Region Inhibit Blink:
    Choice:
    (*) region-stands-out-p
            Whether the region can be distinguished visually. More
    ( ) always
            Always inhibit: never blink point and mark.
    ( ) ignore
            Never inhibit: always blink point and mark.
    ( ) Other predicate function.: ignore
        State : STANDARD.
       Whether we should refrain from blinking the cursor after a copy. ▼
       When this condition holds, ‘kill-ring-save’ will not blink the
       cursor between point and mark to denote the copied region.
    Groups: Killing

                                 ⁂ VS ⁂

    ▼ Copy Region Blink Predicate:
    Choice:
    (*) region-indistinguishable-p
            Whether the current region is not denoted visually. ▼
        This holds when the region is inactive, or when the ‘region’ face
        cannot be distinguished from the ‘default’ face.
    ( ) always
            Always blink point and mark.
    ( ) ignore
            Never blink point and mark.
    ( ) Other predicate function.: ignore
        State : STANDARD.
       Whether the cursor must be blinked after a copy. ▼
       When this condition holds, and the copied region fits in the
       current window, ‘kill-ring-save’ will blink the cursor between
       point and mark for ‘copy-region-blink-delay’ seconds.
    Groups: Killing


inhibit-vs-predicate.diff shows a comparison of both patches once
applied.

Boldly marked the new options with "---" in NEWS, because
copy-region-blink-delay is not documented in the manual either, but let
me know if that should be remedied.


Thank you for your time.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: copy-region-blink-predicate.patch --]
[-- Type: text/x-patch, Size: 5109 bytes --]

From 7ba1128b72fda681f1368d862e5979d8e340957a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?K=C3=A9vin=20Le=20Gouguec?= <kevin.legouguec@gmail.com>
Date: Sun, 29 Jan 2023 11:23:01 +0100
Subject: [PATCH] Avoid spurious pause in kill-ring-save (Bug#60841)

'indicate-copied-region' checks whether the region is "highlighted"
and if not, briefly moves point to mark to give a visual cue of the
extent of text that was saved to the kill ring.

The region is considered "highlighted" if (a) it is active and (b) its
face specifies a :background.  That latter condition does not account
for the multiple ways in which the face can make the region "visually
distinct" from the default face, so switch to a more extensive
predicate.

* lisp/faces.el (face-differs-from-default-p): Filter out :extend; add
rationale for the attributes we ignore.
* lisp/simple.el (copy-region-blink-predicate): Add option to let user
explicitly opt into or out of blinking point and mark.
(region-indistinguishable-p): New function to detect
"if there is currently no active region highlighting", leveraging
face-differs-from-default-p.
(indicate-copied-region): Use it.
* etc/NEWS: Announce user option.
---
 etc/NEWS       | 13 +++++++++++++
 lisp/faces.el  | 11 ++++++++++-
 lisp/simple.el | 22 ++++++++++++++++++++--
 3 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index e0175bacfdf..56b6add5d98 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -59,6 +59,19 @@ This allows the user to customize the prompt that is appended by
 \f
 * Editing Changes in Emacs 30.1
 
+---
+** New user option 'copy-region-blink-predicate'.
+By default, when copying a region with 'kill-ring-save', Emacs only
+blinks point and mark when the region is not denoted visually, that
+is, when either the region is inactive, or the 'region' face is
+indistinguishable from the 'default' face.
+
+The way Emacs evaluated this latter condition sometimes yielded false
+positives, so point and mark would blink despite the region being
+active and visually distinct.  The false positive was fixed; users who
+consider blinking unconditionally to be a feature can now set this
+user option to 'always'.
+
 ---
 ** New command 'kill-matching-buffers-no-ask'.
 This works like 'kill-matching-buffers', but without asking for
diff --git a/lisp/faces.el b/lisp/faces.el
index 3323eab205a..4933b495a6c 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -304,7 +304,16 @@ face-differs-from-default-p
 If FRAME is t, report on the defaults for face FACE (for new frames).
 If FRAME is omitted or nil, use the selected frame."
   (let ((attrs
-	 (delq :inherit (mapcar 'car face-attribute-name-alist)))
+         ;; The _value_ of :inherit teaches us nothing about how FACE
+         ;; looks compared to the default face.  Instead, we will ask
+         ;; `face-attribute' to take inheritance into account when
+         ;; examining other attributes.
+         (delq :inherit
+               ;; A difference in extension past EOL only matters when
+               ;; relevant attributes (such as :background) also
+               ;; differ from the default; otherwise this difference
+               ;; is a false positive.
+               (delq :extend (mapcar 'car face-attribute-name-alist))))
 	(differs nil))
     (while (and attrs (not differs))
       (let* ((attr (pop attrs))
diff --git a/lisp/simple.el b/lisp/simple.el
index 7bda368d85d..5832d6449bf 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -5871,6 +5871,25 @@ copy-region-blink-delay
   :group 'killing
   :version "28.1")
 
+(defcustom copy-region-blink-predicate #'region-indistinguishable-p
+  "Whether the cursor must be blinked after a copy.
+When this condition holds, and the copied region fits in the
+current window, `kill-ring-save' will blink the cursor between
+point and mark for `copy-region-blink-delay' seconds."
+  :type '(radio (function-item region-indistinguishable-p)
+                (function-item :doc "Always blink point and mark." always)
+                (function-item :doc "Never blink point and mark." ignore)
+                (function :tag "Other predicate function."))
+  :group 'killing
+  :version "30.1")
+
+(defun region-indistinguishable-p ()
+  "Whether the current region is not denoted visually.
+This holds when the region is inactive, or when the `region' face
+cannot be distinguished from the `default' face."
+  (not (and (region-active-p)
+            (face-differs-from-default-p 'region))))
+
 (defun indicate-copied-region (&optional message-len)
   "Indicate that the region text has been copied interactively.
 If the mark is visible in the selected window, blink the cursor between
@@ -5891,8 +5910,7 @@ indicate-copied-region
 	;; was selected.  Don't do it if the region is highlighted.
 	(when (and (numberp copy-region-blink-delay)
 		   (> copy-region-blink-delay 0)
-		   (or (not (region-active-p))
-		       (not (face-background 'region nil t))))
+		   (funcall copy-region-blink-predicate))
 	  ;; Swap point and mark.
 	  (set-marker (mark-marker) (point) (current-buffer))
 	  (goto-char mark)
-- 
2.39.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: copy-region-inhibit-blink.patch --]
[-- Type: text/x-patch, Size: 5042 bytes --]

From 5fc9250f02cbb26d6c2765a3fa7893f66fa4e6c5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?K=C3=A9vin=20Le=20Gouguec?= <kevin.legouguec@gmail.com>
Date: Sun, 29 Jan 2023 11:23:01 +0100
Subject: [PATCH] Avoid spurious pause in kill-ring-save (Bug#60841)

'indicate-copied-region' checks whether the region is "highlighted"
and if not, briefly moves point to mark to give a visual cue of the
extent of text that was saved to the kill ring.

The region is considered "highlighted" if (a) it is active and (b) its
face specifies a :background.  That latter condition does not account
for the multiple ways in which the face can make the region "visually
distinct" from the default face, so switch to a more extensive
predicate.

* lisp/faces.el (face-differs-from-default-p): Filter out :extend; add
rationale for the attributes we ignore.
* lisp/simple.el (copy-region-inhibit-blink): Add option to let user
explicitly opt into or out of blinking point and mark.
(region-stands-out-p): New function to detect "if the
region is highlighted", leveraging face-differs-from-default-p.
(indicate-copied-region): Use it.
* etc/NEWS: Announce user option.
---
 etc/NEWS       | 13 +++++++++++++
 lisp/faces.el  | 11 ++++++++++-
 lisp/simple.el | 21 +++++++++++++++++++--
 3 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index e0175bacfdf..b69b5af9101 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -59,6 +59,19 @@ This allows the user to customize the prompt that is appended by
 \f
 * Editing Changes in Emacs 30.1
 
+---
+** New user option 'copy-region-inhibit-blink.
+By default, when copying a region with 'kill-ring-save', Emacs
+refrains from blinking point and mark when the region is denoted
+visually, that is, when the region is active, and the 'region' face is
+visibly distinct from the 'default' face.
+
+The way Emacs evaluated this latter condition sometimes yielded false
+negatives, so point and mark would blink despite the region being
+active and visually distinct.  The false negative was fixed; users who
+consider blinking unconditionally to be a feature can now set this
+user option to 'ignore'.
+
 ---
 ** New command 'kill-matching-buffers-no-ask'.
 This works like 'kill-matching-buffers', but without asking for
diff --git a/lisp/faces.el b/lisp/faces.el
index 3323eab205a..4933b495a6c 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -304,7 +304,16 @@ face-differs-from-default-p
 If FRAME is t, report on the defaults for face FACE (for new frames).
 If FRAME is omitted or nil, use the selected frame."
   (let ((attrs
-	 (delq :inherit (mapcar 'car face-attribute-name-alist)))
+         ;; The _value_ of :inherit teaches us nothing about how FACE
+         ;; looks compared to the default face.  Instead, we will ask
+         ;; `face-attribute' to take inheritance into account when
+         ;; examining other attributes.
+         (delq :inherit
+               ;; A difference in extension past EOL only matters when
+               ;; relevant attributes (such as :background) also
+               ;; differ from the default; otherwise this difference
+               ;; is a false positive.
+               (delq :extend (mapcar 'car face-attribute-name-alist))))
 	(differs nil))
     (while (and attrs (not differs))
       (let* ((attr (pop attrs))
diff --git a/lisp/simple.el b/lisp/simple.el
index 7bda368d85d..b5e6255fca9 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -5871,6 +5871,24 @@ copy-region-blink-delay
   :group 'killing
   :version "28.1")
 
+(defcustom copy-region-inhibit-blink #'region-stands-out-p
+  "Whether we should refrain from blinking the cursor after a copy.
+When this condition holds, `kill-ring-save' will not blink the
+cursor between point and mark to denote the copied region."
+  :type '(radio (function-item region-stands-out-p)
+                (function-item :doc "Always inhibit: never blink point and mark." always)
+                (function-item :doc "Never inhibit: always blink point and mark." ignore)
+                (function :tag "Other predicate function."))
+  :group 'killing
+  :version "30.1")
+
+(defun region-stands-out-p ()
+  "Whether the region can be distinguished visually.
+This takes into account whether the region is active, and whether
+the `region' face displays differently from the default face."
+  (and (region-active-p)
+       (face-differs-from-default-p 'region)))
+
 (defun indicate-copied-region (&optional message-len)
   "Indicate that the region text has been copied interactively.
 If the mark is visible in the selected window, blink the cursor between
@@ -5891,8 +5909,7 @@ indicate-copied-region
 	;; was selected.  Don't do it if the region is highlighted.
 	(when (and (numberp copy-region-blink-delay)
 		   (> copy-region-blink-delay 0)
-		   (or (not (region-active-p))
-		       (not (face-background 'region nil t))))
+		   (not (funcall copy-region-inhibit-blink)))
 	  ;; Swap point and mark.
 	  (set-marker (mark-marker) (point) (current-buffer))
 	  (goto-char mark)
-- 
2.39.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: inhibit-vs-predicate.diff --]
[-- Type: text/x-patch, Size: 3762 bytes --]

diff --git a/etc/NEWS b/etc/NEWS
index b69b5af9101..56b6add5d98 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -60,17 +60,17 @@ This allows the user to customize the prompt that is appended by
 * Editing Changes in Emacs 30.1
 
 ---
-** New user option 'copy-region-inhibit-blink.
-By default, when copying a region with 'kill-ring-save', Emacs
-refrains from blinking point and mark when the region is denoted
-visually, that is, when the region is active, and the 'region' face is
-visibly distinct from the 'default' face.
+** New user option 'copy-region-blink-predicate'.
+By default, when copying a region with 'kill-ring-save', Emacs only
+blinks point and mark when the region is not denoted visually, that
+is, when either the region is inactive, or the 'region' face is
+indistinguishable from the 'default' face.
 
 The way Emacs evaluated this latter condition sometimes yielded false
-negatives, so point and mark would blink despite the region being
-active and visually distinct.  The false negative was fixed; users who
+positives, so point and mark would blink despite the region being
+active and visually distinct.  The false positive was fixed; users who
 consider blinking unconditionally to be a feature can now set this
-user option to 'ignore'.
+user option to 'always'.
 
 ---
 ** New command 'kill-matching-buffers-no-ask'.
diff --git a/lisp/simple.el b/lisp/simple.el
index b5e6255fca9..5832d6449bf 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -5871,23 +5871,24 @@ copy-region-blink-delay
   :group 'killing
   :version "28.1")
 
-(defcustom copy-region-inhibit-blink #'region-stands-out-p
-  "Whether we should refrain from blinking the cursor after a copy.
-When this condition holds, `kill-ring-save' will not blink the
-cursor between point and mark to denote the copied region."
-  :type '(radio (function-item region-stands-out-p)
-                (function-item :doc "Always inhibit: never blink point and mark." always)
-                (function-item :doc "Never inhibit: always blink point and mark." ignore)
+(defcustom copy-region-blink-predicate #'region-indistinguishable-p
+  "Whether the cursor must be blinked after a copy.
+When this condition holds, and the copied region fits in the
+current window, `kill-ring-save' will blink the cursor between
+point and mark for `copy-region-blink-delay' seconds."
+  :type '(radio (function-item region-indistinguishable-p)
+                (function-item :doc "Always blink point and mark." always)
+                (function-item :doc "Never blink point and mark." ignore)
                 (function :tag "Other predicate function."))
   :group 'killing
   :version "30.1")
 
-(defun region-stands-out-p ()
-  "Whether the region can be distinguished visually.
-This takes into account whether the region is active, and whether
-the `region' face displays differently from the default face."
-  (and (region-active-p)
-       (face-differs-from-default-p 'region)))
+(defun region-indistinguishable-p ()
+  "Whether the current region is not denoted visually.
+This holds when the region is inactive, or when the `region' face
+cannot be distinguished from the `default' face."
+  (not (and (region-active-p)
+            (face-differs-from-default-p 'region))))
 
 (defun indicate-copied-region (&optional message-len)
   "Indicate that the region text has been copied interactively.
@@ -5909,7 +5910,7 @@ indicate-copied-region
 	;; was selected.  Don't do it if the region is highlighted.
 	(when (and (numberp copy-region-blink-delay)
 		   (> copy-region-blink-delay 0)
-		   (not (funcall copy-region-inhibit-blink)))
+		   (funcall copy-region-blink-predicate))
 	  ;; Swap point and mark.
 	  (set-marker (mark-marker) (point) (current-buffer))
 	  (goto-char mark)

bug#60841: 30.0.50; kill-ring-save pauses despite region being highlighted

[Eli Zaretskii]

> From: Kévin Le Gouguec <kevin.legouguec@gmail.com>
> Cc: gregory@heytings.org,  60841@debbugs.gnu.org, Juri Linkov <juri@linkov.net>
> Date: Sun, 29 Jan 2023 15:54:14 +0100
> 
> OK, two tentative patches attached, because I don't know which makes for
> the better UX:
> 
>     ▼ Copy Region Inhibit Blink:
>     Choice:
>     (*) region-stands-out-p
>             Whether the region can be distinguished visually. More
>     ( ) always
>             Always inhibit: never blink point and mark.
>     ( ) ignore
>             Never inhibit: always blink point and mark.
>     ( ) Other predicate function.: ignore
>         State : STANDARD.
>        Whether we should refrain from blinking the cursor after a copy. ▼
>        When this condition holds, ‘kill-ring-save’ will not blink the
>        cursor between point and mark to denote the copied region.
>     Groups: Killing
> 
>                                  ⁂ VS ⁂
> 
>     ▼ Copy Region Blink Predicate:
>     Choice:
>     (*) region-indistinguishable-p
>             Whether the current region is not denoted visually. ▼
>         This holds when the region is inactive, or when the ‘region’ face
>         cannot be distinguished from the ‘default’ face.
>     ( ) always
>             Always blink point and mark.
>     ( ) ignore
>             Never blink point and mark.
>     ( ) Other predicate function.: ignore
>         State : STANDARD.
>        Whether the cursor must be blinked after a copy. ▼
>        When this condition holds, and the copied region fits in the
>        current window, ‘kill-ring-save’ will blink the cursor between
>        point and mark for ‘copy-region-blink-delay’ seconds.
>     Groups: Killing

I prefer the second one, since inhibit-SOMETHING is slightly harder to
grasp, due to the negation.

The second paragraph of the NEWS entry shouldn't be there: it tells
that we fixed a bug.

bug#60841: 30.0.50; kill-ring-save pauses despite region being highlighted

[Kévin Le Gouguec]

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

Eli Zaretskii <eliz@gnu.org> writes:

>>     ▼ Copy Region Inhibit Blink:
>>     Choice:
>>     (*) region-stands-out-p
>>             Whether the region can be distinguished visually. More
>>     ( ) always
>>             Always inhibit: never blink point and mark.
>>     ( ) ignore
>>             Never inhibit: always blink point and mark.
>>     ( ) Other predicate function.: ignore
>>         State : STANDARD.
>>        Whether we should refrain from blinking the cursor after a copy. ▼
>>        When this condition holds, ‘kill-ring-save’ will not blink the
>>        cursor between point and mark to denote the copied region.
>>     Groups: Killing
>> 
>>                                  ⁂ VS ⁂
>> 
>>     ▼ Copy Region Blink Predicate:
>>     Choice:
>>     (*) region-indistinguishable-p
>>             Whether the current region is not denoted visually. ▼
>>         This holds when the region is inactive, or when the ‘region’ face
>>         cannot be distinguished from the ‘default’ face.
>>     ( ) always
>>             Always blink point and mark.
>>     ( ) ignore
>>             Never blink point and mark.
>>     ( ) Other predicate function.: ignore
                                   ^
                                   (Booh, that's ugly.  Removed in the
                                    attached)
>>         State : STANDARD.
>>        Whether the cursor must be blinked after a copy. ▼
>>        When this condition holds, and the copied region fits in the
>>        current window, ‘kill-ring-save’ will blink the cursor between
>>        point and mark for ‘copy-region-blink-delay’ seconds.
>>     Groups: Killing
>
> I prefer the second one, since inhibit-SOMETHING is slightly harder to
> grasp, due to the negation.

ACK.

> The second paragraph of the NEWS entry shouldn't be there: it tells
> that we fixed a bug.

Right; in this instance I wondered if we should call out what some users
could perceive as a "regression".  I suppose it doesn't matter; if they
are indeed reading NEWS, they'll find this entry whether it mentions the
bug or not (since it mentions kill-ring-save).

Adjusted the entry in the attached.

Re. :stipple:

> The problem with :stipple is that we don't allow nil as the value of
> :stipple.  If you evaluate
> 
>   (setq list-matching-lines-prefix-face 'default)
>   (face-differs-from-default-p list-matching-lines-prefix-face)
> 
> then look in *Messages*, you will see:
> 
>   Invalid face attribute :stipple nil

(Œil de lynx !)

> So we need to treat :stipple specially, or maybe fix merge_face_ref to
> allow the nil value.

You mean something like the following?  (Thoroughly untested, beside
passing the (null (face-differs-from-default-p 'default)) test)

diff --git a/src/xfaces.c b/src/xfaces.c
index 35b79154805..62d7823f308 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -2780,8 +2780,7 @@ merge_face_ref (struct window *w,
 	      else if (EQ (keyword, QCstipple))
 		{
 #if defined (HAVE_WINDOW_SYSTEM)
-		  Lisp_Object pixmap_p = Fbitmap_spec_p (value);
-		  if (!NILP (pixmap_p))
+		  if (NILP (value) || !NILP (Fbitmap_spec_p (value)))
 		    to[LFACE_STIPPLE_INDEX] = value;
 		  else
 		    err = true;


OT1H that's the kind of sleeping dragon that I wouldn't want to tickle,
OTOH I see that other spots in the code seem to accept QCstipple mapping
to a NILP value:

    else if (EQ (attr, QCstipple))
      {
  #if defined (HAVE_WINDOW_SYSTEM)
        if (!UNSPECIFIEDP (value)
            && !IGNORE_DEFFACE_P (value)
            && !RESET_P (value)
            && !NILP (value)
            && NILP (Fbitmap_spec_p (value)))
          signal_error ("Invalid stipple attribute", value);
        old_value = LFACE_STIPPLE (lface);
        ASET (lface, LFACE_STIPPLE_INDEX, value);
  #endif /* HAVE_WINDOW_SYSTEM */
      }
— Finternal_set_lisp_face_attribute


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Avoid-spurious-pause-in-kill-ring-save-Bug-60841.patch --]
[-- Type: text/x-patch, Size: 5017 bytes --]

From d767d5c658278bb102dea9a716da7a04dccb34aa Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?K=C3=A9vin=20Le=20Gouguec?= <kevin.legouguec@gmail.com>
Date: Sun, 29 Jan 2023 11:23:01 +0100
Subject: [PATCH] Avoid spurious pause in kill-ring-save (Bug#60841)

'indicate-copied-region' checks whether the region is "highlighted"
and if not, briefly moves point to mark to give a visual cue of the
extent of text that was saved to the kill ring.

The region is considered "highlighted" if (a) it is active and (b) its
face specifies a :background.  That latter condition does not account
for the multiple ways in which the face can make the region "visually
distinct" from the default face, so switch to a more extensive
predicate.

* lisp/faces.el (face-differs-from-default-p): Filter out :extend; add
rationale for the attributes we ignore.
* lisp/simple.el (copy-region-blink-predicate): Add option to let user
explicitly opt into or out of blinking point and mark.
(region-indistinguishable-p): New function to detect
"if there is currently no active region highlighting", leveraging
face-differs-from-default-p.
(indicate-copied-region): Use it.
* etc/NEWS: Announce user option.
---
 etc/NEWS       | 12 ++++++++++++
 lisp/faces.el  | 11 ++++++++++-
 lisp/simple.el | 22 ++++++++++++++++++++--
 3 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index e0175bacfdf..6a069b74332 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -59,6 +59,18 @@ This allows the user to customize the prompt that is appended by
 \f
 * Editing Changes in Emacs 30.1
 
+---
+** New user option 'copy-region-blink-predicate'.
+By default, when copying a region with 'kill-ring-save', Emacs only
+blinks point and mark when the region is not denoted visually, that
+is, when either the region is inactive, or the 'region' face is
+indistinguishable from the 'default' face.
+
+Users who would rather enable blinking unconditionally can now set
+this user option to 'always'.  To disable blinking unconditionally,
+either set this option to 'ignore', or set 'copy-region-blink-delay'
+to 0.
+
 ---
 ** New command 'kill-matching-buffers-no-ask'.
 This works like 'kill-matching-buffers', but without asking for
diff --git a/lisp/faces.el b/lisp/faces.el
index 3323eab205a..4933b495a6c 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -304,7 +304,16 @@ face-differs-from-default-p
 If FRAME is t, report on the defaults for face FACE (for new frames).
 If FRAME is omitted or nil, use the selected frame."
   (let ((attrs
-	 (delq :inherit (mapcar 'car face-attribute-name-alist)))
+         ;; The _value_ of :inherit teaches us nothing about how FACE
+         ;; looks compared to the default face.  Instead, we will ask
+         ;; `face-attribute' to take inheritance into account when
+         ;; examining other attributes.
+         (delq :inherit
+               ;; A difference in extension past EOL only matters when
+               ;; relevant attributes (such as :background) also
+               ;; differ from the default; otherwise this difference
+               ;; is a false positive.
+               (delq :extend (mapcar 'car face-attribute-name-alist))))
 	(differs nil))
     (while (and attrs (not differs))
       (let* ((attr (pop attrs))
diff --git a/lisp/simple.el b/lisp/simple.el
index 7bda368d85d..94b08543f44 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -5871,6 +5871,25 @@ copy-region-blink-delay
   :group 'killing
   :version "28.1")
 
+(defcustom copy-region-blink-predicate #'region-indistinguishable-p
+  "Whether the cursor must be blinked after a copy.
+When this condition holds, and the copied region fits in the
+current window, `kill-ring-save' will blink the cursor between
+point and mark for `copy-region-blink-delay' seconds."
+  :type '(radio (function-item region-indistinguishable-p)
+                (function-item :doc "Always blink point and mark." always)
+                (function-item :doc "Never blink point and mark." ignore)
+                (function :tag "Other predicate function"))
+  :group 'killing
+  :version "30.1")
+
+(defun region-indistinguishable-p ()
+  "Whether the current region is not denoted visually.
+This holds when the region is inactive, or when the `region' face
+cannot be distinguished from the `default' face."
+  (not (and (region-active-p)
+            (face-differs-from-default-p 'region))))
+
 (defun indicate-copied-region (&optional message-len)
   "Indicate that the region text has been copied interactively.
 If the mark is visible in the selected window, blink the cursor between
@@ -5891,8 +5910,7 @@ indicate-copied-region
 	;; was selected.  Don't do it if the region is highlighted.
 	(when (and (numberp copy-region-blink-delay)
 		   (> copy-region-blink-delay 0)
-		   (or (not (region-active-p))
-		       (not (face-background 'region nil t))))
+		   (funcall copy-region-blink-predicate))
 	  ;; Swap point and mark.
 	  (set-marker (mark-marker) (point) (current-buffer))
 	  (goto-char mark)
-- 
2.39.0

bug#60841: 30.0.50; kill-ring-save pauses despite region being highlighted

[Eli Zaretskii]

> From: Kévin Le Gouguec <kevin.legouguec@gmail.com>
> Cc: gregory@heytings.org,  60841@debbugs.gnu.org,  juri@linkov.net
> Date: Sun, 29 Jan 2023 23:57:33 +0100
> 
> > So we need to treat :stipple specially, or maybe fix merge_face_ref to
> > allow the nil value.
> 
> You mean something like the following?

Yes.

> OT1H that's the kind of sleeping dragon that I wouldn't want to tickle,
> OTOH I see that other spots in the code seem to accept QCstipple mapping
> to a NILP value:

Exactly.  The nil value is also documented as a valid one.  So we must
make that change in xfaces.c.

> >From d767d5c658278bb102dea9a716da7a04dccb34aa Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?K=C3=A9vin=20Le=20Gouguec?= <kevin.legouguec@gmail.com>
> Date: Sun, 29 Jan 2023 11:23:01 +0100
> Subject: [PATCH] Avoid spurious pause in kill-ring-save (Bug#60841)
> 
> 'indicate-copied-region' checks whether the region is "highlighted"
> and if not, briefly moves point to mark to give a visual cue of the
> extent of text that was saved to the kill ring.
> 
> The region is considered "highlighted" if (a) it is active and (b) its
> face specifies a :background.  That latter condition does not account
> for the multiple ways in which the face can make the region "visually
> distinct" from the default face, so switch to a more extensive
> predicate.
> 
> * lisp/faces.el (face-differs-from-default-p): Filter out :extend; add
> rationale for the attributes we ignore.
> * lisp/simple.el (copy-region-blink-predicate): Add option to let user
> explicitly opt into or out of blinking point and mark.
> (region-indistinguishable-p): New function to detect
> "if there is currently no active region highlighting", leveraging
> face-differs-from-default-p.
> (indicate-copied-region): Use it.
> * etc/NEWS: Announce user option.

Please add the xfaces.c change, and then this can go in (to the
emacs-29 branch).

Thanks.

bug#60841: 30.0.50; kill-ring-save pauses despite region being highlighted

[Kévin Le Gouguec]

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

Eli Zaretskii <eliz@gnu.org> writes:

> Please add the xfaces.c change, and then this can go in (to the
> emacs-29 branch).

Done; see attached.

* lightly amended the commit message (added a third paragraph);
* tried to find an appropriate spot in NEWS;
* de-bumped the defcustom :version;
* ran a couple of sanity checks in *scratch* after applying this on top
  of 2023-01-30 "Update to Transient v0.3.7-196-gb91f509" (1684e254a3b).

  (face-differs-from-default-p 'default)
  nil
  (face-differs-from-default-p 'region)
  :background
  (set-face-attribute 'region nil :background 'unspecified)
  nil
  (face-differs-from-default-p 'region)
  nil
  ;; Tested all 3 function-item values of copy-region-blink-predicate.
  (set-face-attribute 'region nil :inverse-video t)
  nil
  (face-differs-from-default-p 'region)
  :inverse-video
  ;; Tested all 3 function-item values of copy-region-blink-predicate.

Let me know if anything is amiss.


[-- Attachment #2: 0001-Avoid-spurious-pause-in-kill-ring-save-Bug-60841.patch --]
[-- Type: text/x-patch, Size: 5951 bytes --]

From fa7426224d5c5dbccd22d19720acbbd620dc8723 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?K=C3=A9vin=20Le=20Gouguec?= <kevin.legouguec@gmail.com>
Date: Sun, 29 Jan 2023 11:23:01 +0100
Subject: [PATCH] Avoid spurious pause in kill-ring-save (Bug#60841)

'indicate-copied-region' checks whether the region is "highlighted"
and if not, briefly moves point to mark to give a visual cue of the
extent of text that was saved to the kill ring.

The region is considered "highlighted" if (a) it is active and (b) its
face specifies a :background.  That latter condition does not account
for the multiple ways in which the face can make the region "visually
distinct" from the default face, so switch to the more extensive
predicate face-differs-from-default-p.

The patch also fixes a couple of issues with the predicate's
implementation, and introduces a new user option in case anyone
happened to enjoy unconditional blinking.

* lisp/faces.el (face-differs-from-default-p): Filter out :extend; add
rationale for the attributes we ignore.
* lisp/simple.el (copy-region-blink-predicate): Add option to let
users explicitly opt into or out of blinking point and mark.
(region-indistinguishable-p): New function to detect
"if there is currently no active region highlighting", leveraging
face-differs-from-default-p.
(indicate-copied-region): Use it.
* src/xfaces.c (merge_face_ref): Allow :stipple to be nil, since it is
a documented valid value for that attribute.
* etc/NEWS: Announce user option.
---
 etc/NEWS       | 12 ++++++++++++
 lisp/faces.el  | 11 ++++++++++-
 lisp/simple.el | 22 ++++++++++++++++++++--
 src/xfaces.c   |  3 +--
 4 files changed, 43 insertions(+), 5 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index fb211f9b7d0..13472de9e6e 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1339,6 +1339,18 @@ dragged.
 Customize this option to limit the number of entries in the menu
 "Edit → Paste from Kill Menu".  The default is 60.
 
+---
+** New user option 'copy-region-blink-predicate'.
+By default, when copying a region with 'kill-ring-save', Emacs only
+blinks point and mark when the region is not denoted visually, that
+is, when either the region is inactive, or the 'region' face is
+indistinguishable from the 'default' face.
+
+Users who would rather enable blinking unconditionally can now set
+this user option to 'always'.  To disable blinking unconditionally,
+either set this option to 'ignore', or set 'copy-region-blink-delay'
+to 0.
+
 +++
 ** Performing a pinch gesture on a touchpad now increases the text scale.
 
diff --git a/lisp/faces.el b/lisp/faces.el
index 3323eab205a..4933b495a6c 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -304,7 +304,16 @@ face-differs-from-default-p
 If FRAME is t, report on the defaults for face FACE (for new frames).
 If FRAME is omitted or nil, use the selected frame."
   (let ((attrs
-	 (delq :inherit (mapcar 'car face-attribute-name-alist)))
+         ;; The _value_ of :inherit teaches us nothing about how FACE
+         ;; looks compared to the default face.  Instead, we will ask
+         ;; `face-attribute' to take inheritance into account when
+         ;; examining other attributes.
+         (delq :inherit
+               ;; A difference in extension past EOL only matters when
+               ;; relevant attributes (such as :background) also
+               ;; differ from the default; otherwise this difference
+               ;; is a false positive.
+               (delq :extend (mapcar 'car face-attribute-name-alist))))
 	(differs nil))
     (while (and attrs (not differs))
       (let* ((attr (pop attrs))
diff --git a/lisp/simple.el b/lisp/simple.el
index 861fe193fb8..c58acfe3adc 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -5852,6 +5852,25 @@ copy-region-blink-delay
   :group 'killing
   :version "28.1")
 
+(defcustom copy-region-blink-predicate #'region-indistinguishable-p
+  "Whether the cursor must be blinked after a copy.
+When this condition holds, and the copied region fits in the
+current window, `kill-ring-save' will blink the cursor between
+point and mark for `copy-region-blink-delay' seconds."
+  :type '(radio (function-item region-indistinguishable-p)
+                (function-item :doc "Always blink point and mark." always)
+                (function-item :doc "Never blink point and mark." ignore)
+                (function :tag "Other predicate function"))
+  :group 'killing
+  :version "29.1")
+
+(defun region-indistinguishable-p ()
+  "Whether the current region is not denoted visually.
+This holds when the region is inactive, or when the `region' face
+cannot be distinguished from the `default' face."
+  (not (and (region-active-p)
+            (face-differs-from-default-p 'region))))
+
 (defun indicate-copied-region (&optional message-len)
   "Indicate that the region text has been copied interactively.
 If the mark is visible in the selected window, blink the cursor between
@@ -5872,8 +5891,7 @@ indicate-copied-region
 	;; was selected.  Don't do it if the region is highlighted.
 	(when (and (numberp copy-region-blink-delay)
 		   (> copy-region-blink-delay 0)
-		   (or (not (region-active-p))
-		       (not (face-background 'region nil t))))
+		   (funcall copy-region-blink-predicate))
 	  ;; Swap point and mark.
 	  (set-marker (mark-marker) (point) (current-buffer))
 	  (goto-char mark)
diff --git a/src/xfaces.c b/src/xfaces.c
index 35b79154805..62d7823f308 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -2780,8 +2780,7 @@ merge_face_ref (struct window *w,
 	      else if (EQ (keyword, QCstipple))
 		{
 #if defined (HAVE_WINDOW_SYSTEM)
-		  Lisp_Object pixmap_p = Fbitmap_spec_p (value);
-		  if (!NILP (pixmap_p))
+		  if (NILP (value) || !NILP (Fbitmap_spec_p (value)))
 		    to[LFACE_STIPPLE_INDEX] = value;
 		  else
 		    err = true;
-- 
2.39.0

bug#60841: 30.0.50; kill-ring-save pauses despite region being highlighted

[Eli Zaretskii]

> From: Kévin Le Gouguec <kevin.legouguec@gmail.com>
> Cc: gregory@heytings.org,  60841@debbugs.gnu.org,  juri@linkov.net
> Date: Mon, 30 Jan 2023 23:38:24 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Please add the xfaces.c change, and then this can go in (to the
> > emacs-29 branch).
> 
> Done; see attached.
> 
> * lightly amended the commit message (added a third paragraph);
> * tried to find an appropriate spot in NEWS;
> * de-bumped the defcustom :version;
> * ran a couple of sanity checks in *scratch* after applying this on top
>   of 2023-01-30 "Update to Transient v0.3.7-196-gb91f509" (1684e254a3b).
> 
>   (face-differs-from-default-p 'default)
>   nil
>   (face-differs-from-default-p 'region)
>   :background
>   (set-face-attribute 'region nil :background 'unspecified)
>   nil
>   (face-differs-from-default-p 'region)
>   nil
>   ;; Tested all 3 function-item values of copy-region-blink-predicate.
>   (set-face-attribute 'region nil :inverse-video t)
>   nil
>   (face-differs-from-default-p 'region)
>   :inverse-video
>   ;; Tested all 3 function-item values of copy-region-blink-predicate.
> 
> Let me know if anything is amiss.

LGTM, please install on the emacs-29 branch.

Thanks.

bug#60841: 30.0.50; kill-ring-save pauses despite region being highlighted

[Kévin Le Gouguec]

Eli Zaretskii <eliz@gnu.org> writes:

> LGTM, please install on the emacs-29 branch.

Done; closing with this message.

> Thanks.

Thank you all for your help.

bug#60841: 30.0.50; kill-ring-save pauses despite region being highlighted

[Juri Linkov]

>>> * :stipple nil:      foot blown with or without the patch   🤷
>>> * :inherit 'default: foot blown with or without the patch   🤷
>>>
>>> Problem with :stipple nil and :inherit 'default explained in [1].
>>> indicate-copied-region will become affected if the current patch goes
>>> in.
>>
>> I wouldn't be bothered by :stipple whose value is nil.  Why would
>> someone do such a thing, except when the face is meant to be merged
>> with other faces (which are expected to have non-nil :stipple
>> attributes)?
>
> The only place where this "matters" is in replace.el, AFAICT:
>
> 1. Customize list-matching-lines-prefix-face to 'default,
>
> 2. (face-differs-from-default-p list-matching-lines-prefix-face)
>    ↦ :stipple

I still don't understand why (face-differs-from-default-p 'default)
should return :stipple even in a clean state after emacs -Q.
This means that the default face always differs from itself?
Isn't it natural to expect that this relation should be reflexive?

> 3. Luckily this does not impact replace-el:occur-1, since this check…
>
>     (if (face-differs-from-default-p list-matching-lines-prefix-face)
>         list-matching-lines-prefix-face)
>
>    … is redundant AFAIU: the goal is to _not_ apply
>    list-matching-lines-prefix-face if it is close to 'default, and
>    instead… leave the default face?  So why bother checking?
>
> CC'ing Juri, who installed this check in bug#14017, for comment in case
> I missed something.

Sorry, I don't remember the reason for this check.  But looking now
it seems it's just optimization to not run the code that puts
unnecessary font-lock-face properties.

> (Hi Juri 👋  For context, the current report is about refining
> indicate-copied-region to avoid spuriously considering the region as
> "unhighlighted" and triggering the point-mark swap; see attached
> patches.  I believe this aspect of the report might also be of interest
> to you, given your participation in bug#42865)

Thanks, I like your patch that adds copy-region-blink-predicate.

bug#60841: 30.0.50; kill-ring-save pauses despite region being highlighted

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: Eli Zaretskii <eliz@gnu.org>,  gregory@heytings.org,  60841@debbugs.gnu.org
> Date: Sun, 29 Jan 2023 19:55:00 +0200
> 
> > The only place where this "matters" is in replace.el, AFAICT:
> >
> > 1. Customize list-matching-lines-prefix-face to 'default,
> >
> > 2. (face-differs-from-default-p list-matching-lines-prefix-face)
> >    ↦ :stipple
> 
> I still don't understand why (face-differs-from-default-p 'default)
> should return :stipple even in a clean state after emacs -Q.
> This means that the default face always differs from itself?

Because face-differs-from-default-p thinks attributes are unset only
if their value is 'unspecified'.  We should fix that so the function
knows about nil as well.  (:stipple is simply the last attribute in
the list returned by

   (delq :inherit (mapcar 'car face-attribute-name-alist))

bug#60841: 30.0.50; kill-ring-save pauses despite region being highlighted

[Eli Zaretskii]

> Cc: gregory@heytings.org, 60841@debbugs.gnu.org, kevin.legouguec@gmail.com
> Date: Sun, 29 Jan 2023 21:09:18 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > > 2. (face-differs-from-default-p list-matching-lines-prefix-face)
> > >    ↦ :stipple
> > 
> > I still don't understand why (face-differs-from-default-p 'default)
> > should return :stipple even in a clean state after emacs -Q.
> > This means that the default face always differs from itself?
> 
> Because face-differs-from-default-p thinks attributes are unset only
> if their value is 'unspecified'.  We should fix that so the function
> knows about nil as well.

I think face-differs-from-default-p should look like this:

(defun face-differs-from-default-p (face &optional frame)
  "Return non-nil if FACE displays differently from the default face.
If the optional argument FRAME is given, report on face FACE in that frame.
If FRAME is t, report on the defaults for face FACE (for new frames).
If FRAME is omitted or nil, use the selected frame."
  (let ((attrs
	 (delq :inherit (delq :extend (mapcar 'car face-attribute-name-alist))))
	(differs nil))
    (while (and attrs (not differs))
      (let* ((attr (pop attrs))
	     (attr-val (face-attribute face attr frame t)))
	(when (and
	       (not (eq attr-val 'unspecified))
	       (not (equal attr-val (face-attribute 'default attr frame)))
	       (display-supports-face-attributes-p (list attr attr-val)
						   frame))
	  (setq differs attr))))
    differs))

bug#60841: 30.0.50; kill-ring-save pauses despite region being highlighted

[Eli Zaretskii]

> Cc: gregory@heytings.org, 60841@debbugs.gnu.org, kevin.legouguec@gmail.com
> Date: Sun, 29 Jan 2023 21:33:38 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> 
> I think face-differs-from-default-p should look like this:
> 
> (defun face-differs-from-default-p (face &optional frame)
>   "Return non-nil if FACE displays differently from the default face.
> If the optional argument FRAME is given, report on face FACE in that frame.
> If FRAME is t, report on the defaults for face FACE (for new frames).
> If FRAME is omitted or nil, use the selected frame."
>   (let ((attrs
> 	 (delq :inherit (delq :extend (mapcar 'car face-attribute-name-alist))))
> 	(differs nil))
>     (while (and attrs (not differs))
>       (let* ((attr (pop attrs))
> 	     (attr-val (face-attribute face attr frame t)))
> 	(when (and
> 	       (not (eq attr-val 'unspecified))
> 	       (not (equal attr-val (face-attribute 'default attr frame)))
> 	       (display-supports-face-attributes-p (list attr attr-val)
> 						   frame))
> 	  (setq differs attr))))
>     differs))

Actually, I take this back: the 'equal' part is already done by
display-supports-face-attributes-p.

The problem with :stipple is that we don't allow nil as the value of
:stipple.  If you evaluate

  (setq list-matching-lines-prefix-face 'default)
  (face-differs-from-default-p list-matching-lines-prefix-face)

then look in *Messages*, you will see:

  Invalid face attribute :stipple nil

So we need to treat :stipple specially, or maybe fix merge_face_ref to
allow the nil value.