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: More yank pulsing followups #1859

Closed
wants to merge 1 commit into from

Conversation

basil-conto
Copy link
Collaborator

(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 (re: #1856 (comment)).
(ivy--pulse-cleanup): Explicitly set ivy--pulse-timer and ivy--pulse-overlay to nil (re: #1856 (comment)).

Re: #1856
Cc: @mookid

@basil-conto
Copy link
Collaborator Author

@abo-abo Is there a more appropriate place to call ivy--pulse-cleanup in ivy-read?

(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
Copy link
Owner

abo-abo commented Dec 17, 2018

Thanks.

Is there a more appropriate place to call ivy--pulse-cleanup in ivy-read?

Not sure, let's keep it as is for now.

I would also probably like to have a defcustom to turn the pulse off, with default value being on.

@basil-conto
Copy link
Collaborator Author

basil-conto commented Dec 18, 2018

Thanks.

I would also probably like to have a defcustom to turn the pulse off, with default value being on.

At the moment I'm disabling the pulsing by setting ivy-pulse-delay to zero. Would you like to promote this variable to a user option or would you prefer a separate user option?

@basil-conto basil-conto deleted the blc/pulse branch December 18, 2018 13:29
basil-conto added a commit to basil-conto/swiper that referenced this pull request Dec 18, 2018
(ivy--yank-by): Reindent.
(ivy--pulse-region): Simplify logic by ordering START and END.

Re: abo-abo#1859
abo-abo added a commit that referenced this pull request Dec 18, 2018
@abo-abo
Copy link
Owner

abo-abo commented Dec 18, 2018

Would you like to promote this variable to a user option or would you prefer a separate user option?

I've promoted the variable to a defcustom, please have a look.

@basil-conto
Copy link
Collaborator Author

I've promoted the variable to a defcustom, please have a look.

Thanks, just a couple of comments (I'm AFK at the moment):

  1. The docstring needs a second sentence such as "Nil means disable such highlighting."
  2. The custom type should be number, not float.

@abo-abo
Copy link
Owner

abo-abo commented Dec 18, 2018

@basil-conto Thanks

basil-conto added a commit to basil-conto/swiper that referenced this pull request Dec 19, 2018
(ivy--yank-by): Reindent.
(ivy--pulse-region): Simplify logic by ordering START and END.

Re: abo-abo#1859
abo-abo pushed a commit that referenced this pull request Dec 19, 2018
(ivy--yank-by): Reindent.
(ivy--pulse-region): Simplify logic by ordering START and END.

Re: #1859
Fixes #1861
astoff pushed a commit to astoff/swiper that referenced this pull request Jan 1, 2021
(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
Fixes abo-abo#1859
astoff pushed a commit to astoff/swiper that referenced this pull request Jan 1, 2021
astoff pushed a commit to astoff/swiper that referenced this pull request Jan 1, 2021
(ivy--yank-by): Reindent.
(ivy--pulse-region): Simplify logic by ordering START and END.

Re: abo-abo#1859
Fixes abo-abo#1861
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants