Little improvements on pulse.el

Little improvements on pulse.el

[Gabriel]

I have been playing with pulse.el in the last few days and some
ideas/questions come to my mind.

This is the current source code of 'pulse-line-hook-function':

(defun pulse-line-hook-function ()
  "Function used in hooks to pulse the current line.
Only pulses the line if `pulse-command-advice-flag' is non-nil."
  (when pulse-command-advice-flag
    (pulse-momentary-highlight-one-line (point))))

1. It can be useful in some cases to pulse the current line by using an
interactive command, for example, when doing a presentation, a pairing
with other developer, or to easily find the cursor position. Since
'pulse-line-hook-function' is not a good name for an interactive
function, we can create a new command for that.

2. The name and the docstring mention the use with hooks, but would be
nice to use with advices as well. For example, to highlight the current
line when the cursor, buffer or window changes:

(dolist (symbol '(scroll-up-command
                  scroll-down-command
                  recenter-top-bottom
                  other-window
                  windmove-do-window-select
                  kill-current-buffer
                  delete-window))
  (advice-add symbol :after #'pulse-line-hook-function))

The code above does not work, since 'pulse-line-hook-function' does not
accept any argument (sent by the advice function). We can make it accept
optional arguments, but again the name will not be the best one.

3. I could not find any documentation or usage in emacs source code for
'pulse-line-hook-function' and 'pulse-command-advice-flag'. Is the flag
'pulse-command-advice-flag' really needed ? The git history says the
last modification of this code was 12+ years ago.

4. Would be nice to have support for 'pulse-highlight-start-face' in the
themes provided by Emacs, especially on modus-themes.

5. The code on 'pulse-lighten-highlight' can be significantly improved
by replacing 'pulse-int-to-hex' and 'pulse-color-values-to-hex' with
'color-gradient' from color.el. The idea is to build the list of color
gradients beforehand and use a timer to set each color according to
'pulse-iterations' and 'pulse-delay'. A very basic example:

(let* ((start (color-name-to-rgb (face-background 'pulse-highlight-start-face)))
       (end (color-name-to-rgb (face-background 'default)))
       (gradient-rgb (append (list start)
                             (color-gradient start end pulse-iterations)
                             (list end)))
       (gradient-hex (mapcar (apply-partially 'apply 'color-rgb-to-hex) gradient-rgb)))
  (seq-do-indexed (lambda (color index)
                    (run-with-timer (* index pulse-delay)
                                    nil
                                    (apply-partially 'funcall
                                                     (lambda (color)
                                                       (set-face-background 'default
                                                                            color))
                                                     color)))
                  gradient-hex))

Please share your comments and suggestions. I have FSF copyright
assignment and can open patches for these (or other) improvements/bugs.

Regards,
Gabriel

Re: Little improvements on pulse.el

[Eli Zaretskii]

> From: Gabriel <gabriel376@hotmail.com>
> Date: Wed, 10 Mar 2021 02:39:56 -0300
> 
> I have been playing with pulse.el in the last few days and some
> ideas/questions come to my mind.

Thanks.

> 1. It can be useful in some cases to pulse the current line by using an
> interactive command, for example, when doing a presentation, a pairing
> with other developer, or to easily find the cursor position. Since
> 'pulse-line-hook-function' is not a good name for an interactive
> function, we can create a new command for that.

How about pulse-line-command?  Or even just pulse-line?

> 2. The name and the docstring mention the use with hooks, but would be
> nice to use with advices as well. For example, to highlight the current
> line when the cursor, buffer or window changes:
> 
> (dolist (symbol '(scroll-up-command
>                   scroll-down-command
>                   recenter-top-bottom
>                   other-window
>                   windmove-do-window-select
>                   kill-current-buffer
>                   delete-window))
>   (advice-add symbol :after #'pulse-line-hook-function))
> 
> The code above does not work, since 'pulse-line-hook-function' does not
> accept any argument (sent by the advice function). We can make it accept
> optional arguments, but again the name will not be the best one.

I'd prefer not to use advices for such a simple job.  For example, how
about defining a new hook, which will receive the argument(s) you want
to pass it?

> 3. I could not find any documentation or usage in emacs source code for
> 'pulse-line-hook-function' and 'pulse-command-advice-flag'. Is the flag
> 'pulse-command-advice-flag' really needed ? The git history says the
> last modification of this code was 12+ years ago.

You will find one usage of the hook in CEDET.  In any case, after so
many years I don't think we should consider removing these without a
good reason.  Is there such a reason?

> 4. Would be nice to have support for 'pulse-highlight-start-face' in the
> themes provided by Emacs, especially on modus-themes.

That's up to the themes, no?  They don't necessarily modify all the
faces.

> 5. The code on 'pulse-lighten-highlight' can be significantly improved
> by replacing 'pulse-int-to-hex' and 'pulse-color-values-to-hex' with
> 'color-gradient' from color.el. The idea is to build the list of color
> gradients beforehand and use a timer to set each color according to
> 'pulse-iterations' and 'pulse-delay'. A very basic example:

I'm not sure I understand the improvement, can you elaborate?

Re: Little improvements on pulse.el

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

>> 2. The name and the docstring mention the use with hooks, but would be
>> nice to use with advices as well. For example, to highlight the current
>> line when the cursor, buffer or window changes:
[...]
>
> I'd prefer not to use advices for such a simple job.  For example, how
> about defining a new hook, which will receive the argument(s) you want
> to pass it?

Besides the implementation details, how about packing this up in a
global minor mode, like `pulse-change-window-mode' or something?
I could see myself using something like that.

Re: Little improvements on pulse.el

[Juri Linkov]

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> 2. The name and the docstring mention the use with hooks, but would be
>>> nice to use with advices as well. For example, to highlight the current
>>> line when the cursor, buffer or window changes:
> [...]
>>
>> I'd prefer not to use advices for such a simple job.  For example, how
>> about defining a new hook, which will receive the argument(s) you want
>> to pass it?
>
> Besides the implementation details, how about packing this up in a
> global minor mode, like `pulse-change-window-mode' or something?
> I could see myself using something like that.

Maybe simply something like:

  (defun pulse-line (&rest _)
    (pulse-momentary-highlight-one-line (point)))

  (add-hook 'window-state-change-functions #'pulse-line)
or
  (add-hook 'window-configuration-change-hook #'pulse-line)

Re: Little improvements on pulse.el

[Juri Linkov]

>> Besides the implementation details, how about packing this up in a
>> global minor mode, like `pulse-change-window-mode' or something?
>> I could see myself using something like that.
>
> Maybe simply something like:
>
>   (defun pulse-line (&rest _)
>     (pulse-momentary-highlight-one-line (point)))
>
>   (add-hook 'window-state-change-functions #'pulse-line)

BTW, currently 'pulse-momentary-highlight-one-line'
has no visible indication on an empty line, so it's face
should be fixed to have the face attribute :extend t.

Re: Little improvements on pulse.el

[Juri Linkov]

> 2. The name and the docstring mention the use with hooks, but would be
> nice to use with advices as well. For example, to highlight the current
> line when the cursor, buffer or window changes:
>
> (dolist (symbol '(scroll-up-command
>                   scroll-down-command
>                   recenter-top-bottom

Scrolling commands can be handled by this hook:

(add-hook 'window-scroll-functions #'pulse-line)

>                   other-window
>                   windmove-do-window-select

(add-hook 'window-configuration-change-hook #'pulse-line)

>                   kill-current-buffer
>                   delete-window))

(add-hook 'window-state-change-functions #'pulse-line)

> The code above does not work, since 'pulse-line-hook-function' does not
> accept any argument (sent by the advice function). We can make it accept
> optional arguments, but again the name will not be the best one.

A new command could ignore its arguments:

  (defun pulse-line (&rest _)
    (interactive)
    (pulse-momentary-highlight-one-line (point)))

Re: Little improvements on pulse.el

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: Gabriel <gabriel376@hotmail.com>,  Eli Zaretskii <eliz@gnu.org>,
>   emacs-devel@gnu.org
> Date: Wed, 10 Mar 2021 20:51:57 +0200
> 
> BTW, currently 'pulse-momentary-highlight-one-line'
> has no visible indication on an empty line, so it's face
> should be fixed to have the face attribute :extend t.

I disagree with doing this unconditionally.  Current uses of this face
don't need to show empty lines, so adding that attribute would
gratuitously change previous behavior.

Commands that want to use this for potentially empty lines can always
produce a derived face if they so want.

Re: Little improvements on pulse.el

[Stefan Monnier]

> 2. The name and the docstring mention the use with hooks, but would be

Indeed, and that is generally a bad idea: docstring should aim to
describe what the function does rather than when/where it's expected to
be used [insert general disclaimer that all generalizations are bad].

> 5. The code on 'pulse-lighten-highlight' can be significantly improved
> by replacing 'pulse-int-to-hex' and 'pulse-color-values-to-hex' with
> 'color-gradient' from color.el.

I think pulse.el was written before color.el was added to Emacs, so
I wouldn't be surprised that there's room for simplifying and improving
the code.


        Stefan

Re: Little improvements on pulse.el

[Gabriel]

>> 3. I could not find any documentation or usage in emacs source code for
>> 'pulse-line-hook-function' and 'pulse-command-advice-flag'. Is the flag
>> 'pulse-command-advice-flag' really needed ? The git history says the
>> last modification of this code was 12+ years ago.
>
> You will find one usage of the hook in CEDET.  In any case, after so
> many years I don't think we should consider removing these without a
> good reason.  Is there such a reason?

The only reason to remove it was because I thought it was not being
used anywhere. If it's used by CEDET, we can keep as it is.

>> 5. The code on 'pulse-lighten-highlight' can be significantly improved
>> by replacing 'pulse-int-to-hex' and 'pulse-color-values-to-hex' with
>> 'color-gradient' from color.el. The idea is to build the list of color
>> gradients beforehand and use a timer to set each color according to
>> 'pulse-iterations' and 'pulse-delay'. A very basic example:
>
> I'm not sure I understand the improvement, can you elaborate?

I will try to briefly explain how it works and how it could be
improved. The main function ('pulse-momentary-highlight-overlay') runs
'pulse-tick' with a timer, passing a stop-time as parameter. The
'pulse-tick' calls 'pulse-lighten-highlight' that has a complex logic to
calculate the next color for the pulse overlay. It basically checks for
the current iteration number and calculates the appropriate RGB (a
little bit lighter than the previous one), using a helper function in
the same file ('pulse-color-values-to-hex'). All this code can be
replaced by 'color-gradient' from 'color.el' that, according to Stefan
Monnier in another thread, was added to Emacs after pulse.el. The
pseudo-code is:

for each color in color-gradients(start, stop, length):
    update-overlay(color)
    sleep()

Re: Little improvements on pulse.el

[Eli Zaretskii]

> From: Gabriel <gabriel376@hotmail.com>
> Date: Thu, 11 Mar 2021 20:42:40 -0300
> 
> >> 5. The code on 'pulse-lighten-highlight' can be significantly improved
> >> by replacing 'pulse-int-to-hex' and 'pulse-color-values-to-hex' with
> >> 'color-gradient' from color.el. The idea is to build the list of color
> >> gradients beforehand and use a timer to set each color according to
> >> 'pulse-iterations' and 'pulse-delay'. A very basic example:
> >
> > I'm not sure I understand the improvement, can you elaborate?
> 
> I will try to briefly explain how it works and how it could be
> improved. The main function ('pulse-momentary-highlight-overlay') runs
> 'pulse-tick' with a timer, passing a stop-time as parameter. The
> 'pulse-tick' calls 'pulse-lighten-highlight' that has a complex logic to
> calculate the next color for the pulse overlay. It basically checks for
> the current iteration number and calculates the appropriate RGB (a
> little bit lighter than the previous one), using a helper function in
> the same file ('pulse-color-values-to-hex'). All this code can be
> replaced by 'color-gradient' from 'color.el' that, according to Stefan
> Monnier in another thread, was added to Emacs after pulse.el. The
> pseudo-code is:
> 
> for each color in color-gradients(start, stop, length):
>     update-overlay(color)
>     sleep()

OK, so the timer thing is still needed, right?  color-gradients only
replace the calculation of the next color.  SGTM, thanks.