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