Subject: bug#77715: [PATCH] Add ring-bell functions for mode line and header line.

From: Elijah Gabe Pérez <[email-masked]>
Date: Mon, 28 Apr 2025 13:04:53 -0600

Eli Zaretskii <[email-masked]> writes:

>> --- a/lisp/faces.el
>> +++ b/lisp/faces.el
>> @@ -2825,6 +2825,17 @@ header-line-inactive
>>    :group 'mode-line-faces
>>    :group 'basic-faces)
>>  
>> +(defface flash-face
>> +  '((((class color) (min-colors 88))
>> +     :background "red3" :foreground "white")
>> +    (((class color grayscale) (min-colors 88))
>> +     :background "grey10" :foreground "white")
>> +    (t
>> +     :inherit error :inverse-video t))
>> +  "Basic face used for flash a face briefly."
>> +  :version "31.1"
>> +  :group 'basic-faces)
>
> Can you tell why this face is defined in faces.el?  We don't need to
> have it preloaded in Emacs.  Moreover, AFAIU this is not really a
> face, to be used like other faces.  Instead, this is just a collection
> of face attributes to add to other faces for producing the "flash"
> effect.  So I'd rather we would not even use defface for it.  Or if
> the alternatives are too awkward and inconvenient, at least let's make
> the name of this face be an internal name (with two dashes), and
> explain in the doc string how is this used.

I made it a defface specially for any custom theme that wants to set it.
So it can be "updated" to fit the current theme.

Anyway, I've changed it to a defcustom.

>> --- a/lisp/simple.el
>> +++ b/lisp/simple.el
>> @@ -9389,6 +9389,50 @@ auto-save-mode
>>    (and (< buffer-saved-size 0)
>>         (setq buffer-saved-size 0)))
>
> I don't think we need this in simple.el.  This is a minor convenience
> feature, so it doesn't have to be preloaded.  It can be on a separate
> file with the 2 functions it exports autoloaded.

Sure, but wouldn't be redundant making yet another file for just these 2
functions? I added to simple.el because i didn't know where to put the code.
In that case I have moved it to a separate file.

>> +(defcustom flash-face-duration 0.05
>> +  "Time (in seconds) used for flash face duration."
>> +  :type 'float
>      ^^^^^^^^^^^^
> Why not 'number instead?  Do you really want to support only floats?

Ops, i forgot it.

>> +Intended to be used in `ring-bell-function'."
>> +  (when-let* (flash-face-duration ; Ensure time is non-nil
>                                        ^^^^^^^^^^^^^^^^^^^^^^
> Why non-nil and not numberp?

Fixed.

>> +              (cookies (mapcar (lambda (f) (face-remap-add-relative f 'flash-face))
>> +                               flash-face-faces)))
>
> Are you sure using face-remap-add-relative will work as expected with
> any face in flash-face-faces?  Support for face remapping is not
> magic, it must be explicitly coded in several places.  It might work
> quite well for basic faces, but I'm not sure what happens with faces
> defined by various packages.  For that reason, I'm not sure asking
> users to specify a list of faces is the best idea.  How many different
> faces did you test this with?  Did you try the default face, or
> diff-mode faces, for example?

I've tested it with built-in faces and with some third-party packages;
it works fine:

  (setopt flash-face-faces
          '(mode-line-active
            mode-line
            
            default
            hl-line
            trailing-whitespace
            whitespace-trailing
            header-line-active
            header-line
            diff-added
            diff-removed
            diff-refine-added
            diff-indicator-added
            tab-bar
            tab-bar-tab
            centaur-tabs-selected
            centaur-tabs-close-selected
            solaire-mode-line-inactive-face
            solaire-mode-line-active-face))

And it should work for every face defined.

Asking to user to specify others faces is necessary from my POV,
there are packages that overrides some faces with their own one.

>> +    (unwind-protect
>> +        (sit-for flash-face-duration)
>> +      (mapc #'face-remap-remove-relative cookies))))
>
> Using sit-for means any input event will interrupt the wait.  Other
> ring-bell-functions don't behave like that.  Are you sure sleep-for is
> not better for this purpose?

Emacs would freeze if I use sleep-for instead, I've tested using it
but it seems that emacs is going to crash, very uncomfortable when I'm
editing quickly.  Using sit-for at least makes it noticeable and without
interrupting you while editing.  I had planned to use `run-with-timer'
but as Ship pointed, it is not recommended.

>> +(defun flash-echo-area-bell-function ()
>> +  "Flash echo area as ring a bell.
>> +Intended to be used in `ring-bell-function'."
>> +  ;; like `with-temp-message' but suppress message log for not flood
>> +  ;; messages buffer
>> +  (unless (minibufferp) ; Avoid conflicts if cursor is in minibuffer
>
> What conflict is that?
>
> It seems like the above means the feature will not work (i.e., no
> flash) if the minibuffer is active?  If so, is that a good idea?

When the cursor is the minibuffer (eval-expression, M-x and like) and the
ring-bell function is called the message is displayed as:
"[                                                                ]"
   ^ The bell face.

I found that it's caused by `minibuffer-message', but I'm not sure why
and how to remove the "[" "]".

I honestly think that while in the minibuffer this function is not
necessary since it already displays a message (since you are supposed to
be focused on the minibuffer)

For example:
 Typing `M-x' inside minibuffer displays message:
 [Command attempted to use minibuffer while in minibuffer]

 If this function (flash-echo-area-bell-function) is enabled there
 it would display:
 [
]

 and later:

 [Command attempted to use minibuffer while in minibuffer]

>> +    (let ((with-temp-message
>> +              (propertize " " 'display `(space :align-to right) 'face
>> +                          'flash-face))
>
> Why do you use propertize here instead of face-remap-add-relative?

That is for the echo-area/minibuffer, which AFAIK doesn't have a face
that i can use for this.  So instead it displays an empty message with
a face that extends to the end of minibuffer.

>From 129a5a772f1a578fd60ed4ba13cd12ae6fcd6229 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?El=C3=ADas=20Gabriel=20P=C3=A9rez?= <[email-masked]>
Date: Thu, 10 Apr 2025 11:38:21 -0600
Subject: [PATCH] New ring-bell functions for flash faces.  (bug#77715)

New functions for flash any face or echo area once whether
`ring-bell-function' is called.
* lisp/flash-bell.el: New file.
* etc/NEWS: Announce changes.
---
 etc/NEWS           | 25 +++++++++++++
 lisp/flash-bell.el | 90 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 115 insertions(+)
 create mode 100644 lisp/flash-bell.el

diff --git a/etc/NEWS b/etc/NEWS
index a5936b1c6cf..6eee690e09b 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1935,6 +1935,31 @@ change their face if the current line exceeds the 'fill-column'.  The
 new face 'display-fill-column-indicator-warning-face' is used to
 highlight the fill-column indicators.  By default, this is disabled.
 
+---
+*** New function 'flash-face-bell-function'.
+This function flash a face briefly.
+Intended to be used in 'ring-bell-function'.
+
+---
+*** New function 'flash-echo-area-bell-function'.
+This function flash current echo area briefly.
+Intended to be used in 'ring-bell-function'.
+
+---
+*** New user option 'flash-face-duration'.
+This option controls flash duration for 'flash-face-bell-function' and
+'flash-echo-area-bell-function'.
+
+---
+*** New user option 'flash-face-faces'.
+This option tells 'flash-face-bell-function' which faces should flash.
+
+---
+*** New user option 'flash-face-attributes'
+This option tells 'flash-face-bell-function' and
+'flash-echo-area-bell-function' which face attributes should use
+for flash.
+
 ---
 ** Flymake
 
diff --git a/lisp/flash-bell.el b/lisp/flash-bell.el
new file mode 100644
index 00000000000..a50c23ae3ea
--- /dev/null
+++ b/lisp/flash-bell.el
@@ -0,0 +1,90 @@
+;;; flash-bell.el --- Functions intended to be used in ring-bell-function  -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2025 Free Software Foundation, Inc.
+
+;; Author: Elijah Gabe Pérez <[email-masked]>
+;; Keywords: faces
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;; Functions for flash something whether `ring-bell-function' is called.
+
+;;; Code:
+(require 'face-remap)
+
+(defcustom flash-face-attributes
+  '(:background "red" :foreground "white")
+  "Face attributes to use in any function from flash-bell.el.
+This is intended to be used in any function from flash-bell.el such as
+`flash-face-bell-function' and `flash-echo-area-bell-function' for make
+the flash face more noticeable."
+  :type 'plist
+  :group 'faces
+  :version "31.1")
+
+(defcustom flash-face-duration 0.05
+  "Time (in seconds) used for flash face duration."
+  :type 'number
+  :group 'faces
+  :version "31.1")
+
+(defcustom flash-face-faces
+  '(mode-line-active)
+  "A list of faces to be flashed by `flash-face-bell-function'."
+  :type '(repeat face)
+  :group 'faces
+  :version "31.1")
+
+;;;###autoload
+(defun flash-face-bell-function ()
+  "Ring-bell function that flashes faces.
+Intended to be used in `ring-bell-function'."
+  (when-let* (((numberp flash-face-duration)) ; Ensure time is a number
+              (cookies (mapcar (lambda (f) (apply #'face-remap-add-relative f flash-face-attributes))
+                               flash-face-faces)))
+    (unwind-protect
+        (sit-for flash-face-duration)
+      (mapc #'face-remap-remove-relative cookies))))
+
+;;;###autoload
+(defun flash-echo-area-bell-function ()
+  "Flash echo area as ring a bell.
+Intended to be used in `ring-bell-function'."
+  ;; like `with-temp-message' but suppress message log for not flood
+  ;; messages buffer.
+  ;; If we are inside minibuffer, it already displays a noticeable message
+  ;; that can make our bell does not display properly, so disable it
+  ;; only if we are there.
+  (unless (minibufferp)
+    (let ((with-temp-message
+              (propertize " " 'display `(space :align-to right) 'face
+                          flash-face-attributes))
+          (current-message))
+      (unwind-protect
+          (progn
+            (when with-temp-message
+              (setq current-message (current-message))
+              (let (message-log-max)
+                (message "%s" with-temp-message)))
+            (sit-for flash-face-duration))
+        (and with-temp-message
+             (if current-message (message "%s" current-message)
+               (message nil)))))))
+
+(provide 'flash-bell)
+;;; flash-bell.el ends here
-- 
2.49.0

-- 
                                          - E.G via GNU Emacs and Org.