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

Previous Next

Full log

Message #71 received at 77715 <at> debbugs.gnu.org (full text, mbox):

From: Elijah Gabe Pérez <eg642616 <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 77715 <at> debbugs.gnu.org, shipmints <at> gmail.com, drew.adams <at> oracle.com
Subject: Re: bug#77715: [PATCH] Add ring-bell functions for mode line and
 header line.
Date: Tue, 29 Apr 2025 18:07:06 -0600

[Message part 1 (text/plain, inline)]

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.

[0001-New-ring-bell-functions-for-flash-faces.-bug-77715.patch (text/x-patch, attachment)]

[Message part 3 (text/plain, inline)]

-- 
                                          - E.G via GNU Emacs and Org.

Previous Next

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