From 0e3b4e0b641253fcd6736d6dd5ba61f21d6b4ead Mon Sep 17 00:00:00 2001 From: michael-hawker <24302614+michael-hawker@users.noreply.github.com> Date: Thu, 20 Oct 2022 14:00:22 -0700 Subject: [PATCH 1/4] Use new C# new initialization for vars --- .../Converters/Test_TaskResultConverter.cs | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/UnitTests/UnitTests.UWP/Converters/Test_TaskResultConverter.cs b/UnitTests/UnitTests.UWP/Converters/Test_TaskResultConverter.cs index 3c615928ac5..23f9768de1c 100644 --- a/UnitTests/UnitTests.UWP/Converters/Test_TaskResultConverter.cs +++ b/UnitTests/UnitTests.UWP/Converters/Test_TaskResultConverter.cs @@ -19,9 +19,9 @@ public class Test_TaskResultConverter [UITestMethod] public void Test_TaskResultConverter_Instance_Int32() { - var converter = new TaskResultConverter(); + TaskResultConverter converter = new(); - TaskCompletionSource tcs = new TaskCompletionSource(); + TaskCompletionSource tcs = new(); Assert.AreEqual(null, converter.Convert(tcs.Task, null, null, null)); @@ -46,9 +46,9 @@ public void Test_TaskResultConverter_Instance_Int32() [UITestMethod] public void Test_TaskResultConverter_Instance_String() { - var converter = new TaskResultConverter(); + TaskResultConverter converter = new(); - TaskCompletionSource tcs = new TaskCompletionSource(); + TaskCompletionSource tcs = new(); Assert.AreEqual(null, converter.Convert(tcs.Task, null, null, null)); @@ -73,7 +73,7 @@ public void Test_TaskResultConverter_Instance_String() [UITestMethod] public void Test_TaskResultConverter_Instance_UnsetValue() { - var converter = new TaskResultConverter(); + TaskResultConverter converter = new(); Assert.AreEqual(DependencyProperty.UnsetValue, converter.Convert(null, null, null, null)); Assert.AreEqual(DependencyProperty.UnsetValue, converter.Convert("Hello world", null, null, null)); @@ -83,9 +83,9 @@ public void Test_TaskResultConverter_Instance_UnsetValue() [UITestMethod] public void Test_TaskResultConverter_Instance_Null() { - var converter = new TaskResultConverter(); + TaskResultConverter converter = new(); - var cts = new CancellationTokenSource(); + CancellationTokenSource cts = new(); cts.Cancel(); @@ -93,11 +93,11 @@ public void Test_TaskResultConverter_Instance_Null() Assert.AreEqual(null, converter.Convert(Task.FromException(new Exception()), null, null, null)); Assert.AreEqual(null, converter.Convert(Task.CompletedTask, null, null, null)); - TaskCompletionSource tcs1 = new TaskCompletionSource(); + TaskCompletionSource tcs1 = new(); Assert.AreEqual(null, converter.Convert(tcs1.Task, null, null, null)); - TaskCompletionSource tcs2 = new TaskCompletionSource(); + TaskCompletionSource tcs2 = new(); Assert.AreEqual(null, converter.Convert(tcs2.Task, null, null, null)); } @@ -107,7 +107,7 @@ public void Test_TaskResultConverter_Instance_Null() [ExpectedException(typeof(NotImplementedException))] public void Test_TaskResultConverter_Instance_ConvertBack() { - var converter = new TaskResultConverter(); + TaskResultConverter converter = new(); Assert.AreEqual(null, converter.ConvertBack(null, null, null, null)); } From b8ec35e080aa1de89003125a3a86955de661d9f4 Mon Sep 17 00:00:00 2001 From: michael-hawker <24302614+michael-hawker@users.noreply.github.com> Date: Thu, 20 Oct 2022 15:14:44 -0700 Subject: [PATCH 2/4] Add new test cases which show issues with current TaskResultConverter. --- .../Converters/Test_TaskResultConverter.cs | 58 ++++++++++++------- 1 file changed, 36 insertions(+), 22 deletions(-) diff --git a/UnitTests/UnitTests.UWP/Converters/Test_TaskResultConverter.cs b/UnitTests/UnitTests.UWP/Converters/Test_TaskResultConverter.cs index 23f9768de1c..4df2d11ad17 100644 --- a/UnitTests/UnitTests.UWP/Converters/Test_TaskResultConverter.cs +++ b/UnitTests/UnitTests.UWP/Converters/Test_TaskResultConverter.cs @@ -23,23 +23,23 @@ public void Test_TaskResultConverter_Instance_Int32() TaskCompletionSource tcs = new(); - Assert.AreEqual(null, converter.Convert(tcs.Task, null, null, null)); + Assert.AreEqual(0, (int)converter.Convert(tcs.Task, typeof(int), null, null)); tcs.SetCanceled(); - Assert.AreEqual(null, converter.Convert(tcs.Task, null, null, null)); + Assert.AreEqual(0, (int)converter.Convert(tcs.Task, typeof(int), null, null)); tcs = new TaskCompletionSource(); tcs.SetException(new InvalidOperationException("Test")); - Assert.AreEqual(null, converter.Convert(tcs.Task, null, null, null)); + Assert.AreEqual(0, (int)converter.Convert(tcs.Task, typeof(int), null, null)); tcs = new TaskCompletionSource(); tcs.SetResult(42); - Assert.AreEqual(42, converter.Convert(tcs.Task, null, null, null)); + Assert.AreEqual(42, (int)converter.Convert(tcs.Task, typeof(int), null, null)); } [TestCategory("Converters")] @@ -50,38 +50,60 @@ public void Test_TaskResultConverter_Instance_String() TaskCompletionSource tcs = new(); - Assert.AreEqual(null, converter.Convert(tcs.Task, null, null, null)); + Assert.AreEqual(null, (string)converter.Convert(tcs.Task, typeof(string), null, null)); tcs.SetCanceled(); - Assert.AreEqual(null, converter.Convert(tcs.Task, null, null, null)); + Assert.AreEqual(null, (string)converter.Convert(tcs.Task, typeof(string), null, null)); - tcs = new TaskCompletionSource(); + tcs = new(); tcs.SetException(new InvalidOperationException("Test")); - Assert.AreEqual(null, converter.Convert(tcs.Task, null, null, null)); + Assert.AreEqual(null, (string)converter.Convert(tcs.Task, typeof(string), null, null)); - tcs = new TaskCompletionSource(); + tcs = new(); tcs.SetResult("Hello world"); - Assert.AreEqual("Hello world", converter.Convert(tcs.Task, null, null, null)); + Assert.AreEqual("Hello world", (string)converter.Convert(tcs.Task, typeof(string), null, null)); } [TestCategory("Converters")] [UITestMethod] - public void Test_TaskResultConverter_Instance_UnsetValue() + public void Test_TaskResultConverter_Instance_RawValue() { TaskResultConverter converter = new(); - Assert.AreEqual(DependencyProperty.UnsetValue, converter.Convert(null, null, null, null)); - Assert.AreEqual(DependencyProperty.UnsetValue, converter.Convert("Hello world", null, null, null)); + Assert.AreEqual(42, converter.Convert(42, null, null, null)); + + Assert.AreEqual(42, converter.Convert(42, typeof(int), null, null)); + + Assert.AreEqual("Hello world", converter.Convert("Hello world", null, null, null)); + + Assert.AreEqual("Hello world", converter.Convert("Hello world", typeof(string), null, null)); + } + + [TestCategory("Converters")] + [UITestMethod] + public void Test_TaskResultConverter_Instance_NullObject() + { + TaskResultConverter converter = new(); + + Assert.AreEqual(null, converter.Convert(null, null, null, null)); + + Assert.AreEqual(0, (int)converter.Convert(null, typeof(int), null, null)); + + Assert.AreEqual(false, (bool)converter.Convert(null, typeof(bool), null, null)); + + Assert.AreEqual(null, (int?)converter.Convert(null, typeof(int?), null, null)); + + Assert.AreEqual(null, (string)converter.Convert(null, typeof(string), null, null)); } [TestCategory("Converters")] [UITestMethod] - public void Test_TaskResultConverter_Instance_Null() + public void Test_TaskResultConverter_Instance_TaskNull() { TaskResultConverter converter = new(); @@ -92,14 +114,6 @@ public void Test_TaskResultConverter_Instance_Null() Assert.AreEqual(null, converter.Convert(Task.FromCanceled(cts.Token), null, null, null)); Assert.AreEqual(null, converter.Convert(Task.FromException(new Exception()), null, null, null)); Assert.AreEqual(null, converter.Convert(Task.CompletedTask, null, null, null)); - - TaskCompletionSource tcs1 = new(); - - Assert.AreEqual(null, converter.Convert(tcs1.Task, null, null, null)); - - TaskCompletionSource tcs2 = new(); - - Assert.AreEqual(null, converter.Convert(tcs2.Task, null, null, null)); } [TestCategory("Converters")] From 1398210158ce53c88e6f248210911f7e8db095f2 Mon Sep 17 00:00:00 2001 From: michael-hawker <24302614+michael-hawker@users.noreply.github.com> Date: Thu, 20 Oct 2022 15:19:09 -0700 Subject: [PATCH 3/4] Fixes #4505 - TaskResultConverter properly returns default value in case task is not set or has not completed. Updated description to include unset Task scenario (i.e. null being passed in) Updated to include proper default value return for Value types Included value passthru for non-Task values --- .../Converters/TaskResultConverter.cs | 21 ++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/Microsoft.Toolkit.Uwp.UI/Converters/TaskResultConverter.cs b/Microsoft.Toolkit.Uwp.UI/Converters/TaskResultConverter.cs index 373e1768ad5..085a7655ad6 100644 --- a/Microsoft.Toolkit.Uwp.UI/Converters/TaskResultConverter.cs +++ b/Microsoft.Toolkit.Uwp.UI/Converters/TaskResultConverter.cs @@ -15,19 +15,34 @@ namespace Microsoft.Toolkit.Uwp.UI.Converters /// completed yet will block the current thread and might cause a deadlock (eg. if the task was /// scheduled on the same synchronization context where the result is being retrieved from). /// The methods in this converter will safely return if the input - /// task is still running, or if it has faulted or has been canceled. + /// task is not set yet, still running, has faulted, or has been canceled. /// + /// Default values of C# types public sealed class TaskResultConverter : IValueConverter { /// public object Convert(object value, Type targetType, object parameter, string language) { + //// Check if we need to return a specific type, which only matters for value types where the default won't be null. + var hasValueTypeTarget = targetType is not null && targetType.IsValueType; + //// If we have a value type, then calculate it's default value to return, as we probably need it unless the task is completed. + var defaultValue = hasValueTypeTarget ? Activator.CreateInstance(targetType) : null; + if (value is Task task) { - return task.GetResultOrDefault(); + // If we have a task, check if we have a result now, otherwise the non-generic version of this + // function always returns null, so we want to use whatever we actually want the default value to be + // for our target type (in case it's a value type). + return task.GetResultOrDefault() ?? defaultValue; + } + else if (value is null) + { + // If we have a value type, return that value, otherwise this will be null. + return defaultValue; } - return DependencyProperty.UnsetValue; + // Otherwise, we'll just pass through whatever value/result was given to us. + return value; } /// From e3fe539d52660d9bd20e75aeb280304d5bc8ec93 Mon Sep 17 00:00:00 2001 From: michael-hawker <24302614+michael-hawker@users.noreply.github.com> Date: Thu, 20 Oct 2022 16:17:33 -0700 Subject: [PATCH 4/4] Remove ValueType support and reflection piece from TaskResultConverter Update description to explicitly call out returning null instead of default if no task or task result not ready. --- .../Converters/TaskResultConverter.cs | 16 +++------------- .../Converters/Test_TaskResultConverter.cs | 11 +++++++++-- 2 files changed, 12 insertions(+), 15 deletions(-) diff --git a/Microsoft.Toolkit.Uwp.UI/Converters/TaskResultConverter.cs b/Microsoft.Toolkit.Uwp.UI/Converters/TaskResultConverter.cs index 085a7655ad6..f381f5d4f1c 100644 --- a/Microsoft.Toolkit.Uwp.UI/Converters/TaskResultConverter.cs +++ b/Microsoft.Toolkit.Uwp.UI/Converters/TaskResultConverter.cs @@ -14,31 +14,21 @@ namespace Microsoft.Toolkit.Uwp.UI.Converters /// This is needed because accessing when the task has not /// completed yet will block the current thread and might cause a deadlock (eg. if the task was /// scheduled on the same synchronization context where the result is being retrieved from). - /// The methods in this converter will safely return if the input + /// The methods in this converter will safely return if the input /// task is not set yet, still running, has faulted, or has been canceled. /// - /// Default values of C# types public sealed class TaskResultConverter : IValueConverter { /// public object Convert(object value, Type targetType, object parameter, string language) { - //// Check if we need to return a specific type, which only matters for value types where the default won't be null. - var hasValueTypeTarget = targetType is not null && targetType.IsValueType; - //// If we have a value type, then calculate it's default value to return, as we probably need it unless the task is completed. - var defaultValue = hasValueTypeTarget ? Activator.CreateInstance(targetType) : null; - if (value is Task task) { - // If we have a task, check if we have a result now, otherwise the non-generic version of this - // function always returns null, so we want to use whatever we actually want the default value to be - // for our target type (in case it's a value type). - return task.GetResultOrDefault() ?? defaultValue; + return task.GetResultOrDefault(); } else if (value is null) { - // If we have a value type, return that value, otherwise this will be null. - return defaultValue; + return null; } // Otherwise, we'll just pass through whatever value/result was given to us. diff --git a/UnitTests/UnitTests.UWP/Converters/Test_TaskResultConverter.cs b/UnitTests/UnitTests.UWP/Converters/Test_TaskResultConverter.cs index 4df2d11ad17..de34f0c1d7a 100644 --- a/UnitTests/UnitTests.UWP/Converters/Test_TaskResultConverter.cs +++ b/UnitTests/UnitTests.UWP/Converters/Test_TaskResultConverter.cs @@ -17,6 +17,7 @@ public class Test_TaskResultConverter { [TestCategory("Converters")] [UITestMethod] + [Ignore] // Ignore this value type test. Behavior will return null currently and not default. public void Test_TaskResultConverter_Instance_Int32() { TaskResultConverter converter = new(); @@ -92,9 +93,15 @@ public void Test_TaskResultConverter_Instance_NullObject() Assert.AreEqual(null, converter.Convert(null, null, null, null)); - Assert.AreEqual(0, (int)converter.Convert(null, typeof(int), null, null)); + // TODO: Think there may still be a problem for value types in x:Bind expressions, represented by these tests here, + // but was going to be too big a change for 7.1.3, will have to get more feedback and evaluate later. + /*Assert.AreEqual(0, (int)converter.Convert(null, typeof(int), null, null)); - Assert.AreEqual(false, (bool)converter.Convert(null, typeof(bool), null, null)); + Assert.AreEqual(false, (bool)converter.Convert(null, typeof(bool), null, null));*/ + + Assert.AreEqual(null, converter.Convert(null, typeof(int), null, null)); + + Assert.AreEqual(null, converter.Convert(null, typeof(bool), null, null)); Assert.AreEqual(null, (int?)converter.Convert(null, typeof(int?), null, null));