GNU bug report logs -
#77715
[PATCH] Add ring-bell functions for mode line and header line.
Previous Next
Full log
Message #74 received at 77715 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On Tue, Apr 29, 2025 at 8:07 PM Elijah Gabe Pérez <eg642616 <at> gmail.com>
wrote:
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> >> From: Elijah Gabe Pérez <eg642616 <at> gmail.com>
> >> Cc: 77715 <at> debbugs.gnu.org, shipmints <at> gmail.com, drew.adams <at> 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.
[Message part 2 (text/html, inline)]
This bug report was last modified 3 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.