GNU bug report logs -
#77715
[PATCH] Add ring-bell functions for mode line and header line.
Previous Next
Full log
Message #65 received at 77715 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Eli Zaretskii <eliz <at> gnu.org> writes:
>> --- a/lisp/faces.el
>> +++ b/lisp/faces.el
>> @@ -2825,6 +2825,17 @@ header-line-inactive
>> :group 'mode-line-faces
>> :group 'basic-faces)
>>
>> +(defface flash-face
>> + '((((class color) (min-colors 88))
>> + :background "red3" :foreground "white")
>> + (((class color grayscale) (min-colors 88))
>> + :background "grey10" :foreground "white")
>> + (t
>> + :inherit error :inverse-video t))
>> + "Basic face used for flash a face briefly."
>> + :version "31.1"
>> + :group 'basic-faces)
>
> Can you tell why this face is defined in faces.el? We don't need to
> have it preloaded in Emacs. Moreover, AFAIU this is not really a
> face, to be used like other faces. Instead, this is just a collection
> of face attributes to add to other faces for producing the "flash"
> effect. So I'd rather we would not even use defface for it. Or if
> the alternatives are too awkward and inconvenient, at least let's make
> the name of this face be an internal name (with two dashes), and
> explain in the doc string how is this used.
I made it a defface specially for any custom theme that wants to set it.
So it can be "updated" to fit the current theme.
Anyway, I've changed it to a defcustom.
>> --- 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? I added to simple.el because i didn't know where to put the code.
In that case I have moved it to a separate file.
>> +(defcustom flash-face-duration 0.05
>> + "Time (in seconds) used for flash face duration."
>> + :type 'float
> ^^^^^^^^^^^^
> Why not 'number instead? Do you really want to support only floats?
Ops, i forgot it.
>> +Intended to be used in `ring-bell-function'."
>> + (when-let* (flash-face-duration ; Ensure time is non-nil
> ^^^^^^^^^^^^^^^^^^^^^^
> Why non-nil and not numberp?
Fixed.
>> + (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've tested it with built-in faces and with some third-party packages;
it works fine:
(setopt flash-face-faces
'(mode-line-active
mode-line
default
hl-line
trailing-whitespace
whitespace-trailing
header-line-active
header-line
diff-added
diff-removed
diff-refine-added
diff-indicator-added
tab-bar
tab-bar-tab
centaur-tabs-selected
centaur-tabs-close-selected
solaire-mode-line-inactive-face
solaire-mode-line-active-face))
And it should work for every face defined.
Asking to user to specify others faces is necessary from my POV,
there are packages that overrides some faces with their own one.
>> + (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. Using sit-for at least makes it noticeable and without
interrupting you while editing. I had planned to use `run-with-timer'
but as Ship pointed, it is not recommended.
>> +(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 "[" "]".
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)
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]
>> + (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.
[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.
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.