Skip to content

Conversation

@jcouv
Copy link
Member

@jcouv jcouv commented Jan 28, 2018

Customer scenario

Type Class[] x = new Class[]. You expect the Class on the right-hand-side to be completed. But actually, there was a suggestion mode for implicit array creation that prevented such completion.
I don't think such a suggestion mode makes sense. Suggestion modes prevent completion when the user might type a never-seen word, but that is not the case here. So I removed it.

Bugs this fixes

Fixes #24432

Workarounds, if any

You can always fight against the completion, but it's annoying.

Risk

Performance impact

Is this a regression from a previous update?

No

Root cause analysis

How was the bug found?

Reported by customer

FYI @CyrusNajmabadi @sharwell

@jcouv jcouv added the Area-IDE label Jan 28, 2018
@jcouv jcouv added this to the 15.7 milestone Jan 28, 2018
@jcouv jcouv self-assigned this Jan 28, 2018
@jcouv jcouv requested a review from a team as a code owner January 28, 2018 06:00
@CyrusNajmabadi
Copy link
Member

the issue here is that the user may type Foo[] f = new [. So i'm fine with any changes here as long as the above can be written out without intellisense completion interfering and causing you to write: ```Foo[] f = new Foo[".

@CyrusNajmabadi
Copy link
Member

Suggestion modes prevent completion when the user might type a never-seen word, but that is not the case here

I agree we probably don't need suggestion mode. But we do need to ensure that the items is not hard selected.

@jcouv
Copy link
Member Author

jcouv commented Jan 28, 2018

From my understanding, if the item were soft-selected, then the common scenario would become less convenient (user must use arrow to select).
I'm afraid this would also affect the object creation scenario (not just array creation), which is very common and should be preserved.

One potential way out would be to keep hard-selection for object creation, but use soft-selection when inferred type is an array. I'll try that.

@CyrusNajmabadi
Copy link
Member

From my understanding, if the item were soft-selected, then the common scenario would become less convenient (user must use arrow to select).

Yes. That's intentional though. Otherwise the other normal scenario (implicit arrays) becomes painful. implicit arrays are totally reasonable and normal to type. It was intentional to make it so that we don't aggressively complete arrays and annoy the user when they type this.

@sharwell sharwell self-requested a review January 28, 2018 21:12
@sharwell
Copy link
Contributor

sharwell commented Jan 28, 2018

Here are a few cases I'll be looking to verify:

  • Typing Foo[] data = new $$ should result in Foo being soft-selected ($$ is used as a position marker, not literal characters)
  • Typing Foo[] data = new F$$ should result in Foo being hard-selected
  • Typing Foo[] data = n should result in new being hard-selected
  • Typing Foo[] data = n[ should produce Foo[] data = new[
  • Typing Foo[] data = n [ should produce Foo[] data = new [
  • Typing Foo[] data = new [ should produce Foo[] data = new [
  • Pressing Ctrl+Space in Foo[] data = new $$ should hard select Foo

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Jan 28, 2018

Pressing Ctrl+Space in Foo[] data = new $$ should hard select Foo

Interesting! Does ctrl-space change soft-selection to hard selection?

Another test case would be:

Foo[] data = new<space><tab>

Should make "new Foo"

@sharwell
Copy link
Contributor

sharwell commented Jan 28, 2018

Interesting! Does ctrl-space change soft-selection to hard selection?

The Edit.CompleteWord command is allowed to always assume that the user is attempting to insert a reference to something that already exists.

Another test case would be:

Foo[] data = new<space><tab>

This case is covered by my first typing case, assuming that pressing Tab completes a soft-selected item.

@jcouv
Copy link
Member Author

jcouv commented Jan 28, 2018

@sharwell I added some tests cases as you suggested, although I think they relate more to completion on "new" which is a different provider.
I tested Tab, but wasn't sure how to test Ctrl+Space.

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Jan 28, 2018

I think ctrl-space is "complete word" right? If so, you can use testState.SendCommitUniqueCompletionListItem

@CyrusNajmabadi
Copy link
Member

Note: i don't see anything in the complete-word codepath that would cause soft-selection to become hard selection. This is functionality i think we'd have to add.

@jcouv
Copy link
Member Author

jcouv commented Jan 29, 2018

@sharwell ctrl+space sounds like a separate issue.
OK with current change?

@sharwell
Copy link
Contributor

sharwell commented Jan 29, 2018

@jcouv It looks good. Do we have any tests yet for this syntax? I expect they will all pass but it'd still be good to have them.

int[] = { 0 };

Copy link
Contributor

@sharwell sharwell left a comment

Choose a reason for hiding this comment

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

💡 Consider adding tests for the array initializer syntax if there aren't any already.

@jcouv
Copy link
Member Author

jcouv commented Jan 29, 2018

@Pilchie I'm not sure the current ask-mode process for 15.7. Please approve if ask-mode in effect. Thanks

@jcouv
Copy link
Member Author

jcouv commented Jan 29, 2018

Checked with Jared. No ask-mode approval required for this branch yet. I'll go ahead and merge. Thanks

@jcouv jcouv merged commit 450cfb5 into dotnet:dev15.7.x Jan 29, 2018
@jcouv jcouv deleted the array-completion branch January 29, 2018 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants