Components: Ignore user completers when reaching >= 4 words after trigger char #30589
Conversation
fullofcaffeine
requested review from
ajitbohra and
chrisvanpatten
as code owners
| @@ -48,6 +48,16 @@ export default { | |||
| </span>, | |||
| ]; | |||
| }, | |||
| allowContext( before, after, textWithoutTrigger ) { | |||
There was a problem hiding this comment.
I took the freedom to experiment a bit by turning the allowContext callback into a more general-purpose "filter" for the completer, by also allowing the textWithoutTrigger to be passed to it as the last parameter. The previous version of this changeset was clunkier as it included the logic below in the Autocomplete component, and it'd run for all completers, which was not ideal.
I'm adding to the method signature, so I'm not sure how safe this change is. I'd bet it is relatively safe. Also, you could argue that the name allowContext still somewhat fits semantically in this context. Let me know if you think a new function would be better, or if I should fall back to 1949a4a.
There was a problem hiding this comment.
See #30589 (comment) where I left some more thought. I think we need something general like the maximum number of chars that gets processed and a way to fine-tune for individual completers like you propose 👍🏻
There was a problem hiding this comment.
TBH I don't love the idea of bending allowContext to suit our needs here 😛
OTOH, I also don't love the idea of a customizable max number of chars for individual completers -- I don't think that reflects the behavior of completion as I'd imagine it.
I'll leave a top-level comment with some more discussion.
fullofcaffeine
changed the title
|
Size Change: +35 B (0%) Total Size: 1.42 MB
ℹ️ View Unchanged
|
…ccount the text and not only the surrounding context
fullofcaffeine
force-pushed
the
fix/infinite-match-for-inline-completers
branch
from
1cde370 to
2016c4f
Compare
My understanding was that you use Other than that, I see the proposed change as an interesting way to give more control over completers so we should explore that.
That's an interesting discovery. Should we make a default limit of characters to match? Or maybe better, can we count the number of characters that you typed vs the last match and stop the autocompletion at some point? E.g. the last match is |
gziolo
added
[Package] Components
|
I'm glad we're looking at this, because I believe there are more issues of state hiding under the surface. In both screencasts below, the dev tools are focused on
Gravacao.do.ecra.2021-04-08.as.11.07.41.mov
Gravacao.do.ecra.2021-04-08.as.11.10.04.movConclusions:
|
I was on a 1- or 2-day old |
TBH I'd rather stop autocompletion after the first mismatch: Up until that point, the user will see the completion suggestions box, but if they keep typing and type something for which there are no suggestions, it makes sense to end auto-completion, IMO. That would also resolve the first item that @mcsf brought up in his comment. I'm assuming that the main point for stopping autocompletion later -- e.g. after n words, or m characters -- would be to allow for a user to make a typo, notice it, and fix it (by pressing backspace), and have the completion suggestions box available again. While I do want this sort of behavior, I don't think that we should implement it through a customizable/per-completer/quantitative parameter (as that doesn't capture the essence of the problem IMO, and will lead to fairly arbitrary values of that parameter for individual completers, I think). Instead, maybe we can re-start the autocompleter once a user presses backspace. We could maybe implement this in two steps (so it doesn't grow too complex):
|
@ockham, sounds like a good path to explore. The only question is how much work it is going to be to retrigger autocompletion when pressing the backspace. |
I've found a simpler way to avoid triggering the match/completion upon a mismatch, but still unsure how to handle the retriggering of the autocomplete. I have two WIP experiments:
|
Description
Attempt at fixing #30640.
Fixes an issue introduced in #29939, where the
userautocompleter enters an infinite matching loop, causing the editor to eventually slow down and become close to unusable.The changes in #29939 are somewhat incompatible with the UX for the
userautocomplete as its triggers can happen anywhere and stay on the editor even after the completion is done. Once the autocomplete finds the trigger, it runs through its matching/querying algorithm, but then doesn't know when to stop. What happens in practice is that the text after the trigger starts being considered as part of the text to match and it goes up to infinity.The fix limits the number of words that could be matched for the
usercompleter to 3, and bails out by on the 4th onwards. This would allow a user name with 3 words to be matched and then wouldn't try to match anymore. This is not perfect as sometimes the match will still be triggered when it shouldn't (i.e when the user already found the user and confirmed the choice), and the popup with the matched username might still be shown even after the name is found and output on the page if within the 3 words limit, but should be good enough to prevent the deadlock/slowdown.How has this been tested?
The autocomplete search results should appear every time the search trigger is typed, i.e. the user search via @. If you continue typing after the trigger, it should stop showing the popup (if any matches) after you type 3 words. If you continue typing, you should not notice any slowdowns and the editor should be snappy.
Type of change
Bugfix.