-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Simplify inline hint tagging and fix duplicated tags. #76525
Conversation
1d71650
to
5e2c113
Compare
{ | ||
private readonly ITagAggregator<InlineHintDataTag> _tagAggregator; | ||
private static ConditionalWeakTable<TagSpan<InlineHintDataTag>, TagSpan<IntraTextAdornmentTag>> s_dataTagToAdornmentTag = new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: if we're concerned about a CWT here, we could switch to a more explicit dictionary that we clear whenever the text snapshot changes.
Or, alternatively, we can just cache teh adornment tag on the data tag. i'll actually update to that.
ClassificationTypeMap typeMap, | ||
Lazy<IStreamingFindUsagesPresenter> streamingFindUsagesPresenter, | ||
EditorOptionsService editorOptionsService) | ||
private readonly InlineHintsDataTaggerProvider _dataTaggerProvider = new(taggerHost, inlineHintKeyProcessor); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@akhera99 THis was the suggestion i had before. Where we do not use a tag aggregator, we just have the inlint hints tagger call into the underlying data tagger provider. I thought you were going to make that change.
[TagType(typeof(InlineHintDataTag))] | ||
[VSUtilities.Name(nameof(InlineHintsDataTaggerProvider))] | ||
[method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)] | ||
[method: ImportingConstructor] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no longer exported. The view tagger now just instantiates this type directly.
{ | ||
_taggerProvider.ThreadingContext.ThrowIfNotOnUIThread(); | ||
InvalidateCache(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for invalidation when the underlying tagger produces tags. adornment tags are directly associated with the data tags, allowing them to be GC'ed when the data tag goes away.
/// cached tag should not be reused.</param> | ||
/// <param name="format">The text formatting used to create the hint. If this format no longer matches the current | ||
/// formatting, this should not be reused.</param> | ||
/// <param name="adornmentTagSpan">The actual adornment tag to render.</param> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: this is a much simpler model. Instead of trying to have a cache we control. We just remember the data we used to create the adornment tag. if it changes, we just don't reuse it.
{ | ||
TagsChanged?.Invoke(this, new SnapshotSpanEventArgs(tag.Span)); | ||
} | ||
OnTagsChanged(this, new SnapshotSpanEventArgs(_subjectBuffer.CurrentSnapshot.GetFullSpan())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
much simpler. when the classification format changes, we just ask the view to retag. we'll see that we can't reuse the existing adornments and will make new ones.
{ | ||
// Reclassify everything. | ||
if (e.HasOption(option => option.Equals(InlineHintsViewOptionsStorage.ColorHints))) | ||
OnTagsChanged(this, new SnapshotSpanEventArgs(_subjectBuffer.CurrentSnapshot.GetFullSpan())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, when the option for coloring changes, we just ask the view to retag. we'll see that we can't reuse the existing adornments and will make new ones.
// tags. Only dealing with the dataTagSpans if the count is 1 because we do not see a multi-buffer case | ||
// occurring | ||
var dataTagSpans = tag.Span.GetSpans(snapshot); | ||
if (dataTagSpans.Count == 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
none of this made sense. how could we have multiple spans when we got the tagspans for the snapshot we currently have, and the tag spans are for that snpashot.
if (snapshot != _cacheSnapshot) | ||
{ | ||
// Calculate UI elements | ||
_cache.Clear(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this logic was busted. we only cleared the cache when the snapshot changed. but this is a view tagger. the snapshot may not change while the view does change. but we wouldnt' know to then get teh new tags.
var colorHints = _taggerProvider.EditorOptionsService.GlobalOptions.GetOption(InlineHintsViewOptionsStorage.ColorHints, document.Project.Language); | ||
|
||
using var _1 = SegmentedListPool.GetPooledList<TagSpan<InlineHintDataTag>>(out var dataTagSpans); | ||
_underlyingTagger.AddTags(spans, dataTagSpans); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new logic is much simpler. our client asks for tags for a certain span, so we just ask the data tagger for its tags from that same span. no need to cache more or compute more than what we're directly asked for.
_formatMap.ClassificationFormatMappingChanged -= OnClassificationFormatMappingChanged; | ||
// If we've never computed the adornment info, or options have changed, then compute and cache the new information. | ||
var cachedTagInformation = (CachedAdornmentTagSpan?)dataTagSpan.Tag.AdditionalData; | ||
if (cachedTagInformation is null || cachedTagInformation.Classified != classify || cachedTagInformation.Format != format) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for a complicated cache. we have the data tag. it can just hold onto this data for us.
{ | ||
var tagSpans = _cache[i].mappingTagSpan.Span.GetSpans(snapshot); | ||
if (tagSpans.Count == 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same issue here. this doesn't make sense.
if (tagSpans.Count == 1) | ||
{ | ||
var tagSpan = tagSpans[0]; | ||
if (spans.IntersectsWith(tagSpan)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we defer to the underlying tagger to do this. as we passed along the span we were asked for to it, we don't have to do any additional processing.
@akhera99 ptal |
Fixes #76527