Switch to a more appropriate model for supporting prefix/suffix text in completoin items. #26764

CyrusNajmabadi · 2018-05-10T19:27:58Z

Note: one nice thing about this model is that it actually cleans up a bunch of stuff in our code. For example, all the places we used to have to jam on special suffixes (like <>) which we then had to special case later.

CyrusNajmabadi · 2018-05-10T19:31:19Z

src/Features/Core/Portable/PublicAPI.Unshipped.txt

@@ -1 +1,7 @@
-
+*REMOVED*static Microsoft.CodeAnalysis.Completion.CompletionItem.Create(string displayText, string filterText = null, string sortText = null, System.Collections.Immutable.ImmutableDictionary<string, string> properties = null, System.Collections.Immutable.ImmutableArray<string> tags = default(System.Collections.Immutable.ImmutableArray<string>), Microsoft.CodeAnalysis.Completion.CompletionItemRules rules = null) -> Microsoft.CodeAnalysis.Completion.CompletionItem


intentional. The existing factory method was replaced with one with non-optional params (for binary compat). A new factory method was added that allows for the new pieces of data to be passed along. This is our pattern for adding new data to types without breaking binary compat.

CyrusNajmabadi · 2018-05-10T19:33:38Z

src/Features/Core/Portable/Completion/CompletionItem.cs

+        /// Pattern-matching of user input will not be performed against this, but only against <see
+        /// cref="DisplayText"/>.
+        /// </summary>
+        public string DisplayTextPrefix { get; }


these are new public APIs that will have to be approved.

…in completion items.

CyrusNajmabadi · 2018-05-11T18:31:10Z

@dotnet/roslyn-ide for review.
@jinujoseph for buddy (probably @dpoeschl)

CyrusNajmabadi · 2018-05-11T18:32:15Z

Note: our test infrastructure here is pretty bad (likely due to all the completoin rewrites, without really investing in moving the test harness forward as well). Fortunately, we do have a ton of coverage. So there is good confidence this is all working properly.

CyrusNajmabadi · 2018-05-11T18:34:23Z

src/EditorFeatures/TestUtilities/Completion/AbstractCompletionProviderTests.cs

@@ -413,7 +420,7 @@ protected virtual void SetWorkspaceOptions(TestWorkspace workspace)
                await VerifyProviderCommitCheckResultsAsync(document2, position, itemToCommit, expectedCodeAfterCommit, commitChar, textTypedSoFar);
            }
        }
-
+        
        private async Task VerifyProviderCommitCheckResultsAsync(
            Document document, int position, string itemToCommit, string expectedCodeAfterCommit, char? commitCharOpt, string textTypedSoFar)


this entire test method is effectively bogus. It's "attempting" to validate that post commit, the file will look like what is expected. However, it's not going through the actual controller commit codepath. Instead, it's just simulating what it thinks would happen. This entire test method should be removed, and all tests that depend on it should become CompletoinCommandHandler tests.

CyrusNajmabadi · 2018-05-30T23:03:38Z

@dpoeschl @dotnet/roslyn-ide @sharwell Can this be looked at?

Note: it's been almost 3 weeks without any correspondence from the team/buddy.

…fixSuffix

CyrusNajmabadi · 2018-06-08T11:48:16Z

@ivanbasov Can this be looked at? It's been a month completely stalled :-/

CyrusNajmabadi · 2018-06-13T18:27:01Z

@dpoeschl @dotnet/roslyn-ide @sharwell @ivanbasov Can this be looked at? It's been a month completely stalled :-/

jinujoseph · 2018-06-15T01:39:32Z

@CyrusNajmabadi , would like to take this as a follow-up after #25770

…fixSuffix

CyrusNajmabadi · 2018-10-16T20:41:27Z

@jinujoseph Honestly, i think this PR shoudl go in now (once reviewed). Based on the approach taken in the async-completion branch, i actually think it's more critical that we have sufficient designs/testing here to ensure that the async-completion work is actually suitable and capable of doing all the things we need, without changing behavior or otherwise regressing anything.

This PR puts in a proper design for how we handle things like "intellicode stars" while also removing technical debt and bugs incurred with the original approach here. I think it would be sensible to get this in sooner rather than later. Thanks!

CyrusNajmabadi · 2018-10-16T20:44:10Z

...EditorFeatures/Core/Implementation/IntelliSense/Completion/Controller.Session_FilterModel.cs

@@ -498,7 +498,7 @@ private bool IsBetterDeletionMatch(FilterResult result1, FilterResult result2)

            private static int GetRecentItemIndex(ImmutableArray<string> recentItems, CompletionItem item)
            {
-                var index = recentItems.IndexOf(CompletionHelper.GetDisplayTextForMatching(item));
+                var index = recentItems.IndexOf(item.DisplayText);


note: this is super weird. effectively, this means the intellicode 'star' prefix now becomes part of the MRU... That doesn't seem to make any sense whatsoever.

CyrusNajmabadi · 2018-10-16T20:46:54Z

@dpoeschl @ivanbasov Please take a look and let me know your thoughts here. If you want, i can help merge this into your feature branch.

ivanbasov · 2018-10-18T18:43:56Z

src/EditorFeatures/TestUtilities2/Intellisense/ITestState.vb

        Function AssertSelectedCompletionItem(
                               Optional displayText As String = Nothing,
                               Optional description As String = Nothing,
                               Optional isSoftSelected As Boolean? = Nothing,
                               Optional isHardSelected As Boolean? = Nothing,
+                               Optional displayTextSuffix As String = Nothing,


Optional displayTextSuffix As String = Nothing, [](start = 31, length = 47)

put it closer in order to displayText?

that woudl require updating a huge amount of tests taht currently do not pass along displayTextSuffix, but do pass along display/description. i'd prefer to not do that :)

ivanbasov · 2018-10-18T18:45:40Z

src/Features/Core/Portable/Completion/CommonCompletionProvider.cs

@@ -104,6 +104,7 @@ protected CompletionItem CreateSuggestionModeItem(string displayText, string des
        {
            return CommonCompletionItem.Create(
                displayText: displayText ?? string.Empty,
+                displayTextSuffix: "",


"" [](start = 35, length = 2)

nit: string.Empty

ivanbasov · 2018-10-18T18:49:20Z

src/Features/Core/Portable/Completion/CompletionItem.cs

+            string sortText,
+            ImmutableDictionary<string, string> properties,
+            ImmutableArray<string> tags,
+            CompletionItemRules rules)


should we care about consistency of order of parameters for the CompletionItem and CommonCompleitonItem?

Right now they're completely inconsistent. CompletionItem needs to work this way because it's a public API. CommonCompletionItem is basically a grab bag of whatever we needed in whatever order we wanted over time. We could reorder it if we wanted, but it didn't seem necessary.

ivanbasov

🕐

ivanbasov

…fixSuffix

CyrusNajmabadi · 2018-10-19T02:45:13Z

@ivanbasov Ready to merge. Thanks!

ivanbasov · 2018-10-19T16:05:44Z

Thank you very much, @CyrusNajmabadi, for you help!

CyrusNajmabadi · 2018-10-19T17:10:16Z

Thanks @ivanbasov !

CyrusNajmabadi requested a review from a team as a code owner May 10, 2018 19:27

CyrusNajmabadi commented May 10, 2018

View reviewed changes

CyrusNajmabadi force-pushed the completionItemPrefixSuffix branch from 26b8184 to 63ae355 Compare May 10, 2018 21:34

CyrusNajmabadi requested a review from a team as a code owner May 10, 2018 21:34

Switch to a more appropriate model for supporting prefix/suffix text …

14aaf8b

…in completion items.

CyrusNajmabadi force-pushed the completionItemPrefixSuffix branch from 1a2783a to 14aaf8b Compare May 10, 2018 23:57

CyrusNajmabadi closed this May 11, 2018

CyrusNajmabadi reopened this May 11, 2018

CyrusNajmabadi closed this May 11, 2018

CyrusNajmabadi reopened this May 11, 2018

CyrusNajmabadi commented May 11, 2018

View reviewed changes

jinujoseph assigned dpoeschl May 13, 2018

jinujoseph added Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. labels May 13, 2018

jinujoseph added this to the 15.8 milestone May 13, 2018

jinujoseph assigned ivanbasov May 31, 2018

CyrusNajmabadi mentioned this pull request Jun 4, 2018

Incorrect text inserted from IntelliCode suggestions in Quick Watch screen #27395

Closed

Merge remote-tracking branch 'upstream/master' into completionItemPre…

e6dfc45

…fixSuffix

jinujoseph modified the milestones: 15.8, 16.0 Jun 15, 2018

jinujoseph added the Blocked label Aug 24, 2018

Merge remote-tracking branch 'upstream/master' into completionItemPre…

9e26ef5

…fixSuffix

CyrusNajmabadi commented Oct 16, 2018

View reviewed changes

ivanbasov reviewed Oct 18, 2018

View reviewed changes

ivanbasov requested changes Oct 18, 2018

View reviewed changes

ivanbasov approved these changes Oct 18, 2018

View reviewed changes

CyrusNajmabadi force-pushed the completionItemPrefixSuffix branch from 17c40d2 to 9e26ef5 Compare October 18, 2018 20:30

CyrusNajmabadi added 2 commits October 18, 2018 13:39

Merge remote-tracking branch 'upstream/master' into completionItemPre…

2a0a859

…fixSuffix

Fixup test.

251b2c7

ivanbasov removed the Blocked label Oct 19, 2018

ivanbasov merged commit ca0e683 into dotnet:master Oct 19, 2018

CyrusNajmabadi deleted the completionItemPrefixSuffix branch October 19, 2018 17:10

ivanbasov mentioned this pull request Feb 25, 2019

Writing code to call a function using identifiers that don’t exist yet. #33667

Closed

CyrusNajmabadi mentioned this pull request Jul 10, 2019

Completion should select IntelliCode item when possible #37066

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to a more appropriate model for supporting prefix/suffix text in completoin items. #26764

Switch to a more appropriate model for supporting prefix/suffix text in completoin items. #26764

CyrusNajmabadi commented May 10, 2018 •

edited

Loading

CyrusNajmabadi May 10, 2018

CyrusNajmabadi May 10, 2018

CyrusNajmabadi commented May 11, 2018

CyrusNajmabadi commented May 11, 2018

CyrusNajmabadi May 11, 2018

CyrusNajmabadi commented May 30, 2018

CyrusNajmabadi commented Jun 8, 2018

CyrusNajmabadi commented Jun 13, 2018

jinujoseph commented Jun 15, 2018

CyrusNajmabadi commented Oct 16, 2018

CyrusNajmabadi Oct 16, 2018

CyrusNajmabadi commented Oct 16, 2018

ivanbasov Oct 18, 2018

CyrusNajmabadi Oct 18, 2018

ivanbasov Oct 18, 2018

ivanbasov Oct 18, 2018

CyrusNajmabadi Oct 18, 2018

ivanbasov left a comment

ivanbasov left a comment

CyrusNajmabadi commented Oct 19, 2018

ivanbasov commented Oct 19, 2018

CyrusNajmabadi commented Oct 19, 2018 •

edited

Loading

		@@ -1 +1,7 @@

		REMOVEDstatic Microsoft.CodeAnalysis.Completion.CompletionItem.Create(string displayText, string filterText = null, string sortText = null, System.Collections.Immutable.ImmutableDictionary<string, string> properties = null, System.Collections.Immutable.ImmutableArray<string> tags = default(System.Collections.Immutable.ImmutableArray<string>), Microsoft.CodeAnalysis.Completion.CompletionItemRules rules = null) -> Microsoft.CodeAnalysis.Completion.CompletionItem

Switch to a more appropriate model for supporting prefix/suffix text in completoin items. #26764

Switch to a more appropriate model for supporting prefix/suffix text in completoin items. #26764

Conversation

CyrusNajmabadi commented May 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented May 11, 2018

CyrusNajmabadi commented May 11, 2018

Choose a reason for hiding this comment

CyrusNajmabadi commented May 30, 2018

CyrusNajmabadi commented Jun 8, 2018

CyrusNajmabadi commented Jun 13, 2018

jinujoseph commented Jun 15, 2018

CyrusNajmabadi commented Oct 16, 2018

Choose a reason for hiding this comment

CyrusNajmabadi commented Oct 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivanbasov left a comment

Choose a reason for hiding this comment

ivanbasov left a comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Oct 19, 2018

ivanbasov commented Oct 19, 2018

CyrusNajmabadi commented Oct 19, 2018 • edited Loading

CyrusNajmabadi commented May 10, 2018 •

edited

Loading

CyrusNajmabadi commented Oct 19, 2018 •

edited

Loading