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

Completion by words in all loaded documents #3328

Closed

Conversation

estin
Copy link
Contributor

@estin estin commented Aug 4, 2022

Hi!
Draft: completion by words in all loaded documents

Works for document

  • with language server support (merge lsp result and finded words)
  • and without it

May be such feature must be implemented via plugins in future?

Снимок экрана в 2022-08-04 15-07-02

@estin estin marked this pull request as draft August 4, 2022 12:09
@the-mikedavis
Copy link
Member

See #2403

@estin
Copy link
Contributor Author

estin commented Aug 4, 2022

Current solution gather completion options from all loaded documents/buffers and merge it with language server response.

Very often langauge server don't give me suggestions on sophisticated python code (too many magic and meta programming).
And don't works on html templates and so on.

I'm create this PR as draft, because know it not so optimal yet. But it works well for my cases.

For my happiness I need on helix only ths things:

Sorry for my poor English and not usefull noise here

@archseer
Copy link
Member

archseer commented Aug 4, 2022

#2403 has a better implementation but I'm still concerned about performance.

@estin
Copy link
Contributor Author

estin commented Aug 5, 2022

New approach. Draft not fully implemented yet.
Introduced worker.rs - separate thread to extract and filter words for completion (nothing special or new).

workflow

  • on editor initialization starts Worker
  • on first document load, it's content sended to Worker
  • on text typing collects prefix and send it to Worker
  • for language server merge items for completion

todo:

  • on changes send it content to Worker to extract words (only new inserted/modified text).
  • associate word with document id (for cleanup)
  • find more optimal ways to extract words and filter (may by use rayon and text indixing)

Please, check this out.
Works without any lags on 10mb text file and small rust project loaded in buffers
If it right way then I will try to make this PR more suitable

@estin
Copy link
Contributor Author

estin commented Aug 15, 2022

any way i'm continued to implementing this feature and now it works like that

sequence diagram

Words associated with doc and doc-line on two separate hashmaps

  • Doc-Words hashmap need to store initial doc's words, populated on load/save document
  • Doc-Line-Words not accurate hashmap to store changed lines, populated on text changed and reseted on load/save docuemnt.

The Doc-Line-Words allow to do not accurate syncing with changed and unsaved text. It can contain duplicated data especially on lines deletion. But it's allow without any lags sends changed lines to the worker to update completion dictionary state.

PR is not perfect and may be improved by this tasks

  • use BTreeSet instead of HashSet and try improve search speed by prefix (currently on HashSet works without lags)
  • use oneshot signals pool
  • don't send doc text to the worker on :wq
  • implement integration tests with language server
  • get timeout value and completion items from config
  • abstract completion item to separate option from language server and worker, it's allow implement ordering (ls items first) and some visualization

imho PR ready to review

@estin estin marked this pull request as ready for review August 15, 2022 07:26
@IvanGoncharov
Copy link

@estin Thanks a lot for this PR 🙇
For me, it is impossible to use helix as an editor for git commit messages without this feature (no completion from commit diff).
I running this PR locally, and it works as a charm 👍

@3h15
Copy link

3h15 commented Aug 15, 2022

@estin Thanks for this PR. Same problem here with elixir.

@estin estin force-pushed the completion-by-words-without-lsp branch from cc4fdd5 to 4138ab2 Compare August 16, 2022 04:42
@IvanGoncharov
Copy link

IvanGoncharov commented Aug 16, 2022

@estin Not sure if it is problem in your PR specifically, but I notice a strange behavior with the . operation, and I can reproduce it on master with LSP completion.
Steps:

  1. change some text using c
  2. change another selection with .
  3. completion menu is stuck on a screen.

See recording here: https://asciinema.org/a/zeenKdKViSoTQiYrodanWIhez

@AceofSpades5757
Copy link
Contributor

@estin Not sure if it is problem in your PR specifically, but I notice a strange behavior with the . operation, and I can reproduce it on master with LSP completion. Steps:

  1. change some text using c
  2. change another selection with .
  3. completion menu is stuck on a screen.

See recording here: https://asciinema.org/a/zeenKdKViSoTQiYrodanWIhez

The text getting stuck happens in other circumstances too. ctrl+c will fix it. I think there might have been an issue tracking it.

@estin
Copy link
Contributor Author

estin commented Aug 17, 2022

@IvanGoncharov I hope completion menu is stuck now it fixed (see e3b9ed1 )
You case of menu stuck was PR's bug

@AceofSpades5757 Can you show me another completion menu stuck?
Currently on master at 2968756 I often saw this bug

  • python
  • insert class name Event() and wait on it
  • opens popup with class constructor
  • change cursor position to another line or switch buffer will't close it
  • and as you say Ctrl-c will close it

Seems some bug on master, but I don't investigate time to research/solve it

Снимок экрана в 2022-08-17 10-59-10

@IvanGoncharov
Copy link

I hope completion menu is stuck now it fixed (see e3b9ed1 )
You case of menu stuck was PR's bug

@estin Thanks, I can confirm it's fixed 😄

I also found another one, your completion doesn't honor completion-trigger-len in case I use default (which is 2), but now it triggers after the first key.
Note: this sims to happen only on files without LSP completion (e.g. toml).

@estin
Copy link
Contributor Author

estin commented Aug 17, 2022

@estin Thanks, I can confirm it's fixed

nice!

I also found another one, your completion doesn't honor completion-trigger-len in case I use default (which is 2), but now it triggers after the first key. Note: this sims to happen only on files without LSP completion (e.g. toml).

It's was planned like that.

There are two modes:

  • with LSP: completion triggers as usual with honor for all current settings, and words completions would be merged to LSP result (two concurrent async requests to LSP and worker and then merge results)
  • no LSP: on each char typed collects prefix and send request to the worker. It's very useful for me and works without any lags on huge files (tested on ~245Mb file json-line log)

P.S. I don't believe about that PR would be merge to upstream in near future. But it's allow me use helix as my primary editor, and I will sync it with master periodically for my personal usage )

@IvanGoncharov
Copy link

@estin Thanks for the explanation.
In case the situation changes and this PR merges to master I think it should honor completion-trigger-len completion on the first char creates unexpected visual noise for me.

For now, I'm totally happy with PR, and big thanks for keeping it up to date 👍

@estin estin force-pushed the completion-by-words-without-lsp branch from e3b9ed1 to f80cd92 Compare August 18, 2022 14:02
@IvanGoncharov
Copy link

Thanks @estin 👍
With completion-trigger-len it works like a charm!

@estin estin force-pushed the completion-by-words-without-lsp branch 2 times, most recently from c71819e to 255c406 Compare September 4, 2022 17:48
@estin estin force-pushed the completion-by-words-without-lsp branch 2 times, most recently from cf5e8f5 to 1490fb5 Compare September 13, 2022 07:31
@kirawi kirawi added A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer. labels Sep 13, 2022
@estin estin force-pushed the completion-by-words-without-lsp branch from 1490fb5 to b1c0d96 Compare September 24, 2022 08:19
@estin estin force-pushed the completion-by-words-without-lsp branch from 57d69c3 to 5124897 Compare October 1, 2022 15:10
@estin estin force-pushed the completion-by-words-without-lsp branch from 5124897 to 048d5cd Compare October 13, 2022 09:41
@estin estin force-pushed the completion-by-words-without-lsp branch from 048d5cd to cea3076 Compare October 29, 2022 21:00
@archseer archseer changed the title Completion with/without lsp Completion by words in all loaded documents Nov 22, 2022
@gibbz00 gibbz00 mentioned this pull request Feb 22, 2023
5 tasks
@estin estin mentioned this pull request Feb 27, 2023
@IvanGoncharov
Copy link

@estin Big thanks for keeping this PR up to date for almost a year 🙇
I use it daily, saving me a ton of time writing git commits messages.
When writing a detailed description of my changes, I don't need to waste time and manually copy-paste identifiers from diff.

@danillos
Copy link
Contributor

@estin great work. I can't see I'm using Helix without it anymore.

@estin
Copy link
Contributor Author

estin commented May 23, 2023

Currently this PR is HACK, and marked as draft again.
It works.
(integration test would be failed - language server required)

  • completion item required language_server_id
  • to enable words completion document must has at least one language server
  • to enable language server document must has url (file path)

To resolve this changes currently introduced in languages.toml

  • stub languages server with command: rust-analyzer
  • stub language with stub ls

May be this task must be solved in language server terms #6731

To enable words comletion on document without language server

  1. Configure languages.toml
[language-server.stub]
name = "stub"
command = "<binary of any available language server>" # by default rust-analyzer
  1. Set language for document (documen must has filepath)
:set-language stub

Future plans on this PR

  • find simple way to implement embeded stub language server
  • or allow completions without language server at all
  • or waiting before core team resolve this task )

@pascalkuthe
Copy link
Member

This feature should be possible to implement efficiently by running aho-corsik on the current buffer (as an async completion task, the infrastructure is already provided by the multi lsp PR).

aho-corsic already implements a streaming matcher (so works efficiently with ropey) and an (ASCII) ignore case option. Simply collecting all matches into a HashSet and expanding them to entire words should work quite well.

For performance reasons I dont think we want to run completions for every buffer tough. Instead the completion should simply scan the current buffer which is what you want in most cases anyway.

@estin
Copy link
Contributor Author

estin commented Jun 14, 2023

Multi lsp feature allow to use external language server for completion.
No longer needed to continue maintain this hacky PR.

For my use cases works fine with https://github.com/estin/simple-completion-language-server

By #3328 (comment) I will try implement naive not optimized language server

  • incremental doc updates (use it's own ropey::Rope for each opened doc)
  • on each completion request applied aho_corasick searcher on each opened doc as stream

simple-completion-language-server

@estin estin closed this Jun 14, 2023
@danillos
Copy link
Contributor

Nice work @estin and thank you for work on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants