Adding option to configure debounce time

rheng commented

2024-01-03 01:36:10 +01:00

(Migrated from github.com)

This PR proposes two updates:

Configurable Debounce Time:

Current 50ms is too restrictive for certain use cases in our project. Adding an option to customize this value would enhance flexibility.

Revised Default Debounce Time:

Based on UX Stack Exchange insights, a longer default debounce time might be more user-friendly. I suggest adjusting it to 500ms, improving user experience per community feedback.
For backward compatibility, we could retain the old default or consider other solutions.

Looking forward to your thoughts and suggestions.

This PR proposes two updates: Configurable Debounce Time: - Current 50ms is too restrictive for certain use cases in our project. Adding an option to customize this value would enhance flexibility. Revised Default Debounce Time: - Based on [UX Stack Exchange insights](https://ux.stackexchange.com/questions/95336/how-long-should-the-debounce-timeout-be), a longer default debounce time might be more user-friendly. I suggest adjusting it to 500ms, improving user experience per community feedback. For backward compatibility, we could retain the old default or consider other solutions. Looking forward to your thoughts and suggestions.

marijnh commented

2024-01-03 11:21:16 +01:00

(Migrated from github.com)

Making autocompletion react 10 times slower by default is absolutely not acceptable.

As for making this an option, I'd like to hear more about why—what does "too restrictive" mean here? What kind of problems are you running into with the current behavior?

Making autocompletion react 10 times slower by default is absolutely not acceptable. As for making this an option, I'd like to hear more about why—what does "too restrictive" mean here? What kind of problems are you running into with the current behavior?

rheng commented

2024-01-03 18:27:47 +01:00

(Migrated from github.com)

Great questions!

For context, we are trying to make the autocomplete fetch options from an API. With the current 50ms setting, requests are happening at almost every keystroke, similar to not having a debounce at all. Having it configurable may be helpful us and for others control what is acceptable in terms of how quickly the input is reacting vs how many times costly APIs are invoked.

I've reduced the default back to the previous value (50ms) based on your feedback. I do think it is probably too short, but happy to adjust to whatever you prefer.

Great questions! For context, we are trying to make the autocomplete fetch options from an API. With the current 50ms setting, requests are happening at almost every keystroke, similar to not having a debounce at all. Having it configurable may be helpful us and for others control what is acceptable in terms of how quickly the input is reacting vs how many times costly APIs are invoked. I've reduced the default back to the previous value (50ms) based on your feedback. I do think it is probably too short, but happy to adjust to whatever you prefer.

marijnh commented

2024-01-04 11:57:06 +01:00

(Migrated from github.com)

You should be able to implement the debounce in your completion source, using abort handlers on the context.

You should be able to implement the debounce in your completion source, using [abort handlers](https://codemirror.net/docs/ref/#autocomplete.CompletionContext.addEventListener) on the context.

rheng commented

2024-01-05 09:16:39 +01:00

(Migrated from github.com)

Thanks for the suggestion! Had a few questions though.

Please forgive me as I might be misunderstanding, but previously you mentioned that we shouldn't be implementing debounce in our completion source:
https://discuss.codemirror.net/t/async-autocomplete-debounce/4347/2

Why would we want to create our own debounce when there is native support already?

With that said, I also looked at the source, and while I could add an abort handler, I need some help understanding how I can trigger the abort. It seems only transactions that modify the selection or edits the document can cause the abort handlers to invoke:
github.com/codemirror/autocomplete@36067ab2b9/src/view.ts (L86-L88)

The problem is, within the completion handler, I don't think I should be editing the document or selection state, and it is not clear to me how I can trigger this abort without modifying the state.

Again, I could be misunderstanding, but definitely appreciate any guidance or help.

Thanks for the suggestion! Had a few questions though. Please forgive me as I might be misunderstanding, but previously you mentioned that we shouldn't be implementing debounce in our completion source: https://discuss.codemirror.net/t/async-autocomplete-debounce/4347/2 Why would we want to create our own debounce when there is native support already? With that said, I also looked at the source, and while I could add an abort handler, I need some help understanding how I can trigger the abort. It seems only transactions that modify the selection or edits the document can cause the abort handlers to invoke: https://github.com/codemirror/autocomplete/blob/36067ab2b98cc0531fb53909a622d4e0f682d8fc/src/view.ts#L86-L88 The problem is, within the completion handler, I don't think I should be editing the document or selection state, and it is not clear to me how I can trigger this abort without modifying the state. Again, I could be misunderstanding, but definitely appreciate any guidance or help.

marijnh commented

2024-01-05 11:22:05 +01:00

(Migrated from github.com)

You generally shouldn't need to debounce these. But if you really insist on doing so, you can use abort handlers to do it—something like

async fucnction(cx) {
  let aborted = false
  cx.addEventListener("abort", () => aborted = true)
  await new Promise(r => setTimeout(r, 500))
  if (aborted) return null
  ...
}

But, again, if you include the appropriate validFor field in your results, you shouldn't see a lot of wasted requests anyway.

You generally shouldn't need to debounce these. But if you really insist on doing so, you can use abort handlers to do it—something like ``` async fucnction(cx) { let aborted = false cx.addEventListener("abort", () => aborted = true) await new Promise(r => setTimeout(r, 500)) if (aborted) return null ... } ``` But, again, if you include the appropriate `validFor` field in your results, you shouldn't see a lot of wasted requests anyway.

rheng commented

2024-01-06 02:23:28 +01:00

(Migrated from github.com)

Thanks for the feedback.

So here is an example using the sample you provided:
https://codesandbox.io/p/sandbox/codemirror-autocomplete-debounce-56fgtg

https://github.com/codemirror/autocomplete/assets/4783091/f554a776-0291-440e-86ae-d2430110ee09

As you can see, none of the calls get aborted. Also the expensive call gets called multiple times, even though ideally it would be called once when the user is done typing.

I think it is because there is no way to trigger the abort from the completion handler.

The validFor doesn't seem to improve the situation either.

I managed to get it working using global event handlers, but I still think having this configurable is a bit cleaner.

Is there some architectural reason why the denounce time variable shouldn't be exposed?

Ultimately up to you though. Happy to close the PR if you prefer it not to be exposed.

Thanks for the feedback. So here is an example using the sample you provided: https://codesandbox.io/p/sandbox/codemirror-autocomplete-debounce-56fgtg https://github.com/codemirror/autocomplete/assets/4783091/f554a776-0291-440e-86ae-d2430110ee09 As you can see, none of the calls get aborted. Also the expensive call gets called multiple times, even though ideally it would be called once when the user is done typing. I think it is because there is no way to trigger the abort from the completion handler. The `validFor` doesn't seem to improve the situation either. I managed to get it working using global event handlers, but I still think having this configurable is a bit cleaner. Is there some architectural reason why the denounce time variable shouldn't be exposed? Ultimately up to you though. Happy to close the PR if you prefer it not to be exposed.

marijnh commented

2024-01-12 19:26:32 +01:00

(Migrated from github.com)

There's going to be a maximum of one request in flight at any time.

The validFor doesn't seem to improve the situation either.

It should, when the user types a single identifier, allow the completion to use the result from the request fired off when typing started.

Is there some architectural reason why the denounce time variable shouldn't be exposed?

Because the number you're changing here is just not set up to be set to something high. Activating the completion with ctrl-space will pointlessly wait half a second too.

I guess what you're trying to do is debounce typing-based completion, rather than this number in general. Attached patch might be what you need.

There's going to be a maximum of one request in flight at any time. > The validFor doesn't seem to improve the situation either. It should, when the user types a single identifier, allow the completion to use the result from the request fired off when typing started. > Is there some architectural reason why the denounce time variable shouldn't be exposed? Because the number you're changing here is just not set up to be set to something high. Activating the completion with ctrl-space will pointlessly wait half a second too. I guess what you're trying to do is debounce typing-based completion, rather than this number in general. Attached patch might be what you need.

rheng commented

2024-01-12 20:17:43 +01:00

(Migrated from github.com)

Yes, this is exactly what I need.

Thank you for the clarification and patch!

Yes, this is exactly what I need. Thank you for the clarification and patch!

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Rows
Columns

Adding option to configure debounce time #20

Pull request closed