~protesilaos/pulsar

Re: [PATCH] Add ability to pulse on every window change

Details
Message ID
<875yng88j3.fsf@protesilaos.com>
DKIM signature
pass
Download raw message
Sorry, I forgot to "reply to all".  The message did not reach the
mailing list...

On 2022-04-09, 22:03 +0300, Protesilaos Stavrou <info@protesilaos.com> wrote:

> On 2022-04-09, 18:24 +0300, Ivan Popovych <ivan.popovich000@gmail.com> wrote:
>
>> As previously discussed via email i send patch to implement this feature
>
> Thank you!  I installed your patch.  Though check below for some
> comments and refer to the recent commits I made.
>
>> Few things to consider before applying this patch:
>> - I set 'pulsar-pulse-on-window-change' to nil to preserve default
>>   behavior, you may want to set it to t.
>
> Yes, this is better.  I removed the relevant entries from the
> 'pulsar-pulse-functions'.
>
>> - Parts that add hooks in pulsar-mode are over 79 characters wide, I
>>   think they need to be reformatted to fit in 79 character limit, but I
>>   am not sure of the best way to do it.
>
> No worries.  Generally it is a good idea to avoid long lines, but we
> want to keep the code looking natural instead of enforcing the rule at
> all times.  They look good right now.
>
>> - Documentation in REAME needs to be updated, but I dont think I can fit
>>   more changes in this patch without signing copyright assigment.
>
> I updated the manual to mention the new user option and included your
> name in the "Acknowledgements" section.
>
>> - Variable docstring may need to be adjusted to make it clearer that
>>   it will superseed same window change functions in
>>   'pulsar-pulse-functions'.
>
> I removed the commands from 'pulsar-pulse-functions' which, I think,
> simplifies everything.
>
>> +(defun pulsar--pulse-on-window-change (arg)
>> +  "Run `pulsar-pulse-line' on window change, ARG is ignored."
>> +  (when (or pulsar-mode pulsar-global-mode)
>> +    (pulsar-pulse-line)))
>
> Here you forgot to check for the 'pulsar-pulse-on-window-change'.  I
> added the 'and' condition.
>
> As for the ARG which is ignored, you can use '&rest _' and not document
> it.  The '_' tells the compiler to not complain, while the '&rest'
> accepts any number of arguments (and will ignore them all in this case).

-- 
Protesilaos Stavrou
https://protesilaos.com
Reply to thread Export thread (mbox)