Handle completion items with leading/trailing decorative text#26341
Handle completion items with leading/trailing decorative text#26341dpoeschl merged 1 commit intodotnet:dev15.7.xfrom
Conversation
Leading/trailing decorative text can interfere with the pattern matcher's ability to find/bold matching spans and add redundant items to the MRU list.
|
we already discussed with people offline and decide to do this for 15.7 |
| } | ||
|
|
||
| internal static string GetDisplayTextForMatching(CompletionItem item) | ||
| => item.Properties.TryGetValue(DisplayTextForMatching, out var displayText) ? displayText : item.DisplayText; |
There was a problem hiding this comment.
i don't quite get how this works. Who puts the items in the property bag to be read out here?
There was a problem hiding this comment.
In other words, all i see are reads of this data, no writes anywhere in this PR. How does htis work?
There was a problem hiding this comment.
@CyrusNajmabadi Sorry for the lack of context. This is just a workaround for a third-party CompletionProvider that is trying to do some fancy things with the display text. We'll eventually develop better, public APIs to accomplish our shared goals in a more robust way, but for now we're taking a couple tactical fixes to make these experiences better.
|
retest windows_debug_spanish_unit32_prtest please |
|
@dpoeschl Sorry it took me a while to get back to this. Based on what you said, and based on the title of this issue, i think this is not the right way to code htings up. Specifically, the purpose here is to be able to allow completion providers to provide prefix/suffix decorative text for a completion item, but for matching to only happen against the non-decorative 'middle' section of the display text. The problem is that with the approach taken here, there is nothing ensuring consistency between the "full display text" and the "display text for matching", even though it must be the case that the latter is always contained in the former. It would be quite possible (and easy) for a client to end up screwing this up badly. For example, producing a DisplayTextForMatching of Note: we ran into this exact issue when doing highlighting in NavigateTo. I addressed this by changing how things work and not having two distinct strings. Instead, with NavigateTo, there are two strings, the "Name" and the "NameSuffix": /// <summary>
/// The name to pattern match against, and to show in a final presentation layer.
/// </summary>
public string Name { get; }
/// <summary>
/// An optional suffix to be shown in a presentation layer appended to <see cref="Name"/>.
/// Can be null.
/// </summary>
public string NameSuffix { get; }As you can see, only the name is pattern matched against, but then the client can specify a suffix to be provided with that name. This ensures that pattern matching, and hte display shown to the user, are always in sync. Because we will construct things so that hte Name part is always contained in the final display. -- I think something similar should be done here. If there is a need for just suffixes, then just add a DisplayTextSuffix property you look for in the bag. If you also need prefixes, do something similar. Then just produce the final DisplayText out of all three of these. For bolding, you just pattern matching against hte DisplayText, but push the spans forward by whatever length the prefix is (if you have one). In hte future, we can then make DisplayTextSuffix and DisplayTextPrefix part of the standard API for a completion item, instead of digging them out of hte property bag. Would it be possible for you to make this change? If not, i would be willing to do it so we have the right internal data model and so we're protected against inconsistencies which can crash the IDE due to IndexOutOfRange exceptions and the like. |
|
also tagging @jinujoseph ^ |
|
@dpoeschl did you see ^ Can you let me know what you think about this? I think it's the right thing to do, especially if we wantd to make this a public part of a the completion api in teh future (which i think would be a good thing). If you want, i can make this change for you guys if you do not have the time to do it. @jinujoseph as well. |
| textForBolding, pattern, CultureInfo.CurrentCulture); | ||
|
|
||
| return highlightedSpans.SelectAsArray(s => s.ToSpan()); | ||
| return highlightedSpans.SelectAsArray(s => new Span(s.Start + Math.Max(0, displayText.IndexOf(textForBolding)), s.Length)); |
There was a problem hiding this comment.
this is not good:
- you are calling "displayText.IndexOf(textForBolding)" multiple times. and this is in completoin where perf matters and the bolding codepath impacted perf negatively for quite a while.
- this assumes that textForBolding is contained in displayText. If not, this will just produce broken highlights.
If you want to go this path (which i would not recommend), you should first ensure that textForBolding is contained within displayText. If so, you can take this approach. Otherwise you should highlight hte actual display text.
|
Ok. Now that the presumed example where this leading/trailing text has been announced, i'll go and reiterate my ^ points on the issues in this PR and what the likely fixes should be :) |
|
@CyrusNajmabadi Yes, but can you open an issue that links to one or more comments in this PR? |
|
Sure. |
Leading/trailing decorative text can interfere with the pattern
matcher's ability to find/bold matching spans and add redundant items to
the MRU list.
Ask Mode template not completed
Customer scenario
What does the customer do to get into this situation, and why do we think this
is common enough to address for this release. (Granted, sometimes this will be
obvious "Open project, VS crashes" but in general, I need to understand how
common a scenario is)
Bugs this fixes
(either VSO or GitHub links)
Workarounds, if any
Also, why we think they are insufficient for RC vs. RC2, RC3, or RTW
Risk
This is generally a measure our how central the affected code is to adjacent
scenarios and thus how likely your fix is to destabilize a broader area of code
Performance impact
(with a brief justification for that assessment (e.g. "Low perf impact because no extra allocations/no complexity changes" vs. "Low")
Is this a regression from a previous update?
Root cause analysis
How did we miss it? What tests are we adding to guard against it in the future?
How was the bug found?
(E.g. customer reported it vs. ad hoc testing)
Test documentation updated?
If this is a new non-compiler feature or a significant improvement to an existing feature, update https://github.com/dotnet/roslyn/wiki/Manual-Testing once you know which release it is targeting.