Completion should select IntelliCode item when possible

[ivanbasov]

Here is a summary of the design meeting with the IntelliCode team:

1. Intellicode items should always be preferred comparing to non-Intellicode items.
2. All current Roslyn heuristics such as MRU or matching should be kept without changes.
3. If there is a starred copy for an item to be selected, the starred copy should be selected.
4. If user committed the unstarred instance of an item in a previous session, the starred instance should be considered in the MRU selection.
5. If the current selected item is a starred one and changing selection switches the item to a non-starred one, selection should make a jump in the completion popup maybe many frames below.
6. If the current selected item is a non-starred one and changing selection switches the item to an item with a starred copy, selection should make a jump in the completion popup to the starred one maybe many frames above.

[sharwell]

No specific change request, but I'd like to go over the specific rules with you to make sure I'm on the same page here.

[dpoeschl]

Same as Sam, I want to understand 5 and 6 more as they don't make sense to me currently.

[ivanbasov]

@dpoeschl , @sharwell , please unblock the PR:

1. @sharwell , let us have a discussion with the IntelliCode team. Added you to the thread. Once the IntelliCode team will agree with the idea and confirm that they are ready, we change the order of MRU and IntelliCode. I do not mind. I just want this to be supported and agreed with the IntelliCode team who will get more responsibility for the completion. FYI: @CyrusNajmabadi

2. 5 & 6 is what exactly confirmed with Mark and Allison on the meeting you attended: They said user do not complain when completion jumps between intellicode and non-intellicode items. If you have items abc and abd, and abc has a starred copy, once we should select (by Roslyn heuristics) abc, we should select the starred instance. If we need then to select abd, we should jump to it even through multiple frames if necessary.

Question answered.

[dpoeschl]

@ivanbasov Okay. I thought you meant that the user would press down and it'd jump to somewhere else in the list that was "next to" the starred item alphabetically. But I think what you mean is that if the user types an additional character that changes from having a starred match to a non-starred match, then it's okay to jump. Correct?

[ivanbasov]

@ivanbasov Okay. I thought you meant that the user would press down and it'd jump to somewhere else in the list that was "next to" the starred item alphabetically. But I think what you mean is that if the user types an additional character that changes from having a starred match to a non-starred match, then it's okay to jump. Correct?

Thank you for the confirmation, @dpoeschl ! Correct: I mean that adding or deleting characters may jump between starred and non-starred items. If you do not mind, please review the PR. Thank you!

[CyrusNajmabadi]

Definitely interested in this convo. While i thnk intellicode is great, i'm a little concerned about it overriding things like the MRU when incorrect.

[CyrusNajmabadi]

If user committed the unstarred instance of an item in a previous session, the starred instance should be considered in the MRU selection.

I'm a little confused by this. that's how MRU should be working anyways. MRU certainly shouldn't see the 'star' or not. I believe i submitted a PR to ensure that things like the star were not part of the 'text' and were simply a 'prefix adornment' precisely so we wouldn't have any sorts of leaks between these two systems.

Please advise on this.

[CyrusNajmabadi]

See:

        /// <summary>
        /// The text that is displayed to the user.
        /// </summary>
        public string DisplayText { get; }

        /// <summary>
        /// An optional prefix to be displayed prepended to <see cref="DisplayText"/>. Can be null.
        /// Pattern-matching of user input will not be performed against this, but only against <see
        /// cref="DisplayText"/>.
        /// </summary>
        public string DisplayTextPrefix { get; }

Note that the MRU works like this:

        internal static int GetRecentItemIndex(ImmutableArray<string> recentItems, RoslynCompletionItem item)
        {
            var index = recentItems.IndexOf(item.DisplayText);
            return -index;
        }

So i cannot explain any sort of behavior you're seeing that would need fixing...

[ivanbasov]

If user committed the unstarred instance of an item in a previous session, the starred instance should be considered in the MRU selection.

I'm a little confused by this. that's how MRU should be working anyways. MRU certainly shouldn't see the 'star' or not. I believe i submitted a PR to ensure that things like the star were not part of the 'text' and were simply a 'prefix adornment' precisely so we wouldn't have any sorts of leaks between these two systems.

Please advise on this.

Thank you, @CyrusNajmabadi ! The star is a part of the DisplayText now. @dpoeschl may know more why.

[CyrusNajmabadi]

The star is a part of the DisplayText now. @dpoeschl may know more why.

Honestly, can we just fix this by loooking for that and rewriting the completion item to be in the appropriate form? If we're going to add all this code to 'unhork' things, that seems like the most sensible way to do things.

[ivanbasov]

The star is a part of the DisplayText now. @dpoeschl may know more why.

Honestly, can we just fix this by loooking for that and rewriting the completion item to be in the appropriate form? If we're going to add all this code to 'unhork' things, that seems like the most sensible way to do things.

1. @dpoeschl do you see we can move starts from DisplayText to Prefix?
2. @CyrusNajmabadi , with the proposed change we would be able to keep MRU matching by DisplayText. However, we are not guaranteed that items are provided for processing in the display order (starred before non-starred). Therefore, we still have to add the lowest level comparison: if two items have the same display text, we still should check for stars to prefer the starred one. Agree?

[CyrusNajmabadi]

However, we are not guaranteed that items are provided for processing in the display order (starred before non-starred). Therefore, we still have to add the lowest level comparison: if two items have the same display text, we still should check for stars to prefer the starred one. Agree?

Works for me. Thanks!

[CyrusNajmabadi]

Note: the PR to fix these issues with display-texts and prefixes was added about 10 months ago here: #26764. The internal work to get it properly used doesn't seem to have been done.

[dpoeschl]

I'm a little foggy, but I think the reason IntelliCode offers items the way it does is because it is backward compatible to VS 15.something. If/when we bump our Roslyn reference version we can get rid of some reflection and improve things like this.

[CyrusNajmabadi]

I'm a little foggy, but I think the reason IntelliCode offers items the way it does is because it is backward compatible to VS 15.something. If/when we bump our Roslyn reference version we can get rid of some reflection and improve things like this.

interesting. but none of our changes here will help VS15 right? Since these changes will only go into VS2019 and above?

If htat's the case, can we have intellicode just check whcih version they're on and use the right entrypoints accordingly?

At the same time, we could put in a simple hack on our side to check if the display string starts with the star, and munge things properly so that we odn't have to then fix the fallout in later points in the code.

Thanks!

[ivanbasov]

1. @dpoeschl and @CyrusNajmabadi . From @dpoeschl explanations, my understanding that we cannot move stars from DisplayText to Prefix due to back compatibility reasons. Then, we have to use FilterText not DisplayText in MRU. Is it correct? If so, @CyrusNajmabadi do you have any concerns to proceed?

2. @sharwell , please unblock the PR. There is an email thread to discuss the priority of IntelliCode vs MRU. I think this would be a decision not a quick fix. The current PR is a fix to restore VS 2017 behavior.

[sharwell]

❓ How is sorting performed? Can we not use the same strategy here (i.e. among two items with the same filter text, prefer to select the first one in sort order)?

[ivanbasov]

Unfortunately, no. Editor maintains multiple items lists: full, sorted, and filtered. They ask us for sorting before. However, they send us the full unsorted list for filtering.

[sharwell]

💡 !bestItem.IsPreferredItem() is an unnecessary check here

Suggested change

	(mruIndex2 == mruIndex1 && !bestItem.IsPreferredItem() && chosenItem.IsPreferredItem()))
	(mruIndex2 == mruIndex1 && chosenItem.IsPreferredItem()))

[ivanbasov]

Well, it depends. For example, there are starred Last and Length, and match by L. Which one we should prefer? The current idea is to prefer the first item. Also the pattern we follow in this method is to use the chosen item only if it exceeds the current best one. So, let us follow the pattern consistently.

Feedback was addressed

[CyrusNajmabadi]

@dpoeschl and @CyrusNajmabadi . From @dpoeschl explanations, my understanding that we cannot move stars from DisplayText to Prefix due to back compatibility reasons.

I'm not sure i undertand this. Why can't we move the stars? I'm not asking for intellicode to move tehm (though they should). I'm saying: we can detect things on our end and fix things up so that our internal model is being used correctly.