From 4f6d51e1664b09aff1e6f4f6b92c82178d706bfb Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Fri, 16 Dec 2022 11:54:12 +0100 Subject: [PATCH 1/6] Add tests for property binding threading. - Added a failing tests for styled property binding with threading - Made direct property test actually confirm that binding was done on UI thread --- .../Avalonia.Base.UnitTests.csproj | 3 + .../AvaloniaObjectTests_Binding.cs | 115 ++++++++++++++++++ .../AvaloniaObjectTests_Direct.cs | 44 ++++--- 3 files changed, 148 insertions(+), 14 deletions(-) diff --git a/tests/Avalonia.Base.UnitTests/Avalonia.Base.UnitTests.csproj b/tests/Avalonia.Base.UnitTests/Avalonia.Base.UnitTests.csproj index 602e5e4496..0162f53f5e 100644 --- a/tests/Avalonia.Base.UnitTests/Avalonia.Base.UnitTests.csproj +++ b/tests/Avalonia.Base.UnitTests/Avalonia.Base.UnitTests.csproj @@ -17,6 +17,9 @@ + + + diff --git a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs index c42cb0241b..547b2cb176 100644 --- a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs +++ b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs @@ -12,6 +12,7 @@ using Avalonia.Platform; using Avalonia.Threading; using Avalonia.UnitTests; using Moq; +using Nito.AsyncEx; using Xunit; #nullable enable @@ -920,6 +921,120 @@ namespace Avalonia.Base.UnitTests } } + [Theory] + [InlineData(BindingPriority.LocalValue)] + [InlineData(BindingPriority.Style)] + public void Typed_Bind_Executes_On_UIThread(BindingPriority priority) + { + AsyncContext.Run(async () => + { + var target = new Class1(); + var source = new Subject(); + var currentThreadId = Thread.CurrentThread.ManagedThreadId; + var raised = 0; + + var threadingInterfaceMock = new Mock(); + threadingInterfaceMock.SetupGet(mock => mock.CurrentThreadIsLoopThread) + .Returns(() => Thread.CurrentThread.ManagedThreadId == currentThreadId); + + var services = new TestServices( + threadingInterface: threadingInterfaceMock.Object); + + target.PropertyChanged += (s, e) => + { + Assert.Equal(currentThreadId, Thread.CurrentThread.ManagedThreadId); + ++raised; + }; + + using (UnitTestApplication.Start(services)) + { + target.Bind(Class1.FooProperty, source, priority); + + await Task.Run(() => source.OnNext("foobar")); + Dispatcher.UIThread.RunJobs(); + + Assert.Equal("foobar", target.GetValue(Class1.FooProperty)); + Assert.Equal(1, raised); + } + }); + } + + [Theory] + [InlineData(BindingPriority.LocalValue)] + [InlineData(BindingPriority.Style)] + public void Untyped_Bind_Executes_On_UIThread(BindingPriority priority) + { + AsyncContext.Run(async () => + { + var target = new Class1(); + var source = new Subject(); + var currentThreadId = Thread.CurrentThread.ManagedThreadId; + var raised = 0; + + var threadingInterfaceMock = new Mock(); + threadingInterfaceMock.SetupGet(mock => mock.CurrentThreadIsLoopThread) + .Returns(() => Thread.CurrentThread.ManagedThreadId == currentThreadId); + + var services = new TestServices( + threadingInterface: threadingInterfaceMock.Object); + + target.PropertyChanged += (s, e) => + { + Assert.Equal(currentThreadId, Thread.CurrentThread.ManagedThreadId); + ++raised; + }; + + using (UnitTestApplication.Start(services)) + { + target.Bind(Class1.FooProperty, source, priority); + + await Task.Run(() => source.OnNext("foobar")); + Dispatcher.UIThread.RunJobs(); + + Assert.Equal("foobar", target.GetValue(Class1.FooProperty)); + Assert.Equal(1, raised); + } + }); + } + + [Theory] + [InlineData(BindingPriority.LocalValue)] + [InlineData(BindingPriority.Style)] + public void BindingValue_Bind_Executes_On_UIThread(BindingPriority priority) + { + AsyncContext.Run(async () => + { + var target = new Class1(); + var source = new Subject>(); + var currentThreadId = Thread.CurrentThread.ManagedThreadId; + var raised = 0; + + var threadingInterfaceMock = new Mock(); + threadingInterfaceMock.SetupGet(mock => mock.CurrentThreadIsLoopThread) + .Returns(() => Thread.CurrentThread.ManagedThreadId == currentThreadId); + + var services = new TestServices( + threadingInterface: threadingInterfaceMock.Object); + + target.PropertyChanged += (s, e) => + { + Assert.Equal(currentThreadId, Thread.CurrentThread.ManagedThreadId); + ++raised; + }; + + using (UnitTestApplication.Start(services)) + { + target.Bind(Class1.FooProperty, source, priority); + + await Task.Run(() => source.OnNext("foobar")); + Dispatcher.UIThread.RunJobs(); + + Assert.Equal("foobar", target.GetValue(Class1.FooProperty)); + Assert.Equal(1, raised); + } + }); + } + [Fact] public async Task Bind_With_Scheduler_Executes_On_Scheduler() { diff --git a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Direct.cs b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Direct.cs index 20172eea88..973090ee92 100644 --- a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Direct.cs +++ b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Direct.cs @@ -7,8 +7,10 @@ using System.Threading.Tasks; using Avalonia.Data; using Avalonia.Logging; using Avalonia.Platform; +using Avalonia.Threading; using Avalonia.UnitTests; using Moq; +using Nito.AsyncEx; using Xunit; namespace Avalonia.Base.UnitTests @@ -519,25 +521,39 @@ namespace Avalonia.Base.UnitTests } [Fact] - public async Task Bind_Executes_On_UIThread() + public void Bind_Executes_On_UIThread() { - var target = new Class1(); - var source = new Subject(); - var currentThreadId = Thread.CurrentThread.ManagedThreadId; + AsyncContext.Run(async () => + { + var target = new Class1(); + var source = new Subject(); + var currentThreadId = Thread.CurrentThread.ManagedThreadId; + var raised = 0; - var threadingInterfaceMock = new Mock(); - threadingInterfaceMock.SetupGet(mock => mock.CurrentThreadIsLoopThread) - .Returns(() => Thread.CurrentThread.ManagedThreadId == currentThreadId); + var threadingInterfaceMock = new Mock(); + threadingInterfaceMock.SetupGet(mock => mock.CurrentThreadIsLoopThread) + .Returns(() => Thread.CurrentThread.ManagedThreadId == currentThreadId); - var services = new TestServices( - threadingInterface: threadingInterfaceMock.Object); + var services = new TestServices( + threadingInterface: threadingInterfaceMock.Object); - using (UnitTestApplication.Start(services)) - { - target.Bind(Class1.FooProperty, source); + target.PropertyChanged += (s, e) => + { + Assert.Equal(currentThreadId, Thread.CurrentThread.ManagedThreadId); + ++raised; + }; - await Task.Run(() => source.OnNext("foobar")); - } + using (UnitTestApplication.Start(services)) + { + target.Bind(Class1.FooProperty, source); + + await Task.Run(() => source.OnNext("foobar")); + Dispatcher.UIThread.RunJobs(); + + Assert.Equal("foobar", target.Foo); + Assert.Equal(1, raised); + } + }); } [Fact] From 9d4b73df0bb4f9f2a687f334c724ed1b1ccf0540 Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Fri, 16 Dec 2022 12:15:00 +0100 Subject: [PATCH 2/6] Marshal bindings to UI thread. --- .../PropertyStore/BindingEntryBase.cs | 45 +++++++++++----- .../LocalValueBindingObserver.cs | 53 ++++++++++++++++--- .../LocalValueUntypedBindingObserver.cs | 53 +++++++++++++------ 3 files changed, 113 insertions(+), 38 deletions(-) diff --git a/src/Avalonia.Base/PropertyStore/BindingEntryBase.cs b/src/Avalonia.Base/PropertyStore/BindingEntryBase.cs index ef14211902..b5ac5c817d 100644 --- a/src/Avalonia.Base/PropertyStore/BindingEntryBase.cs +++ b/src/Avalonia.Base/PropertyStore/BindingEntryBase.cs @@ -2,6 +2,7 @@ using System.Collections.Generic; using System.Reactive.Disposables; using Avalonia.Data; +using Avalonia.Threading; namespace Avalonia.PropertyStore { @@ -116,26 +117,42 @@ namespace Avalonia.PropertyStore private void SetValue(BindingValue value) { - if (Frame.Owner is null) - return; + static void Execute(BindingEntryBase instance, BindingValue value) + { + if (instance.Frame.Owner is null) + return; - LoggingUtils.LogIfNecessary(Frame.Owner.Owner, Property, value); + LoggingUtils.LogIfNecessary(instance.Frame.Owner.Owner, instance.Property, value); - if (value.HasValue) - { - if (!_hasValue || !EqualityComparer.Default.Equals(_value, value.Value)) + if (value.HasValue) + { + if (!instance._hasValue || !EqualityComparer.Default.Equals(instance._value, value.Value)) + { + instance._value = value.Value; + instance._hasValue = true; + if (instance._subscription is not null && instance._subscription != s_creatingQuiet) + instance.Frame.Owner?.OnBindingValueChanged(instance, instance.Frame.Priority); + } + } + else if (value.Type != BindingValueType.DoNothing) { - _value = value.Value; - _hasValue = true; - if (_subscription is not null && _subscription != s_creatingQuiet) - Frame.Owner?.OnBindingValueChanged(this, Frame.Priority); + instance.ClearValue(); + if (instance._subscription is not null && instance._subscription != s_creatingQuiet) + instance.Frame.Owner?.OnBindingValueCleared(instance.Property, instance.Frame.Priority); } } - else if (value.Type != BindingValueType.DoNothing) + + if (Dispatcher.UIThread.CheckAccess()) { - ClearValue(); - if (_subscription is not null && _subscription != s_creatingQuiet) - Frame.Owner?.OnBindingValueCleared(Property, Frame.Priority); + Execute(this, value); + } + else + { + // To avoid allocating closure in the outer scope we need to capture variables + // locally. This allows us to skip most of the allocations when on UI thread. + var instance = this; + var newValue = value; + Dispatcher.UIThread.Post(() => Execute(instance, newValue)); } } diff --git a/src/Avalonia.Base/PropertyStore/LocalValueBindingObserver.cs b/src/Avalonia.Base/PropertyStore/LocalValueBindingObserver.cs index 4dca6c0100..8acb885604 100644 --- a/src/Avalonia.Base/PropertyStore/LocalValueBindingObserver.cs +++ b/src/Avalonia.Base/PropertyStore/LocalValueBindingObserver.cs @@ -1,5 +1,6 @@ using System; using Avalonia.Data; +using Avalonia.Threading; namespace Avalonia.PropertyStore { @@ -40,20 +41,56 @@ namespace Avalonia.PropertyStore public void OnNext(T value) { - if (Property.ValidateValue?.Invoke(value) != false) - _owner.SetValue(Property, value, BindingPriority.LocalValue); + static void Execute(ValueStore owner, StyledPropertyBase property, T value) + { + if (property.ValidateValue?.Invoke(value) != false) + owner.SetValue(property, value, BindingPriority.LocalValue); + else + owner.ClearLocalValue(property); + } + + if (Dispatcher.UIThread.CheckAccess()) + { + Execute(_owner, Property, value); + } else - _owner.ClearLocalValue(Property); + { + // To avoid allocating closure in the outer scope we need to capture variables + // locally. This allows us to skip most of the allocations when on UI thread. + var instance = _owner; + var property = Property; + var newValue = value; + Dispatcher.UIThread.Post(() => Execute(instance, property, newValue)); + } } public void OnNext(BindingValue value) { - LoggingUtils.LogIfNecessary(_owner.Owner, Property, value); + static void Execute(LocalValueBindingObserver instance, BindingValue value) + { + var owner = instance._owner; + var property = instance.Property; + + LoggingUtils.LogIfNecessary(owner.Owner, property, value); - if (value.HasValue) - _owner.SetValue(Property, value.Value, BindingPriority.LocalValue); - else if (value.Type != BindingValueType.DataValidationError) - _owner.ClearLocalValue(Property); + if (value.HasValue) + owner.SetValue(property, value.Value, BindingPriority.LocalValue); + else if (value.Type != BindingValueType.DataValidationError) + owner.ClearLocalValue(property); + } + + if (Dispatcher.UIThread.CheckAccess()) + { + Execute(this, value); + } + else + { + // To avoid allocating closure in the outer scope we need to capture variables + // locally. This allows us to skip most of the allocations when on UI thread. + var instance = this; + var newValue = value; + Dispatcher.UIThread.Post(() => Execute(instance, newValue)); + } } } } diff --git a/src/Avalonia.Base/PropertyStore/LocalValueUntypedBindingObserver.cs b/src/Avalonia.Base/PropertyStore/LocalValueUntypedBindingObserver.cs index 099e997d38..7c529591b6 100644 --- a/src/Avalonia.Base/PropertyStore/LocalValueUntypedBindingObserver.cs +++ b/src/Avalonia.Base/PropertyStore/LocalValueUntypedBindingObserver.cs @@ -1,5 +1,7 @@ using System; +using System.Security.Cryptography; using Avalonia.Data; +using Avalonia.Threading; namespace Avalonia.PropertyStore { @@ -34,28 +36,47 @@ namespace Avalonia.PropertyStore public void OnNext(object? value) { - if (value is BindingNotification n) + static void Execute(LocalValueUntypedBindingObserver instance, object? value) { - value = n.Value; - LoggingUtils.LogIfNecessary(_owner.Owner, Property, n); - } + var owner = instance._owner; + var property = instance.Property; - if (value == AvaloniaProperty.UnsetValue) - { - _owner.ClearLocalValue(Property); - } - else if (value == BindingOperations.DoNothing) - { - // Do nothing! + if (value is BindingNotification n) + { + value = n.Value; + LoggingUtils.LogIfNecessary(owner.Owner, property, n); + } + + if (value == AvaloniaProperty.UnsetValue) + { + owner.ClearLocalValue(property); + } + else if (value == BindingOperations.DoNothing) + { + // Do nothing! + } + else if (UntypedValueUtils.TryConvertAndValidate(property, value, out var typedValue)) + { + owner.SetValue(property, typedValue, BindingPriority.LocalValue); + } + else + { + owner.ClearLocalValue(property); + LoggingUtils.LogInvalidValue(owner.Owner, property, typeof(T), value); + } } - else if (UntypedValueUtils.TryConvertAndValidate(Property, value, out var typedValue)) + + if (Dispatcher.UIThread.CheckAccess()) { - _owner.SetValue(Property, typedValue, BindingPriority.LocalValue); + Execute(this, value); } - else + else if (value != BindingOperations.DoNothing) { - _owner.ClearLocalValue(Property); - LoggingUtils.LogInvalidValue(_owner.Owner, Property, typeof(T), value); + // To avoid allocating closure in the outer scope we need to capture variables + // locally. This allows us to skip most of the allocations when on UI thread. + var instance = this; + var newValue = value; + Dispatcher.UIThread.Post(() => Execute(instance, newValue)); } } } From 7db71d2f556416051f71c4ec5e719acdc4d72807 Mon Sep 17 00:00:00 2001 From: Max Katz Date: Fri, 16 Dec 2022 08:31:51 -0500 Subject: [PATCH 3/6] Remove ReflectionBinding from NativeMenuBar --- src/Avalonia.Controls/NativeMenuBar.cs | 9 -------- .../Controls/NativeMenuBar.xaml | 17 +++++++-------- .../Controls/NativeMenuBar.xaml | 21 +++++++++---------- 3 files changed, 18 insertions(+), 29 deletions(-) diff --git a/src/Avalonia.Controls/NativeMenuBar.cs b/src/Avalonia.Controls/NativeMenuBar.cs index 8b3e875e5a..ac7a45a547 100644 --- a/src/Avalonia.Controls/NativeMenuBar.cs +++ b/src/Avalonia.Controls/NativeMenuBar.cs @@ -23,15 +23,6 @@ namespace Avalonia.Controls }); } - [DynamicDependency(DynamicallyAccessedMemberTypes.All, typeof(NativeMenu))] - [DynamicDependency(DynamicallyAccessedMemberTypes.All, typeof(NativeMenuItem))] - [DynamicDependency(DynamicallyAccessedMemberTypes.All, typeof(NativeMenuItemBase))] - [DynamicDependency(DynamicallyAccessedMemberTypes.All, typeof(NativeMenuItemSeparator))] - public NativeMenuBar() - { - - } - public static void SetEnableMenuItemClickForwarding(MenuItem menuItem, bool enable) { menuItem.SetValue(EnableMenuItemClickForwardingProperty, enable); diff --git a/src/Avalonia.Themes.Fluent/Controls/NativeMenuBar.xaml b/src/Avalonia.Themes.Fluent/Controls/NativeMenuBar.xaml index 5c48c297b8..d5c95ab46c 100644 --- a/src/Avalonia.Themes.Fluent/Controls/NativeMenuBar.xaml +++ b/src/Avalonia.Themes.Fluent/Controls/NativeMenuBar.xaml @@ -9,17 +9,16 @@ IsVisible="{Binding !$parent[TopLevel].(NativeMenu.IsNativeMenuExported)}" Items="{Binding $parent[TopLevel].(NativeMenu.Menu).Items}"> - - diff --git a/src/Avalonia.Themes.Simple/Controls/NativeMenuBar.xaml b/src/Avalonia.Themes.Simple/Controls/NativeMenuBar.xaml index ba1b35e2ee..945622f22f 100644 --- a/src/Avalonia.Themes.Simple/Controls/NativeMenuBar.xaml +++ b/src/Avalonia.Themes.Simple/Controls/NativeMenuBar.xaml @@ -9,17 +9,16 @@ - - From 6b07b9016198e444850c17b8c2e4421d139e1d4b Mon Sep 17 00:00:00 2001 From: Dmitry Zhelnin Date: Fri, 16 Dec 2022 16:38:53 +0300 Subject: [PATCH 4/6] DataGridValueConverter: fix processing of nullable values --- .../DataGridValueConverter.cs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/Avalonia.Controls.DataGrid/DataGridValueConverter.cs b/src/Avalonia.Controls.DataGrid/DataGridValueConverter.cs index 6144679b60..c823ee1b9c 100644 --- a/src/Avalonia.Controls.DataGrid/DataGridValueConverter.cs +++ b/src/Avalonia.Controls.DataGrid/DataGridValueConverter.cs @@ -22,14 +22,18 @@ namespace Avalonia.Controls return DefaultValueConverter.Instance.Convert(value, targetType, parameter, culture); } - // This suppresses a warning saying that we should use String.IsNullOrEmpty instead of a string - // comparison, but in this case we want to explicitly check for Empty and not Null. + public object ConvertBack(object value, Type targetType, object parameter, CultureInfo culture) { if (targetType != null && targetType.IsNullableType()) { var strValue = value as string; - if (string.IsNullOrEmpty(strValue)) + + // This suppresses a warning saying that we should use String.IsNullOrEmpty instead of a string + // comparison, but in this case we want to explicitly check for Empty and not Null. +#pragma warning disable CA1820 + if (strValue == string.Empty) +#pragma warning restore CA1820 { return null; } From e0c2d5b63dcc92f5af44433e9e20c88cd9f0c8a4 Mon Sep 17 00:00:00 2001 From: Dmitry Zhelnin Date: Fri, 16 Dec 2022 18:29:34 +0300 Subject: [PATCH 5/6] DataGridColumn: change access modifier of OwningGrid property to protected internal --- src/Avalonia.Controls.DataGrid/DataGridColumn.cs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/Avalonia.Controls.DataGrid/DataGridColumn.cs b/src/Avalonia.Controls.DataGrid/DataGridColumn.cs index d417c87746..2b715d6a9f 100644 --- a/src/Avalonia.Controls.DataGrid/DataGridColumn.cs +++ b/src/Avalonia.Controls.DataGrid/DataGridColumn.cs @@ -50,10 +50,13 @@ namespace Avalonia.Controls InheritsWidth = true; } - internal DataGrid OwningGrid + /// + /// Gets the control that contains this column. + /// + protected internal DataGrid OwningGrid { get; - set; + internal set; } internal int Index From 04db818a477471ace38b684c5557dc486d438134 Mon Sep 17 00:00:00 2001 From: 0xDB6 <120456350+0xDB6@users.noreply.github.com> Date: Fri, 16 Dec 2022 16:30:56 +0100 Subject: [PATCH 6/6] Defer execution of UnregisterClass to fix RWM atom table leak (#9700) --- .../Avalonia.Win32/Interop/UnmanagedMethods.cs | 2 +- .../Avalonia.Win32/WindowImpl.AppWndProc.cs | 4 ++++ src/Windows/Avalonia.Win32/WindowImpl.cs | 15 +++++++++------ 3 files changed, 14 insertions(+), 7 deletions(-) diff --git a/src/Windows/Avalonia.Win32/Interop/UnmanagedMethods.cs b/src/Windows/Avalonia.Win32/Interop/UnmanagedMethods.cs index a7cca5b0f3..f828b156da 100644 --- a/src/Windows/Avalonia.Win32/Interop/UnmanagedMethods.cs +++ b/src/Windows/Avalonia.Win32/Interop/UnmanagedMethods.cs @@ -1409,7 +1409,7 @@ namespace Avalonia.Win32.Interop [DllImport("user32.dll")] public static extern bool TranslateMessage(ref MSG lpMsg); - [DllImport("user32.dll")] + [DllImport("user32.dll", CharSet = CharSet.Unicode)] public static extern bool UnregisterClass(string lpClassName, IntPtr hInstance); [DllImport("user32.dll", SetLastError = true, CharSet = CharSet.Unicode, EntryPoint = "SetWindowTextW")] diff --git a/src/Windows/Avalonia.Win32/WindowImpl.AppWndProc.cs b/src/Windows/Avalonia.Win32/WindowImpl.AppWndProc.cs index fe7d881f11..060366a8ac 100644 --- a/src/Windows/Avalonia.Win32/WindowImpl.AppWndProc.cs +++ b/src/Windows/Avalonia.Win32/WindowImpl.AppWndProc.cs @@ -10,6 +10,7 @@ using Avalonia.Controls.Remote; using Avalonia.Input; using Avalonia.Input.Raw; using Avalonia.Platform; +using Avalonia.Threading; using Avalonia.Win32.Automation; using Avalonia.Win32.Input; using Avalonia.Win32.Interop.Automation; @@ -106,6 +107,9 @@ namespace Avalonia.Win32 _touchDevice?.Dispose(); //Free other resources Dispose(); + + // Schedule cleanup of anything that requires window to be destroyed + Dispatcher.UIThread.Post(AfterCloseCleanup); return IntPtr.Zero; } diff --git a/src/Windows/Avalonia.Win32/WindowImpl.cs b/src/Windows/Avalonia.Win32/WindowImpl.cs index 9e11959101..fa4517dc92 100644 --- a/src/Windows/Avalonia.Win32/WindowImpl.cs +++ b/src/Windows/Avalonia.Win32/WindowImpl.cs @@ -643,12 +643,6 @@ namespace Avalonia.Win32 _hwnd = IntPtr.Zero; } - if (_className != null) - { - UnregisterClass(_className, GetModuleHandle(null)); - _className = null; - } - _framebuffer.Dispose(); } @@ -1144,6 +1138,15 @@ namespace Avalonia.Win32 } } + private void AfterCloseCleanup() + { + if (_className != null) + { + UnregisterClass(_className, GetModuleHandle(null)); + _className = null; + } + } + private void MaximizeWithoutCoveringTaskbar() { IntPtr monitor = MonitorFromWindow(_hwnd, MONITOR.MONITOR_DEFAULTTONEAREST);