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

code-completion doesn't replace expected text when cursor is not at the end of name #10266

Open
DanTup opened this issue Aug 7, 2016 · 63 comments

Comments

@DanTup
Copy link
Contributor

@DanTup DanTup commented Aug 7, 2016

  • VSCode Version: 1.4.0
  • OS Version: Windows 10

Steps to Reproduce:

  1. Create a method called methodOne
  2. Write some code that calls this method as methodTwo()
  3. Place the cursor between the d and T and invoke completion
  4. Select methodOne from the completion list

You would expect methodTwo to be replaced with methodOne but you end up with methodOneTwo.

@dbaeumer

This comment has been minimized.

Copy link
Member

@dbaeumer dbaeumer commented Aug 8, 2016

@jrieken is there anything special I need to do to achieve this?

@DanTup

This comment has been minimized.

Copy link
Contributor Author

@DanTup DanTup commented Aug 8, 2016

If it helps, my Dart extension seems to handle this correctly:

https://github.com/DanTup/Dart-Code/blob/master/src/dart_completion_item_provider.ts#L41

You can provide a textEdit property on the CompletionItem with a Range to replace. The Dart language service I'm using already returned the correct length here so it just worked out of the box. I guess in your MyCompletionItem class you may need to set up the textEdit (assuming you have all the necessary info from the service).

@dbaeumer

This comment has been minimized.

Copy link
Member

@dbaeumer dbaeumer commented Aug 8, 2016

I don't :-(. I can reconstruct this using may be model API but I was hoping not being forced to do this and @jrieken knows a better way.

If we need this information to handle cases like this then it should IMO come from the tsserver since he has the full knowledge about the context of the code complete.

@jrieken

This comment has been minimized.

Copy link
Member

@jrieken jrieken commented Aug 8, 2016

When a completion item doesn't specify a text edit we construct one with the word range until the current position. So, you can achieve the behaviour using a different range or we just change our default - tho that makes it sound more like a user option cos I am not sure if overwrite after is always the right thing to do?

@dbaeumer

This comment has been minimized.

Copy link
Member

@dbaeumer dbaeumer commented Aug 8, 2016

Changing the default is not a good thing to do since we will break others. I will see if I can recompute this for now and make a request against the tsserver to better provide this info.

@dbaeumer dbaeumer added this to the August 2016 milestone Aug 8, 2016
@jrieken

This comment has been minimized.

Copy link
Member

@jrieken jrieken commented Aug 8, 2016

You can always ask for the word range at position. That's usually a pretty good guess

@dbaeumer

This comment has been minimized.

Copy link
Member

@dbaeumer dbaeumer commented Aug 8, 2016

Agree.

@dbaeumer

This comment has been minimized.

Copy link
Member

@dbaeumer dbaeumer commented Aug 26, 2016

@jrieken and I discussed that again over lunch and we agreed that we should investigate into a language neutral solution to not have to many language specific settings. Moving to September and adding @jrieken as an owner.

@dbaeumer dbaeumer modified the milestones: September 2016, August 2016 Aug 26, 2016
@jrieken jrieken added the suggest label Aug 26, 2016
@jrieken jrieken added feature-request and removed bug labels Sep 7, 2016
@jrieken jrieken removed this from the September 2016 milestone Sep 20, 2016
@dbaeumer dbaeumer assigned waderyan and unassigned dbaeumer Nov 15, 2016
@jrieken jrieken removed the typescript label Mar 13, 2017
@jrieken jrieken changed the title Typescript code-completion doesn't replace expected text when cursor is not at the end of name code-completion doesn't replace expected text when cursor is not at the end of name Mar 13, 2017
@ramya-rao-a

This comment has been minimized.

Copy link
Member

@ramya-rao-a ramya-rao-a commented Apr 27, 2017

Related to #4878 ?

@jrieken

This comment has been minimized.

Copy link
Member

@jrieken jrieken commented Oct 22, 2019

The default behaviour becomes more complex but also gives extensions the chance to overrule our defaults. My current thinking is the following

  • range is of type object -> use it as given, extension author as adopted this API and made up his mind. We would validate that an insert range is a prefix of a replace range
  • range is undefined -> We would fill the word range for replace and the cropped word range for insert (the latter already happens today)
  • range is of type Range -> use range for insert range and replace. Q: try to use word range defaults when the given range matches word ranges? Q: log a message for extension devs to update?

The configuration value (editor.suggest.overwriteOnAccept) will make us use the insert or replace range when inserting a suggestion and the default will stay insert.

@DanTup

This comment has been minimized.

Copy link
Contributor Author

@DanTup DanTup commented Oct 22, 2019

Q: try to use word range defaults when the given range matches word ranges?

I'm not sure what this means - if the range matches the word range, they would be the same? I would prefer when given a range you use it as-is though. Trying to be smart will likely result in quirks (like found in TS above, and in issues like #63129 - which is await a response FWIW :-)).

Q: log a message for extension devs to update?

I don't know if you usually do that for new features extensions can take advantage of - though it could be useful. If you do do this, it should only write once-per-session or something.. code completion is invoked a lot and whever it goes could be really spammy if it happens every time.

@jrieken

This comment has been minimized.

Copy link
Member

@jrieken jrieken commented Oct 22, 2019

I'm not sure what this means - if the range matches the word range, they would be the same? I would prefer when given a range you use it as-is though.

Very simple sample: with fo|oo default ranges would select fo for insert, and fooo for replace. Now, if an extension provides one them as range we could detect that it's either replace or insert, infer the other range, and assign them correctly. That's btw what the TS issue fix does. I believe that's reasonable and goes away as soon as an extension jumps onto the new API.

@DanTup

This comment has been minimized.

Copy link
Contributor Author

@DanTup DanTup commented Oct 23, 2019

Oh, I see. My concern with that is that it's a breaking change before the extension has opted in to anything. I think it would be better to preserve existing behaviour until the extension opts-in, so they can set the configurationDefault (if they choose) and avoid users getting a change in behaviour, and then having it change back later (since if I understand correctly, there's no way an extension could ship the fix in advance).

@jrieken

This comment has been minimized.

Copy link
Member

@jrieken jrieken commented Oct 23, 2019

(since if I understand correctly, there's no way an extension could ship the fix in advance).

Unfortunately not - tho I think we should only ship this feature/setting when the API has been finalised for vscode.d.ts and for LSP. Anything else would be unfair towards extension authors

@DanTup

This comment has been minimized.

Copy link
Contributor Author

@DanTup DanTup commented Oct 23, 2019

In that case - if you ship the API change first, then change behaviour in a future version so I can ship the fix, I'm not against the above (though it's worth ensuring the other two languages that behave this way are aware they need to do the same).

It would definitely help if extensions configurationDefaults were more obvious at the same time - it was briefly mentioned above. Should I file an issue about it? (I thought there was one, but I can't find it now).

jrieken added a commit that referenced this issue Oct 24, 2019
@TylerLeonhardt

This comment has been minimized.

Copy link
Member

@TylerLeonhardt TylerLeonhardt commented Oct 25, 2019

@jrieken it looks like snippets are not honoring editor.suggest.overwriteOnAccept.

{
        // python snippet
	"listcompPy": {
		"prefix":"x for x in",
		"body":[
			"[x for x in range(1,20) if x%2==0 ]"
		]
	},
        // powershell snippet
	"PSCustomObject": {
        "prefix":"PSCustomObject",
        "body":[
            "[PSCustomObject] @{ Key = Value }"
        ]
    }
}

Is that intentional?

@jrieken

This comment has been minimized.

Copy link
Member

@jrieken jrieken commented Oct 29, 2019

fyi - we are holding back with this feature to give extensions enough time to prepare the adoption of the proposed API. that means for 1.40 we won't have the new setting and commands (see #10266 (comment)) but that we are delaying until 1.41 so that 3rd and also our own extensions have enough time to adopt

jrieken added a commit that referenced this issue Oct 29, 2019
@jrieken jrieken modified the milestones: October 2019, November 2019 Oct 29, 2019
@TylerLeonhardt

This comment has been minimized.

Copy link
Member

@TylerLeonhardt TylerLeonhardt commented Oct 29, 2019

@jrieken is there an action item you're looking from language servers?

@dbaeumer

This comment has been minimized.

Copy link
Member

@dbaeumer dbaeumer commented Oct 30, 2019

@jrieken

This comment has been minimized.

Copy link
Member

@jrieken jrieken commented Nov 1, 2019

@TylerLeonhardt for snippets adoption work is required, see #83834. Since you seem v e r y interested in this, how about doing a PR?

For LSP see #10266 (comment), tho completion items without a range a covered by a reasonable default and only completion items that provide a range need to be adopted

@bmewburn

This comment has been minimized.

Copy link

@bmewburn bmewburn commented Nov 7, 2019

range?: Range | { insert: Range, replace: Range };

I have a case where the inserted text will be different depending on whether insert or replace is desired. When suggesting methodTwo for method|One($param), parentheses and a command to trigger parameter hints may be included in the CompletionItem. If the intention to insert or replace is sent in CompletionContext this would work, but providing multiple ranges would not be useful.

@jrieken

This comment has been minimized.

Copy link
Member

@jrieken jrieken commented Nov 8, 2019

When suggesting methodTwo for method|One($param), parentheses and a command to trigger parameter hints may be included in the CompletionItem.

I don't understand why that yields to a different insert text for insert vs replace cases? We dropped the CompletionContext approach because most editors (and so will VS Code) allow to change between insert and replace when accepting a completion, e.g Enter vs Shift+Enter, and that approach would prohibit this.

@bmewburn

This comment has been minimized.

Copy link

@bmewburn bmewburn commented Nov 8, 2019

Insert methodTwo at method|One($some, $long, $arg, $list) might be newText of methodTwo($0) giving methodTwo($0)One($some, $long, $arg, $list) whereas replace might be
newText of methodTwo to give methodTwo($some, $long, $arg, $list) and preserve the argument list.

Maybe the former case is never desirable, though? As I understand it, to prevent an outcome of methodTwo($0)($some, $long, $arg, $list) I'd always have to drop the parentheses in the newText in such cases.

@jrieken jrieken removed the api-proposal label Nov 11, 2019
NTaylorMullen added a commit to aspnet/AspNetCore-Tooling that referenced this issue Nov 13, 2019
- Looks like VSCode misbehaves in re-execution of completion scenarios. They've been [experimenting](microsoft/vscode#10266) with new completion range APIs and it seems that it translates O#'s non-new range based completion request into a new type of range completion request (`range2`).
- We now fallback to using a `range2` property on completion items if it exists and properly re-map.

aspnet/AspNetCore-ManualTests#52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.