Connect inline rename UI to editor smart rename session #70420

Cosifne · 2023-10-17T23:32:20Z

Thanks to @AmadeusW which added an internal preview API to suggested names based on copilot results.
This PR connects this to our inline rename UI
How it looks:

User could double-click & navigate using the keyboard to select the suggested names.

A few points:

This would only be available in inline rename UI now. For our legacy UI, when the suggested identifier in ViewModel changed, this line would stop the change from propagating to the text editor, because traditionally we only expect text change from the editor in legacy UI. I don't want to regress this behavior so we need a way to tell if the feature is ON in the editor feature layer (Currently it is only targeting net472, and no good way to check if from option service). I have talked to Amadeus this is on his note.
It needs to consume 17.9 APIs, if the integration test fails I will create main-vs-deps branch for it.
Would be best if could be covered by an integration test, but given it's quite difficult to enable this I feel it's not possible. The real logic is mostly handled by the editor, we are just providing a thin UI wrapper around it, so I guess it might be fine now?

Cosifne · 2023-10-19T05:19:13Z

i have mixed feelings abouthe spinner (since it's pretty distracting). Could this just be cycling:
Generating suggestions .
Generating suggestions ..
Generating suggestions ...
?

As @AmadeusW proposed they want to have this checked to asap to get the backend tested.
I do have some draft UI idea & impl, will create following PR

Cosifne · 2023-10-19T05:20:36Z

Synced with the infra folks so it's agreed now we use release/dev17.9 as our old vs-deps branch.
Waiting for the branch model/policy to update now..

…h/smartRename

CyrusNajmabadi · 2023-10-20T19:53:07Z

src/EditorFeatures/Core.Wpf/InlineRename/UI/InlineRenameAdornmentManager.cs

+                        _renameService.ActiveSession,
+                        session => new RenameFlyoutViewModel(session,
+                            identifierSelection,
+                            registerOleComponent: true,


do you know what the registerOleComponent bit is about?

I just warp all the argument here, guess Andrew @ryzngard would know this.

Registering for Ole fails in unit tests (I'd have to dig deeper to remember why) so we have this for testing purposes. Production code should always be true.

CyrusNajmabadi · 2023-10-20T19:53:23Z

src/EditorFeatures/Core.Wpf/InlineRename/UI/SmartRename/SmartRenameControl.xaml

@@ -0,0 +1,91 @@
+<UserControl x:Class="Microsoft.CodeAnalysis.InlineRename.UI.SmartRename.SmartRenameControl"


@ryzngard for xaml review.

CyrusNajmabadi · 2023-10-20T19:53:48Z

src/EditorFeatures/Core.Wpf/InlineRename/UI/SmartRename/SmartRenameControl.xaml

+            <Setter Property="ItemsPanel">
+                <Setter.Value>
+                    <ItemsPanelTemplate>
+                        <WrapPanel Orientation="Horizontal" Width="238" HorizontalAlignment="Left"/>


slightly concerned about hardcoded values like 238. where did that number come from?

I got this from the editor side impl, tag @AmadeusW here

I'd try it without tbh. It should fill the space of the panel

Without the width limitation, it will try to put everything in the same line

CyrusNajmabadi · 2023-10-20T19:54:27Z

src/EditorFeatures/Core.Wpf/InlineRename/UI/SmartRename/SmartRenameControl.xaml.cs

+    {
+        if (e.ClickCount == 1)
+        {
+            var identifierName = ((FrameworkElement)sender).Tag.ToString();


how do we know that the sender is a framework eleemnt, or that the tag contains the info we need?

By wpf definition https://learn.microsoft.com/en-us/dotnet/api/system.windows.frameworkelement.tag?view=windowsdesktop-7.0
Then tag is directly binded to the display string in xaml

Yea, this is safe in WPF as long as this method is used for mouse events.

CyrusNajmabadi · 2023-10-20T19:55:41Z

src/EditorFeatures/Core.Wpf/InlineRename/UI/SmartRename/SmartRenameViewModel.cs

+            foreach (var name in _smartRenameSession.SuggestedNames)
+            {
+                SuggestedNames.Add(name);
+            }


i think an observable collection has a way to say "i'm about to make a bunch of changes" so it doesn't notify on each change.

CyrusNajmabadi · 2023-10-20T19:57:31Z

Ok. This looks good to me. However, i'd like @jasonmalinowski @sharwell to have a pass, since they're experts in hooking things into the UI properly :)

Cosifne · 2023-10-23T17:06:15Z

src/EditorFeatures/Test2/IntelliSense/VisualBasicCompletionCommandHandlerTests.vb

@@ -396,7 +396,7 @@ End Class
            End Using
        End Function

-        <WpfFact>
+        <WpfFact(Skip:="https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1906990")>


This failed after updating the VS editor package to 17.9. (Only fails in our CI, Spanish VM)
#70471

Test is testing the behavior for both our code & editor side impl of IAsyncCompletionBroker.
So, if this is regressed, it's already in VS, since editor team owns the insertion of the dll.
I have talked to Gen and we can temp skip this test and let editor folks to investigate this

src/EditorFeatures/Core.Wpf/InlineRename/UI/SmartRename/SmartRenameViewModel.cs

Cosifne · 2023-10-24T04:03:20Z

Will monitor the health of Integration test from Dart lab

Cosifne added 26 commits October 16, 2023 11:28

Update all the essential packages

71c4553

Fix the package warnings

7da4fef

Add Editor.Internal

42b5591

Move to wpf layer

463bf68

Link smart rename session to the view model

3d94c3f

Add control

65e61d3

Hook up view model

52ea0c2

Hook task

fd3d2be

Fix build errors

a9475e4

Rename

a03834b

Use TestVM to tune the UI

4c66ea0

Extract to common resources

835dd6b

Bind to view model

b0c07b5

Link view model

11c26ab

Only update the selection when isSelect is true

6b183aa

Move logic to code behind

4a534ff

Sync the life time with view model

5391c9d

Add mock in test

2263da0

Correct bind to inline model

d894a4a

Tune the UI

74b7121

Remove the UI from legacy rename control

6c020fc

Add scrolling function

a9a9784

Add resources

845a542

Rename

12adddf

Suppress warning in prod code

a391e9b

Suppress warning in test code

7b10d42

Cosifne requested review from a team as code owners October 17, 2023 23:32

dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Oct 17, 2023

Cosifne added 7 commits October 18, 2023 16:16

Pass threading context to control

e990fd4

Address PR feedback

68fce28

Plase cursor to the end

9f9b1ae

When navigate the suggestions, circle the list

350b42e

Use file scope namespace

2a7320d

Add comment

2837fdb

Add comments

21c9797

Cosifne added 3 commits October 19, 2023 10:26

Change version reference

a23dfdb

Add more assert message to help debug

7c3c064

Merge remote-tracking branch 'upstream/release/dev17.9' into dev/shec…

b09f0e1

…h/smartRename

CyrusNajmabadi reviewed Oct 20, 2023

View reviewed changes

Cosifne added 3 commits October 23, 2023 09:53

Use another event

394f56a

Skip the broken test

2a20482

Reset TestState.vb

c618e5c

Cosifne commented Oct 23, 2023

View reviewed changes

ryzngard reviewed Oct 23, 2023

View reviewed changes

src/EditorFeatures/Core.Wpf/InlineRename/UI/SmartRename/SmartRenameViewModel.cs Outdated Show resolved Hide resolved

ryzngard approved these changes Oct 23, 2023

View reviewed changes

Add CompleteAsyncOperation

a5a4627

Cosifne merged commit 65b05fa into dotnet:release/dev17.9 Oct 24, 2023
22 of 27 checks passed

Cosifne deleted the dev/shech/smartRename branch October 24, 2023 04:03

allisonchou mentioned this pull request Oct 24, 2023

[Automated] PRs inserted in VS build main-34224.107 #70544

Closed

allisonchou mentioned this pull request Nov 17, 2023

[Automated] PRs inserted in VS build main-34317.88 #70885

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Connect inline rename UI to editor smart rename session #70420

Connect inline rename UI to editor smart rename session #70420

Cosifne commented Oct 17, 2023 •

edited

Loading

Cosifne commented Oct 19, 2023

Cosifne commented Oct 19, 2023

CyrusNajmabadi Oct 20, 2023

Cosifne Oct 20, 2023

ryzngard Oct 23, 2023

CyrusNajmabadi Oct 20, 2023

CyrusNajmabadi Oct 20, 2023

Cosifne Oct 20, 2023

ryzngard Oct 23, 2023

Cosifne Oct 23, 2023

CyrusNajmabadi Oct 20, 2023

Cosifne Oct 20, 2023

ryzngard Oct 23, 2023

CyrusNajmabadi Oct 20, 2023

CyrusNajmabadi commented Oct 20, 2023

Cosifne Oct 23, 2023

Cosifne commented Oct 24, 2023

		@@ -0,0 +1,91 @@
		<UserControl x:Class="Microsoft.CodeAnalysis.InlineRename.UI.SmartRename.SmartRenameControl"

Connect inline rename UI to editor smart rename session #70420

Connect inline rename UI to editor smart rename session #70420

Conversation

Cosifne commented Oct 17, 2023 • edited Loading

Cosifne commented Oct 19, 2023

Cosifne commented Oct 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Oct 20, 2023

Choose a reason for hiding this comment

Cosifne commented Oct 24, 2023

Cosifne commented Oct 17, 2023 •

edited

Loading