fix(autocomplete): apply the replacement range of matches given to the popup - #6003
Open
boogie wants to merge 1 commit into
Open
fix(autocomplete): apply the replacement range of matches given to the popup#6003boogie wants to merge 1 commit into
boogie wants to merge 1 commit into
Conversation
…e popup Opening the popup with a prepared list of matches, the way `startAutocomplete` does when it is given a `matches` option, builds the completion provider without an initial position. Inserting a completion that carries a `range` then reads `prefix` off that missing position and throws a TypeError, so the completion never makes it into the document. Give the provider the position the popup was opened at, taken from the cursor rather than from the start of the selection: ranges are applied as offsets around that reference point, and the regular path uses the cursor, so anchoring on the start of a non-empty selection would widen the replacement by the length of the selection. Guard the range handling as well, since the provider is exported and can be constructed without an initial position.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Inserting a completion that carries a
rangethrows when the popup was opened with a prepared list of matches:The
options.matchesbranch ofupdateCompletionsbuilds theCompletionProviderwithout an initial position, andinsertMatchreadsthis.initialPosition.prefixwhen converting a completion'srangeinto the span to replace. The completion never makes it into the document.rangeis part of the completion API and is what language server integrations use to honour atextEdit, so this path is reachable for anyone feeding Ace prepared matches.Change
Give the provider the position the popup was opened at.
The position is taken from the cursor rather than from the start of the selection. Ranges are applied as offsets around that reference point, and the regular completion path uses the cursor, so anchoring on the start of a non-empty selection would widen the replacement by the length of the selection. With the fix both paths agree:
The range handling is also guarded on the initial position, since
CompletionProvideris exported and can be constructed directly.Out of scope, unchanged by this PR: ranges spanning more than one row are still ignored by the surrounding condition, on this path as on any other.
Test
test: replacement range of matches passed to execCommand is appliedcovers a range that extends on both sides of the cursor, and a non-empty selection that must not widen the replacement. Without the change the first case throws; anchoring on the selection start instead of the cursor fails the second with'xyz' == 'xyzGHI'.Open kitchen-sink @ a3e40f4ae27a7f0d62c253a4a0be8a2430a12be1