From ba82d645462e2bb42ee887e4d3724ad017897b4b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javier=20Su=C3=A1rez?= Date: Thu, 17 Nov 2022 10:05:42 +0100 Subject: [PATCH 1/5] Fix crash using BackButtonBehavior and navigating to root --- src/Controls/src/Core/ShellToolbar.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Controls/src/Core/ShellToolbar.cs b/src/Controls/src/Core/ShellToolbar.cs index 84af6e3fffee..c5937e63a84f 100644 --- a/src/Controls/src/Core/ShellToolbar.cs +++ b/src/Controls/src/Core/ShellToolbar.cs @@ -166,7 +166,7 @@ void OnBackButtonPropertyChanged(object sender, System.ComponentModel.PropertyCh void OnBackButtonCanExecuteChanged(object sender, EventArgs e) { BackButtonEnabled = - _backButtonCommand.CanExecute(_backButtonBehavior.CommandParameter); + _backButtonCommand != null && _backButtonCommand.CanExecute(_backButtonBehavior?.CommandParameter); } } From 781cb5b60adece2302d7c52ee8519babce8d06a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javier=20Sua=CC=81rez?= Date: Tue, 22 Nov 2022 12:22:03 +0100 Subject: [PATCH 2/5] Added Device Test --- .../DeviceTests/Elements/Shell/ShellTests.cs | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/src/Controls/tests/DeviceTests/Elements/Shell/ShellTests.cs b/src/Controls/tests/DeviceTests/Elements/Shell/ShellTests.cs index d8bf55da859e..7df55f9e5ee9 100644 --- a/src/Controls/tests/DeviceTests/Elements/Shell/ShellTests.cs +++ b/src/Controls/tests/DeviceTests/Elements/Shell/ShellTests.cs @@ -596,6 +596,38 @@ await CreateHandlerAndAddToWindow(shell, async (handler) => }); } + [Fact(DisplayName = "Navigate to Root with BackButtonBehavior no Crash")] + public async Task NavigateToRootWithBackButtonBehaviorNoCrash() + { + SetupBuilder(); + + var shell = await CreateShellAsync(shell => + { + shell.CurrentItem = new ContentPage(); + }); + + await CreateHandlerAndAddToWindow(shell, async (handler) => + { + Assert.False(IsBackButtonVisible(shell.Handler)); + await shell.Navigation.PushAsync(new ContentPage()); + Assert.True(IsBackButtonVisible(shell.Handler)); + + BackButtonBehavior behavior = new BackButtonBehavior() + { + IsVisible = false + }; + + Shell.SetBackButtonBehavior(shell.CurrentPage, behavior); + Assert.False(IsBackButtonVisible(shell.Handler)); + behavior.IsVisible = true; + NavigationPage.SetHasBackButton(shell.CurrentPage, true); + + await shell.Navigation.PopToRootAsync(false); + await Task.Delay(100); + Assert.NotNull(shell.Handler); + }); + } + protected Task CreateShellAsync(Action action) => InvokeOnMainThreadAsync(() => { From 24ccfdc4331f8d5d61d301fd22baa6ff29a23a82 Mon Sep 17 00:00:00 2001 From: Shane Neuville Date: Thu, 1 Dec 2022 12:41:44 -0600 Subject: [PATCH 3/5] - add unit test and fixup ShellToolbar to unhook from previous command --- src/Controls/src/Core/ShellToolbar.cs | 58 +++++++++++-------- .../tests/Core.UnitTests/ShellToolbarTests.cs | 23 ++++++++ .../DeviceTests/Elements/Shell/ShellTests.cs | 32 ---------- 3 files changed, 56 insertions(+), 57 deletions(-) diff --git a/src/Controls/src/Core/ShellToolbar.cs b/src/Controls/src/Core/ShellToolbar.cs index c5937e63a84f..70ba673355bb 100644 --- a/src/Controls/src/Core/ShellToolbar.cs +++ b/src/Controls/src/Core/ShellToolbar.cs @@ -1,4 +1,6 @@ -using System; +#nullable enable + +using System; using System.Collections.Generic; using System.Runtime.CompilerServices; using System.Text; @@ -10,9 +12,10 @@ namespace Microsoft.Maui.Controls internal class ShellToolbar : Toolbar { Shell _shell; - Page _currentPage; - BackButtonBehavior _backButtonBehavior; + Page? _currentPage; + BackButtonBehavior? _backButtonBehavior; ToolbarTracker _toolbarTracker = new ToolbarTracker(); + ICommand? _backButtonCommand; #if WINDOWS MenuBarTracker _menuBarTracker = new MenuBarTracker(); #endif @@ -77,7 +80,7 @@ internal void ApplyChanges() _menuBarTracker.Target = _shell; #endif - Page previousPage = null; + Page? previousPage = null; if (stack.Count > 1) previousPage = stack[stack.Count - 1]; @@ -131,7 +134,6 @@ internal void ApplyChanges() DynamicOverflowEnabled = PlatformConfiguration.WindowsSpecific.Page.GetToolbarDynamicOverflowEnabled(currentPage); } - ICommand _backButtonCommand; void UpdateBackbuttonBehavior() { var bbb = Shell.GetBackButtonBehavior(_currentPage); @@ -140,37 +142,43 @@ void UpdateBackbuttonBehavior() return; if (_backButtonBehavior != null) - _backButtonBehavior.PropertyChanged -= OnBackButtonPropertyChanged; + _backButtonBehavior.PropertyChanged -= OnBackButtonCommandPropertyChanged; _backButtonBehavior = bbb; if (_backButtonBehavior != null) - _backButtonBehavior.PropertyChanged += OnBackButtonPropertyChanged; + _backButtonBehavior.PropertyChanged += OnBackButtonCommandPropertyChanged; - void OnBackButtonPropertyChanged(object sender, System.ComponentModel.PropertyChangedEventArgs e) - { - ApplyChanges(); + UpdateBackButtonBehaviorCommand(); + } + + void OnBackButtonCommandPropertyChanged(object? sender, System.ComponentModel.PropertyChangedEventArgs e) + { + ApplyChanges(); + UpdateBackButtonBehaviorCommand(); + } - if (_backButtonBehavior.Command == _backButtonCommand) - return; + void UpdateBackButtonBehaviorCommand() + { + if (_backButtonBehavior?.Command == _backButtonCommand) + return; - if (_backButtonCommand != null) - _backButtonCommand.CanExecuteChanged -= OnBackButtonCanExecuteChanged; + if (_backButtonCommand != null) + _backButtonCommand.CanExecuteChanged -= OnBackButtonCanExecuteChanged; - _backButtonCommand = _backButtonBehavior.Command; + _backButtonCommand = _backButtonBehavior?.Command; - if (_backButtonCommand != null) - _backButtonCommand.CanExecuteChanged += OnBackButtonCanExecuteChanged; - } + if (_backButtonCommand != null) + _backButtonCommand.CanExecuteChanged += OnBackButtonCanExecuteChanged; + } - void OnBackButtonCanExecuteChanged(object sender, EventArgs e) - { - BackButtonEnabled = - _backButtonCommand != null && _backButtonCommand.CanExecute(_backButtonBehavior?.CommandParameter); - } + void OnBackButtonCanExecuteChanged(object? sender, EventArgs e) + { + BackButtonEnabled = + _backButtonCommand != null && _backButtonCommand.CanExecute(_backButtonBehavior?.CommandParameter); } - void OnCurrentPagePropertyChanged(object sender, System.ComponentModel.PropertyChangedEventArgs e) + void OnCurrentPagePropertyChanged(object? sender, System.ComponentModel.PropertyChangedEventArgs e) { if (e.Is(Page.TitleProperty)) UpdateTitle(); @@ -195,7 +203,7 @@ internal void UpdateTitle() return; } - Page currentPage = _shell.GetCurrentShellPage() as Page; + Page? currentPage = _shell.GetCurrentShellPage() as Page; if (currentPage?.IsSet(Page.TitleProperty) == true) { Title = currentPage.Title ?? String.Empty; diff --git a/src/Controls/tests/Core.UnitTests/ShellToolbarTests.cs b/src/Controls/tests/Core.UnitTests/ShellToolbarTests.cs index e8dabe174eef..c82ab55b6668 100644 --- a/src/Controls/tests/Core.UnitTests/ShellToolbarTests.cs +++ b/src/Controls/tests/Core.UnitTests/ShellToolbarTests.cs @@ -101,6 +101,29 @@ public async Task BackButtonDisabledWhenCommandDisabled() Assert.True(testShell.Toolbar.BackButtonEnabled); } + [Fact] + public async Task BackButtonBehaviorCommandFromPoppedPageIsCorrectlyUnsubscribedFrom() + { + var firstPage = new ContentPage(); + var secondPage = new ContentPage(); + bool canExecute = true; + var backButtonBehavior = new BackButtonBehavior(); + TestShell testShell = new TestShell(firstPage); + + await testShell.Navigation.PushAsync(secondPage); + + Shell.SetBackButtonBehavior(secondPage, backButtonBehavior); + + backButtonBehavior.Command = new Command(() => { }, () => canExecute); + + await testShell.Navigation.PopAsync(); + + canExecute = false; + (backButtonBehavior.Command as Command).ChangeCanExecute(); + + Assert.True(testShell.Toolbar.BackButtonEnabled); + } + [Fact] public async Task ShellToolbarUpdatesFromNewBackButtonBehavior() { diff --git a/src/Controls/tests/DeviceTests/Elements/Shell/ShellTests.cs b/src/Controls/tests/DeviceTests/Elements/Shell/ShellTests.cs index 7df55f9e5ee9..d8bf55da859e 100644 --- a/src/Controls/tests/DeviceTests/Elements/Shell/ShellTests.cs +++ b/src/Controls/tests/DeviceTests/Elements/Shell/ShellTests.cs @@ -596,38 +596,6 @@ await CreateHandlerAndAddToWindow(shell, async (handler) => }); } - [Fact(DisplayName = "Navigate to Root with BackButtonBehavior no Crash")] - public async Task NavigateToRootWithBackButtonBehaviorNoCrash() - { - SetupBuilder(); - - var shell = await CreateShellAsync(shell => - { - shell.CurrentItem = new ContentPage(); - }); - - await CreateHandlerAndAddToWindow(shell, async (handler) => - { - Assert.False(IsBackButtonVisible(shell.Handler)); - await shell.Navigation.PushAsync(new ContentPage()); - Assert.True(IsBackButtonVisible(shell.Handler)); - - BackButtonBehavior behavior = new BackButtonBehavior() - { - IsVisible = false - }; - - Shell.SetBackButtonBehavior(shell.CurrentPage, behavior); - Assert.False(IsBackButtonVisible(shell.Handler)); - behavior.IsVisible = true; - NavigationPage.SetHasBackButton(shell.CurrentPage, true); - - await shell.Navigation.PopToRootAsync(false); - await Task.Delay(100); - Assert.NotNull(shell.Handler); - }); - } - protected Task CreateShellAsync(Action action) => InvokeOnMainThreadAsync(() => { From 6c55afe6999b528bcea46384893a80fe4a5d78de Mon Sep 17 00:00:00 2001 From: Shane Neuville Date: Thu, 1 Dec 2022 12:44:16 -0600 Subject: [PATCH 4/5] - remove extra call to OnBackButtonCommandPropertyChanged --- src/Controls/src/Core/ShellToolbar.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Controls/src/Core/ShellToolbar.cs b/src/Controls/src/Core/ShellToolbar.cs index 70ba673355bb..4ee740e72de1 100644 --- a/src/Controls/src/Core/ShellToolbar.cs +++ b/src/Controls/src/Core/ShellToolbar.cs @@ -155,7 +155,6 @@ void UpdateBackbuttonBehavior() void OnBackButtonCommandPropertyChanged(object? sender, System.ComponentModel.PropertyChangedEventArgs e) { ApplyChanges(); - UpdateBackButtonBehaviorCommand(); } void UpdateBackButtonBehaviorCommand() From 10732c2cf9e878dfd0ddfdd699c01631c402943d Mon Sep 17 00:00:00 2001 From: Shane Neuville Date: Thu, 1 Dec 2022 13:38:00 -0600 Subject: [PATCH 5/5] - remove excess code from ShellToolbar --- src/Controls/src/Core/ShellToolbar.cs | 33 +--------------- .../tests/Core.UnitTests/ShellToolbarTests.cs | 21 ++++++++++ .../DeviceTests/Elements/Shell/ShellTests.cs | 39 +++++++++++++++++++ 3 files changed, 62 insertions(+), 31 deletions(-) diff --git a/src/Controls/src/Core/ShellToolbar.cs b/src/Controls/src/Core/ShellToolbar.cs index 4ee740e72de1..f887035a275d 100644 --- a/src/Controls/src/Core/ShellToolbar.cs +++ b/src/Controls/src/Core/ShellToolbar.cs @@ -2,6 +2,7 @@ using System; using System.Collections.Generic; +using System.ComponentModel; using System.Runtime.CompilerServices; using System.Text; using System.Windows.Input; @@ -15,7 +16,6 @@ internal class ShellToolbar : Toolbar Page? _currentPage; BackButtonBehavior? _backButtonBehavior; ToolbarTracker _toolbarTracker = new ToolbarTracker(); - ICommand? _backButtonCommand; #if WINDOWS MenuBarTracker _menuBarTracker = new MenuBarTracker(); #endif @@ -98,13 +98,6 @@ internal void ApplyChanges() BackButtonVisible = backButtonVisible && stack.Count > 1; BackButtonEnabled = _backButtonBehavior?.IsEnabled ?? true; - if (_backButtonBehavior?.Command != null) - { - BackButtonEnabled = - BackButtonEnabled && - _backButtonBehavior.Command.CanExecute(_backButtonBehavior.CommandParameter); - } - UpdateTitle(); if (_currentPage != null && @@ -148,35 +141,13 @@ void UpdateBackbuttonBehavior() if (_backButtonBehavior != null) _backButtonBehavior.PropertyChanged += OnBackButtonCommandPropertyChanged; - - UpdateBackButtonBehaviorCommand(); } - void OnBackButtonCommandPropertyChanged(object? sender, System.ComponentModel.PropertyChangedEventArgs e) + void OnBackButtonCommandPropertyChanged(object? sender, PropertyChangedEventArgs e) { ApplyChanges(); } - void UpdateBackButtonBehaviorCommand() - { - if (_backButtonBehavior?.Command == _backButtonCommand) - return; - - if (_backButtonCommand != null) - _backButtonCommand.CanExecuteChanged -= OnBackButtonCanExecuteChanged; - - _backButtonCommand = _backButtonBehavior?.Command; - - if (_backButtonCommand != null) - _backButtonCommand.CanExecuteChanged += OnBackButtonCanExecuteChanged; - } - - void OnBackButtonCanExecuteChanged(object? sender, EventArgs e) - { - BackButtonEnabled = - _backButtonCommand != null && _backButtonCommand.CanExecute(_backButtonBehavior?.CommandParameter); - } - void OnCurrentPagePropertyChanged(object? sender, System.ComponentModel.PropertyChangedEventArgs e) { if (e.Is(Page.TitleProperty)) diff --git a/src/Controls/tests/Core.UnitTests/ShellToolbarTests.cs b/src/Controls/tests/Core.UnitTests/ShellToolbarTests.cs index c82ab55b6668..a48763678e84 100644 --- a/src/Controls/tests/Core.UnitTests/ShellToolbarTests.cs +++ b/src/Controls/tests/Core.UnitTests/ShellToolbarTests.cs @@ -124,6 +124,27 @@ public async Task BackButtonBehaviorCommandFromPoppedPageIsCorrectlyUnsubscribed Assert.True(testShell.Toolbar.BackButtonEnabled); } + [Fact] + public async Task BackButtonUpdatesWhenSetToNewCommand() + { + var firstPage = new ContentPage(); + var secondPage = new ContentPage(); + bool canExecute = true; + var backButtonBehavior = new BackButtonBehavior(); + TestShell testShell = new TestShell(firstPage); + + await testShell.Navigation.PushAsync(secondPage); + + Shell.SetBackButtonBehavior(secondPage, backButtonBehavior); + + backButtonBehavior.Command = new Command(() => { }, () => true); + Assert.True(testShell.Toolbar.BackButtonEnabled); + backButtonBehavior.Command = new Command(() => { }, () => false); + Assert.False(testShell.Toolbar.BackButtonEnabled); + backButtonBehavior.Command = null; + Assert.True(testShell.Toolbar.BackButtonEnabled); + } + [Fact] public async Task ShellToolbarUpdatesFromNewBackButtonBehavior() { diff --git a/src/Controls/tests/DeviceTests/Elements/Shell/ShellTests.cs b/src/Controls/tests/DeviceTests/Elements/Shell/ShellTests.cs index d8bf55da859e..b8ffeddb44c7 100644 --- a/src/Controls/tests/DeviceTests/Elements/Shell/ShellTests.cs +++ b/src/Controls/tests/DeviceTests/Elements/Shell/ShellTests.cs @@ -596,6 +596,45 @@ await CreateHandlerAndAddToWindow(shell, async (handler) => }); } + [Fact(DisplayName = "Navigate to Root with BackButtonBehavior no Crash")] + public async Task NavigateToRootWithBackButtonBehaviorNoCrash() + { + SetupBuilder(); + + var shell = await CreateShellAsync(shell => + { + shell.CurrentItem = new ContentPage(); + }); + + await CreateHandlerAndAddToWindow(shell, async (handler) => + { + Assert.False(IsBackButtonVisible(shell.Handler)); + var secondPage = new ContentPage(); + await shell.Navigation.PushAsync(secondPage); + Assert.True(IsBackButtonVisible(shell.Handler)); + + bool canExecute = true; + Command command = new Command(() => { }, () => canExecute); + BackButtonBehavior behavior = new BackButtonBehavior() + { + IsVisible = false, + Command = command + }; + + Shell.SetBackButtonBehavior(secondPage, behavior); + Assert.False(IsBackButtonVisible(shell.Handler)); + behavior.IsVisible = true; + NavigationPage.SetHasBackButton(shell.CurrentPage, true); + + await shell.Navigation.PopToRootAsync(false); + await Task.Delay(100); + canExecute = false; + + command.ChangeCanExecute(); + Assert.NotNull(shell.Handler); + }); + } + protected Task CreateShellAsync(Action action) => InvokeOnMainThreadAsync(() => {