UI Changes - Inline Parameter Name Hints by akhera99 · Pull Request #45800 · dotnet/roslyn

akhera99 · 2020-07-08T21:07:57Z

Hello,

This is a PR for merging the UI changes I have made to the project. I am now putting the textblock within a border, so it has curved edges and looks to be more of a part of the editor. I also am now pulling from the Tools->Options->Fonts and Colors location in order to retrieve the font/font size of the editor so that the hints get updated from that. Also, there are now default colors for the hints in each theme which are also customizable.

The reason that this relies on the cache added in a separate PR is that when the format mapping changes, I now need to clear the cache so that they can be recreated with the changes made in the option page.

Here are samples from each IDE theme-

Light:

Blue:

Blue (Extra Contrast):

Dark:

Please let me know what you guys think!
@JoeRobich PTAL

CyrusNajmabadi · 2020-07-08T23:56:42Z

i'm feeling too sick to review now. but i will say, on a visual level alone, taht i think that looks really nice :)

for those who don't know. We used some of the colors that Teams uses for chat bubbles to inform the design here. So as long as that product has appropriately tested things like contrast, we should be good on our end.

…e of tags showing up when deleting them

akhera99 · 2020-07-09T20:11:32Z

i'm feeling too sick to review now. but i will say, on a visual level alone, taht i think that looks really nice :)

for those who don't know. We used some of the colors that Teams uses for chat bubbles to inform the design here. So as long as that product has appropriately tested things like contrast, we should be good on our end.

To add to this, I used this website that Joey suggested which determines the ratio between the colors and made sure they meet the minimum for their respective themes.

I also went ahead and made the hint slightly smaller so that it would look more floaty.

davidwengier · 2020-07-09T22:30:13Z

on a visual level alone, taht i think that looks really nice :)

I agree! This looks really good, and fits really well with how I would imagine using it, which is to most often be quickly toggling it on, and then off, for reference, but I think it also works for people who would want to leave it on permanently.

src/EditorFeatures/Core.Wpf/InlineParameterNameHints/InlineParameterNameHintsTag.cs

src/EditorFeatures/Core.Wpf/InlineParameterNameHints/InlineParameterNameHintsTagger.cs

src/EditorFeatures/Core.Wpf/InlineParameterNameHints/InlineParameterNameHintsTaggerProvider.cs

jasonmalinowski · 2020-07-10T01:04:30Z

...EditorFeatures/Core.Wpf/InlineParameterNameHints/InlineParameterNameHintsFormatDefinition.cs

+        [Export]
+        [Name(InlineParameterNameHintsTag.TagId)]
+        [BaseDefinition(PredefinedClassificationTypeNames.FormalLanguage)]
+        internal ClassificationTypeDefinition InlineParameterNameHints;


What does this bit actually do? I had thought base definitions only mattered for which classification takes priority if a text span has multiple, and that wouldn't be applicable here.

This is specifically for getting the IClassificationType in the Tagger. I need to essentially register the TagId I specified with the ClassificationTypeDefinition in order to do so.

That would be on me. I asked Ankita to copy all the attributes because I couldn't remember what BaseDefinition was used for.

jasonmalinowski · 2020-07-10T01:04:48Z

...EditorFeatures/Core.Wpf/InlineParameterNameHints/InlineParameterNameHintsFormatDefinition.cs

+
+namespace Microsoft.CodeAnalysis.Editor.InlineParameterNameHints
+{
+    internal sealed class ClassificationTypeDefinitions


Class name doesn't match file name; file name looks better I think?

src/EditorFeatures/Core.Wpf/InlineParameterNameHints/InlineParameterNameHintsTag.cs

jasonmalinowski · 2020-07-10T01:15:40Z

src/EditorFeatures/Core.Wpf/InlineParameterNameHints/InlineParameterNameHintsTagger.cs

            _tagAggregator.TagsChanged += OnTagAggregatorTagsChanged;
        }

+        private void OnClassificationFormatMappingChanged(object sender, EventArgs e)


This only runs on the UI thread?

The assembly online says this Microsoft.VisualStudio.Text.UI.Wpf.dll, so I believe so

we have ways of asserting this. it woudl be good for us to do that assertion so that if this ever turns out to be a wrong assumption we can get a crash immediately instead of a hard to figure out race condition.

jasonmalinowski · 2020-07-10T01:16:51Z

src/EditorFeatures/Core.Wpf/InlineParameterNameHints/InlineParameterNameHintsTagger.cs

            }

-            return tagsList;
+            return _cache;


We aren't filtering this to only the spans that were asked for? That can sometimes cause downstream surprises but if the editor is filtering them out right away then it's not too much of a concern.

jasonmalinowski · 2020-07-10T01:18:10Z

src/EditorFeatures/Core.Wpf/InlineParameterNameHints/InlineParameterNameHintsTagger.cs

+                var fullSpan = new SnapshotSpan(snapshot, 0, snapshot.Length);
+                var requestSpans = new NormalizedSnapshotSpanCollection(fullSpan);
+                var dataTags = _tagAggregator.GetTags(requestSpans);


I recognize we're still investigating why this cache fixes the hang issue but if we merge this in let's file a tracking bug to not forget to remove this if we get distracted by other work. Making UI for all the tags on the UI thread even if they're not visible isn't great, but works for the short term.

@jasonmalinowski is it possible the following is happening:

we compute tags

we report tags.

editor calls into us to get tags and then displays them.

displaying the tag causes the line to change (due to the adornment)

we're listening to line changes and we kick off anotehr computation

rinse repeat?

@CyrusNajmabadi We figure it's something like that, yeah. Displaying the tag will cause the editor to have to lay out the text again but that won't create a new snapshot or make our async tagger reprocess it. We chatted with the editor team and they were aware of one potential issue; the editor calls for tags twice during the process of laying everything out; and if between those two times you are replacing text with an adornment (which the editor supports, but we're not doing here) and you changed the tags between those two calls you might end up with extra layouts or bit of a cycle until things stabilize. But we're not using that editor feature, and our async tagger correctly raises tags changed on the UI thread, and only then reports new tags, so it looks like everything is fine there.

The samples that the editor team have often use a cache of the adornment tags in general and we could bring that code into here; our original belief was because our async tagger wasn't doing the bad things the editor knew to avoid we thought we were good and didn't need a caching layer atop the async tagger's caching layer. @akhera99 is still investigating further and trying to narrow it down.

jasonmalinowski · 2020-07-10T01:19:14Z

src/EditorFeatures/Core.Wpf/InlineParameterNameHints/InlineParameterNameHintsTagger.cs

-            var dataTags = _tagAggregator.GetTags(spans);
            var snapshot = spans[0].Snapshot;
-            foreach (var tag in dataTags)
+            if (_cache.Count == 0 || snapshot != _cacheSnapshot)


So if the file has no spans, then we'll keep requerying the aggregator over and over again? Any concerns about performance or cache impacts there? I'm not sure if the cache needs to better distinguish "empty" versus "not cached".

That's a good point. I will try to figure out a way to distinguish the cases.

src/EditorFeatures/Core.Wpf/InlineParameterNameHints/InlineParameterNameHintsTagger.cs

CyrusNajmabadi · 2020-07-10T18:49:18Z

I also went ahead and made the hint slightly smaller so that it would look more floaty.

That floaty look looks really good :)

CyrusNajmabadi · 2020-07-10T18:50:15Z

src/EditorFeatures/Core.Wpf/InlineParameterNameHints/InlineParameterNameHintsTag.cs

+                VerticalAlignment = VerticalAlignment.Center,
                FontStyle = FontStyles.Normal,
+                FontFamily = format.Typeface.FontFamily,
+                FontSize = format.FontRenderingEmSize - (0.25 * format.FontHintingEmSize),


doc. also, can you post some pictures at:

different font sizes in the editor

different fonts in the editor

different zoom sizes in teh editor. (for example, i run at 137%)

different DPI levels for windows?

Font Size 7:

Font Size 14:

Font Size 19:

Font Size 24:

Comic Sans:

Elephant:

Pristina:

50%:

67%:

100%:

137%:

DPI Level 100%:

DPI Level 125%:

DPI Level 150% (Recommended for my computer):

DPI Level 175%:

src/EditorFeatures/Core.Wpf/InlineParameterNameHints/InlineParameterNameHintsTag.cs

CyrusNajmabadi · 2020-07-10T18:51:46Z

src/EditorFeatures/Core.Wpf/InlineParameterNameHints/InlineParameterNameHintsTagger.cs

+        /// <summary>
+        /// _cache stores the parameter hint tags in a global location 
+        /// </summary>
+        private readonly List<ITagSpan<IntraTextAdornmentTag>> _cache;


i'm still opposed to teh cache.

regardless of the performance, I do think I will still need some sort of logic where the tagspans get stored globally because when the options change, I still need to clear that out and completely recreate them

i would say you don't need to do that. intead, the way tagging works is you hook up "Event sources", i.e. things you're listening to taht should cause you to recompute tags. in your case, you'd have an event source listening for wahtever option this is to change, and then firing the appropraite "something changed" notification to retrigger tagging.

See ITaggerEventSource and TaggerEventSources. We'll likely need a new source here, but it should hopefully not be hard to hook up and make work.

I see, okay. I will look into it

src/EditorFeatures/Core.Wpf/InlineParameterNameHints/InlineParameterNameHintsTagger.cs

CyrusNajmabadi · 2020-07-10T18:55:03Z

src/EditorFeatures/Core/EditorFeaturesResources.resx

    <value>(external)</value>
  </data>
+  <data name="Inline_Parameter_Name_Hints" xml:space="preserve">
+    <value>Inline Parameter Name Hints</value>


where is this disdplayed. Capital Case Is A Bit Weird versus Most UI string should be title cased.

It's being displayed on the Tools->Options->Fonts And Colors->Display Items

CyrusNajmabadi · 2020-07-10T18:55:24Z

Done with pass.

jasonmalinowski · 2020-07-14T22:19:34Z

So @akhera99 and I debugged into why that cache was preventing hangs. If we don't have a cache and re-create new FrameworkElement tag each time, what can happen is:

The editor calls into GetTags. We return some FrameworkElements.
The editor then adds those FrameworkElements to the editor surface. It subscribes to FrameworkElements.SizedChanges so that way if an adornment needs to change size, it can lay out the editor again.
For some reason we get a spurious SizeChanged. This causes the editor to run layout again.
The layout change calls GetTags again, which since we create FrameworkElements we're back to step 1.

This causes the dispatcher queue to always be filled as we're requeuing layouts over and over again.

The thing we don't understand yet is why the SizeChanged is being triggered; even if we do have the cache that probably means we're doing a bunch of extra layouts each time we add new adornments which isn't good even if we keep the cache in place.

…ix-UI

JoeRobich · 2020-07-16T20:16:36Z

...EditorFeatures/Core.Wpf/InlineParameterNameHints/InlineParameterNameHintsFormatDefinition.cs

+        [Export]
+        [Name(InlineParameterNameHintsTag.TagId)]
+        [BaseDefinition(PredefinedClassificationTypeNames.FormalLanguage)]
+        internal ClassificationTypeDefinition InlineParameterNameHints;


That would be on me. I asked Ankita to copy all the attributes because I couldn't remember what BaseDefinition was used for.

src/EditorFeatures/Core.Wpf/InlineParameterNameHints/InlineParameterNameHintsTag.cs

JoeRobich · 2020-07-16T20:27:37Z

src/VisualStudio/Core/Def/ColorSchemes/VisualStudio2017.xml

            </Color>
            <Color Name="label name">
            </Color>
+            <Color Name="inline parameter name hints">


Each file is its own self contained world, so they need to be defined in both places. If we wanted to say these colors are not going to be scheme-able we could move this to the VS EditorColors.xml. But really, I should finish the job of moving every Roslyn color into these schemes.

…ieve just the tags from the current view and not the entire file

src/EditorFeatures/Core.Wpf/InlineParameterNameHints/InlineParameterNameHintsTag.cs

jasonmalinowski · 2020-07-17T21:09:56Z

src/EditorFeatures/Core.Wpf/InlineParameterNameHints/InlineParameterNameHintsTag.cs

+                VerticalAlignment = VerticalAlignment.Center,
                FontStyle = FontStyles.Normal,
+                FontFamily = format.Typeface.FontFamily,
+                FontSize = format.FontRenderingEmSize - (0.25 * format.FontHintingEmSize),


Do we need to be setting both the height of the border and the height of the text? I would have imagined setting one was sufficient and the other would just resize accordingly.

I remember reading somewhere that, when encased in a border, you need to have the border set the height in order for the sizing of the text to change relative to that height.

src/EditorFeatures/Core.Wpf/InlineParameterNameHints/InlineParameterNameHintsTag.cs

src/EditorFeatures/Core.Wpf/InlineParameterNameHints/InlineParameterNameHintsTagger.cs

src/EditorFeatures/Core/InlineParameterNameHints/InlineParameterNameHintsDataTaggerProvider.cs

jasonmalinowski · 2020-07-17T21:33:56Z

src/EditorFeatures/Core.Wpf/InlineParameterNameHints/InlineParameterNameHintsTaggerProvider.cs

+                                                       IClassificationFormatMapService classificationFormatMapService,
+                                                       IClassificationTypeRegistryService classificationTypeRegistryService,
+                                                       IThreadingContext threadingContext)


We're not using these, right? We can just delete?

They're being used in the Tagger.

src/EditorFeatures/Core.Wpf/InlineParameterNameHints/InlineParameterNameHintsTaggerProvider.cs

Ankita Khera added 5 commits July 7, 2020 14:12

Edited UI elements

09fb3b8

Retrieving the options set for the font/font size in the options page

96f230c

trying to get colors to change

e49f1ab

Finished updating the UI

60a593e

Added defaults to high contrast

f2d19a1

akhera99 changed the base branch from master to features/parameter-hints July 8, 2020 21:08

Ankita Khera added 3 commits July 9, 2020 10:45

Made minor changes to UI, need to fix some errors with user experienc…

cf3cf12

…e of tags showing up when deleting them

minor changes

aa5691f

Fixed caching issue

b796698

akhera99 marked this pull request as ready for review July 9, 2020 20:08

akhera99 requested a review from a team as a code owner July 9, 2020 20:08

akhera99 requested a review from a team July 9, 2020 20:08

davidwengier reviewed Jul 9, 2020

View reviewed changes

Made some minor changes

c12baca

jasonmalinowski reviewed Jul 10, 2020

View reviewed changes

Changes based on PR comments

9c98db5

vatsalyaagrawal added the Area-IDE label Jul 10, 2020

CyrusNajmabadi reviewed Jul 10, 2020

View reviewed changes

src/EditorFeatures/Core.Wpf/InlineParameterNameHints/InlineParameterNameHintsTag.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Jul 10, 2020

View reviewed changes

src/EditorFeatures/Core.Wpf/InlineParameterNameHints/InlineParameterNameHintsTag.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Jul 10, 2020

View reviewed changes

src/EditorFeatures/Core.Wpf/InlineParameterNameHints/InlineParameterNameHintsTagger.cs Show resolved Hide resolved

CyrusNajmabadi reviewed Jul 10, 2020

View reviewed changes

Added more comments regarding the UI elements being used

42cf763

Ankita Khera added 2 commits July 16, 2020 15:08

Added threading assertions

63c48ca

Merge remote-tracking branch 'dotnet/features/parameter-hints' into f…

bf30576

…ix-UI

JoeRobich reviewed Jul 16, 2020

View reviewed changes

Ankita Khera and others added 4 commits July 16, 2020 16:29

Fixed spacing issue

6f5a5f0

Removed unnecessary items

a308372

Converted the TaggerProvider to a ViewTaggerProvider in order to retr…

acee5a4

…ieve just the tags from the current view and not the entire file

Merge branch 'features/parameter-hints' into fix-UI

44b2310

jasonmalinowski approved these changes Jul 17, 2020

View reviewed changes

Ankita Khera added 3 commits July 20, 2020 11:37

Commenting changes primarily

91316e2

Merge branch 'fix-UI' of https://github.com/akhera99/roslyn into fix-UI

a42de1a

Skipping the interactive window

7e41e81

JoeRobich reviewed Jul 20, 2020

View reviewed changes

src/EditorFeatures/Core.Wpf/InlineParameterNameHints/InlineParameterNameHintsTaggerProvider.cs Show resolved Hide resolved

Ankita Khera added 2 commits July 21, 2020 10:54

Added comment

af96897

Moved the adornment up a bit

4586a43

akhera99 merged commit 7aedbb7 into dotnet:features/parameter-hints Jul 21, 2020

jaredpar mentioned this pull request Jul 22, 2020

TestDefaultDiagnosticProviderRemove is flaky #46230

Closed

333fred mentioned this pull request Jul 29, 2020

DefaultDiagnosticUpdateSourceTests contains flaky tests #45877

Closed

mavasani mentioned this pull request Aug 6, 2020

Add an analyzer to recommend using named parameters when passing literals as arguments #43988

Open

akhera99 deleted the fix-UI branch June 18, 2024 20:50

Conversation

akhera99 commented Jul 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrusNajmabadi commented Jul 8, 2020

Uh oh!

akhera99 commented Jul 9, 2020

Uh oh!

davidwengier commented Jul 9, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akhera99 Jul 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski Jul 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CyrusNajmabadi commented Jul 10, 2020

Uh oh!

CyrusNajmabadi Jul 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akhera99 commented Jul 8, 2020 •

edited

Loading

akhera99 Jul 10, 2020 •

edited

Loading

jasonmalinowski Jul 10, 2020 •

edited

Loading

CyrusNajmabadi Jul 10, 2020 •

edited

Loading

akhera99 Jul 10, 2020 •

edited

Loading