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: 3516 bytes --]

Hello Emacs,

kill-ring-save calls indicate-copied-region, which does a quick
point-and-mark swap (for copy-region-blink-delay seconds) to give the
user an idea of the extent of text that has been saved.

The docstring says that this is done "if there is currently no active
region highlighting"; in practice, this translates to:

	;; Swap point-and-mark quickly so as to show the region that
	;; 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))                  ; (a)
		       (not (face-background 'region nil t))))  ; (b)

IOW "active region highlighting" means "(a) an active region, and (b)
any background for the region face".  face-background, however, is not
enough to asses (b); consider, from emacs -Q:

M-: (custom-set-faces '(region ((t (:foreground "blue" :inverse-video t)))))
C-x h
M-w

In this situation, the region face has a clearly visible background, yet
indicate-copied-region still behaves as if the region is not
"highlighted", and triggers the (harmless and entirely interruptible)
point-and-mark swap followed by a pause.

I did not know about this feature, and only found out about it by
accident after customizing the region face.  Only after (1) assuming
something was off between Emacs and the system clipboard (2) failing to
use debug-on-quit to figure out where Emacs was "stuck" (it wasn't, of
course) (3) diving into kill-ring-save, did I realize what was going on.

The attached patch handles this foreground + inverse-video switcheroo.
Not sure how many themes actually do that in the wild, so I'd understand
if there wasn't much enthusiasm for applying.  That face-background
check has been with us since 2004; haven't found any hint in the BTS
that anyone else has been bothered by this false negative.


(FWIW, I stumbled on this while working on a dark theme with a limited
 palette; having exhausted all backgrounds on other faces and struggling
 to find one to dedicate to the region, I figured I could combine one of
 my bright foreground colors with :inverse-video, and leave :background
 unspecified.

 Of course, instead of:

   :foreground SOMETHING-BRIGHT
   :inverse-video t

 I could specify:

   :background SOMETHING-BRIGHT
   :foreground SOMETHING-DARK

 … but my initial idea was to let the chips fall where they may and
 allow any text background to become a foreground within the region,
 since all of the theme's backgrounds ought to be dark enough to
 contrast with SOMETHING-BRIGHT)


Thanks for your time.


 GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version
 3.24.35, cairo version 1.17.6) of 2023-01-02 built on amdahl30
Repository revision: c209802f7b3721a1b95113290934a23fee88f678
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12101006
System Description: openSUSE Tumbleweed

Configured using:
 'configure --with-cairo --with-gconf --with-sqlite3 --with-xinput2'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES NOTIFY
INOTIFY PDUMPER PNG RSVG SECCOMP SOUND SQLITE3 THREADS TIFF
TOOLKIT_SCROLL_BARS TREE_SITTER WEBP X11 XDBE XIM XINPUT2 XPM GTK3 ZLIB

Important settings:
  value of $LANG: en_US.UTF-8
  value of $XMODIFIERS: @im=local
  locale-coding-system: utf-8-unix


[-- 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: 2327 bytes --]

From 7bfb6883fb39fe44282a547a4562f9f4d60058ae 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 assumed to be "highlighted" if (a) it is active and (b)
its face specifies a :background.  That latter condition does not
account for this somewhat adversarial face definition:

(custom-set-faces
  '(region ((t (:foreground "blue" :inverse-video t)))))

With this customization, the check will fail and users will experience
an (interruptible) pause, despite the region being clearly
highlighted.

* lisp/simple.el (region-highlighted-p): New function to detect "if
the region is highlighted".  If :background is nil, double-check
:foreground and :inverse-video.
(indicate-copied-region): Use it.
---
 lisp/simple.el | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/lisp/simple.el b/lisp/simple.el
index 4b09f41de5..2029584a94 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -5853,6 +5853,15 @@ 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 has a colored background."
+  (and (region-active-p)
+       (or (face-background 'region nil t)
+           (and (face-foreground 'region nil t)
+                (face-inverse-video-p 'region nil t)))))
+
 (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
@@ -5873,8 +5882,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