Inline Hints: Added double click on hint to insert text by akhera99 · Pull Request #55737 · dotnet/roslyn

akhera99 · 2021-08-19T22:48:17Z

Added functionality in for parameter and type hints to be double-clicked and inserted into the text. Type hints replace var

akhera99 · 2021-08-19T22:49:49Z

Edited behavior:

ConsoleApp7.-.Microsoft.Visual.Studio.Int.Preview.2021-08-20.15-30-56.mp4

JoeRobich

Looks good. I am excited to try this out

CyrusNajmabadi · 2021-08-19T23:16:49Z

Added an option in C#/VB for parameter hints to be double-clicked and inserted into the text.

do we need an option for this? why not always support this? what's the use case for a user double-clicking on the hint and not inserting it?

src/EditorFeatures/Core.Wpf/InlineHints/InlineHintsTag.cs

CyrusNajmabadi · 2021-08-19T23:18:17Z

src/Features/Core/Portable/InlineHints/AbstractInlineParameterNameHintsService.cs

+
                        result.Add(new InlineHint(
                            new TextSpan(position, 0),
                            ImmutableArray.Create(new TaggedText(TextTags.Text, parameter.Name + ": ")),


isn't the replacement text exactly the same as this string here? why not just compute it here and just add to the hint?

confused. you still generate parameter.Name + ": " here.

right, for C# and VB we generated "parameter:" for the hint itself, which I thought should remain the same. It only matters to generate the parameter:= for VB when you double click and replace the hint, which is why I added the "inlineHintText" var on line 94.

interesting. it's arguable that the VB hint is wrong and should be foo:= to indicate the actual VB implied text... can you try that out and show me what it looks like? if it's not hideous, we could mvoe to that. If it's awful, then your solution is a good workaround.

src/Features/Core/Portable/InlineHints/InlineHintHelpers.cs

CyrusNajmabadi · 2021-08-19T23:18:58Z

src/Features/Core/Portable/InlineHints/InlineHint.cs

            => _getDescriptionAsync?.Invoke(document, cancellationToken) ?? SpecializedTasks.EmptyImmutableArray<TaggedText>();
+
+        public string? GetReplacementText()
+            => _getReplacementText?.Invoke() ?? null;


why do we need an indirection here? it should be practically free to just store the TextChange struct.

At that point, why would you need the TextChange struct as well?

to know what text change to make :)

isn't it easier to use the ITextBuffer.Replace instead of worrying about TextChange etc? We already have the IWpfTextView stored so calling Replace on the underlying buffer is trivial

hints are data, and should work cross process. we would not want them making any changes themselves.

src/Features/Core/Portable/InlineHints/InlineHintsOptions.cs

src/Features/Core/Portable/InlineHints/InlineHint.cs

CyrusNajmabadi

i think there's a bit of overkill in this :)

src/Features/Core/Portable/InlineHints/InlineHint.cs

…uage level

CyrusNajmabadi · 2021-09-08T21:00:01Z

src/EditorFeatures/Core.Wpf/InlineHints/InlineHintsTag.cs

+            {
+                e.Handled = true;
+                var replacementValue = _hint.ReplacementTextChange!.Value;
+                _ = _textView.TextBuffer.Replace(new VisualStudio.Text.Span(replacementValue.Span.Start, replacementValue.Span.Length), replacementValue.NewText);


still wont' work in any projection scenario.

CyrusNajmabadi · 2021-09-08T21:01:21Z

src/Features/CSharp/Portable/InlineHints/CSharpInlineTypeHintsService.cs

            // a space after it to make things feel less cramped.
            var trailingSpace = span.Length == 0;
-            return new TypeHint(type, span, trailingSpace: trailingSpace);
+            return new TypeHint(type, span, new TextChange(displayAllSpan.Span, type.ToDisplayString(s_minimalTypeStyle)), trailingSpace: trailingSpace);


it would be good to have a test to ensure that double clickong on a non-overwrite hint overwrites teh right text.

How would you go about testing that? The testing suite we currently have has access to the InlineHInts, but not the tags themselves. If we had access to the tags we could invoke a double click on the adornment, but is there an easier way to do it? @CyrusNajmabadi

well, we can have two sorts of tests. Unit tests that would validate our internal API and hte data it returns. Then an integration test that actually loads this into VS and interacts with the real tags on the editor side.

We should likely have both. Most tests should be at teh unit level, but we should have a couple of tests that actually do the full end to end validation at the editor level.

this code feels like it should be in the constructor of TypeHint. Pass both spans.

Cosifne · 2021-09-28T04:58:27Z

Retarget this to main because 17.1 branch is not used and main is 17.1 now

JoeRobich · 2021-12-07T22:24:48Z

src/EditorFeatures/Core.Wpf/InlineHints/InlineHintsTagger.cs

                        {
                            var parameterHintUITag = InlineHintsTag.Create(
-                                    _cache[i].mappingTagSpan.Tag.Hint, Format, _textView, tagSpan, _taggerProvider, _formatMap, classify);
+                                    _cache[i].mappingTagSpan.Tag.Hint, Format, _textView, tagSpan, _taggerProvider, _formatMap, snapshot.TextBugger, classify);


Suggested change

_cache[i].mappingTagSpan.Tag.Hint, Format, _textView, tagSpan, _taggerProvider, _formatMap, snapshot.TextBugger, classify);

_cache[i].mappingTagSpan.Tag.Hint, Format, _textView, tagSpan, _taggerProvider, _formatMap, snapshot.TextBuffer, classify);

CyrusNajmabadi · 2021-12-07T22:27:18Z

src/EditorFeatures/Core.Wpf/InlineHints/InlineHintsTag.cs

+            {
+                e.Handled = true;
+                var replacementValue = _hint.ReplacementTextChange!.Value;
+                _ = _subjectBuffer.Replace(new VisualStudio.Text.Span(replacementValue.Span.Start, replacementValue.Span.Length), replacementValue.NewText);


this span may be computed against a different snapshot version of hte buffer than the one currently displayed. it woudl be probably hard to hit, but this could cause a crash. You should validate that the span is still within the bounds of the buffer's current snapshot prior to making this change.

CyrusNajmabadi · 2021-12-07T22:27:44Z

src/EditorFeatures/Core.Wpf/InlineHints/InlineHintsTag.cs

+            {
+                e.Handled = true;
+                var replacementValue = _hint.ReplacementTextChange!.Value;
+                _ = _subjectBuffer.Replace(new VisualStudio.Text.Span(replacementValue.Span.Start, replacementValue.Span.Length), replacementValue.NewText);


Suggested change

_ = _subjectBuffer.Replace(new VisualStudio.Text.Span(replacementValue.Span.Start, replacementValue.Span.Length), replacementValue.NewText);

_subjectBuffer.Replace(new VisualStudio.Text.Span(replacementValue.Span.Start, replacementValue.Span.Length), replacementValue.NewText);

adornment.MouseLeftButtonDown -= Adornment_MouseLeftButtonDown;

CyrusNajmabadi · 2021-12-07T22:27:56Z

src/EditorFeatures/Test2/InlineHints/AbstractInlineHintsTests.vb

            End Using
        End Function
+
+        'Protected Async Function VerifyTypeHintsDoubleClick(test As XElement, Optional optionIsEnabled As Boolean = True, Optional ephemeral As Boolean = False) As Task


src/Features/Core/Portable/InlineHints/TypeHint.cs

akhera99 · 2021-12-08T19:54:00Z

src/Features/CSharp/Portable/InlineHints/CSharpInlineTypeHintsService.cs

                        IsValidType(parameter?.Type))
                    {
-                        return new(parameter.Type, new TextSpan(parameterNode.Identifier.SpanStart, 0), trailingSpace: true);
+                        if (parameterNode.Parent!.Parent.IsKind(SyntaxKind.ParenthesizedLambdaExpression))


Would we everr have a null Parent in this case?

i would jsut write this as:

var textChange = parameterNode.Parent?.Parent?.Kind is SyntaxKind.ParenthesizedLambdaExpression ? new TextChange(...) : null;

then just return once.

CyrusNajmabadi · 2022-01-06T20:04:03Z

src/EditorFeatures/Core.Wpf/InlineHints/InlineHintsTag.cs

        private InlineHintsTag(
            FrameworkElement adornment,
            ITextView textView,
+            ITextBuffer subjectBuffer,


this feels a bit redundant. the snapshot span already points at this right? perhaps just use that so there's no chance of this being out of date?

CyrusNajmabadi

approved, but have some small sgugestions on cleanup :)

Added double click on hint to insert text

0e45ca6

ghost added the Area-IDE label Aug 19, 2021

akhera99 added the Feature - Inline Parameter Hints label Aug 19, 2021

akhera99 added 2 commits August 19, 2021 15:58

removed unnecessary usings

29d2b9e

don't explicitly check for false

efb9085

akhera99 marked this pull request as ready for review August 19, 2021 23:04

akhera99 requested a review from a team as a code owner August 19, 2021 23:04

akhera99 requested a review from a team August 19, 2021 23:04

JoeRobich approved these changes Aug 19, 2021

View reviewed changes

don't call add twice

705543d

akhera99 changed the base branch from main to release/dev17.1-preview1 August 19, 2021 23:12

akhera99 requested review from a team as code owners August 19, 2021 23:12

akhera99 changed the base branch from release/dev17.1-preview1 to main August 19, 2021 23:13

akhera99 removed request for a team August 19, 2021 23:13

CyrusNajmabadi reviewed Aug 19, 2021

View reviewed changes

src/EditorFeatures/Core.Wpf/InlineHints/InlineHintsTag.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Aug 19, 2021

View reviewed changes

src/Features/Core/Portable/InlineHints/InlineHintHelpers.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Aug 19, 2021

View reviewed changes

src/Features/Core/Portable/InlineHints/InlineHintsOptions.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Aug 19, 2021

View reviewed changes

src/Features/Core/Portable/InlineHints/InlineHint.cs Outdated Show resolved Hide resolved

CyrusNajmabadi suggested changes Aug 19, 2021

View reviewed changes

akhera99 added 3 commits August 19, 2021 17:17

removed functions and options

b7441a3

leftover removal

09f245c

more pr feedback

ef90167

CyrusNajmabadi reviewed Aug 20, 2021

View reviewed changes

src/Features/Core/Portable/InlineHints/InlineHint.cs Show resolved Hide resolved

akhera99 added 2 commits September 8, 2021 12:21

moved to only construct string when necessary

2cacb17

create textchange at the start, create the display string at the lang…

a634842

…uage level

CyrusNajmabadi reviewed Sep 8, 2021

View reviewed changes

akhera99 added 2 commits September 8, 2021 14:42

do not use the top level buffer, get the subject buffer

7f96d2b

need to figure out how test double click on an adornment

c123b93

Cosifne changed the base branch from release/dev17.1-preview1 to main September 28, 2021 04:58

merge main into double click

6e4b096

JoeRobich reviewed Dec 7, 2021

View reviewed changes

CyrusNajmabadi reviewed Dec 7, 2021

View reviewed changes

src/Features/Core/Portable/InlineHints/TypeHint.cs Show resolved Hide resolved

akhera99 added 2 commits December 8, 2021 09:41

tests + other feedback

61efe06

type hints tests

f847cc9

akhera99 commented Dec 8, 2021

View reviewed changes

added vb tests

82707b5

CyrusNajmabadi reviewed Jan 6, 2022

View reviewed changes

CyrusNajmabadi approved these changes Jan 6, 2022

View reviewed changes

akhera99 added 4 commits January 6, 2022 16:24

Merge branch 'main' into inline_hints_double_click

da6bfe4

merge main

0325cd1

fedback

2cb2ed8

fix bug

36a0c92

akhera99 merged commit 13a91f0 into dotnet:main Jan 7, 2022

ghost added this to the Next milestone Jan 7, 2022

RikkiGibson modified the milestones: Next, 17.2.P1 Feb 4, 2022

Youssef1313 mentioned this pull request Apr 30, 2022

Double clicking inline text hint for var inserts incorrectly #61052

Closed

akhera99 deleted the inline_hints_double_click branch June 18, 2024 20:52

	_cache[i].mappingTagSpan.Tag.Hint, Format, _textView, tagSpan, _taggerProvider, _formatMap, snapshot.TextBugger, classify);
	_cache[i].mappingTagSpan.Tag.Hint, Format, _textView, tagSpan, _taggerProvider, _formatMap, snapshot.TextBuffer, classify);

	_ = _subjectBuffer.Replace(new VisualStudio.Text.Span(replacementValue.Span.Start, replacementValue.Span.Length), replacementValue.NewText);
	_subjectBuffer.Replace(new VisualStudio.Text.Span(replacementValue.Span.Start, replacementValue.Span.Length), replacementValue.NewText);
	adornment.MouseLeftButtonDown -= Adornment_MouseLeftButtonDown;

Conversation

akhera99 commented Aug 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akhera99 commented Aug 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JoeRobich left a comment

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Aug 19, 2021

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!

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!

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

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!

Choose a reason for hiding this comment

Uh oh!

Cosifne commented Sep 28, 2021

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

akhera99 commented Aug 19, 2021 •

edited

Loading

akhera99 commented Aug 19, 2021 •

edited

Loading