On Mon, Apr 28, 2025 at 7:35 AM Eli Zaretskii <eliz@gnu.org> wrote:

> > +              (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 use my version of this implementation with the following list of faces:
'(internal-border tab-bar mode-line-active mode-line-inactive)

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

My implementation uses sit-for and the duration is short enough (mine is
set to 0.05 seconds) that this is fine.  And if it were any longer, I'd
want C-g to interrupt it and remove the remappings.

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

My implementation doesn't have this condition so I'm not sure what it tries
to do or avoid.

-Stephane