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.

Sign up for GitHub

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

Jump to bottom

Inline completion preselection of intellisense items doesn't work if it includes a closing brace which is in the document. #135440

Closed
vivlimmsft opened this issue Oct 20, 2021 · 7 comments
Closed

Inline completion preselection of intellisense items doesn't work if it includes a closing brace which is in the document. #135440

vivlimmsft opened this issue Oct 20, 2021 · 7 comments
Assignees
@hediet
Labels
author-verification-requested Issues potentially verifiable by issue author feature-request Request for new features or functionality inline-completions insiders-released Patch has been released in VS Code Insiders verified Verification succeeded
Milestone
November 2021

Comments

@vivlimmsft
Copy link

vivlimmsft commented Oct 20, 2021 •
edited

Does this issue occur when all extensions are disabled?: Yes/No (because I would not get any inline completions)

  • VS Code Version: 1.62.0
  • OS Version: win11 22000.258

Steps to Reproduce:

  1. Trigger an inline completion which meets all of the following conditions
  • Inline completion matches an item in the intellisense list
  • Inline completion contains a closing parenthesis
  • The closing parenthesis already exists in the document.

(thanks @lostintangent for providing this repro gif, using this file)
c6766371-8c43-48a4-a660-3bbdc5a9650d

  1. Trigger intellisense (i.e. by ctrl+space). The matching item is not preselected.

I dug into this a bit and found that the ranges that the inline completion item and intellisense item are applying to differ.

At this location:
image
I saw these watch values:
image

the second condition, that the beginning of the text matches, is true. the condition that the range matches is false, because the endColumn is 53 and 54.

image
the 53rd character on this line is the trailing )
image
image

so my guess of why they differ is that the inline completion range overlaps character 53 because the completion includes the closing paren which is already in the document.

@hediet , can the condition used to match inline suggestions to intellisense items be relaxed here, perhaps only requiring the startLineNumbers and startColumns to match?

( cc @juihanamshet1 and @ayim )
@jrieken jrieken assigned hediet and unassigned jrieken Oct 20, 2021
@hediet
Copy link
Member

hediet commented Oct 20, 2021 •
edited

Thanks for the detailed issue and your investigation!

I thought minimizeInlineCompletion would prevent this issue:

vscode/src/vs/editor/contrib/inlineCompletions/inlineCompletionsModel.ts

Line 607 in 3eead37

const commonSuffixLen = commonSuffixLength(remainingValueToReplace, inlineCompletion.text);

But the semicolon that your suggestions appends after the parenthesis prevents this from working.

can the condition used to match inline suggestions to intellisense items be relaxed here, perhaps only requiring the startLineNumbers and startColumns to match?

Hmm, I have to admit that I don't fully understand the consequences of this and why that is the right thing to do (though I see that it would help in this case).

Maybe, for now, VS Code should just check if the suggestion range is a "prefix" of the inline suggestion range, i.e. same start, but the end of the suggestion range must be less than or equal to the end of the inline suggestion range.

Long-term, the diffs should be computed first.

@hediet hediet added feature-request Request for new features or functionality inline-completions labels Oct 20, 2021
@hediet hediet added this to the October 2021 milestone Oct 20, 2021
@vivlimmsft
Copy link
Author

can the condition used to match inline suggestions to intellisense items be relaxed here, perhaps only requiring the startLineNumbers and startColumns to match?

Hmm, I have to admit that I don't fully understand the consequences of this and why that is the right thing to do (though I see that it would help in this case).

I'm not really sure what the consequences of this are, since I'm not familiar with any of the surrounding code- it just seemed like a reasonable solution to propose to open the conversation with.

Maybe, for now, VS Code should just check if the suggestion range is a "prefix" of the inline suggestion range, i.e. same start, but the end of the suggestion range must be less than or equal to the end of the inline suggestion range.

This seems OK to me. There aren't any counterexamples jumping out at me that would break this.

Long-term, the diffs should be computed first.

I'm not sure I understand this - is the idea to compute the diffs of the suggestion & inline suggestions, and then see if the suggestion's diff is somehow a subset of the inline suggestion's diff?

@hediet hediet closed this as completed in 32744a0 Oct 25, 2021
@hediet
Copy link
Member

hediet commented Oct 25, 2021

Please let me know if the fix is valid (should be available in tomorrows insider build)!

@hediet hediet added author-verification-requested Issues potentially verifiable by issue author verification-needed Verification of issue is requested labels Oct 25, 2021
@connor4312 connor4312 added the verified Verification succeeded label Oct 26, 2021
@vivlimmsft
Copy link
Author

@hediet , sorry for the >2 week delay in validating the fix, but I found that it didn't work for me.

I made this change locally which fixed it:
image

I assume the range to test should always be as long as or longer than the prefix that it may start with. Does that make sense?
Should there also be an inversion of rangeToTest.endLineNumber < prefix.endLineNumber?

@hediet hediet modified the milestones: October 2021, November 2021 Nov 12, 2021
@hediet hediet removed verified Verification succeeded verification-needed Verification of issue is requested author-verification-requested Issues potentially verifiable by issue author insiders-released Patch has been released in VS Code Insiders labels Nov 12, 2021
@hediet hediet reopened this Nov 12, 2021
@hediet hediet closed this as completed in 4e8450a Nov 12, 2021
@hediet
Copy link
Member

hediet commented Nov 12, 2021

@vivlimmsft sorry for that. Can you test it again in Mondays insider build?

@hediet hediet added the author-verification-requested Issues potentially verifiable by issue author label Nov 12, 2021
@vivlimmsft
Copy link
Author

@hediet Looks good. Thanks!

@hediet hediet added the verified Verification succeeded label Nov 30, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Dec 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@hediet hediet

Labels
author-verification-requested Issues potentially verifiable by issue author feature-request Request for new features or functionality inline-completions insiders-released Patch has been released in VS Code Insiders verified Verification succeeded
Projects
None yet
Milestone
November 2021
Development

No branches or pull requests
5 participants
@jrieken @connor4312 @hediet @vivlimmsft and others