From 38d68865fde632c113833d8148f312300c4cc1c8 Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Wed, 15 May 2019 17:14:44 +0200 Subject: [PATCH] Correctly handle command.CanExecute state. Added a new `IsEnabledCore` property to `InputElement` which is overridden in `Button` and `MenuItem` to override the `IsEffectivelyEnabled` state with the enabled state of the command. Also add data validation of the `Command` property to `MenuItem` to make it behave the same as `Button` when `Command` is bound to a non-existent property. Fixes #2501 --- src/Avalonia.Controls/Button.cs | 23 +++- src/Avalonia.Controls/MenuItem.cs | 36 ++++-- src/Avalonia.Input/InputElement.cs | 67 +++++------ .../ButtonTests.cs | 29 +++-- .../MenuItemTests.cs | 113 +++++++++++++++--- .../InputElement_Enabled.cs | 101 ++++++++++++++++ 6 files changed, 294 insertions(+), 75 deletions(-) create mode 100644 tests/Avalonia.Input.UnitTests/InputElement_Enabled.cs diff --git a/src/Avalonia.Controls/Button.cs b/src/Avalonia.Controls/Button.cs index cc9e6b7444..c47413c14b 100644 --- a/src/Avalonia.Controls/Button.cs +++ b/src/Avalonia.Controls/Button.cs @@ -33,8 +33,6 @@ namespace Avalonia.Controls /// public class Button : ContentControl { - private ICommand _command; - /// /// Defines the property. /// @@ -75,6 +73,9 @@ namespace Avalonia.Controls public static readonly StyledProperty IsPressedProperty = AvaloniaProperty.Register(nameof(IsPressed)); + private ICommand _command; + private bool _commandCanExecute = true; + /// /// Initializes static members of the class. /// @@ -147,6 +148,8 @@ namespace Avalonia.Controls private set { SetValue(IsPressedProperty, value); } } + protected override bool IsEnabledCore => base.IsEnabledCore && _commandCanExecute; + /// protected override void OnAttachedToVisualTree(VisualTreeAttachmentEventArgs e) { @@ -289,7 +292,11 @@ namespace Avalonia.Controls { if (status?.ErrorType == BindingErrorType.Error) { - IsEnabled = false; + if (_commandCanExecute) + { + _commandCanExecute = false; + UpdateIsEffectivelyEnabled(); + } } } } @@ -348,9 +355,13 @@ namespace Avalonia.Controls /// The event args. private void CanExecuteChanged(object sender, EventArgs e) { - // HACK: Just set the IsEnabled property for the moment. This needs to be changed to - // use IsEnabledCore etc. but it will do for now. - IsEnabled = Command == null || Command.CanExecute(CommandParameter); + var canExecute = Command == null || Command.CanExecute(CommandParameter); + + if (canExecute != _commandCanExecute) + { + _commandCanExecute = canExecute; + UpdateIsEffectivelyEnabled(); + } } /// diff --git a/src/Avalonia.Controls/MenuItem.cs b/src/Avalonia.Controls/MenuItem.cs index d8473dc613..4cd215c238 100644 --- a/src/Avalonia.Controls/MenuItem.cs +++ b/src/Avalonia.Controls/MenuItem.cs @@ -9,6 +9,7 @@ using Avalonia.Controls.Generators; using Avalonia.Controls.Mixins; using Avalonia.Controls.Primitives; using Avalonia.Controls.Templates; +using Avalonia.Data; using Avalonia.Input; using Avalonia.Interactivity; using Avalonia.LogicalTree; @@ -20,8 +21,6 @@ namespace Avalonia.Controls /// public class MenuItem : HeaderedSelectingItemsControl, IMenuItem, ISelectable { - private ICommand _command; - /// /// Defines the property. /// @@ -91,9 +90,8 @@ namespace Avalonia.Controls private static readonly ITemplate DefaultPanel = new FuncTemplate(() => new StackPanel()); - /// - /// The submenu popup. - /// + private ICommand _command; + private bool _commandCanExecute = true; private Popup _popup; /// @@ -231,6 +229,8 @@ namespace Avalonia.Controls /// IMenuElement IMenuItem.Parent => Parent as IMenuElement; + protected override bool IsEnabledCore => base.IsEnabledCore && _commandCanExecute; + /// bool IMenuElement.MoveSelection(NavigationDirection direction, bool wrap) => MoveSelection(direction, wrap); @@ -400,6 +400,22 @@ namespace Avalonia.Controls } } + protected override void UpdateDataValidation(AvaloniaProperty property, BindingNotification status) + { + base.UpdateDataValidation(property, status); + if (property == CommandProperty) + { + if (status?.ErrorType == BindingErrorType.Error) + { + if (_commandCanExecute) + { + _commandCanExecute = false; + UpdateIsEffectivelyEnabled(); + } + } + } + } + /// /// Closes all submenus of the menu item. /// @@ -443,9 +459,13 @@ namespace Avalonia.Controls /// The event args. private void CanExecuteChanged(object sender, EventArgs e) { - // HACK: Just set the IsEnabled property for the moment. This needs to be changed to - // use IsEnabledCore etc. but it will do for now. - IsEnabled = Command == null || Command.CanExecute(CommandParameter); + var canExecute = Command == null || Command.CanExecute(CommandParameter); + + if (canExecute != _commandCanExecute) + { + _commandCanExecute = canExecute; + UpdateIsEffectivelyEnabled(); + } } /// diff --git a/src/Avalonia.Input/InputElement.cs b/src/Avalonia.Input/InputElement.cs index a1b00f47fb..e1183e7154 100644 --- a/src/Avalonia.Input/InputElement.cs +++ b/src/Avalonia.Input/InputElement.cs @@ -29,8 +29,10 @@ namespace Avalonia.Input /// /// Defines the property. /// - public static readonly StyledProperty IsEffectivelyEnabledProperty = - AvaloniaProperty.Register(nameof(IsEffectivelyEnabled), true); + public static readonly DirectProperty IsEffectivelyEnabledProperty = + AvaloniaProperty.RegisterDirect( + nameof(IsEffectivelyEnabled), + o => o.IsEffectivelyEnabled); /// /// Gets or sets associated mouse cursor. @@ -146,6 +148,7 @@ namespace Avalonia.Input /// public static readonly RoutedEvent DoubleTappedEvent = Gestures.DoubleTappedEvent; + private bool _isEffectivelyEnabled = true; private bool _isFocused; private bool _isPointerOver; @@ -344,31 +347,25 @@ namespace Avalonia.Input internal set { SetAndRaise(IsPointerOverProperty, ref _isPointerOver, value); } } - /// - /// Gets a value indicating whether the control is effectively enabled for user interaction. - /// - /// - /// The property is used to toggle the enabled state for individual - /// controls. The property takes into account the - /// value of this control and its parent controls. - /// - bool IInputElement.IsEffectivelyEnabled => IsEffectivelyEnabled; + /// + public bool IsEffectivelyEnabled + { + get => _isEffectivelyEnabled; + private set => SetAndRaise(IsEffectivelyEnabledProperty, ref _isEffectivelyEnabled, value); + } + + public List KeyBindings { get; } = new List(); /// - /// Gets a value indicating whether the control is effectively enabled for user interaction. + /// Allows a derived class to override the enabled state of the control. /// /// - /// The property is used to toggle the enabled state for individual - /// controls. The property takes into account the - /// value of this control and its parent controls. + /// Derived controls may wish to disable the enabled state of the control without overwriting the + /// user-supplied setting. This can be done by overriding this property + /// to return the overridden enabled state. If the value returned from + /// should change, then the derived control should call . /// - protected bool IsEffectivelyEnabled - { - get { return GetValue(IsEffectivelyEnabledProperty); } - set { SetValue(IsEffectivelyEnabledProperty, value); } - } - - public List KeyBindings { get; } = new List(); + protected virtual bool IsEnabledCore => IsEnabled; /// /// Focuses the control. @@ -486,6 +483,15 @@ namespace Avalonia.Input { } + /// + /// Updates the property value according to the parent + /// control's enabled state and . + /// + protected void UpdateIsEffectivelyEnabled() + { + UpdateIsEffectivelyEnabled(this.GetVisualParent()); + } + private static void IsEnabledChanged(AvaloniaPropertyChangedEventArgs e) { ((InputElement)e.Sender).UpdateIsEffectivelyEnabled(); @@ -511,14 +517,6 @@ namespace Avalonia.Input OnPointerLeave(e); } - /// - /// Updates the property value. - /// - private void UpdateIsEffectivelyEnabled() - { - UpdateIsEffectivelyEnabled(this.GetVisualParent()); - } - /// /// Updates the property based on the parent's /// . @@ -526,14 +524,7 @@ namespace Avalonia.Input /// The parent control. private void UpdateIsEffectivelyEnabled(InputElement parent) { - if (parent != null) - { - IsEffectivelyEnabled = IsEnabled && parent.IsEffectivelyEnabled; - } - else - { - IsEffectivelyEnabled = IsEnabled; - } + IsEffectivelyEnabled = IsEnabledCore && (parent?.IsEffectivelyEnabled ?? true); foreach (var child in this.GetVisualChildren().OfType()) { diff --git a/tests/Avalonia.Controls.UnitTests/ButtonTests.cs b/tests/Avalonia.Controls.UnitTests/ButtonTests.cs index 9255b00e50..fe0ac47a7d 100644 --- a/tests/Avalonia.Controls.UnitTests/ButtonTests.cs +++ b/tests/Avalonia.Controls.UnitTests/ButtonTests.cs @@ -24,11 +24,11 @@ namespace Avalonia.Controls.UnitTests }; var root = new TestRoot { Child = target }; - Assert.False(target.IsEnabled); + Assert.False(target.IsEffectivelyEnabled); command.IsEnabled = true; - Assert.True(target.IsEnabled); + Assert.True(target.IsEffectivelyEnabled); command.IsEnabled = false; - Assert.False(target.IsEnabled); + Assert.False(target.IsEffectivelyEnabled); } [Fact] @@ -54,7 +54,8 @@ namespace Avalonia.Controls.UnitTests [!Button.CommandProperty] = new Binding("Command"), }; - Assert.False(target.IsEnabled); + Assert.True(target.IsEnabled); + Assert.False(target.IsEffectivelyEnabled); } [Fact] @@ -72,8 +73,12 @@ namespace Avalonia.Controls.UnitTests }; Assert.True(target.IsEnabled); + Assert.True(target.IsEffectivelyEnabled); + target.DataContext = null; - Assert.False(target.IsEnabled); + + Assert.True(target.IsEnabled); + Assert.False(target.IsEffectivelyEnabled); } [Fact] @@ -90,9 +95,13 @@ namespace Avalonia.Controls.UnitTests [!Button.CommandProperty] = new Binding("Command"), }; - Assert.False(target.IsEnabled); + Assert.True(target.IsEnabled); + Assert.False(target.IsEffectivelyEnabled); + target.DataContext = viewModel; + Assert.True(target.IsEnabled); + Assert.True(target.IsEffectivelyEnabled); } [Fact] @@ -109,9 +118,13 @@ namespace Avalonia.Controls.UnitTests [!Button.CommandProperty] = new Binding("Command"), }; - Assert.False(target.IsEnabled); + Assert.True(target.IsEnabled); + Assert.False(target.IsEffectivelyEnabled); + target.DataContext = viewModel; - Assert.False(target.IsEnabled); + + Assert.True(target.IsEnabled); + Assert.False(target.IsEffectivelyEnabled); } [Fact] diff --git a/tests/Avalonia.Controls.UnitTests/MenuItemTests.cs b/tests/Avalonia.Controls.UnitTests/MenuItemTests.cs index ebf2c72ab4..34371916df 100644 --- a/tests/Avalonia.Controls.UnitTests/MenuItemTests.cs +++ b/tests/Avalonia.Controls.UnitTests/MenuItemTests.cs @@ -2,6 +2,7 @@ using System.Collections.Generic; using System.Text; using System.Windows.Input; +using Avalonia.Data; using Avalonia.Input; using Avalonia.UnitTests; using Xunit; @@ -26,6 +27,103 @@ namespace Avalonia.Controls.UnitTests Assert.False(target.Focusable); } + + [Fact] + public void MenuItem_Is_Disabled_When_Command_Is_Enabled_But_IsEnabled_Is_False() + { + var command = new TestCommand(true); + var target = new MenuItem + { + IsEnabled = false, + Command = command, + }; + + var root = new TestRoot { Child = target }; + + Assert.False(((IInputElement)target).IsEffectivelyEnabled); + } + + [Fact] + public void MenuItem_Is_Disabled_When_Bound_Command_Doesnt_Exist() + { + var target = new MenuItem + { + [!MenuItem.CommandProperty] = new Binding("Command"), + }; + + Assert.True(target.IsEnabled); + Assert.False(target.IsEffectivelyEnabled); + } + + [Fact] + public void MenuItem_Is_Disabled_When_Bound_Command_Is_Removed() + { + var viewModel = new + { + Command = new TestCommand(true), + }; + + var target = new MenuItem + { + DataContext = viewModel, + [!MenuItem.CommandProperty] = new Binding("Command"), + }; + + Assert.True(target.IsEnabled); + Assert.True(target.IsEffectivelyEnabled); + + target.DataContext = null; + + Assert.True(target.IsEnabled); + Assert.False(target.IsEffectivelyEnabled); + } + + [Fact] + public void MenuItem_Is_Enabled_When_Bound_Command_Is_Added() + { + var viewModel = new + { + Command = new TestCommand(true), + }; + + var target = new MenuItem + { + DataContext = new object(), + [!MenuItem.CommandProperty] = new Binding("Command"), + }; + + Assert.True(target.IsEnabled); + Assert.False(target.IsEffectivelyEnabled); + + target.DataContext = viewModel; + + Assert.True(target.IsEnabled); + Assert.True(target.IsEffectivelyEnabled); + } + + [Fact] + public void MenuItem_Is_Disabled_When_Disabled_Bound_Command_Is_Added() + { + var viewModel = new + { + Command = new TestCommand(false), + }; + + var target = new MenuItem + { + DataContext = new object(), + [!MenuItem.CommandProperty] = new Binding("Command"), + }; + + Assert.True(target.IsEnabled); + Assert.False(target.IsEffectivelyEnabled); + + target.DataContext = viewModel; + + Assert.True(target.IsEnabled); + Assert.False(target.IsEffectivelyEnabled); + } + [Fact] public void MenuItem_Does_Not_Subscribe_To_Command_CanExecuteChanged_Until_Added_To_Logical_Tree() { @@ -59,21 +157,6 @@ namespace Avalonia.Controls.UnitTests Assert.Equal(0, command.SubscriptionCount); } - [Fact] - public void MenuItem_Is_Disabled_When_Command_Is_Enabled_But_IsEnabled_Is_False() - { - var command = new TestCommand(true); - var target = new MenuItem - { - IsEnabled = false, - Command = command, - }; - - var root = new TestRoot { Child = target }; - - Assert.False(((IInputElement)target).IsEffectivelyEnabled); - } - private class TestCommand : ICommand { private bool _enabled; diff --git a/tests/Avalonia.Input.UnitTests/InputElement_Enabled.cs b/tests/Avalonia.Input.UnitTests/InputElement_Enabled.cs new file mode 100644 index 0000000000..5dd66e3190 --- /dev/null +++ b/tests/Avalonia.Input.UnitTests/InputElement_Enabled.cs @@ -0,0 +1,101 @@ +// Copyright (c) The Avalonia Project. All rights reserved. +// Licensed under the MIT license. See licence.md file in the project root for full license information. + +using Avalonia.Controls; +using Xunit; + +namespace Avalonia.Input.UnitTests +{ + public class InputElement_Enabled + { + [Fact] + public void IsEffectivelyEnabled_Follows_IsEnabled() + { + var target = new Decorator(); + + Assert.True(target.IsEnabled); + Assert.True(target.IsEffectivelyEnabled); + + target.IsEnabled = false; + + Assert.False(target.IsEnabled); + Assert.False(target.IsEffectivelyEnabled); + } + + [Fact] + public void IsEffectivelyEnabled_Follows_Ancestor_IsEnabled() + { + Decorator child; + Decorator grandchild; + var target = new Decorator + { + Child = child = new Decorator + { + Child = grandchild = new Decorator(), + } + }; + + Assert.True(target.IsEnabled); + Assert.True(target.IsEffectivelyEnabled); + Assert.True(child.IsEnabled); + Assert.True(child.IsEffectivelyEnabled); + Assert.True(grandchild.IsEnabled); + Assert.True(grandchild.IsEffectivelyEnabled); + + target.IsEnabled = false; + + Assert.False(target.IsEnabled); + Assert.False(target.IsEffectivelyEnabled); + Assert.True(child.IsEnabled); + Assert.False(child.IsEffectivelyEnabled); + Assert.True(grandchild.IsEnabled); + Assert.False(grandchild.IsEffectivelyEnabled); + } + + [Fact] + public void Disabled_Pseudoclass_Follows_IsEffectivelyEnabled() + { + Decorator child; + var target = new Decorator + { + Child = child = new Decorator() + }; + + Assert.DoesNotContain(":disabled", child.Classes); + + target.IsEnabled = false; + + Assert.Contains(":disabled", child.Classes); + } + + [Fact] + public void IsEffectivelyEnabled_Respects_IsEnabledCore() + { + Decorator child; + var target = new TestControl + { + Child = child = new Decorator() + }; + + target.ShouldEnable = false; + + Assert.True(target.IsEnabled); + Assert.False(target.IsEffectivelyEnabled); + Assert.True(child.IsEnabled); + Assert.False(child.IsEffectivelyEnabled); + } + + private class TestControl : Decorator + { + private bool _shouldEnable; + + public bool ShouldEnable + { + get => _shouldEnable; + set { _shouldEnable = value; UpdateIsEffectivelyEnabled(); } + } + + protected override bool IsEnabledCore => IsEnabled && _shouldEnable; + } + } +}