Skip to content
This repository has been archived by the owner on Jul 11, 2022. It is now read-only.

Do we still need the auto complete fork? #47

Closed
thien-do opened this issue Sep 19, 2020 · 8 comments · Fixed by #50
Closed

Do we still need the auto complete fork? #47

thien-do opened this issue Sep 19, 2020 · 8 comments · Fixed by #50

Comments

@thien-do
Copy link
Contributor

Afaik the fork is created for 2 primary reasons:

Since both issues are addressed by codemirror's autocomplete module, do you think we can go back to the original module yet? Is there some plan for the fork?

@Nexucis
Copy link
Member

Nexucis commented Sep 19, 2020

It requires to be tested and validated we have the same functionality than today.

Note that the autocomplete fork is including a limitation of the autocomplete list. It's a performance issue fixed by @juliusv when you have to display 100k of element. It actually displays only the first 100 or 1000 ( don't remember exactly the limit)
And I don't think we reported this issue to cmn yet.

@thien-do
Copy link
Contributor Author

CM is going to release a new version soon, which I will try to validate the functionality and report here :D hope it helps

Note that the autocomplete fork is including a limitation of the autocomplete list.

I think this should be implemented at the user land. It's difficult for the autocomplete module to decide a good limitation for all use cases. WDYT @juliusv ?

Afaik the above fix is the only difference between the fork and the recently updated original

@juliusv
Copy link
Member

juliusv commented Sep 22, 2020

@dvkndn Yeah, in the general case this should likely be a configurable limit, but when I last looked at the code it seemed cumbersome to pass a user-supplied limit setting all the way down to the code path that applies the limit. Probably is necessary though.

@thien-do
Copy link
Contributor Author

yes it's necessary. If we can't configure the autocomplete module, is it possible to apply this at user land? Like, I don't know, maybe after fetching from the server?

@juliusv
Copy link
Member

juliusv commented Sep 22, 2020

@dvkndn As I understand it, this wouldn't be solvable by the autocomplete sources that we provide because they still need to provide all items for further filtering etc.. We only want to limit the display, not how many items are processed before that.

@thien-do
Copy link
Contributor Author

Oh.. it needs to be handled in the module then. Let me find if there's a related issue on codemirror yet. To be honest it's good to limit the display in general for performance reason anyway :D I think we can suggest this to CM

@juliusv
Copy link
Member

juliusv commented Sep 22, 2020

@dvkndn Yeah for sure, I think it makes sense for any user who has a huge number of underlying items, since it brings down the display time from multiple seconds to something like 100ms.

@juliusv
Copy link
Member

juliusv commented Sep 22, 2020

@dvkndn Btw., I filed codemirror/dev#293.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants