Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ivy.el: Touch up yank pulsing #1856

Closed
wants to merge 1 commit into from

Conversation

@basil-conto
Copy link
Collaborator

commented Dec 13, 2018

(ivy-pulse-delay): New variable.
(ivy--pulse-timer): Add markup to docstring.
(ivy--pulse-region): If ivy--pulse-overlay still exists from the previous invocation, expand its region, rather than moving it. Remove redundant call to move-overlay. Use ivy-pulse-delay.
(ivy--pulse-cleanup): New function for ensuring ivy--pulse-timer and ivy--pulse-overlay are disposed of.

Re: #1581, #1850
Cc: @mookid, @hotpxl

Question

Sorry for not raising this sooner, but is the name of the face ivy-yanked-word too specific? Mightn't we want to use it with yanking that isn't limited to words?

Furthermore, I don't think highlight is the right face to inherit from, as it clashes with swiper's highlighting. Or is this feature not intended to be compatible with swiper?

@mookid Thoughts?

ivy.el: Touch up yank pulsing
(ivy-pulse-delay): New variable.
(ivy--pulse-timer): Add markup to docstring.
(ivy--pulse-region): If ivy--pulse-overlay still exists from the
previous invocation, expand its region, rather than moving it.
Remove redundant call to move-overlay.  Use ivy-pulse-delay.
(ivy--pulse-cleanup): New function for ensuring ivy--pulse-timer and
ivy--pulse-overlay are disposed of.

Re: #1581, #1850
(when ivy--pulse-timer
(setq ivy--pulse-timer (cancel-timer ivy--pulse-timer)))
(when ivy--pulse-overlay
(setq ivy--pulse-overlay (delete-overlay ivy--pulse-overlay))))

This comment has been minimized.

Copy link
@mookid

mookid Dec 13, 2018

Contributor

Does delete overlay return anything meaningful?

Its docstring does not make that clear.

Why not set that to nil?

This comment has been minimized.

Copy link
@basil-conto

basil-conto Dec 14, 2018

Author Collaborator

Does delete overlay return anything meaningful?

It always returns nil. It's not documented as doing that, but it's unlikely to change, so I just used it to save myself having to write a second line. Same with cancel-timer. Feel free to change that if it irks you.

This comment has been minimized.

Copy link
@mookid

mookid Dec 14, 2018

Contributor

I won't spend my time rewriting your code PRs, especially when you are yourself rewriting something I did. This is not a healthy way of working on anything. I am more interested in fixing actual bugs.

I am just surprised as you are usually the one nitpicking about this kind of stuff.

This comment has been minimized.

Copy link
@basil-conto

basil-conto Dec 14, 2018

Author Collaborator

You are free to work on anything you like and suggest any changes you like. Same goes for any other contributor around here. There is nothing unhealthy about that; it's what many people find most fun about FOSS. Each person has different itches.

Re: the nitpicking - either you have misunderstood my intention, or my tastes have changed. I'm only human, you know. I don't recall ever saying anything about a guaranteed return value in a context where the intention is clear, but it's definitely possible. I'm sorry you find it so frustrating to work on the same project as me.

This comment has been minimized.

Copy link
@basil-conto

basil-conto Dec 14, 2018

Author Collaborator

@mookid I forgot to thank you for your efforts in fixing Ivy bugs, it's much appreciated.

@mookid

This comment has been minimized.

Copy link
Contributor

commented Dec 13, 2018

(ivy--pulse-cleanup): ...... and ivy--pulse-overlay are disposed of.

Not disposing of the overlay is intentional: I made it a global because I don't expect two such overlays to be visible at a time.

(ivy-pulse-delay): New variable.

I am not so sure there is a real need for that, but why not.

(ivy--pulse-region): If ivy--pulse-overlay still exists from the previous invocation, expand its region, rather than moving it.

I am neither for nor against this new semantics. But see inline comments about the implementation.

(move-overlay ivy--pulse-overlay
(min begin (overlay-start ivy--pulse-overlay))
(max end (overlay-end ivy--pulse-overlay)))
(setq ivy--pulse-overlay (make-overlay begin end))

This comment has been minimized.

Copy link
@mookid

mookid Dec 13, 2018

Contributor

In principle, this may highlight the wrong region if the user manages to move the point (say, to another line) between calls to ivy--pulse-region and before the timer runs out of time.

Of course in practical terms, it is unlikely to happen.

This comment has been minimized.

Copy link
@basil-conto

basil-conto Dec 14, 2018

Author Collaborator

Good point. I'll either revert my change or protect against such a scenario in my next PR on this, unless someone beats me to it.

@abo-abo abo-abo closed this in e6dcf73 Dec 13, 2018

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Dec 13, 2018

Thanks.

@basil-conto

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 14, 2018

Not disposing of the overlay is intentional: I made it a global because I don't expect two such overlays to be visible at a time.

Deleting the overlay is not meant to protect from simultaneous overlays, it's just being cautious about not leaving markers lying around. The previous code also deleted the overlay; I just additionally set its variable to nil to improve its chances of getting garbage collected.

I am not so sure there is a real need for that, but why not.

I would like to be able to disable this feature (though I haven't tested whether disabling the delay does that yet). Update: it does :)

@basil-conto

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 14, 2018

@mookid @abo-abo Do you have any comments on my question about the face ivy-yanked-word in the description of this PR?

basil-conto added a commit to basil-conto/swiper that referenced this pull request Dec 14, 2018
ivy.el: More yank pulsing followups
(ivy-read): Ensure ivy--pulse-overlay is deleted before exiting
completion.
(ivy--pulse-region): Add docstring.  Only extend existing overlay's
region if it is contiguous with the new region.
(ivy--pulse-cleanup): Explicitly set ivy--pulse-timer and
ivy--pulse-overlay to nil.

Re: abo-abo#1856
basil-conto added a commit to basil-conto/swiper that referenced this pull request Dec 14, 2018
ivy.el: More yank pulsing followups
(ivy-read): Ensure ivy--pulse-overlay is deleted before exiting
completion.
(ivy--pulse-region): Add docstring.  Only extend existing overlay's
region if it is contiguous with the new region.
(ivy--pulse-cleanup): Explicitly set ivy--pulse-timer and
ivy--pulse-overlay to nil.

Re: abo-abo#1856
basil-conto added a commit to basil-conto/swiper that referenced this pull request Dec 15, 2018
ivy.el: More yank pulsing followups
(ivy-read): Ensure ivy--pulse-overlay is deleted before exiting
completion.
(ivy--pulse-region): Add docstring.  Only extend existing overlay's
region if it is contiguous with the new region.
(ivy--pulse-cleanup): Explicitly set ivy--pulse-timer and
ivy--pulse-overlay to nil.

Re: abo-abo#1856
abo-abo added a commit that referenced this pull request Dec 17, 2018
ivy.el: More yank pulsing followups
(ivy-read): Ensure ivy--pulse-overlay is deleted before exiting
completion.
(ivy--pulse-region): Add docstring.  Only extend existing overlay's
region if it is contiguous with the new region.
(ivy--pulse-cleanup): Explicitly set ivy--pulse-timer and
ivy--pulse-overlay to nil.

Re: #1856
Fixes #1859
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.