Skip to content
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.

Sign up for GitHub

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

Merged
merged 12 commits into from
Dec 19, 2024
+181 −192
Merged

Simplify inline hint tagging and fix duplicated tags. #76525

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
0f2abb6
Simplify inlint hint tagging
CyrusNajmabadi Dec 19, 2024
632cfc4
In progress
CyrusNajmabadi Dec 19, 2024
5e2c113
Clean up
CyrusNajmabadi Dec 19, 2024
5840f8b
Clean up
CyrusNajmabadi Dec 19, 2024
4a424c1
Docs
CyrusNajmabadi Dec 19, 2024
f1a2f7f
Docs
CyrusNajmabadi Dec 19, 2024
f70d25b
Simplify
CyrusNajmabadi Dec 19, 2024
14e8575
Docs
CyrusNajmabadi Dec 19, 2024
11822ce
Docs
CyrusNajmabadi Dec 19, 2024
b724568
Extract type
CyrusNajmabadi Dec 19, 2024
e5777b3
Wrapping
CyrusNajmabadi Dec 19, 2024
a1438f1
Make strongly typed
CyrusNajmabadi Dec 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions src/EditorFeatures/Core.Wpf/InlineHints/CachedAdornmentTagSpan.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using Microsoft.VisualStudio.Text.Editor;
using Microsoft.VisualStudio.Text.Formatting;
using Microsoft.VisualStudio.Text.Tagging;

namespace Microsoft.CodeAnalysis.Editor.InlineHints;

internal partial class InlineHintsTaggerProvider
{
/// <summary>The computed adornment tag for an inline hint, along with information needed to determine if it can be
/// reused. This is created and cached on <see cref="InlineHintDataTag.AdditionalData"/> on demand so that we only
/// create adornment tags once and reuse as long as possible.</summary>
/// <param name="classified">Whether or not the adornment tag was classified. If the option for this changes, this
/// 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>
Copy link
Member Author

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.

private sealed class CachedAdornmentTagSpan(
bool classified,
TextFormattingRunProperties format,
TagSpan<IntraTextAdornmentTag> adornmentTagSpan)
{
public bool Classified { get; } = classified;
public TextFormattingRunProperties Format { get; } = format;
public TagSpan<IntraTextAdornmentTag> AdornmentTagSpan { get; } = adornmentTagSpan;
}
}
192 changes: 73 additions & 119 deletions src/EditorFeatures/Core.Wpf/InlineHints/InlineHintsTagger.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,37 +3,35 @@
// See the LICENSE file in the project root for more information.

using System;
using System.Collections.Generic;
using System.Runtime.CompilerServices;
using Microsoft.CodeAnalysis.Collections;
using Microsoft.CodeAnalysis.Editor.Shared.Extensions;
using Microsoft.CodeAnalysis.Editor.Tagging;
using Microsoft.CodeAnalysis.ErrorReporting;
using Microsoft.CodeAnalysis.Options;
using Microsoft.CodeAnalysis.PooledObjects;
using Microsoft.CodeAnalysis.Text;
using Microsoft.CodeAnalysis.Text.Shared.Extensions;
using Microsoft.CodeAnalysis.Utilities;
using Microsoft.VisualStudio.Text;
using Microsoft.VisualStudio.Text.Classification;
using Microsoft.VisualStudio.Text.Editor;
using Microsoft.VisualStudio.Text.Formatting;
using Microsoft.VisualStudio.Text.Tagging;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.Editor.InlineHints
namespace Microsoft.CodeAnalysis.Editor.InlineHints;

internal partial class InlineHintsTaggerProvider
{
/// <summary>
/// The purpose of this tagger is to convert the <see cref="InlineHintDataTag"/> to the <see
/// cref="InlineHintsTag"/>, which actually creates the UIElement. It reacts to tags changing and updates the
/// adornments accordingly.
/// </summary>
internal sealed class InlineHintsTagger : ITagger<IntraTextAdornmentTag>, IDisposable
private sealed class InlineHintsTagger : EfficientTagger<IntraTextAdornmentTag>
{
private readonly ITagAggregator<InlineHintDataTag> _tagAggregator;

/// <summary>
/// stores the parameter hint tags in a global location
/// </summary>
private readonly List<(IMappingTagSpan<InlineHintDataTag> mappingTagSpan, TagSpan<IntraTextAdornmentTag>? tagSpan)> _cache = [];

/// <summary>
/// Stores the snapshot associated with the cached tags in <see cref="_cache" />
/// </summary>
private ITextSnapshot? _cacheSnapshot;
private readonly EfficientTagger<InlineHintDataTag> _underlyingTagger;

private readonly IClassificationFormatMap _formatMap;

Expand All @@ -45,77 +43,59 @@ internal sealed class InlineHintsTagger : ITagger<IntraTextAdornmentTag>, IDispo

private readonly InlineHintsTaggerProvider _taggerProvider;

private readonly ITextBuffer _buffer;
private readonly IWpfTextView _textView;

public event EventHandler<SnapshotSpanEventArgs>? TagsChanged;
private readonly ITextBuffer _subjectBuffer;

public InlineHintsTagger(
InlineHintsTaggerProvider taggerProvider,
IWpfTextView textView,
ITextBuffer buffer,
ITagAggregator<InlineHintDataTag> tagAggregator)
ITextBuffer subjectBuffer,
EfficientTagger<InlineHintDataTag> tagger)
{
_taggerProvider = taggerProvider;

_textView = textView;
_buffer = buffer;
_subjectBuffer = subjectBuffer;

// When the underlying tagger produced new data tags, inform any clients of us that we have new adornment tags.
_underlyingTagger = tagger;
_underlyingTagger.TagsChanged += OnTagsChanged;

_tagAggregator = tagAggregator;
_formatMap = taggerProvider.ClassificationFormatMapService.GetClassificationFormatMap(textView);
_hintClassification = taggerProvider.ClassificationTypeRegistryService.GetClassificationType(InlineHintsTag.TagId);

_formatMap.ClassificationFormatMappingChanged += this.OnClassificationFormatMappingChanged;
_tagAggregator.BatchedTagsChanged += TagAggregator_BatchedTagsChanged;
_taggerProvider.GlobalOptionService.AddOptionChangedHandler(this, OnGlobalOptionChanged);
}

/// <summary>
/// Goes through all the spans in which tags have changed and
/// invokes a TagsChanged event. Using the BatchedTagsChangedEvent since it is raised
/// on the same thread that created the tag aggregator, unlike TagsChanged.
/// </summary>
private void TagAggregator_BatchedTagsChanged(object sender, BatchedTagsChangedEventArgs e)
public override void Dispose()
{
_taggerProvider.ThreadingContext.ThrowIfNotOnUIThread();
InvalidateCache();
Copy link
Member Author

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.


var tagsChanged = TagsChanged;
if (tagsChanged is null)
{
return;
}

var mappingSpans = e.Spans;
foreach (var item in mappingSpans)
{
var spans = item.GetSpans(_buffer);
foreach (var span in spans)
{
if (tagsChanged != null)
{
tagsChanged.Invoke(this, new SnapshotSpanEventArgs(span));
}
}
}
_formatMap.ClassificationFormatMappingChanged -= OnClassificationFormatMappingChanged;
_taggerProvider.GlobalOptionService.RemoveOptionChangedHandler(this, OnGlobalOptionChanged);
_underlyingTagger.TagsChanged -= OnTagsChanged;
_underlyingTagger.Dispose();
}

private void OnClassificationFormatMappingChanged(object sender, EventArgs e)
{
_taggerProvider.ThreadingContext.ThrowIfNotOnUIThread();

// When classifications change we need to rebuild the inline tags with updated Font and Color information.

if (_format != null)
{
_format = null;
InvalidateCache();

// When classifications change we need to rebuild the inline tags with updated Font and Color information.
var tags = GetTags(new NormalizedSnapshotSpanCollection(_textView.TextViewLines.FormattedSpan));

foreach (var tag in tags)
{
TagsChanged?.Invoke(this, new SnapshotSpanEventArgs(tag.Span));
}
OnTagsChanged(this, new SnapshotSpanEventArgs(_subjectBuffer.CurrentSnapshot.GetFullSpan()));
Copy link
Member Author

@CyrusNajmabadi CyrusNajmabadi Dec 19, 2024 •
edited
Loading

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.

}
}

private void OnGlobalOptionChanged(object sender, object target, OptionChangedEventArgs e)
{
// Reclassify everything.
if (e.HasOption(option => option.Equals(InlineHintsViewOptionsStorage.ColorHints)))
OnTagsChanged(this, new SnapshotSpanEventArgs(_subjectBuffer.CurrentSnapshot.GetFullSpan()));
Copy link
Member Author

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.

}

private TextFormattingRunProperties Format
{
get
Expand All @@ -126,86 +106,60 @@ private TextFormattingRunProperties Format
}
}

private void InvalidateCache()
{
_taggerProvider.ThreadingContext.ThrowIfNotOnUIThread();
_cacheSnapshot = null;
_cache.Clear();
}

IEnumerable<ITagSpan<IntraTextAdornmentTag>> ITagger<IntraTextAdornmentTag>.GetTags(NormalizedSnapshotSpanCollection spans)
=> GetTags(spans);

public IReadOnlyList<TagSpan<IntraTextAdornmentTag>> GetTags(NormalizedSnapshotSpanCollection spans)
public override void AddTags(
NormalizedSnapshotSpanCollection spans,
SegmentedList<TagSpan<IntraTextAdornmentTag>> adornmentTagSpans)
{
try
{
if (spans.Count == 0)
return [];
return;

// If the snapshot has changed, we can't use any of the cached data, as it is associated with the
// original snapshot they were created against.
var snapshot = spans[0].Snapshot;
if (snapshot != _cacheSnapshot)
{
// Calculate UI elements
_cache.Clear();
Copy link
Member Author

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.

_cacheSnapshot = snapshot;

// Calling into the InlineParameterNameHintsDataTaggerProvider which only responds with the current
// active view and disregards and requests for tags not in that view
var fullSpan = new SnapshotSpan(snapshot, 0, snapshot.Length);
var tags = _tagAggregator.GetTags(new NormalizedSnapshotSpanCollection(fullSpan));
foreach (var tag in tags)
{
// Gets the associated span from the snapshot span and creates the IntraTextAdornmentTag from the data
// 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)
Copy link
Member Author

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.

{
_cache.Add((tag, tagSpan: null));
}
}
}

var document = snapshot.GetOpenDocumentInCurrentContextWithChanges();
var classify = document != null && _taggerProvider.EditorOptionsService.GlobalOptions.GetOption(InlineHintsViewOptionsStorage.ColorHints, document.Project.Language);
if (document is null)
return;

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);
Copy link
Member Author

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.


// Presize so we can add the elements below without continually resizing.
adornmentTagSpans.Capacity += dataTagSpans.Count;

using var _2 = PooledHashSet<int>.GetInstance(out var seenPositions);

var selectedSpans = new List<TagSpan<IntraTextAdornmentTag>>();
for (var i = 0; i < _cache.Count; i++)
var format = this.Format;
foreach (var dataTagSpan in dataTagSpans)
{
var tagSpans = _cache[i].mappingTagSpan.Span.GetSpans(snapshot);
if (tagSpans.Count == 1)
Copy link
Member Author

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.

{
var tagSpan = tagSpans[0];
if (spans.IntersectsWith(tagSpan))
Copy link
Member Author

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.

{
if (_cache[i].tagSpan is not { } hintTagSpan)
{
var hintUITag = InlineHintsTag.Create(
_cache[i].mappingTagSpan.Tag.Hint, Format, _textView, tagSpan, _taggerProvider, _formatMap, classify);

hintTagSpan = new TagSpan<IntraTextAdornmentTag>(tagSpan, hintUITag);
_cache[i] = (_cache[i].mappingTagSpan, hintTagSpan);
}

selectedSpans.Add(hintTagSpan);
}
}
if (seenPositions.Add(dataTagSpan.Span.Start))
adornmentTagSpans.Add(GetOrCreateAdornmentTagsSpan(dataTagSpan, colorHints, format));
}

return selectedSpans;
}
catch (Exception e) when (FatalError.ReportAndPropagateUnlessCanceled(e, ErrorSeverity.General))
{
throw ExceptionUtilities.Unreachable();
}
}

public void Dispose()
private TagSpan<IntraTextAdornmentTag> GetOrCreateAdornmentTagsSpan(
TagSpan<InlineHintDataTag> dataTagSpan, bool classify, TextFormattingRunProperties format)
{
_tagAggregator.BatchedTagsChanged -= TagAggregator_BatchedTagsChanged;
_tagAggregator.Dispose();
_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)
Copy link
Member Author

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 adornmentSpan = dataTagSpan.Span;
cachedTagInformation = new(classify, format, new TagSpan<IntraTextAdornmentTag>(adornmentSpan, InlineHintsTag.Create(
dataTagSpan.Tag.Hint, format, _textView, adornmentSpan, _taggerProvider, _formatMap, classify)));
dataTagSpan.Tag.AdditionalData = cachedTagInformation;
}

return cachedTagInformation.AdornmentTagSpan;
}
}
}
Loading