On Tue, Apr 29, 2025 at 8:07 PM Elijah Gabe Pérez wrote: > Eli Zaretskii writes: > > >> From: Elijah Gabe Pérez > >> Cc: 77715@debbugs.gnu.org, shipmints@gmail.com, drew.adams@oracle.com > >> 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. > > It makes sense, I've renamed the file to `ring-bell-fns.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? > > Fine, i've updated it to only use sleep-for if flash duration time is > long and fallback to sit-for otherwise. > > >> >> + (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.) > > Thanks for the clarification, I've rewrote the function to remap the > default face only in the minibuffer. > > >> +(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. > > I've create its proper defgroup, but I'm not sure about the name and > docstring I've given it. > Face flashing isn't a feature of bell ringing, it's the opposite. I'd consider putting the face flashing code with face-related code so it's clear faces can be flashed anywhere for any reason the users want.