[Menu] [Performance] Improve Menu item can execute logic. Make it work like in WPF (#6438)

* [MenuItem] [Performance] Evaluate CanExecute on menu show and only if menu is visible * [Revert] a change from another PR * Update tests/Avalonia.Controls.UnitTests/MenuItemTests.cs Co-authored-by: Steven Kirk <grokys@users.noreply.github.com> * Update tests/Avalonia.Controls.UnitTests/MenuItemTests.cs Co-authored-by: Steven Kirk <grokys@users.noreply.github.com> * fix typo * [MenuItem] fix IsEffectivelyEnabled and cover with tests Co-authored-by: Steven Kirk <grokys@users.noreply.github.com> Co-authored-by: Max Katz <maxkatz6@outlook.com>
4 years ago · dd61cb9b32
3 changed files with 203 additions and 23 deletions
--- a/src/Avalonia.Controls/MenuItem.cs
+++ b/src/Avalonia.Controls/MenuItem.cs
@ -107,6 +107,7 @@ namespace Avalonia.Controls

        private ICommand? _command;
        private bool _commandCanExecute = true;
+        private bool _commandBindingError;
        private Popup? _popup;
        private KeyGesture? _hotkey;
        private bool _isEmbeddedInMenu;
@ -379,6 +380,8 @@ namespace Avalonia.Controls
            {
                Command.CanExecuteChanged += CanExecuteChanged;
            }
+            
+            TryUpdateCanExecute();

            var parent = Parent;

@ -498,13 +501,11 @@ namespace Avalonia.Controls
            base.UpdateDataValidation(property, value);
            if (property == CommandProperty)
            {
-                if (value.Type == BindingValueType.BindingError)
+                _commandBindingError = value.Type == BindingValueType.BindingError;
+                if (_commandBindingError && _commandCanExecute)
                {
-                    if (_commandCanExecute)
-                    {
-                        _commandCanExecute = false;
-                        UpdateIsEffectivelyEnabled();
-                    }
+                    _commandCanExecute = false;
+                    UpdateIsEffectivelyEnabled();
                }
            }
        }
@ -526,22 +527,20 @@ namespace Avalonia.Controls
        /// <param name="e">The event args.</param>
        private static void CommandChanged(AvaloniaPropertyChangedEventArgs e)
        {
-            if (e.Sender is MenuItem menuItem)
+            if (e.Sender is MenuItem menuItem &&
+                ((ILogical)menuItem).IsAttachedToLogicalTree)
            {
-                if (((ILogical)menuItem).IsAttachedToLogicalTree)
+                if (e.OldValue is ICommand oldCommand)
                {
-                    if (e.OldValue is ICommand oldCommand)
-                    {
-                        oldCommand.CanExecuteChanged -= menuItem.CanExecuteChanged;
-                    }
+                    oldCommand.CanExecuteChanged -= menuItem.CanExecuteChanged;
+                }

-                    if (e.NewValue is ICommand newCommand)
-                    {
-                        newCommand.CanExecuteChanged += menuItem.CanExecuteChanged;
-                    }
+                if (e.NewValue is ICommand newCommand)
+                {
+                    newCommand.CanExecuteChanged += menuItem.CanExecuteChanged;
                }

-                menuItem.CanExecuteChanged(menuItem, EventArgs.Empty);
+                menuItem.TryUpdateCanExecute();
            }
        }

@ -553,7 +552,7 @@ namespace Avalonia.Controls
        {
            if (e.Sender is MenuItem menuItem)
            {
-                menuItem.CanExecuteChanged(menuItem, EventArgs.Empty);
+                menuItem.TryUpdateCanExecute();
            }
        }

@ -564,8 +563,29 @@ namespace Avalonia.Controls
        /// <param name="e">The event args.</param>
        private void CanExecuteChanged(object sender, EventArgs e)
        {
-            var canExecute = Command == null || Command.CanExecute(CommandParameter);
+            TryUpdateCanExecute();
+        }

+        /// <summary>
+        /// Tries to evaluate CanExecute value of a Command if menu is opened
+        /// </summary>
+        private void TryUpdateCanExecute()
+        {
+            if (Command == null)
+            {
+                _commandCanExecute = !_commandBindingError;
+                UpdateIsEffectivelyEnabled();
+                return;
+            }
+            
+            //Perf optimization - only raise CanExecute event if the menu is open
+            if (!((ILogical)this).IsAttachedToLogicalTree ||
+                Parent is MenuItem { IsSubMenuOpen: false })
+            {
+                return;
+            }
+            
+            var canExecute = Command.CanExecute(CommandParameter);
            if (canExecute != _commandCanExecute)
            {
                _commandCanExecute = canExecute;
@ -635,6 +655,11 @@ namespace Avalonia.Controls

            if (value)
            {
+                foreach (var item in Items.OfType<MenuItem>())
+                {
+                    item.TryUpdateCanExecute();
+                }
+
                RaiseEvent(new RoutedEventArgs(SubmenuOpenedEvent));
                IsSelected = true;
                PseudoClasses.Add(":open");
--- a/src/Avalonia.Controls/Platform/DefaultMenuInteractionHandler.cs
+++ b/src/Avalonia.Controls/Platform/DefaultMenuInteractionHandler.cs
@ -116,7 +116,7 @@ namespace Avalonia.Controls.Platform

        protected IMenu? Menu { get; private set; }

-        public static TimeSpan MenuShowDelay { get; set; } = TimeSpan.FromMilliseconds(400);
+        protected static TimeSpan MenuShowDelay { get; } = TimeSpan.FromMilliseconds(400);

        protected internal virtual void GotFocus(object sender, GotFocusEventArgs e)
        {
--- a/tests/Avalonia.Controls.UnitTests/MenuItemTests.cs
+++ b/tests/Avalonia.Controls.UnitTests/MenuItemTests.cs
@ -2,15 +2,21 @@
 using System.Collections.Generic;
 using System.Text;
 using System.Windows.Input;
+using Avalonia.Collections;
+using Avalonia.Controls.Primitives;
 using Avalonia.Data;
 using Avalonia.Input;
+using Avalonia.Platform;
 using Avalonia.UnitTests;
+using Moq;
 using Xunit;

 namespace Avalonia.Controls.UnitTests
 {
    public class MenuItemTests
    {
+        private Mock<IPopupImpl> popupImpl;
+
        [Fact]
        public void Header_Of_Minus_Should_Apply_Separator_Pseudoclass()
        {
@ -79,7 +85,7 @@ namespace Avalonia.Controls.UnitTests
        }

        [Fact]
-        public void MenuItem_Is_Enabled_When_Bound_Command_Is_Added()
+        public void MenuItem_Is_Enabled_When_Added_To_Logical_Tree_And_Bound_Command_Is_Added()
        {
            var viewModel = new
            {
@ -91,7 +97,8 @@ namespace Avalonia.Controls.UnitTests
                DataContext = new object(),
                [!MenuItem.CommandProperty] = new Binding("Command"),
            };
-
+            var root = new TestRoot { Child = target };
+                
            Assert.True(target.IsEnabled);
            Assert.False(target.IsEffectivelyEnabled);

@ -158,10 +165,11 @@ namespace Avalonia.Controls.UnitTests
        }
        
        [Fact]
-        public void MenuItem_Invokes_CanExecute_When_CommandParameter_Changed()
+        public void MenuItem_Invokes_CanExecute_When_Added_To_Logical_Tree_And_CommandParameter_Changed()
        {
            var command = new TestCommand(p => p is bool value && value);
            var target = new MenuItem { Command = command };
+            var root = new TestRoot { Child = target };

            target.CommandParameter = true;
            Assert.True(target.IsEffectivelyEnabled);
@ -169,6 +177,151 @@ namespace Avalonia.Controls.UnitTests
            target.CommandParameter = false;
            Assert.False(target.IsEffectivelyEnabled);
        }
+        
+        [Fact]
+        public void MenuItem_Does_Not_Invoke_CanExecute_When_ContextMenu_Closed()
+        {
+            using (Application())
+            {
+                var canExecuteCallCount = 0;
+                var command = new TestCommand(_ =>
+                {
+                    canExecuteCallCount++;
+                    return true;
+                });
+                var target = new MenuItem();
+                var contextMenu = new ContextMenu { Items = new AvaloniaList<MenuItem> { target } };
+                var window = new Window { Content = new Panel { ContextMenu = contextMenu } };
+                window.ApplyTemplate();
+                window.Presenter.ApplyTemplate();
+                
+                Assert.True(target.IsEffectivelyEnabled);
+                target.Command = command;
+                Assert.Equal(0, canExecuteCallCount);
+
+                target.CommandParameter = false;
+                Assert.Equal(0, canExecuteCallCount);
+
+                command.RaiseCanExecuteChanged();
+                Assert.Equal(0, canExecuteCallCount);
+                
+                contextMenu.Open();
+                Assert.Equal(2, canExecuteCallCount);//2 because popup is changing logical child
+
+                command.RaiseCanExecuteChanged();
+                Assert.Equal(3, canExecuteCallCount);
+
+                target.CommandParameter = true;
+                Assert.Equal(4, canExecuteCallCount);
+            }
+        }
+        
+        [Fact]
+        public void MenuItem_Does_Not_Invoke_CanExecute_When_MenuFlyout_Closed()
+        {
+            using (Application())
+            {
+                var canExecuteCallCount = 0;
+                var command = new TestCommand(_ =>
+                {
+                    canExecuteCallCount++;
+                    return true;
+                });
+                var target = new MenuItem();
+                var flyout = new MenuFlyout { Items = new AvaloniaList<MenuItem> { target } };
+                var button = new Button { Flyout = flyout };
+                var window = new Window { Content = button };
+                window.ApplyTemplate();
+                window.Presenter.ApplyTemplate();
+                
+                Assert.True(target.IsEffectivelyEnabled);
+                target.Command = command;
+                Assert.Equal(0, canExecuteCallCount);
+
+                target.CommandParameter = false;
+                Assert.Equal(0, canExecuteCallCount);
+
+                command.RaiseCanExecuteChanged();
+                Assert.Equal(0, canExecuteCallCount);
+
+                flyout.ShowAt(button);
+                Assert.Equal(2, canExecuteCallCount);
+
+                command.RaiseCanExecuteChanged();
+                Assert.Equal(3, canExecuteCallCount);
+
+                target.CommandParameter = true;
+                Assert.Equal(4, canExecuteCallCount);
+            }
+        }
+        
+        [Fact]
+        public void MenuItem_Does_Not_Invoke_CanExecute_When_Parent_MenuItem_Closed()
+        {
+            using (Application())
+            {
+                var canExecuteCallCount = 0;
+                var command = new TestCommand(_ =>
+                {
+                    canExecuteCallCount++;
+                    return true;
+                });
+                var target = new MenuItem();
+                var parentMenuItem = new MenuItem { Items = new AvaloniaList<MenuItem> { target } };
+                var contextMenu = new ContextMenu { Items = new AvaloniaList<MenuItem> { parentMenuItem } };
+                var window = new Window { Content = new Panel { ContextMenu = contextMenu } };
+                window.ApplyTemplate();
+                window.Presenter.ApplyTemplate();
+                contextMenu.Open();
+                
+                Assert.True(target.IsEffectivelyEnabled);
+                target.Command = command;
+                Assert.Equal(0, canExecuteCallCount);
+
+                target.CommandParameter = false;
+                Assert.Equal(0, canExecuteCallCount);
+
+                command.RaiseCanExecuteChanged();
+                Assert.Equal(0, canExecuteCallCount);
+
+                try
+                {
+                    parentMenuItem.IsSubMenuOpen = true;
+                }
+                catch (InvalidOperationException)
+                {
+                    //popup host creation failed exception
+                }
+                Assert.Equal(1, canExecuteCallCount);
+
+                command.RaiseCanExecuteChanged();
+                Assert.Equal(2, canExecuteCallCount);
+
+                target.CommandParameter = true;
+                Assert.Equal(3, canExecuteCallCount);
+            }
+        }
+        private IDisposable Application()
+        {
+            var screen = new PixelRect(new PixelPoint(), new PixelSize(100, 100));
+            var screenImpl = new Mock<IScreenImpl>();
+            screenImpl.Setup(x => x.ScreenCount).Returns(1);
+            screenImpl.Setup(X => X.AllScreens).Returns( new[] { new Screen(1, screen, screen, true) });
+
+            var windowImpl = MockWindowingPlatform.CreateWindowMock();
+            popupImpl = MockWindowingPlatform.CreatePopupMock(windowImpl.Object);
+            popupImpl.SetupGet(x => x.RenderScaling).Returns(1);
+            windowImpl.Setup(x => x.CreatePopup()).Returns(popupImpl.Object);
+
+            windowImpl.Setup(x => x.Screen).Returns(screenImpl.Object);
+
+            var services = TestServices.StyledWindow.With(
+                inputManager: new InputManager(),
+                windowImpl: windowImpl.Object,
+                windowingPlatform: new MockWindowingPlatform(() => windowImpl.Object, x => popupImpl.Object));
+
+            return UnitTestApplication.Start(services);
+        }

        private class TestCommand : ICommand
        {
@ -198,6 +351,8 @@ namespace Avalonia.Controls.UnitTests
            public bool CanExecute(object parameter) => _canExecute(parameter);

            public void Execute(object parameter) => _execute(parameter);
+
+            public void RaiseCanExecuteChanged() => _canExecuteChanged?.Invoke(this, EventArgs.Empty);
        }
    }
 }