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

From: Eli Zaretskii <[email-masked]>
Date: Tue, 29 Apr 2025 08:36:19 +0300

> From: Elijah Gabe Pérez <[email-masked]>
> Cc: [email-masked],  [email-masked],  [email-masked]
> Date: Mon, 28 Apr 2025 13:04:53 -0600
> 
> >> --- 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?

Nothing wrong with having a separate file for this feature.  We could
call it ring-bell-fns.el or something, and with time other ring-bell
functions might be added to it.

Alternatively, we could add these two functions and the related
variables to subr-x.el.

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

But that's how the built-in ring-bell functions work: they wait
unconditionally.  As I explained earlier, using sit-for might cause
the entire feature to seem not to work.

As a compromise, perhaps use sleep-for if the time is short enough,
and sit-for otherwise?

> >> +(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 "[" "]".

That's a feature: the "[...]" is intended to tell the user that the
message is from some background processing, unrelated to the
minibuffer prompt.  Why did you want to remove the brackets?

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

Again, that's not how other ring-bell alternatives work.

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

This probably means there's some subtle bug in the current
implementation, and avoiding to flash when in the minibuffer just
sweeps the bug under the carpet.  I suggest to debug the reasons and
fix them, or at least understand them well enough so that we could
reason whether fixing them is 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?
> 
> 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.

The face used by default in the echo-area is the default face.  (There
could be cases when echo-area messages use other, non-default faces.)

> +(defcustom flash-face-duration 0.05
> +  "Time (in seconds) used for flash face duration."
> +  :type 'number
> +  :group 'faces

Are you sure this is the best group for this option?  Why not
something related to flash-bell or bell-ring?  Same question regarding
the faces defined for this feature: placing them in the faces group
could drown them in the large sea of faces we have, and make them less
discoverable.