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