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