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

["Kévin Le Gouguec" <kevin.legouguec@gmail.com>]

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