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

Improved Completion #5728

Merged
merged 6 commits into from
Mar 10, 2023
Merged

Conversation

pascalkuthe
Copy link
Member

@pascalkuthe pascalkuthe commented Jan 30, 2023

Fixes #4380
Fixes #5469

This PR improves the completions in helix in multiple subtle ways. These fixes were lumped together as they all interact.

The core change of this PR is a fix for #4380: Adding an option so that helix completions only insert text and don't replace the rest of the word (using item.insert instead of item.replace). I believe this to be the better default, so I changed the default behavior to use item.insert. This not only feels more appropriate for an insert mode action in a modal editor, but I feel that the replace mode has some issues that don't make it as suitable for a default setting (see below).

While working on this I noticed that in cases where a LSP does not supply a TextEdit, and we have to determine the range based on word boundaries ourself helix already behaves this way: We only insert the sent text but don't replace the rest of the word. I changed that behavior there to respect the new setting. Here is another reason why I think insert completions are a better default: There is a third option for sending completions (which is commonly used) where the LS does send a TextEdit but only a single edit (not two separate replace/insert edits). These edits are always insert edits so for many LSPs helix is already inserting by default.

To avoid conflicts with the change above, I also pulled in #5487 (commit remains to credit @Taywee). I discovered that #5487 doesn't respect multi cursors (so the edit was only applied to a single cursor), so I fixed that regression too.

In the process I noticed that completions with multiple cursors can cause overlapping ranges which can cause panics (always a panic in debug mode thanks to debug assertions). For example in helix in document.rs you can select History in the use statement type s[iy]<ret> start a completion and as soon as History is selected the editor instantly crashes in debug mode.I fixed the problem by adding a variant of Selection::change that ignores overlapping edits. This problem rarely occurs in practice and there are some odd edge cases that are not handled 100% right now (the cursor can be moved by the applied transactions which means the preview only applies the edit at once cursor but on hitting enter the cursor has moved and the edit is applied a both cursors) but I doubt people actually run into this problem as there was no bug report about the crash I could find. For now just fixing the crash seems good enough to me. This Problem is also more likely to occur in replace mode (as the rest of the word could contain another cursor) so yet another point for that.

@pascalkuthe pascalkuthe added C-bug Category: This is a bug C-enhancement Category: Improvements A-language-server Area: Language server client E-medium Call for participation: Experience needed to fix: Medium / intermediate S-waiting-on-review Status: Awaiting review from a maintainer. labels Jan 30, 2023
Comment on lines 483 to 490
if last > from {
continue;
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the only change from the Transaction::change function

@kirawi
Copy link
Member

kirawi commented Jan 30, 2023

I don't think I understand the distinction between replace and insert for text edits. If, for example, the text is Apple, but the completion is SomeAppleTree, how would that be possible with a single insert?

@pascalkuthe
Copy link
Member Author

pascalkuthe commented Jan 30, 2023

I don't think I understand the distinction between replace and insert for text edits. If, for example, the text is Apple, but the completion is SomeAppleTree, how would that be possible with a single insert?

The distinction is where the cursor is.
For example when editing Rope|Graphemes you start typing Build so the text is RopeBuild|Grapemes and it's auto completed to RopeBuilder with replace it replaces the rest of the word (or logicl identifier really. The whole point if having an LSP edit is that these replaces do not always start/end at word boundaries). So in replace mode the new text would be RopeBuilder and in insert mode RopeBuilderGraphemes.

You can look at #4380 and #5676 for more examples.

Note that the text before the cursor is always replaced (which is wahtI think uku were confused about). The difference is just whether text after the cursor.is also replaced

@pascalkuthe pascalkuthe force-pushed the autocomplete_config branch 2 times, most recently from 7823dd3 to 0ac9c02 Compare January 31, 2023 17:34
@StratilJakub
Copy link

I've been trying this branch for last hour or so, since autocompletion issues are quite frequent in large codebase (tried multiple idle-timeout values from 0 to 400).

This is helping with most of the issues resulting in wrong autocompletion, so thanks a lot for your work!

Unfortunately, issue mentioned in #1819 is still present in this branch -> Sometimes there is additional character(s) after confirming completion.

@pascalkuthe
Copy link
Member Author

I've been trying this branch for last hour or so, since autocompletion issues are quite frequent in large codebase (tried multiple idle-timeout values from 0 to 400).

This is helping with most of the issues resulting in wrong autocompletion, so thanks a lot for your work!

Unfortunately, issue mentioned in #1819 is still present in this branch -> Sometimes there is additional character(s) after confirming completion.

Thanks for testing. This PR is not intended to fix the issues from #1819 but rather about other issues with completions. The synchronization bugs that #1819 addresses are very subtle and best addressed in a separate PR. I want to also take a look if I can either exactly understand #1819 and give it a thorough review or take that PR over so we can finally get those bugs fixed too. But any such work would be separate from this PR

@StratilJakub
Copy link

StratilJakub commented Feb 1, 2023

Understand and I didn't necessarily expect this to fix everything, just wanted to report my experience and if maybe it helped with that also as a side effect. :)

It's still a big improvement and the autocompletion works predictably now, thanks again.

@archseer
Copy link
Member

archseer commented Mar 8, 2023

With #5864 merged this can now be rebased

@pascalkuthe pascalkuthe added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-review Status: Awaiting review from a maintainer. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 8, 2023
@pascalkuthe
Copy link
Member Author

This has been rebased on #5864. I applied the fixes for the crashes described in the initial PR to the snippet transaction generation. In the process I have fixed multiple bugs with the way tabstops are handled.

In particular this PR now uses a partial ChangeSet (temporarily created by temporarily append a retain to the in progress ChangeSet) to map the start of the replacement range to ensure tabstops always endup in the same place (otherwise the transaction may merge the replacement and the tabstops are merged incorrectly).

This partial ChangeSet can also be applied to the doc if a snippet needs information about it's position in the document (which can not be determined without applying the ChangeSet right now). Right now this is used to determine the indentation level of a line but will also be important to correctly calculate the values of variables. This has some overhead which I want to avoid in the future (by essentially applying the transaction eagerly to the rope, storing the result in the transaction which then gets used in apply instead of recalculating) but that will introduce some complexity and the performance difference probably rarely matters in practice so I decided to leave that to future work.

Finally this PR brings tabstops inline with how selections work in helix in general. This is best demonstrated with an example.

Consider the following snippet:

fn ${1}(#{2}){
  ${0}
}

Now consider the following two selections (text between the two | is selected)

foo fn| bar|
|foo fn| bar

these two cases represent insert mode (i) and append mode (a).

if the snippet is applied to the two cases right now we endup with the following in both cases (just a point cursor):

foo fn |(|){
  
} bar

with this PR it endsup as:

|foo fn |(){
  
} bar

and

foo fn| (){
  
} bar|

helix-core/src/transaction.rs Outdated Show resolved Hide resolved
helix-lsp/src/lib.rs Outdated Show resolved Hide resolved
helix-core/src/transaction.rs Outdated Show resolved Hide resolved
helix-lsp/src/lib.rs Outdated Show resolved Hide resolved
helix-lsp/src/lib.rs Outdated Show resolved Hide resolved
helix-lsp/src/lib.rs Outdated Show resolved Hide resolved
…5469)

Most LSPs will complete case-insensitive matches, particularly from
lowercase to uppercase.  In some cases, notably Pyright, this is given
as a simple insert text instead of TextEdit.  When this happens, the
prefix text was left unedited.
@pascalkuthe pascalkuthe added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Mar 9, 2023
@pascalkuthe pascalkuthe added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 9, 2023
This commit adds new functions to `Transaction` that allow creating
edits that might potentially overlap. Any change that overlaps
previous changes is ignored. Furthermore, a utility method is added
that also drops selections associated with dropped changes (for
transactions that are created from a selection).

This is needed to avoid crashes when applying multicursor
autocompletions, as the edit from a previous cursor may overlap
with the next cursor/edit.
Multicursor completions may overlap and therefore overlapping
completions must be dropped to avoid crashes. Furthermore, multicursor
edits might simply be out of range if the word before/after the cursor
is shorter. This currently leads to crashes, instead these selections
are now also removed for completions.

This commit also significantly refactors snippet transaction generation
so that tabstops behave correctly with the above rules. Furthermore,
snippet tabstops need to be carefully mapped to ensure their position
is correct and consistent with our selection semantics. Finally,
we now keep a partially updated Rope while creating snippet
transactions so that we can fill information into snippets that
depends on the position in the document.
@archseer archseer merged commit d63e570 into helix-editor:master Mar 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-language-server Area: Language server client C-bug Category: This is a bug C-enhancement Category: Improvements E-medium Call for participation: Experience needed to fix: Medium / intermediate S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
7 participants