Add an option to let user could change the position of Inheritance Margin #55674
Conversation
ghost
added
the
|
|
||
| namespace Microsoft.VisualStudio.LanguageServices.Implementation.InheritanceMargin.MarginGlyph | ||
| { | ||
| internal class InheritanceMarginGlyphViewModel |
|
|
||
| namespace Microsoft.VisualStudio.LanguageServices.Implementation.InheritanceMargin.MarginGlyph | ||
| { | ||
| internal partial class InheritanceMarginGlyph |
src/VisualStudio/Core/Def/Implementation/InheritanceMargin/InheritanceMarginViewMargin.xaml.cs
Outdated
Show resolved
Hide resolved
|
I super don't like this. The breakpoints margin is always empty and I don't like more space taken up for something I never use. ❓ How difficult is it to make this an option? |
This PR is for demo purposes to show the problem and how it looks like after the change. |
|
I'm fine witht his as a demo. But please don't check in otherwise if this changes the current behavior (which is great) :) |
Sam found a way to make this optional. (which makes the position could be configured in the Options page) |
|
I totally did 😎 |
|
|
…oBounds is needed
…for other languages
|
OK, I finally figure out a good way to implement the two options, and not use any hack. (with a little perf cost) |
src/VisualStudio/Core/Def/Implementation/InheritanceMargin/InheritanceMarginTaggerProvider.cs
Show resolved
Hide resolved
CyrusNajmabadi
left a comment
CyrusNajmabadi
left a comment
There was a problem hiding this comment.
Overall looks really good. just some cleanup suggesitons.
| _glyphsContainer.Children.Remove(glyph); | ||
| } | ||
|
|
||
| var remainingGlyphData = _glyphDataTree.Except(glyphDataToRemove).ToImmutableArray(); |
There was a problem hiding this comment.
I looked how the Tagger remove item from the interval tree here https://sourceroslyn.io/#Microsoft.CodeAnalysis.EditorFeatures/Tagging/AbstractAsynchronousTaggerProvider.TagSource_ProduceTags.cs,369
But not sure if this is the correct way to do it here.
@CyrusNajmabadi for verify
|
@CyrusNajmabadi Ready to have another round of reviewing |
| // so in order to get the glyphs removed when FeatureOnOffOptions.InheritanceMarginCombinedWithIndicatorMargin is off, | ||
| // we need | ||
| // 1. Generate tags when this option changes. | ||
| // 2. Always return null here to force the editor to remove the glyphs. |
|
|
||
| void IDisposable.Dispose() | ||
| { | ||
| _editorFormatMap.FormatMappingChanged -= FormatMappingChanged; |
There was a problem hiding this comment.
consider a threading assert for this method as well. (but it's ok to elave out).
src/VisualStudio/Core/Def/Implementation/InheritanceMargin/InheritanceGlyphManager.cs
Outdated
Show resolved
Hide resolved
| private readonly IEditorFormatMap _editorFormatMap; | ||
| private readonly IAsynchronousOperationListener _listener; | ||
| private readonly Canvas _glyphsContainer; | ||
| private readonly SimpleIntervalTree<GlyphData, GlyphDataIntrospector> _glyphDataTree; |
| _glyphDataTree.Clear(); | ||
| foreach (var (span, glyph) in allGlyphData) | ||
| { | ||
| var newSpan = span.TranslateTo(snapshot, SpanTrackingMode.EdgeInclusive); |
There was a problem hiding this comment.
so we have TagSpanIntervalTree. can you look if that would make your life easier?
There was a problem hiding this comment.
It seems like TagSpanIntervalTree is tracking the position of the the tag, but here I need a data structure that could also track the glyphs.
So far I feel SimpleIntervalTree is fine.
...Studio/Core/Def/Implementation/InheritanceMargin/InheritanceGlyphManager_IntervalTreeData.cs
Outdated
Show resolved
Hide resolved
src/VisualStudio/Core/Def/Implementation/InheritanceMargin/InheritanceMarginViewMargin.cs
Show resolved
Hide resolved
src/VisualStudio/Core/Def/Implementation/InheritanceMargin/InheritanceMarginViewMargin.cs
Show resolved
Hide resolved
src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Collections/SimpleIntervalTree`2.cs
Outdated
Show resolved
Hide resolved
CyrusNajmabadi
left a comment
CyrusNajmabadi
left a comment
There was a problem hiding this comment.
tentatively approving. please do look into TagSpanIntervalTree as i think it coul dhelp.
also, if we don't have it, we really need some integration tests. once this goes in, please front load work to at least get a basic test in place for both styles of inheritance margin (merged and non-merged).
Once I resolve the comments here I would try to find vendors to test this before checked in. |
4 participants
Snapshot after this change:
I have been thinking about this change for a long time.
Right now InheritanceMargin is using the GlyphMargin provided by the Editor.
And it leads to several problems.
Breakpoint overlapping issue. Because we are sharing the margin with breakpoint, so either we handle the click event or breakpoint handle it. (Right now we are always handling it)
If we yield to the event to breakpoint, I still found some unwanted scenarios:
I.
If breakpoint handles the click then the inheritance margin won't be clickable
Also as Fred mentioned, if some other plugin (VsVim) is also using the GlyphMargin, it becomes very crowded.
Inheritance Margin Icons are too small #53401
The side of the margin is controlled by the editor now, if we moved this to our own column we can then make it larger. (Maybe to match the size of light bulb)
Clicking on inheritance margin does not open up (potentially due to VS being in Chinese) #53708
The current solution is to disable Resharper, but what I guess is Resharper is providing its own GlyphMarginProvider so it catches the mouse click event, which can cause the inheritance margin non-clickable.
Other minor issues benefited from this:
Because currently, editor owns the life cycle of the glyph, if we move to our own view margin, then we can make the margin visible even when the text is collapsed.