From 4f707c453d39345ac827ec68679c3704a1f55c14 Mon Sep 17 00:00:00 2001 From: BDisp Date: Wed, 4 Jun 2025 19:41:03 +0100 Subject: [PATCH] Fixes #4080. TabGroup not always navigate correctly across groups (#4085) * Fixes #4080. TabGroup not always navigate correctly across groups * Remove duplicated code * Improves code by removing duplicate code * Made requested changes * Change to Theory unit test * Cleanup to run git actions again * Trying fix racing fail unit tests --------- Co-authored-by: Tig --- Terminal.Gui/App/Application.Keyboard.cs | 10 +-- Terminal.Gui/ViewBase/View.Hierarchy.cs | 19 +++++ Terminal.Gui/ViewBase/View.Navigation.cs | 70 +++++++++++------- .../FluentTests/BasicFluentAssertionTests.cs | 74 ++++++++++++++++++- Tests/UnitTests/Application/KeyboardTests.cs | 2 +- .../View/SubviewTests.cs | 34 +++++++++ 6 files changed, 173 insertions(+), 36 deletions(-) diff --git a/Terminal.Gui/App/Application.Keyboard.cs b/Terminal.Gui/App/Application.Keyboard.cs index cb5065561..aa817f873 100644 --- a/Terminal.Gui/App/Application.Keyboard.cs +++ b/Terminal.Gui/App/Application.Keyboard.cs @@ -98,17 +98,11 @@ public static partial class Application // Keyboard handling } else { - // BUGBUG: this seems unneeded. - if (!KeyBindings.TryGet (key, out KeyBinding keybinding)) - { - return null; - } - bool? toReturn = null; - foreach (Command command in keybinding.Commands) + foreach (Command command in binding.Commands) { - toReturn = InvokeCommand (command, key, keybinding); + toReturn = InvokeCommand (command, key, binding); } handled = toReturn ?? true; diff --git a/Terminal.Gui/ViewBase/View.Hierarchy.cs b/Terminal.Gui/ViewBase/View.Hierarchy.cs index 64a836138..d7510c2b0 100644 --- a/Terminal.Gui/ViewBase/View.Hierarchy.cs +++ b/Terminal.Gui/ViewBase/View.Hierarchy.cs @@ -469,10 +469,29 @@ public partial class View // SuperView/SubView hierarchy management (SuperView, /// /// Moves to the end of the list. + /// If the is , keeps the original sorting. /// /// The subview to move. public void MoveSubViewToEnd (View subview) { + if (Arrangement.HasFlag (ViewArrangement.Overlapped)) + { + PerformActionForSubView ( + subview, + x => + { + while (InternalSubViews!.IndexOf (x) != InternalSubViews.Count - 1) + { + View v = InternalSubViews [0]; + InternalSubViews!.Remove (v); + InternalSubViews.Add (v); + } + } + ); + + return; + } + PerformActionForSubView ( subview, x => diff --git a/Terminal.Gui/ViewBase/View.Navigation.cs b/Terminal.Gui/ViewBase/View.Navigation.cs index 733a53071..a3628f99a 100644 --- a/Terminal.Gui/ViewBase/View.Navigation.cs +++ b/Terminal.Gui/ViewBase/View.Navigation.cs @@ -62,38 +62,18 @@ public partial class View // Focus and cross-view navigation management (TabStop if (direction == NavigationDirection.Forward && focused == focusChain [^1] && SuperView is null) { // We're at the top of the focus chain. Go back down the focus chain and focus the first TabGroup - View [] views = GetFocusChain (NavigationDirection.Forward, TabBehavior.TabGroup); - - if (views.Length > 0) + if (AdvanceFocusChain ()) { - View [] subViews = views [0].GetFocusChain (NavigationDirection.Forward, TabBehavior.TabStop); - - if (subViews.Length > 0) - { - if (subViews [0].SetFocus ()) - { - return true; - } - } + return true; } } - if (direction == NavigationDirection.Backward && focused == focusChain [0]) + if (direction == NavigationDirection.Backward && focused == focusChain [0] && SuperView is null) { // We're at the bottom of the focus chain - View [] views = GetFocusChain (NavigationDirection.Forward, TabBehavior.TabGroup); - - if (views.Length > 0) + if (AdvanceFocusChain ()) { - View [] subViews = views [^1].GetFocusChain (NavigationDirection.Forward, TabBehavior.TabStop); - - if (subViews.Length > 0) - { - if (subViews [0].SetFocus ()) - { - return true; - } - } + return true; } } } @@ -149,6 +129,46 @@ public partial class View // Focus and cross-view navigation management (TabStop (bool focusSet, bool _) = view.SetHasFocusTrue (Focused); return focusSet; + + bool AdvanceFocusChain () + { + if (focusChain.Length > 0) + { + // Get the index of the currently focused view + int focusedTabGroupIndex = focusChain.IndexOf (Focused); // Will return -1 if Focused can't be found or is null + + if (focusedTabGroupIndex + 1 > focusChain.Length - 1) + { + focusedTabGroupIndex = 0; + } + else + { + focusedTabGroupIndex++; + } + + View [] subViews = focusChain [focusedTabGroupIndex].GetFocusChain (NavigationDirection.Forward, TabBehavior.TabStop); + + if (subViews.Length > 0) + { + if (focusChain [focusedTabGroupIndex]._previouslyFocused is { } + && subViews.Any (v => v == focusChain [focusedTabGroupIndex]._previouslyFocused)) + { + if (focusChain [focusedTabGroupIndex]._previouslyFocused!.SetFocus ()) + { + return true; + } + } + + // We have a subview that can be focused + if (subViews [0].SetFocus ()) + { + return true; + } + } + } + + return false; + } } private bool RaiseAdvancingFocus (NavigationDirection direction, TabBehavior? behavior) diff --git a/Tests/IntegrationTests/FluentTests/BasicFluentAssertionTests.cs b/Tests/IntegrationTests/FluentTests/BasicFluentAssertionTests.cs index f45236ba9..0796d5f00 100644 --- a/Tests/IntegrationTests/FluentTests/BasicFluentAssertionTests.cs +++ b/Tests/IntegrationTests/FluentTests/BasicFluentAssertionTests.cs @@ -12,7 +12,6 @@ public class BasicFluentAssertionTests _out = new TestOutputWriter (outputHelper); } - [Theory] [ClassData (typeof (V2TestDrivers))] public void GuiTestContext_NewInstance_Runs (V2TestDriver d) @@ -24,7 +23,6 @@ public class BasicFluentAssertionTests context.Stop (); } - [Theory] [ClassData (typeof (V2TestDrivers))] public void GuiTestContext_QuitKey_Stops (V2TestDriver d) @@ -153,4 +151,76 @@ public class BasicFluentAssertionTests .WriteOutLogs (_out); Assert.True (clicked); } + + [Theory] + [ClassData (typeof (V2TestDrivers))] + public void Toplevel_TabGroup_Forward_Backward (V2TestDriver d) + { + var v1 = new View { Id = "v1", CanFocus = true }; + var v2 = new View { Id = "v2", CanFocus = true }; + var v3 = new View { Id = "v3", CanFocus = true }; + var v4 = new View { Id = "v4", CanFocus = true }; + var v5 = new View { Id = "v5", CanFocus = true }; + var v6 = new View { Id = "v6", CanFocus = true }; + + using GuiTestContext c = With.A (50, 20, d) + .Then ( + () => + { + var w1 = new Window { Id = "w1" }; + w1.Add (v1, v2); + var w2 = new Window { Id = "w2" }; + w2.Add (v3, v4); + var w3 = new Window { Id = "w3" }; + w3.Add (v5, v6); + Toplevel top = Application.Top!; + Application.Top!.Add (w1, w2, w3); + }) + .WaitIteration () + .Then (() => Assert.True (v5.HasFocus)) + .RaiseKeyDownEvent (Key.F6) + .Then (() => Assert.True (v1.HasFocus)) + .RaiseKeyDownEvent (Key.F6) + .Then (() => Assert.True (v3.HasFocus)) + .RaiseKeyDownEvent (Key.F6.WithShift) + .Then (() => Assert.True (v1.HasFocus)) + .RaiseKeyDownEvent (Key.F6.WithShift) + .Then (() => Assert.True (v5.HasFocus)) + .RaiseKeyDownEvent (Key.F6.WithShift) + .Then (() => Assert.True (v3.HasFocus)) + .RaiseKeyDownEvent (Key.F6) + .Then (() => Assert.True (v5.HasFocus)) + .RaiseKeyDownEvent (Key.F6) + .Then (() => Assert.True (v1.HasFocus)) + .RaiseKeyDownEvent (Key.F6) + .Then (() => Assert.True (v3.HasFocus)) + .RaiseKeyDownEvent (Key.F6.WithShift) + .Then (() => Assert.True (v1.HasFocus)) + .RaiseKeyDownEvent (Key.F6.WithShift) + .Then (() => Assert.True (v5.HasFocus)) + .RaiseKeyDownEvent (Key.F6.WithShift) + .Then (() => Assert.True (v3.HasFocus)) + .RaiseKeyDownEvent (Key.Tab) + .Then (() => Assert.True (v4.HasFocus)) + .RaiseKeyDownEvent (Key.F6) + .Then (() => Assert.True (v5.HasFocus)) + .RaiseKeyDownEvent (Key.F6) + .Then (() => Assert.True (v1.HasFocus)) + .RaiseKeyDownEvent (Key.F6.WithShift) + .Then (() => Assert.True (v5.HasFocus)) + .RaiseKeyDownEvent (Key.Tab) + .Then (() => Assert.True (v6.HasFocus)) + .RaiseKeyDownEvent (Key.F6.WithShift) + .Then (() => Assert.True (v4.HasFocus)) + .RaiseKeyDownEvent (Key.F6) + .Then (() => Assert.True (v6.HasFocus)) + .WriteOutLogs (_out) + .Stop (); + Assert.False (v1.HasFocus); + Assert.False (v2.HasFocus); + Assert.False (v3.HasFocus); + Assert.False (v4.HasFocus); + Assert.False (v5.HasFocus); + Assert.False (v6.HasFocus); + } } diff --git a/Tests/UnitTests/Application/KeyboardTests.cs b/Tests/UnitTests/Application/KeyboardTests.cs index 77db195ac..660b613dd 100644 --- a/Tests/UnitTests/Application/KeyboardTests.cs +++ b/Tests/UnitTests/Application/KeyboardTests.cs @@ -215,7 +215,7 @@ public class KeyboardTests Assert.True (v3.HasFocus); Application.RaiseKeyDownEvent (Key.F6); - Assert.True (v1.HasFocus); + Assert.True (v2.HasFocus); // previously focused view was preserved Application.RequestStop (); }; diff --git a/Tests/UnitTestsParallelizable/View/SubviewTests.cs b/Tests/UnitTestsParallelizable/View/SubviewTests.cs index a9b385714..a2a1aa39c 100644 --- a/Tests/UnitTestsParallelizable/View/SubviewTests.cs +++ b/Tests/UnitTestsParallelizable/View/SubviewTests.cs @@ -118,6 +118,40 @@ public class SubViewTests Assert.Equal (new (5, 5), view.GetContentSize ()); } + [Theory] + [InlineData (ViewArrangement.Fixed)] + [InlineData (ViewArrangement.Overlapped)] + public void MoveSubViewToEnd_ViewArrangement (ViewArrangement arrangement) + { + View superView = new () { Arrangement = arrangement }; + + var subview1 = new View + { + Id = "subview1" + }; + + var subview2 = new View + { + Id = "subview2" + }; + + var subview3 = new View + { + Id = "subview3" + }; + + superView.Add (subview1, subview2, subview3); + + superView.MoveSubViewToEnd (subview1); + Assert.Equal ([subview2, subview3, subview1], superView.SubViews.ToArray ()); + + superView.MoveSubViewToEnd (subview2); + Assert.Equal ([subview3, subview1, subview2], superView.SubViews.ToArray ()); + + superView.MoveSubViewToEnd (subview3); + Assert.Equal ([subview1, subview2, subview3], superView.SubViews.ToArray ()); + } + [Fact] public void MoveSubViewToStart () {