GNU bug report logs - #77715
[PATCH] Add ring-bell functions for mode line and header line.

Previous Next

Full log

View this message in rfc822 format

From: Eli Zaretskii <eliz <at> gnu.org>
To: Elijah Gabe Pérez <eg642616 <at> gmail.com>
Cc: 77715 <at> debbugs.gnu.org, shipmints <at> gmail.com, drew.adams <at> oracle.com
Subject: bug#77715: [PATCH] Add ring-bell functions for mode line and header line.
Date: Mon, 28 Apr 2025 14:35:32 +0300

> From: Elijah Gabe Pérez <eg642616 <at> gmail.com>
> Cc: Ship Mints <shipmints <at> gmail.com>, drew.adams <at> oracle.com, eliz <at> gnu.org
> Date: Sun, 27 Apr 2025 12:12:58 -0600
> 
> I don't know if there still interest on this,
> I'm sending here a fixed and updated version of this patch.
> 
> This is ready to merge into master (unless there is something else).

Thanks, but there are a few aspects of this that I think "need work".

> +** New face 'flash-face'.
> +This face is used for flash a face briefly.
> +Mainly used for 'flash-echo-area-bell-function' and
> +'flash-face-bell-function'.

I see no reason to announce this face in NEWS for the reasons I
explain below.

> +---
> +*** New function 'flash-face-bell-function'.
> +This function when called will flash a face briefly.
> +Intended to be used in 'ring-bell-function'.
> +
> +---
> +*** New function 'flash-echo-area-bell-function'.
> +This function when called will flash current echo area briefly.
> +Intended to be used in 'ring-bell-function'.

The "when called" part is redundant: it should be clear that a
function can do its job only if called.

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

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

> +(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?

> +(defcustom flash-face-faces
> +  '(mode-line-active)
> +  "A list of faces which `flash-face-bell-function' will flash them."

The doc string reads awkwardly.  I suggest

  Faces to be flashed by `flash-face-bell-function'.

> +(defun flash-face-bell-function ()
> +  "Flash a face or faces as ring a bell.

I'd say "Ring-bell function that flashes faces instead."

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

> +              (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?

> +    (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?

> +(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?

> +    (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?

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.