Implement TrySubmit and remove TODOs from ChangeSignatureViewModelTests

src/EditorFeatures/VisualBasicTest/ChangeSignature/ReorderParameters.MultiFile.vb

@@ -52,7 +52,9 @@ class C
             Using testState = ChangeSignatureTestState.Create(workspace)
                 Dim history = testState.Workspace.GetService(Of ITextUndoHistoryRegistry)().RegisterHistory(testState.Workspace.Documents.First().GetTextBuffer())
                 testState.TestChangeSignatureOptionsService.IsCancelled = False
-                testState.TestChangeSignatureOptionsService.UpdatedSignature = permutation
+                testState.TestChangeSignatureOptionsService.UpdatedSignature = permutation.Select(Function(i) As AddedParameterOrExistingIndex
+                                                                                                      Return New AddedParameterOrExistingIndex(i)
+                                                                                                  End Function).ToArray()
                 Dim result = testState.ChangeSignature()
 
                 Dim vbdoc = result.UpdatedSolution.Projects.Single(Function(p) p.AssemblyName = "VBAssembly").Documents.Single()
@@ -106,7 +108,9 @@ class C
             Using testState = ChangeSignatureTestState.Create(workspace)
                 Dim history = testState.Workspace.GetService(Of ITextUndoHistoryRegistry)().RegisterHistory(testState.Workspace.Documents.First().GetTextBuffer())
                 testState.TestChangeSignatureOptionsService.IsCancelled = False
-                testState.TestChangeSignatureOptionsService.UpdatedSignature = permutation
+                testState.TestChangeSignatureOptionsService.UpdatedSignature = permutation.Select(Function(i) As AddedParameterOrExistingIndex
+                                                                                                      Return New AddedParameterOrExistingIndex(i)
+                                                                                                  End Function).ToArray()
                 Dim result = testState.ChangeSignature()
 
                 Dim vbdoc = result.UpdatedSolution.Projects.Single(Function(p) p.AssemblyName = "VBAssembly").Documents.Single()

src/VisualStudio/Core/Def/Implementation/ChangeSignature/ChangeSignatureDialog.xaml

@@ -326,7 +326,6 @@
                     Padding="{StaticResource ResourceKey=okCancelButtonPadding}"
                     Click="OK_Click" 
                     IsDefault="True"
-                    IsEnabled="{Binding IsOkButtonEnabled, Mode=OneWay}"
                     MinWidth="73"
                     MinHeight="21"/>
             <vs:DialogButton x:Uid="CancelButton" 

src/VisualStudio/Core/Def/Implementation/ChangeSignature/ChangeSignatureDialogViewModel.cs

@@ -221,7 +221,6 @@ internal void RemoveRestoreNotifyPropertyChanged()
             NotifyPropertyChanged(nameof(AllParameters));
             NotifyPropertyChanged(nameof(SignatureDisplay));
             NotifyPropertyChanged(nameof(SignaturePreviewAutomationText));
-            NotifyPropertyChanged(nameof(IsOkButtonEnabled));
             NotifyPropertyChanged(nameof(CanRemove));
             NotifyPropertyChanged(nameof(RemoveAutomationText));
             NotifyPropertyChanged(nameof(CanRestore));
@@ -425,12 +424,14 @@ private void Move(List<ParameterViewModel> list, int index, int delta)
             NotifyPropertyChanged(nameof(AllParameters));
             NotifyPropertyChanged(nameof(SignatureDisplay));
             NotifyPropertyChanged(nameof(SignaturePreviewAutomationText));
-            NotifyPropertyChanged(nameof(IsOkButtonEnabled));
         }
 
         internal bool TrySubmit()
         {
-            return IsOkButtonEnabled;
+            return AllParameters.Any(p => p.IsRemoved) ||
+                AllParameters.Any(p => p is AddedParameterViewModel) ||
+            !_parametersWithoutDefaultValues.Select(p => p.ParameterSymbol).SequenceEqual(_originalParameterConfiguration.ParametersWithoutDefaultValues.Cast<ExistingParameter>().Select(p => p.Symbol)) ||
+            !_parametersWithDefaultValues.Select(p => p.ParameterSymbol).SequenceEqual(_originalParameterConfiguration.RemainingEditableParameters.Cast<ExistingParameter>().Select(p => p.Symbol));
         }
 
         private bool IsDisabled(ParameterViewModel parameterViewModel)
@@ -438,14 +439,6 @@ private bool IsDisabled(ParameterViewModel parameterViewModel)
             return _disabledParameters.Contains(parameterViewModel);
         }
 
-        public bool IsOkButtonEnabled
-        {
-            get
-            {
-                return true;
-            }
-        }
-
         private int? _selectedIndex;
         public int? SelectedIndex
         {
@@ -561,6 +554,9 @@ public ParameterViewModel(ChangeSignatureDialogViewModel changeSignatureDialogVi
             internal abstract Parameter CreateParameter();
 
             public abstract string InitialIndex { get; }
+            public abstract string Modifier { get; }
+            public abstract string Default { get; }
+            public abstract IParameterSymbol ParameterSymbol { get; }
         }
 
         public class AddedParameterViewModel : ParameterViewModel
@@ -587,11 +583,20 @@ internal override Parameter CreateParameter()
                 => new AddedParameter(Type, Parameter, Callsite);
 
             public override string InitialIndex => "NEW";
+
+            // Newly added parameters cannot have modifiers yet
+            public override string Modifier => string.Empty;
+
+            // Only required parameters are supported currently
+            public override string Default => string.Empty;
+
+            // Use null as a marker that this is an added parameter
+            public override IParameterSymbol ParameterSymbol => null;
         }
 
         public class ExistingParameterViewModel : ParameterViewModel
         {
-            public IParameterSymbol ParameterSymbol { get; }
+            public override IParameterSymbol ParameterSymbol { get; }
 
             public ExistingParameterViewModel(ChangeSignatureDialogViewModel changeSignatureDialogViewModel, Parameter parameter, int initialIndex)
                 : base(changeSignatureDialogViewModel)
@@ -611,7 +616,7 @@ internal override Parameter CreateParameter()
 
             public override string InitialIndex { get; }
 
-            public string Modifier
+            public override string Modifier
             {
                 get
                 {
@@ -656,7 +661,7 @@ string ModifierText(string @out = default, string @ref = default, string @in = d
 
             public override string Parameter => ParameterSymbol.Name;
 
-            public string Default
+            public override string Default
             {
                 get
                 {

src/VisualStudio/Core/Test/ChangeSignature/ChangeSignatureViewModelTests.vb

@@ -42,7 +42,7 @@ class MyClass
             VerifyAlteredState(
                 viewModelTestState,
                 monitor,
-                isOkButtonEnabled:=False,
+                canCommit:=False,
                 canMoveUp:=False,
                 canMoveDown:=False)
 
@@ -50,7 +50,7 @@ class MyClass
         End Function
 
         <Fact, Trait(Traits.Feature, Traits.Features.ChangeSignature)>
-        Public Async Function ReorderParameters_MethodWithTwoNormalParameters_OkButtonNotOfferedAfterPermutationsResultingInOriginalOrdering() As Tasks.Task
+        Public Async Function ReorderParameters_MethodWithTwoNormalParameters_CannotCommitAfterPermutationsResultingInOriginalOrdering() As Tasks.Task
             Dim markup = <Text><![CDATA[
 class MyClass
 {
@@ -71,7 +71,7 @@ class MyClass
 
             VerifyAlteredState(
                 viewModelTestState,
-                isOkButtonEnabled:=False,
+                canCommit:=False,
                 canMoveUp:=False,
                 canMoveDown:=True)
         End Function
@@ -91,7 +91,6 @@ class MyClass
             VerifyOpeningState(viewModel, "public void M(int x, string y)")
 
             Dim monitor = New PropertyChangedTestMonitor(viewModel)
-            monitor.AddExpectation(Function() viewModel.IsOkButtonEnabled)
             monitor.AddExpectation(Function() viewModel.SignatureDisplay)
             monitor.AddExpectation(Function() viewModel.SignaturePreviewAutomationText)
             monitor.AddExpectation(Function() viewModel.AllParameters)
@@ -105,7 +104,7 @@ class MyClass
             VerifyAlteredState(
                 viewModelTestState,
                 monitor,
-                isOkButtonEnabled:=True,
+                canCommit:=True,
                 canMoveUp:=True,
                 canMoveDown:=False,
                 canEdit:=True,
@@ -130,7 +129,6 @@ class MyClass
             VerifyOpeningState(viewModel, "public void M(int x, string y)")
 
             Dim monitor = New PropertyChangedTestMonitor(viewModel)
-            monitor.AddExpectation(Function() viewModel.IsOkButtonEnabled)
             monitor.AddExpectation(Function() viewModel.SignatureDisplay)
             monitor.AddExpectation(Function() viewModel.SignaturePreviewAutomationText)
             monitor.AddExpectation(Function() viewModel.AllParameters)
@@ -146,7 +144,7 @@ class MyClass
             VerifyAlteredState(
                 viewModelTestState,
                 monitor,
-                isOkButtonEnabled:=True,
+                canCommit:=True,
                 canMoveUp:=False,
                 canMoveDown:=True,
                 canEdit:=False,
@@ -193,7 +191,6 @@ class MyClass
                 canMoveDown:=False)
 
             Dim monitor = New PropertyChangedTestMonitor(viewModel)
-            monitor.AddExpectation(Function() viewModel.IsOkButtonEnabled)
             monitor.AddExpectation(Function() viewModel.SignatureDisplay)
             monitor.AddExpectation(Function() viewModel.SignaturePreviewAutomationText)
             monitor.AddExpectation(Function() viewModel.AllParameters)
@@ -207,7 +204,7 @@ class MyClass
             VerifyAlteredState(
                 viewModelTestState,
                 monitor,
-                isOkButtonEnabled:=True,
+                canCommit:=True,
                 canMoveUp:=False,
                 canMoveDown:=True,
                 permutation:={1, 0},
@@ -300,10 +297,30 @@ class MyClass
                 canEdit:=False)
         End Function
 
+        <Fact, Trait(Traits.Feature, Traits.Features.ChangeSignature)>
+        Public Async Function TestRefKindsDisplayedCorrectly() As Tasks.Task
+            Dim includedInTest = {RefKind.None, RefKind.Ref, RefKind.Out, RefKind.In, RefKind.RefReadOnly}
+            Assert.Equal(includedInTest, EnumUtilities.GetValues(Of RefKind)())
+
+            Dim markup = <Text><![CDATA[
+class Test
+{
+    private void Method$$(int p1, ref int p2, in int p3, out int p4) { }
+}"]]></Text>
+
+            Dim state = Await GetViewModelTestStateAsync(markup, LanguageNames.CSharp)
+            VerifyOpeningState(state.ViewModel, "private void Method(int p1, ref int p2, in int p3, out int p4)")
+
+            Assert.Equal("", state.ViewModel.AllParameters(0).Modifier)
+            Assert.Equal("ref", state.ViewModel.AllParameters(1).Modifier)
+            Assert.Equal("in", state.ViewModel.AllParameters(2).Modifier)
+            Assert.Equal("out", state.ViewModel.AllParameters(3).Modifier)
+        End Function
+
         Private Sub VerifyAlteredState(
            viewModelTestState As ChangeSignatureViewModelTestState,
            Optional monitor As PropertyChangedTestMonitor = Nothing,
-           Optional isOkButtonEnabled As Boolean? = Nothing,
+           Optional canCommit As Boolean? = Nothing,
            Optional canMoveUp As Boolean? = Nothing,
            Optional canMoveDown As Boolean? = Nothing,
            Optional canRemove As Boolean? = Nothing,
@@ -318,8 +335,8 @@ class MyClass
                 monitor.VerifyExpectations()
             End If
 
-            If isOkButtonEnabled IsNot Nothing Then
-                Assert.Equal(isOkButtonEnabled, viewModel.IsOkButtonEnabled)
+            If canCommit IsNot Nothing Then
+                Assert.Equal(canCommit, viewModel.TrySubmit())
             End If
 
             If canMoveUp IsNot Nothing Then
@@ -353,22 +370,19 @@ class MyClass
         End Sub
 
         Private Sub AssertPermuted(permutation As Integer(), actualParameterList As List(Of ChangeSignatureDialogViewModel.ParameterViewModel), originalParameterList As ImmutableArray(Of IParameterSymbol))
-            ' TODO
-            'Dim finalParameterList = actualParameterList.Where(Function(p) Not p.IsRemoved)
-            'For index = 0 To permutation.Length - 1
-            '    Dim expected = originalParameterList(permutation(index))
-            '    Assert.Equal(expected, finalParameterList(index).ParameterSymbol)
-            'Next
+            Dim finalParameterList = actualParameterList.Where(Function(p) Not p.IsRemoved)
+            For index = 0 To permutation.Length - 1
+                Dim expected = originalParameterList(permutation(index))
+                Assert.Equal(expected, finalParameterList(index).ParameterSymbol)
+            Next
         End Sub
 
         Private Sub VerifyOpeningState(viewModel As ChangeSignatureDialogViewModel, openingSignatureDisplay As String)
             Assert.Equal(openingSignatureDisplay, viewModel.TEST_GetSignatureDisplayText())
-            Assert.False(viewModel.IsOkButtonEnabled)
             Assert.False(viewModel.TrySubmit)
             Assert.False(viewModel.CanMoveUp)
         End Sub
 
-        ' TODO
         Private Sub VerifyParameterInfo(
             viewModel As ChangeSignatureDialogViewModel,
                 parameterIndex As Integer,
@@ -377,40 +391,33 @@ class MyClass
                 Optional parameterName As String = Nothing,
                 Optional defaultValue As String = Nothing,
                 Optional isDisabled As Boolean? = Nothing,
-                Optional isRemoved As Boolean? = Nothing,
-                Optional needsBottomBorder As Boolean? = Nothing)
+                Optional isRemoved As Boolean? = Nothing)
 
-            '    Dim parameter = viewModel.AllParameters(parameterIndex)
+            Dim parameter = viewModel.AllParameters(parameterIndex)
 
-            '    If modifier IsNot Nothing Then
-            '        Assert.Equal(modifier, parameter.Modifier)
-            '    End If
-
-            '    If type IsNot Nothing Then
-            '        Assert.Equal(type, parameter.Type)
-            '    End If
-
-            '    If parameterName IsNot Nothing Then
-            '        Assert.Equal(parameterName, parameter.Parameter)
-            '    End If
-
-            '    If defaultValue IsNot Nothing Then
-            '        Assert.Equal(defaultValue, parameter.Default)
-            '    End If
+            If modifier IsNot Nothing Then
+                Assert.Equal(modifier, parameter.Modifier)
+            End If
 
-            '    If isDisabled.HasValue Then
-            '        Assert.Equal(isDisabled.Value, parameter.IsDisabled)
-            '    End If
+            If type IsNot Nothing Then
+                Assert.Equal(type, parameter.Type)
+            End If
 
-            '    If isRemoved.HasValue Then
-            '        Assert.Equal(isRemoved.Value, parameter.IsRemoved)
-            '    End If
+            If parameterName IsNot Nothing Then
+                Assert.Equal(parameterName, parameter.Parameter)
+            End If
 
-            '    If needsBottomBorder.HasValue Then
-            '        Assert.Equal(needsBottomBorder.Value, parameter.NeedsBottomBorder)
-            '    End If
+            If defaultValue IsNot Nothing Then
+                Assert.Equal(defaultValue, parameter.Default)
+            End If
 
+            If isDisabled.HasValue Then
+                Assert.Equal(isDisabled.Value, parameter.IsDisabled)
+            End If
 
+            If isRemoved.HasValue Then
+                Assert.Equal(isRemoved.Value, parameter.IsRemoved)
+            End If
         End Sub
 
         Private Async Function GetViewModelTestStateAsync(
@@ -445,26 +452,5 @@ class MyClass
                 Return New ChangeSignatureViewModelTestState(viewModel, symbol.GetParameters())
             End Using
         End Function
-
-        <Fact, Trait(Traits.Feature, Traits.Features.ChangeSignature)>
-        Public Async Function TestRefKindsDisplayedCorrectly() As Tasks.Task
-            Dim includedInTest = {RefKind.None, RefKind.Ref, RefKind.Out, RefKind.In, RefKind.RefReadOnly}
-            Assert.Equal(includedInTest, EnumUtilities.GetValues(Of RefKind)())
-
-            Dim markup = <Text><![CDATA[
-class Test
-{
-    private void Method$$(int p1, ref int p2, in int p3, out int p4) { }
-}"]]></Text>
-
-            Dim state = Await GetViewModelTestStateAsync(markup, LanguageNames.CSharp)
-            VerifyOpeningState(state.ViewModel, "private void Method(int p1, ref int p2, in int p3, out int p4)")
-
-            'TODO
-            'Assert.Equal("", state.ViewModel.AllParameters(0).Modifier)
-            'Assert.Equal("ref", state.ViewModel.AllParameters(1).Modifier)
-            'Assert.Equal("in", state.ViewModel.AllParameters(2).Modifier)
-            'Assert.Equal("out", state.ViewModel.AllParameters(3).Modifier)
-        End Function
     End Class
 End Namespace