Merge pull request #368 from xamarin/ermau-fix-collection-type-selection

Fix "add ___" popup cancellation combobox results

3 files changed, 112 insertions, 34 deletions

diff --git a/Xamarin.PropertyEditing.Tests/CollectionPropertyViewModelTests.cs b/Xamarin.PropertyEditing.Tests/CollectionPropertyViewModelTests.cs
index c8e8cde..0139aad 100644
--- a/Xamarin.PropertyEditing.Tests/CollectionPropertyViewModelTests.cs
+++ b/Xamarin.PropertyEditing.Tests/CollectionPropertyViewModelTests.cs
@@ -2,9 +2,11 @@ using System;
 using System.Collections;
 using System.Collections.Specialized;
 using System.Linq;
+using System.Threading;
 using System.Threading.Tasks;
 using Moq;
 using NUnit.Framework;
+using Xamarin.PropertyEditing.Properties;
 using Xamarin.PropertyEditing.Reflection;
 using Xamarin.PropertyEditing.Tests.MockControls;
 using Xamarin.PropertyEditing.ViewModels;
@@ -14,8 +16,24 @@ namespace Xamarin.PropertyEditing.Tests
 	[TestFixture]
 	internal class CollectionPropertyViewModelTests
 	{
+		private AsyncSynchronizationContext syncContext;
+
+		[SetUp]
+		public void Setup ()
+		{
+			this.syncContext = new AsyncSynchronizationContext ();
+			SynchronizationContext.SetSynchronizationContext (this.syncContext);
+		}
+
+		[TearDown]
+		public void TearDown ()
+		{
+			this.syncContext.WaitForPendingOperationsToComplete ();
+			SynchronizationContext.SetSynchronizationContext (null);
+		}
+
 		[Test]
-		public async Task AddType ()
+		public void AddType ()
 		{
 			TargetPlatform platform = new TargetPlatform (new MockEditorProvider());
 			var obj = new {
@@ -30,14 +48,14 @@ namespace Xamarin.PropertyEditing.Tests
 				e.SelectedType = buttonType;
 			};
 
-			await vm.AssignableTypes.Task;
+			vm.AssignableTypes.Task.Wait();
 			vm.SelectedType = vm.SuggestedTypes.First ();
 
 			Assert.That (vm.SuggestedTypes, Contains.Item (buttonType));
 		}
 
 		[Test]
-		public async Task AddTarget ()
+		public void AddTarget ()
 		{
 			TargetPlatform platform = new TargetPlatform (new MockEditorProvider());
 
@@ -48,7 +66,7 @@ namespace Xamarin.PropertyEditing.Tests
 			var editor = new ReflectionObjectEditor (obj);
 			
 			var vm = new CollectionPropertyViewModel (platform, editor.Properties.First(), new[] { editor });
-			await vm.AssignableTypes.Task;
+			vm.AssignableTypes.Task.Wait();
 
 			vm.SelectedType = GetTypeInfo (typeof(MockWpfButton));
 			vm.AddTargetCommand.Execute (null);
@@ -58,6 +76,63 @@ namespace Xamarin.PropertyEditing.Tests
 		}
 
 		[Test]
+		public async Task AddTargetCanAdd ()
+		{
+			TargetPlatform platform = new TargetPlatform (new MockEditorProvider ());
+
+			var obj = new {
+				Collection = new ArrayList ()
+			};
+
+			var editor = new ReflectionObjectEditor (obj);
+
+			var vm = new CollectionPropertyViewModel (platform, editor.Properties.First (), new[] { editor });
+			await vm.AssignableTypes.Task;
+
+			Assume.That (vm.SelectedType, Is.Null);
+			Assert.That (vm.AddTargetCommand.CanExecute (null), Is.False);
+
+			bool changed = false;
+			vm.AddTargetCommand.CanExecuteChanged += (o, e) => { changed = true; };
+			vm.SelectedType = GetTypeInfo (typeof (MockWpfButton));
+			Assert.That (vm.AddTargetCommand.CanExecute (null), Is.True);
+			Assert.That (changed, Is.True, "CanExecuteChanged did not fire");
+
+			changed = false;
+			vm.SelectedType = null;
+			Assert.That (vm.AddTargetCommand.CanExecute (null), Is.False);
+			Assert.That (changed, Is.True, "CanExecuteChanged did not fire");
+		}
+
+		[Test]
+		[Description ("When selecting a new type, if it's canceled the suggested type should return to something else")]
+		public void AddTypeCanceled ()
+		{
+			TargetPlatform platform = new TargetPlatform (new MockEditorProvider ());
+
+			var obj = new {
+				Collection = new ArrayList ()
+			};
+
+			var editor = new ReflectionObjectEditor (obj);
+
+			var vm = new CollectionPropertyViewModel (platform, editor.Properties.First (), new[] { editor });
+			vm.AssignableTypes.Task.Wait();
+
+			var buttonType = GetTypeInfo (typeof (MockWpfButton));
+			vm.TypeRequested += (o, e) => {
+				e.SelectedType = buttonType;
+			};
+
+			vm.SelectedType = vm.SuggestedTypes.Last ();
+			Assume.That (vm.SuggestedTypes, Contains.Item (buttonType));
+
+			buttonType = null;
+			vm.SelectedType = vm.SuggestedTypes.Last ();
+			Assert.That (vm.SelectedType, Is.EqualTo (vm.SuggestedTypes.First ()));
+		}
+
+		[Test]
 		public async Task RemoveTarget ()
 		{
 			TargetPlatform platform = new TargetPlatform (new MockEditorProvider());
diff --git a/Xamarin.PropertyEditing/ViewModels/CollectionPropertyViewModel.cs b/Xamarin.PropertyEditing/ViewModels/CollectionPropertyViewModel.cs
index 357cf63..641e19e 100644
--- a/Xamarin.PropertyEditing/ViewModels/CollectionPropertyViewModel.cs
+++ b/Xamarin.PropertyEditing/ViewModels/CollectionPropertyViewModel.cs
@@ -3,6 +3,7 @@ using System.Collections;
 using System.Collections.Generic;
 using System.Collections.Specialized;
 using System.Linq;
+using System.Threading;
 using System.Threading.Tasks;
 using System.Windows.Input;
 using Xamarin.PropertyEditing.Properties;
@@ -67,7 +68,7 @@ namespace Xamarin.PropertyEditing.ViewModels
 				AutoExpand = true
 			};
 
-			AddTargetCommand = new RelayCommand (OnAddTarget);
+			AddTargetCommand = new RelayCommand (OnAddTarget, CanAddTarget);
 			RemoveTargetCommand = new RelayCommand (OnRemoveTarget, CanAffectTarget);
 			MoveUpCommand = new RelayCommand (() => MoveTarget (up: true), () => CanMoveTarget (up: true));
 			MoveDownCommand = new RelayCommand (() => MoveTarget (up: false), () => CanMoveTarget (up: false));
@@ -144,11 +145,14 @@ namespace Xamarin.PropertyEditing.ViewModels
 				if (this.selectedType == value)
 					return;
 
+				ITypeInfo previousType = this.selectedType;
 				this.selectedType = value;
 				OnPropertyChanged();
 
 				if (value == OtherType)
-					RequestOtherType ();
+					RequestOtherType (previousType);
+
+				((RelayCommand)AddTargetCommand).ChangeCanExecute();
 			}
 		}
 
@@ -340,22 +344,22 @@ namespace Xamarin.PropertyEditing.ViewModels
 			await PushValueAsync ();
 		}
 
-		private void RequestOtherType ()
+		private void RequestOtherType (ITypeInfo previousType)
 		{
 			var args = new TypeRequestedEventArgs();
 			TypeRequested?.Invoke (this, args);
 
-			if (args.SelectedType == null) {
-				// We know we have OtherType because we're in this method, and we know its at the bottom
-				SelectedType = SuggestedTypes.Count > 1 ? SuggestedTypes[0] : null;
-				return;
-			}
-
-			if (!this.suggestedTypes.Contains (args.SelectedType)) {
-				this.suggestedTypes.Insert (0, args.SelectedType);
+			ITypeInfo st = args.SelectedType;
+			if (args.SelectedType != null) {
+				if (!this.suggestedTypes.Contains (args.SelectedType))
+					this.suggestedTypes.Insert (0, args.SelectedType);
+			} else {
+				st = previousType;
 			}
 
-			SelectedType = args.SelectedType;
+			// Fixes an issue in Windows where ComboBox won't recognize inline selection changes without modifying the list.
+			SynchronizationContext.Current.Post (o =>
+				SelectedType = (ITypeInfo) o, st);
 		}
 
 		private void OnCollectionViewContentsChanged (object sender, NotifyCollectionChangedEventArgs e)
@@ -392,6 +396,11 @@ namespace Xamarin.PropertyEditing.ViewModels
 			ReIndex();
 		}
 
+		private bool CanAddTarget ()
+		{
+			return SelectedType != null && SelectedType != OtherType;
+		}
+
 		private async void OnAddTarget ()
 		{
 			object target = await TargetPlatform.EditorProvider.CreateObjectAsync (SelectedType);
diff --git a/Xamarin.PropertyEditing/ViewModels/CreateBindingViewModel.cs b/Xamarin.PropertyEditing/ViewModels/CreateBindingViewModel.cs
index 245da34..95610bb 100644
--- a/Xamarin.PropertyEditing/ViewModels/CreateBindingViewModel.cs
+++ b/Xamarin.PropertyEditing/ViewModels/CreateBindingViewModel.cs
@@ -3,6 +3,7 @@ using System.Collections.Generic;
 using System.ComponentModel;
 using System.Linq;
 using System.Text;
+using System.Threading;
 using System.Threading.Tasks;
 using Xamarin.PropertyEditing.Properties;
 
@@ -375,7 +376,8 @@ namespace Xamarin.PropertyEditing.ViewModels
 			set
 			{
 				var vm = GetKnownPropertyViewModel (PropertyBinding.ConverterProperty);
-				if (vm.Value == value)
+				var previous = vm.Value;
+				if (previous == value)
 					return;
 
 				if (value == NoValueConverter)
@@ -385,11 +387,7 @@ namespace Xamarin.PropertyEditing.ViewModels
 				OnPropertyChanged();
 
 				if (value == AddValueConverter) {
-					var request = RequestCreateValueConverter ();
-					if (request.ConverterType != null)
-						RequestCreateRequestedValueConverter (request);
-					else
-						SelectedValueConverter = NoValueConverter;
+					RequestCreateValueConverter (previous);
 				}
 			}
 		}
@@ -636,27 +634,23 @@ namespace Xamarin.PropertyEditing.ViewModels
 			Path = newPath;
 		}
 
-		private CreateValueConverterEventArgs RequestCreateValueConverter ()
+		private async void RequestCreateValueConverter (Resource previous)
 		{
-			var e = new CreateValueConverterEventArgs();
-			CreateValueConverterRequested?.Invoke (this, e);
-			return e;
-		}
+			var request = new CreateValueConverterEventArgs ();
+			CreateValueConverterRequested?.Invoke (this, request);
 
-		private async void RequestCreateRequestedValueConverter (CreateValueConverterEventArgs e)
-		{
-			if (e.ConverterType == null || e.Name == null) {
-				SelectedValueConverter = NoValueConverter;
+			if (request.ConverterType == null || request.Name == null) {
+				SynchronizationContext.Current.Post (p => SelectedValueConverter = (Resource)p, previous);
 				return;
 			}
 
-			object converter = await TargetPlatform.EditorProvider.CreateObjectAsync (e.ConverterType);
-			if (e.Source == null) {
+			object converter = await TargetPlatform.EditorProvider.CreateObjectAsync (request.ConverterType);
+			if (request.Source == null) {
 				// TODO: Set directly outside of a resource
 				return;
 			}
 
-			Resource resource = await TargetPlatform.ResourceProvider.CreateResourceAsync (e.Source, e.Name, converter);
+			Resource resource = await TargetPlatform.ResourceProvider.CreateResourceAsync (request.Source, request.Name, converter);
 			this.valueConverters.Insert (this.valueConverters.Count - 1, resource);
 			SelectedValueConverter = resource;
 		}