Fix Enter on MenuBar context menu crash

tznind · tznind · commit 8d37e77f8261 · 2026-04-21T03:53:55.000+01:00
Fix hit detection of what top level MenuBar item was right clicked
Serialize Separator as Line differently
diff --git a/src/Design.cs b/src/Design.cs
@@ -278,16 +278,7 @@ public IEnumerable<Property> GetDesignableProperties()
         return this.designableProperties;
     }
 
-    /// <summary>
-    /// Returns all operations not to do with setting properties.  Often these
-    /// are view specific e.g. add/remove column from a <see cref="TableView"/>.
-    /// </summary>
-    /// <returns>All view specific <see cref="Operation"/> supported on <see cref="View"/> Type.
-    /// Does not return regular <see cref="Property"/> changing operations.</returns>
-    public IEnumerable<IOperation> GetExtraOperations()
-    {
-        return this.GetExtraOperations(Point.Empty);
-    }
+
 
     /// <summary>
     /// Returns all <see cref="Operation"/> that can be performed on the view at position <paramref name="pos"/>
@@ -297,8 +288,10 @@ public IEnumerable<IOperation> GetExtraOperations()
     /// may inform what operations are returned (e.g. right clicking a specific table view column).  Otherwise
     /// <see cref="Point.Empty"/>.</param>
     /// <returns>All view specific <see cref="IOperation"/> that are supported at the <paramref name="pos"/>.</returns>
-    public IEnumerable<IOperation> GetExtraOperations(Point pos)
+    public IEnumerable<IOperation> GetExtraOperations(Mouse? mouse = null)
     {
+        var pos = mouse == null ? Point.Empty: View.ScreenToViewport(mouse.Position.Value);
+
         // Extra TableView operations
         if (this.View is TableView tv)
         {
@@ -358,7 +351,7 @@ public IEnumerable<IOperation> GetExtraOperations(Point pos)
             {
                 yield return new AddMenuOperation(App, this, null);
 
-                var menu = pos.IsEmpty ? mb.GetSelectedMenuItem() : mb.ScreenToMenuBarItem(pos.X);
+                var menu = mouse == null ? mb.GetSelectedMenuItem() : mb.ScreenToMenuBarItem(App, mouse);
 
                 if (menu != null)
                 {
diff --git a/src/MenuBarExtensions.cs b/src/MenuBarExtensions.cs
@@ -1,7 +1,7 @@
 using Terminal.Gui;
-using Terminal.Gui.Text;
+using Terminal.Gui.App;
+using Terminal.Gui.Input;
 using Terminal.Gui.Views;
-using TerminalGuiDesigner.Operations.MenuOperations;
 
 namespace TerminalGuiDesigner;
 
@@ -10,6 +10,8 @@ namespace TerminalGuiDesigner;
 /// </summary>
 public static class MenuBarExtensions
 {
+    /// <summary>The title used to mark a <see cref="MenuItem"/> as a separator in the designer.</summary>
+    public const string SeparatorTitle = "---";
     /// <summary>
     /// Gets the top level selected <see cref="MenuBarItem"/> in the <paramref name="menuBar"/>
     /// or null if it is not open/no selection is set.  Note that this is the top level menu item only
@@ -19,20 +21,16 @@ public static class MenuBarExtensions
     /// <returns>Selected <see cref="MenuItem"/> or null if none.</returns>
     public static MenuBarItem? GetSelectedMenuItem(this MenuBar menuBar)
     {
-        int selected = menuBar.GetNonNullNonPublicFieldValue<int, MenuBar>( "selected" );
+        if (menuBar.Focused is MenuBarItem mbi)
+            return mbi;
 
-        if (selected < 0 || selected >= menuBar.SubViews.OfType<MenuBarItem>().Count())
-        {
-            return null;
-        }
-
-        return menuBar.SubViews.OfType<MenuBarItem>().ElementAt(selected);
+        return menuBar.SubViews.OfType<MenuBarItem>().FirstOrDefault();
     }
 
     /// <summary>
     /// Walks all menus in <paramref name="menuBar"/> and replaces any <see cref="Line"/> views
     /// (produced by code generation for separators) with sentinel <see cref="MenuItem"/> instances
-    /// whose <see cref="MenuItem.Title"/> is <see cref="ConvertMenuItemToSeperatorOperation.SeparatorTitle"/>.
+    /// whose <see cref="MenuItem.Title"/> is <see cref="SeparatorTitle"/>.
     /// Call this after loading a <see cref="MenuBar"/> from generated code.
     /// </summary>
     public static void ConvertLineSeparatorsToSentinels(this MenuBar menuBar)
@@ -60,7 +58,7 @@ private static void ConvertLineSeparatorsInMenu(Menu menu)
             foreach (var v in allViews)
             {
                 menu.Add(v is Line
-                    ? new MenuItem { Title = ConvertMenuItemToSeperatorOperation.SeparatorTitle }
+                    ? new MenuItem { Title = SeparatorTitle }
                     : v);
             }
         }
@@ -85,48 +83,25 @@ private static void ConvertLineSeparatorsInMenu(Menu menu)
     /// <param name="menuBar"><see cref="MenuBar"/> you want to find the clicked <see cref="MenuBarItem"/> (top level menu) for.</param>
     /// <param name="screenX">Screen coordinate of the click in X.</param>
     /// <returns>The <see cref="MenuBarItem"/> under the mouse at this position or null (only considers X).</returns>
-    public static MenuBarItem? ScreenToMenuBarItem(this MenuBar menuBar, int screenX)
+    public static MenuBarItem? ScreenToMenuBarItem(this MenuBar menuBar, IApplication application, Mouse mouse)
     {
-        // These might be changed in Terminal.Gui library
-        // TODO: Maybe load these from a config file, so we aren't at TG's mercy
-        const int initialWhitespace = 1;
-        const int afterEachItemWhitespace = 2;
+        var hit = menuBar.HitTest(application,mouse, out _, out _);
 
-        if (menuBar.SubViews.OfType<MenuBarItem>().Count() == 0)
+        if(hit == null)
         {
             return null;
         }
 
-        var clientPoint = menuBar.ScreenToViewport(new Point(screenX, 0));
-
-        // if click is not in our client area
-        if (clientPoint.X < initialWhitespace)
+        if (hit is MenuBarItem mbi)
         {
-            return null;
+            return mbi;
         }
-
-        // Calculate the x display positions of each menu
-        int distance = initialWhitespace;
-        Dictionary<int, MenuBarItem?> menuXLocations = new();
-
-        foreach (var mb in menuBar.SubViews.OfType<MenuBarItem>())
-        {
-            menuXLocations.Add(distance, mb);
-            distance += mb.Title.GetColumns() + afterEachItemWhitespace;
-        }
-
-        // anything after this is not a click on a menu
-        menuXLocations.Add(distance, null);
-
-        // LastOrDefault does not work with Dictionaries, if we somehow still have a point outside bounds
-        // of anything then just return null;
-        if (!menuXLocations.Any(m => m.Key <= clientPoint.X))
+        if (hit.SuperView is MenuBarItem super)
         {
-            return null;
+            return super;
         }
 
-        // Return the last menu item that begins rendering before this X point
-        return menuXLocations.Last(m => m.Key <= clientPoint.X).Value;
+        return null;
     }
 
     /// <summary>
diff --git a/src/MenuTracker.cs b/src/MenuTracker.cs
@@ -277,10 +277,10 @@ private void ConvertEmptySubMenus(MenuItem menuItem)
             return potentialParent;
         }
 
-        // Recursively check each child's submenu
+        // Recursively check each child's submenu (SubMenu for MenuItem, PopoverMenu for MenuBarItem)
         foreach (var child in children)
         {
-            if (child.SubMenu != null)
+            if (child is MenuBarItem || child.SubMenu != null)
             {
                 var result = FindParentRecursive(item, child);
                 if (result != null)
diff --git a/src/Operations/MenuOperations/ConvertMenuItemToSeperatorOperation.cs b/src/Operations/MenuOperations/ConvertMenuItemToSeperatorOperation.cs
@@ -8,16 +8,13 @@ namespace TerminalGuiDesigner.Operations.MenuOperations;
 /// <para>
 /// Converts a <see cref="MenuItem"/> into a Separator (horizontal line in menu).
 /// In the designer the separator is stored as a <see cref="MenuItem"/> with
-/// <see cref="SeparatorTitle"/> as its <see cref="MenuItem.Title"/>.
+/// <see cref="MenuBarExtensions.SeparatorTitle"/> as its <see cref="MenuItem.Title"/>.
 /// When code is generated it becomes a <c>new Line { Orientation = Orientation.Horizontal }</c>
 /// and on load those Line views are converted back to sentinel MenuItems.
 /// </para>
 /// </summary>
 public class ConvertMenuItemToSeperatorOperation : MenuItemOperation
 {
-    /// <summary>The title used to mark a MenuItem as a separator in the designer.</summary>
-    public const string SeparatorTitle = "---";
-
     private string? originalTitle;
 
     /// <summary>
@@ -28,7 +25,7 @@ public class ConvertMenuItemToSeperatorOperation : MenuItemOperation
     public ConvertMenuItemToSeperatorOperation(IApplication app, MenuItem toConvert)
         : base(app, toConvert)
     {
-        if (toConvert.Title?.ToString() == SeparatorTitle)
+        if (toConvert.Title?.ToString() == MenuBarExtensions.SeparatorTitle)
         {
             IsImpossible = true;
         }
@@ -61,7 +58,7 @@ protected override bool DoImpl()
         }
 
         this.originalTitle = this.OperateOn.Title?.ToString();
-        this.OperateOn.Title = SeparatorTitle;
+        this.OperateOn.Title = MenuBarExtensions.SeparatorTitle;
         this.Bar?.SetNeedsDraw();
         return true;
     }
diff --git a/src/Operations/MenuOperations/MoveMenuItemRightOperation.cs b/src/Operations/MenuOperations/MoveMenuItemRightOperation.cs
@@ -73,39 +73,51 @@ protected override bool DoImpl()
             return false;
         }
 
-        // When user hits shift right
         var children = this.Parent.GetMenuItems(out _);
         var currentItemIdx = children.IndexOf(this.OperateOn);
         var aboveIdx = currentItemIdx - 1;
 
-        // and there is an item above
         if (aboveIdx < 0)
         {
             return false;
         }
 
-        // Get or create a submenu on the item above
         var itemAbove = children[aboveIdx];
-        if (itemAbove.SubMenu == null)
-        {
-            itemAbove.SubMenu = new Menu();
-        }
 
-        // Remove us from current menu
+        // Remove us from current menu first
         this.Parent.RemoveMenuItem(this.OperateOn);
 
-        // Add us to the submenu of the item above
-        if (this.InsertionIndex != null)
+        if (itemAbove is MenuBarItem existingMbi)
         {
-            itemAbove.InsertMenuItem(this.InsertionIndex.Value, this.OperateOn);
+            // Item above already has a sub-menu — add to it
+            if (this.InsertionIndex != null)
+            {
+                existingMbi.InsertMenuItem(this.InsertionIndex.Value, this.OperateOn);
+            }
+            else
+            {
+                var subItems = existingMbi.GetMenuItems(out _);
+                subItems.Add(this.OperateOn);
+                existingMbi.SetMenuItems(subItems);
+            }
         }
         else
         {
-            itemAbove.SubMenu.Add(this.OperateOn);
+            // Convert plain MenuItem to MenuBarItem with OperateOn as first child.
+            // Re-read children since RemoveMenuItem may have shifted indexes.
+            children = this.Parent.GetMenuItems(out _);
+            var newAboveIdx = children.IndexOf(itemAbove);
+
+            var newMbi = new MenuBarItem(itemAbove.Title, new MenuItem[] { this.OperateOn });
+            newMbi.Data = itemAbove.Data;
+            newMbi.Key = itemAbove.Key;
+
+            children.RemoveAt(newAboveIdx);
+            children.Insert(newAboveIdx, newMbi);
+            this.Parent.SetMenuItems(children);
         }
 
         this.Bar?.SetNeedsDraw();
-
         return true;
     }
 
diff --git a/src/Operations/OperationFactory.cs b/src/Operations/OperationFactory.cs
@@ -109,7 +109,7 @@ private IEnumerable<IOperation> CreateOperations(Mouse? m, Design d)
     {
         var ops = m == null || !m.Position.HasValue?
             d.GetExtraOperations() :
-            d.GetExtraOperations(d.View.ScreenToViewport(m.Position.Value));
+            d.GetExtraOperations(m);
 
         foreach (var extra in ops.Where(c => !c.IsImpossible))
         {
diff --git a/src/ToCode/MenuBarItemsToCode.cs b/src/ToCode/MenuBarItemsToCode.cs
@@ -82,7 +82,7 @@ private void ToCode(CodeDomArgs args, MenuItem child, out string fieldName)
         foreach (var mi in child.GetMenuItems(out wasPopover))
         {
             // Separators are stored as MenuItem("---") in the designer but emitted as Line in code
-            if (mi.Title?.ToString() == Operations.MenuOperations.ConvertMenuItemToSeperatorOperation.SeparatorTitle)
+            if (mi.Title?.ToString() == MenuBarExtensions.SeparatorTitle)
             {
                 children.Add("new Line { Orientation = Terminal.Gui.ViewBase.Orientation.Horizontal }");
                 continue;
diff --git a/tests/Operations/ConvertMenuItemToSeperatorOperationTests.cs b/tests/Operations/ConvertMenuItemToSeperatorOperationTests.cs
@@ -20,13 +20,13 @@ public void TestConvertToSeperator_RoundTrip_Do()
             op.Do();
 
             ClassicAssert.AreEqual(1, fileMenu.GetMenuItems(out _).Count);
-            ClassicAssert.AreEqual(ConvertMenuItemToSeperatorOperation.SeparatorTitle, fileMenu.GetMenuItems(out _)[0].Title.ToString());
+            ClassicAssert.AreEqual(MenuBarExtensions.SeparatorTitle, fileMenu.GetMenuItems(out _)[0].Title.ToString());
 
         }, out _);
 
         var mbInFileMenu = mbIn.SubViews.OfType<MenuBarItem>().First();
         ClassicAssert.AreEqual(1, mbInFileMenu.GetMenuItems(out _).Count);
-        ClassicAssert.AreEqual(ConvertMenuItemToSeperatorOperation.SeparatorTitle, mbInFileMenu.GetMenuItems(out _)[0].Title.ToString());
+        ClassicAssert.AreEqual(MenuBarExtensions.SeparatorTitle, mbInFileMenu.GetMenuItems(out _)[0].Title.ToString());
     }
 
     [Test]
@@ -40,7 +40,7 @@ public void TestConvertToSeperator_RoundTrip_UnDo()
             var op = new ConvertMenuItemToSeperatorOperation(App, orig);
             op.Do();
             ClassicAssert.AreEqual(1, fileMenu.GetMenuItems(out _).Count);
-            ClassicAssert.AreEqual(ConvertMenuItemToSeperatorOperation.SeparatorTitle, fileMenu.GetMenuItems(out _)[0].Title.ToString());
+            ClassicAssert.AreEqual(MenuBarExtensions.SeparatorTitle, fileMenu.GetMenuItems(out _)[0].Title.ToString());
 
             op.Undo();
             ClassicAssert.AreEqual(1, fileMenu.GetMenuItems(out _).Count);
@@ -50,7 +50,7 @@ public void TestConvertToSeperator_RoundTrip_UnDo()
             op.Undo();
             op.Redo();
             ClassicAssert.AreEqual(1, fileMenu.GetMenuItems(out _).Count);
-            ClassicAssert.AreEqual(ConvertMenuItemToSeperatorOperation.SeparatorTitle, fileMenu.GetMenuItems(out _)[0].Title.ToString());
+            ClassicAssert.AreEqual(MenuBarExtensions.SeparatorTitle, fileMenu.GetMenuItems(out _)[0].Title.ToString());
 
             op.Undo();
             op.Undo();
@@ -64,6 +64,6 @@ public void TestConvertToSeperator_RoundTrip_UnDo()
         var mbInFileMenu = mbIn.SubViews.OfType<MenuBarItem>().First();
         ClassicAssert.AreEqual(1, mbInFileMenu.GetMenuItems(out _).Count);
         ClassicAssert.IsNotNull(mbInFileMenu.GetMenuItems(out _)[0]);
-        ClassicAssert.AreNotEqual(ConvertMenuItemToSeperatorOperation.SeparatorTitle, mbInFileMenu.GetMenuItems(out _)[0].Title.ToString());
+        ClassicAssert.AreNotEqual(MenuBarExtensions.SeparatorTitle, mbInFileMenu.GetMenuItems(out _)[0].Title.ToString());
     }
 }

Original file line number	Diff line number	Diff line change
`@@ -277,10 +277,10 @@ private void ConvertEmptySubMenus(MenuItem menuItem)`
`277`	`277`	`return potentialParent;`
`278`	`278`	`}`
`279`	`279`
`280`		`- // Recursively check each child's submenu`
	`280`	`+ // Recursively check each child's submenu (SubMenu for MenuItem, PopoverMenu for MenuBarItem)`
`281`	`281`	`foreach (var child in children)`
`282`	`282`	`{`
`283`		`- if (child.SubMenu != null)`
	`283`	`+ if (child is MenuBarItem \|\| child.SubMenu != null)`
`284`	`284`	`{`
`285`	`285`	`var result = FindParentRecursive(item, child);`
`286`	`286`	`if (result != null)`
Original file line number	Diff line number	Diff line change
`@@ -8,16 +8,13 @@ namespace TerminalGuiDesigner.Operations.MenuOperations;`
`8`	`8`	`/// <para>`
`9`	`9`	`/// Converts a <see cref="MenuItem"/> into a Separator (horizontal line in menu).`
`10`	`10`	`/// In the designer the separator is stored as a <see cref="MenuItem"/> with`
`11`		`-/// <see cref="SeparatorTitle"/> as its <see cref="MenuItem.Title"/>.`
	`11`	`+/// <see cref="MenuBarExtensions.SeparatorTitle"/> as its <see cref="MenuItem.Title"/>.`
`12`	`12`	`/// When code is generated it becomes a <c>new Line { Orientation = Orientation.Horizontal }</c>`
`13`	`13`	`/// and on load those Line views are converted back to sentinel MenuItems.`
`14`	`14`	`/// </para>`
`15`	`15`	`/// </summary>`
`16`	`16`	`public class ConvertMenuItemToSeperatorOperation : MenuItemOperation`
`17`	`17`	`{`
`18`		`- /// <summary>The title used to mark a MenuItem as a separator in the designer.</summary>`
`19`		`- public const string SeparatorTitle = "---";`
`20`		`-`
`21`	`18`	`private string? originalTitle;`
`22`	`19`
`23`	`20`	`/// <summary>`
`@@ -28,7 +25,7 @@ public class ConvertMenuItemToSeperatorOperation : MenuItemOperation`
`28`	`25`	`public ConvertMenuItemToSeperatorOperation(IApplication app, MenuItem toConvert)`
`29`	`26`	`: base(app, toConvert)`
`30`	`27`	`{`
`31`		`- if (toConvert.Title?.ToString() == SeparatorTitle)`
	`28`	`+ if (toConvert.Title?.ToString() == MenuBarExtensions.SeparatorTitle)`
`32`	`29`	`{`
`33`	`30`	`IsImpossible = true;`
`34`	`31`	`}`
`@@ -61,7 +58,7 @@ protected override bool DoImpl()`
`61`	`58`	`}`
`62`	`59`
`63`	`60`	`this.originalTitle = this.OperateOn.Title?.ToString();`
`64`		`- this.OperateOn.Title = SeparatorTitle;`
	`61`	`+ this.OperateOn.Title = MenuBarExtensions.SeparatorTitle;`
`65`	`62`	`this.Bar?.SetNeedsDraw();`
`66`	`63`	`return true;`
`67`	`64`	`}`
Original file line number	Diff line number	Diff line change
`@@ -109,7 +109,7 @@ private IEnumerable<IOperation> CreateOperations(Mouse? m, Design d)`
`109`	`109`	`{`
`110`	`110`	`var ops = m == null \|\| !m.Position.HasValue?`
`111`	`111`	`d.GetExtraOperations() :`
`112`		`- d.GetExtraOperations(d.View.ScreenToViewport(m.Position.Value));`
	`112`	`+ d.GetExtraOperations(m);`
`113`	`113`
`114`	`114`	`foreach (var extra in ops.Where(c => !c.IsImpossible))`
`115`	`115`	`{`
Original file line number	Diff line number	Diff line change
`@@ -82,7 +82,7 @@ private void ToCode(CodeDomArgs args, MenuItem child, out string fieldName)`
`82`	`82`	`foreach (var mi in child.GetMenuItems(out wasPopover))`
`83`	`83`	`{`
`84`	`84`	`// Separators are stored as MenuItem("---") in the designer but emitted as Line in code`
`85`		`- if (mi.Title?.ToString() == Operations.MenuOperations.ConvertMenuItemToSeperatorOperation.SeparatorTitle)`
	`85`	`+ if (mi.Title?.ToString() == MenuBarExtensions.SeparatorTitle)`
`86`	`86`	`{`
`87`	`87`	`children.Add("new Line { Orientation = Terminal.Gui.ViewBase.Orientation.Horizontal }");`
`88`	`88`	`continue;`