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