From 9635a434eda38ee006e70200ece40d74dbaa4e92 Mon Sep 17 00:00:00 2001 From: BDisp Date: Sun, 18 Aug 2024 15:56:34 +0100 Subject: [PATCH] Fixes #3667. Null reference in v2 in FindDeepestView. --- Terminal.Gui/Views/Toplevel.cs | 62 ++-------------------- UICatalog/UICatalog.cs | 12 +++-- UnitTests/Views/ToplevelTests.cs | 91 ++++++++++++++++++++++++++------ 3 files changed, 89 insertions(+), 76 deletions(-) diff --git a/Terminal.Gui/Views/Toplevel.cs b/Terminal.Gui/Views/Toplevel.cs index e51263c4d..dcab31e11 100644 --- a/Terminal.Gui/Views/Toplevel.cs +++ b/Terminal.Gui/Views/Toplevel.cs @@ -69,75 +69,21 @@ public partial class Toplevel : View #region Subviews // TODO: Deprecate - Any view can host a menubar in v2 - /// Gets or sets the menu for this Toplevel. - public MenuBar? MenuBar { get; set; } + /// Gets the latest added into this Toplevel. + public MenuBar? MenuBar => (MenuBar?)Subviews?.LastOrDefault (s => s is MenuBar); // TODO: Deprecate - Any view can host a statusbar in v2 - /// Gets or sets the status bar for this Toplevel. - public StatusBar? StatusBar { get; set; } + /// Gets the latest added into this Toplevel. + public StatusBar? StatusBar => (StatusBar?)Subviews?.LastOrDefault (s => s is StatusBar); /// public override View Add (View view) { CanFocus = true; - AddMenuStatusBar (view); return base.Add (view); } - /// - public override View Remove (View view) - { - if (this is Toplevel { MenuBar: { } }) - { - RemoveMenuStatusBar (view); - } - - return base.Remove (view); - } - - /// - public override void RemoveAll () - { - if (this == Application.Top) - { - MenuBar?.Dispose (); - MenuBar = null; - StatusBar?.Dispose (); - StatusBar = null; - } - - base.RemoveAll (); - } - - internal void AddMenuStatusBar (View view) - { - if (view is MenuBar) - { - MenuBar = view as MenuBar; - } - - if (view is StatusBar) - { - StatusBar = view as StatusBar; - } - } - - internal void RemoveMenuStatusBar (View view) - { - if (view is MenuBar) - { - MenuBar?.Dispose (); - MenuBar = null; - } - - if (view is StatusBar) - { - StatusBar?.Dispose (); - StatusBar = null; - } - } - // TODO: Overlapped - Rename to AllSubviewsClosed - Move to View? /// /// Invoked when the last child of the Toplevel is closed from by diff --git a/UICatalog/UICatalog.cs b/UICatalog/UICatalog.cs index 60353f59f..aff6dd975 100644 --- a/UICatalog/UICatalog.cs +++ b/UICatalog/UICatalog.cs @@ -408,7 +408,7 @@ public class UICatalogApp _themeMenuItems = CreateThemeMenuItems (); _themeMenuBarItem = new ("_Themes", _themeMenuItems); - MenuBar = new () + MenuBar menuBar = new () { Menus = [ @@ -462,13 +462,15 @@ public class UICatalogApp ) ] }; + Add (menuBar); - StatusBar = new () + StatusBar statusBar = new () { Visible = ShowStatusBar, AlignmentModes = AlignmentModes.IgnoreFirstOrLast, CanFocus = false }; + Add (statusBar); if (StatusBar is { }) { @@ -484,7 +486,11 @@ public class UICatalogApp Title = "Show/Hide Status Bar", CanFocus = false, }; - statusBarShortcut.Accept += (sender, args) => { StatusBar.Visible = !StatusBar.Visible; }; + statusBarShortcut.Accept += (sender, args) => + { + StatusBar.Visible = !StatusBar.Visible; + args.Handled = true; + }; ShForce16Colors = new () { diff --git a/UnitTests/Views/ToplevelTests.cs b/UnitTests/Views/ToplevelTests.cs index 3a5cbda58..047399abd 100644 --- a/UnitTests/Views/ToplevelTests.cs +++ b/UnitTests/Views/ToplevelTests.cs @@ -246,14 +246,26 @@ public partial class ToplevelTests (ITestOutputHelper output) top.OnUnloaded (); Assert.Equal ("Unloaded", eventInvoked); - top.AddMenuStatusBar (new MenuBar ()); + top.Add (new MenuBar ()); Assert.NotNull (top.MenuBar); - top.AddMenuStatusBar (new StatusBar ()); + top.Add (new StatusBar ()); Assert.NotNull (top.StatusBar); - top.RemoveMenuStatusBar (top.MenuBar); + var menuBar = top.MenuBar; + top.Remove (top.MenuBar); Assert.Null (top.MenuBar); - top.RemoveMenuStatusBar (top.StatusBar); + Assert.NotNull (menuBar); + var statusBar = top.StatusBar; + top.Remove (top.StatusBar); Assert.Null (top.StatusBar); + Assert.NotNull (statusBar); +#if true + Assert.False (menuBar.WasDisposed); + Assert.False (statusBar.WasDisposed); + menuBar.Dispose (); + statusBar.Dispose (); + Assert.True (menuBar.WasDisposed); + Assert.True (statusBar.WasDisposed); +#endif Application.Begin (top); Assert.Equal (top, Application.Top); @@ -265,7 +277,7 @@ public partial class ToplevelTests (ITestOutputHelper output) Assert.Equal (0, ny); Assert.Null (sb); - top.AddMenuStatusBar (new MenuBar ()); + top.Add (new MenuBar ()); Assert.NotNull (top.MenuBar); // Application.Top with a menu and without status bar. @@ -274,7 +286,7 @@ public partial class ToplevelTests (ITestOutputHelper output) Assert.Equal (1, ny); Assert.Null (sb); - top.AddMenuStatusBar (new StatusBar ()); + top.Add (new StatusBar ()); Assert.NotNull (top.StatusBar); // Application.Top with a menu and status bar. @@ -286,8 +298,10 @@ public partial class ToplevelTests (ITestOutputHelper output) Assert.Equal (2, ny); Assert.NotNull (sb); - top.RemoveMenuStatusBar (top.MenuBar); + menuBar = top.MenuBar; + top.Remove (top.MenuBar); Assert.Null (top.MenuBar); + Assert.NotNull (menuBar); // Application.Top without a menu and with a status bar. View.GetLocationEnsuringFullVisibility (top, 2, 2, out nx, out ny, out sb); @@ -298,8 +312,10 @@ public partial class ToplevelTests (ITestOutputHelper output) Assert.Equal (2, ny); Assert.NotNull (sb); - top.RemoveMenuStatusBar (top.StatusBar); + statusBar = top.StatusBar; + top.Remove (top.StatusBar); Assert.Null (top.StatusBar); + Assert.NotNull (statusBar); Assert.Null (top.MenuBar); var win = new Window { Width = Dim.Fill (), Height = Dim.Fill () }; @@ -318,7 +334,7 @@ public partial class ToplevelTests (ITestOutputHelper output) Assert.Equal (0, ny); Assert.Null (sb); - top.AddMenuStatusBar (new MenuBar ()); + top.Add (new MenuBar ()); Assert.NotNull (top.MenuBar); // Application.Top with a menu and without status bar. @@ -327,7 +343,7 @@ public partial class ToplevelTests (ITestOutputHelper output) Assert.Equal (1, ny); Assert.Null (sb); - top.AddMenuStatusBar (new StatusBar ()); + top.Add (new StatusBar ()); Assert.NotNull (top.StatusBar); // Application.Top with a menu and status bar. @@ -339,10 +355,14 @@ public partial class ToplevelTests (ITestOutputHelper output) Assert.Equal (20, ny); Assert.NotNull (sb); - top.RemoveMenuStatusBar (top.MenuBar); - top.RemoveMenuStatusBar (top.StatusBar); - Assert.Null (top.StatusBar); + menuBar = top.MenuBar; + statusBar = top.StatusBar; + top.Remove (top.MenuBar); Assert.Null (top.MenuBar); + Assert.NotNull (menuBar); + top.Remove (top.StatusBar); + Assert.Null (top.StatusBar); + Assert.NotNull (statusBar); top.Remove (win); @@ -355,7 +375,7 @@ public partial class ToplevelTests (ITestOutputHelper output) Assert.Equal (0, ny); Assert.Null (sb); - top.AddMenuStatusBar (new MenuBar ()); + top.Add (new MenuBar ()); Assert.NotNull (top.MenuBar); // Application.Top with a menu and without status bar. @@ -364,7 +384,7 @@ public partial class ToplevelTests (ITestOutputHelper output) Assert.Equal (2, ny); Assert.Null (sb); - top.AddMenuStatusBar (new StatusBar ()); + top.Add (new StatusBar ()); Assert.NotNull (top.StatusBar); // Application.Top with a menu and status bar. @@ -387,7 +407,21 @@ public partial class ToplevelTests (ITestOutputHelper output) win.NewMouseEvent (new () { Position = new (6, 0), Flags = MouseFlags.Button1Pressed }); //Assert.Null (Toplevel._dragPosition); +#if true + Assert.False (top.MenuBar.WasDisposed); + Assert.False (top.StatusBar.WasDisposed); +#endif + menuBar = top.MenuBar; + statusBar = top.StatusBar; top.Dispose (); + Assert.Null (top.MenuBar); + Assert.Null (top.StatusBar); + Assert.NotNull (menuBar); + Assert.NotNull (statusBar); +#if true + Assert.True (menuBar.WasDisposed); + Assert.True (statusBar.WasDisposed); +#endif } [Fact] @@ -1568,4 +1602,31 @@ public partial class ToplevelTests (ITestOutputHelper output) t.Dispose (); Application.Shutdown (); } + + [Fact] + public void Remove_Do_Not_Dispose_MenuBar_Or_StatusBar () + { + var mb = new MenuBar (); + var sb = new StatusBar (); + var tl = new Toplevel (); + +#if DEBUG + Assert.False (mb.WasDisposed); + Assert.False (sb.WasDisposed); +#endif + tl.Add (mb, sb); + Assert.NotNull (tl.MenuBar); + Assert.NotNull (tl.StatusBar); +#if DEBUG + Assert.False (mb.WasDisposed); + Assert.False (sb.WasDisposed); +#endif + tl.RemoveAll (); + Assert.Null (tl.MenuBar); + Assert.Null (tl.StatusBar); +#if DEBUG + Assert.False (mb.WasDisposed); + Assert.False (sb.WasDisposed); +#endif + } }