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

Commands are not debounced #109

Closed
rakyi opened this issue Jan 1, 2021 · 10 comments
Closed

Commands are not debounced #109

rakyi opened this issue Jan 1, 2021 · 10 comments

Comments

@rakyi
Copy link

rakyi commented Jan 1, 2021

Would it be possible to add (configurable) debouncing to (some) commands?

I think it would be especially useful for the grep and find commands. High amount of candidates can create noticeable slowdown. Although ripgrep is often fast enough on a modern machine, running it less frequently than necessary should also improve battery usage.

Found a similar issue in ivy: abo-abo/swiper#1218

@minad minad closed this as completed in 2fd52e0 Jan 1, 2021
@minad
Copy link
Owner

minad commented Jan 1, 2021

I am already using a timer and I added configuration variables to tune this. Please try it! Maybe some other values for the delays work better for you?

@minad
Copy link
Owner

minad commented Jan 1, 2021

@rakyi I just thought about this issue again and I wonder if the current throttling is not sufficient. Debouncing here would mean that we only run grep after the user hasn't typed something for a certain time. Would that be preferable?

@rakyi
Copy link
Author

rakyi commented Jan 1, 2021

Thanks for the thoughtful consideration! I don't understand emacs and elisp well enough to know what exactly are consult-async-input-delay and consult-async-refresh-delay variables doing. I was scratching my head when I was comparing their default values against slightly higher ones.

Debouncing here would mean that we only run grep after the user hasn't typed something for a certain time.

This is what I had in mind. I don't know if it makes sense to combine the debouncing approach with the existing one or what the majority of users would expect. I'm familiar with debouncing from web development and I think it usually works well in that domain.

@minad
Copy link
Owner

minad commented Jan 2, 2021

Tbh I wasn't aware of the distinction of throttling vs debouncing in this context. But I guess it makes kind of sense since I know debouncing from electronics :)

  • Throttle: allows restarting grep every 0.5s, triggered by user input
  • Refresh: allows refreshing the ui every 0.2s, triggered by incoming strings from the asynchronous background process
  • Debounce (not yet implemented): allow restarting grep only if the last keypress is older than a certain time, e.g. 0.2s

minad added a commit that referenced this issue Jan 2, 2021
@minad
Copy link
Owner

minad commented Jan 2, 2021

I implemented additional debouncing. This is really an improvement! Thank you!

@rakyi
Copy link
Author

rakyi commented Jan 2, 2021

Thanks for adding it! When I tried (setq consult-async-input-throttle 0) I get cancel-timer: Wrong type argument: timerp, nil when I run consult-ripgrep. When I set it to nil instead I don't get any error, but the search is run only once.

Would it be possible to add a way to disable the throttle? I wanted to try relying only on debounce.

@minad
Copy link
Owner

minad commented Jan 2, 2021

I am not eager to add support to disable this. It is good to have both debounce and throttle. I don't see how it hurts to have both. Can you explain?

@rakyi
Copy link
Author

rakyi commented Jan 2, 2021

I was wondering if in the worst case scenario the actual delay wont be 0.5s (or even 0.75s) after the last input when the throttle timer ticks just before the debounce one. Or is the delay always at most the debounce time (0.25s)?

@minad
Copy link
Owner

minad commented Jan 2, 2021

The throttle time just ensures that grep is at most restarted every 0.5s. There is also an initial delay of 0.5s, but we could get rid of that if that is desired. But from my tests it seems reasonable like this.

Furthermore there is the debouncer which ensures that an update does not happen earlier than 0.25s after the last keypress.

To make it clear, current initial total delay is 0.5s.

@minad
Copy link
Owner

minad commented Jan 2, 2021

It might also be sufficient to only have the debouncer. This is what you are suggesting. But I think in some scenarios setting a large throttle value (let's say 2s) and a small debounce value makes sense. The different delays serve different purposes. The debouncer ensures that you are not sending unfinished input. And the throttler ensures that you limit the load.

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

No branches or pull requests

2 participants