From 869c02a1121bcc28b99457a4b4d81ffb0e636179 Mon Sep 17 00:00:00 2001 From: Tig Date: Thu, 3 Apr 2025 15:46:50 -0600 Subject: [PATCH] Fixed Region bug preventing menus without borders from working --- Terminal.Gui/Drawing/Region.cs | 120 +++++++---- Terminal.Gui/Terminal.Gui.sln | 24 +++ .../Drawing/Region/RegionTests.cs | 190 +++++++++++++++++- 3 files changed, 291 insertions(+), 43 deletions(-) create mode 100644 Terminal.Gui/Terminal.Gui.sln diff --git a/Terminal.Gui/Drawing/Region.cs b/Terminal.Gui/Drawing/Region.cs index 2e5131975..1cfea8638 100644 --- a/Terminal.Gui/Drawing/Region.cs +++ b/Terminal.Gui/Drawing/Region.cs @@ -556,72 +556,122 @@ public class Region /// A list of merged rectangles. internal static List MergeRectangles (List rectangles, bool minimize) { - if (rectangles.Count == 0) + if (rectangles.Count <= 1) { - return []; + return rectangles.ToList (); } - // Sweep-line algorithm to merge rectangles - List<(int x, bool isStart, int yTop, int yBottom)> events = new (rectangles.Count * 2); // Pre-allocate - + // Generate events + List<(int x, bool isStart, int yTop, int yBottom)> events = new (rectangles.Count * 2); foreach (Rectangle r in rectangles) { if (!r.IsEmpty) { - events.Add ((r.Left, true, r.Top, r.Bottom)); // Start event - events.Add ((r.Right, false, r.Top, r.Bottom)); // End event + events.Add ((r.Left, true, r.Top, r.Bottom)); + events.Add ((r.Right, false, r.Top, r.Bottom)); } } if (events.Count == 0) { - return []; // Return empty list if no non-empty rectangles exist + return []; } + // Sort events: + // 1. Primarily by x-coordinate. + // 2. Secondary: End events before Start events at the same x. + // 3. Tertiary: By yTop coordinate as a tie-breaker. + // 4. Quaternary: By yBottom coordinate as a final tie-breaker. events.Sort ( (a, b) => { + // 1. Sort by X int cmp = a.x.CompareTo (b.x); + if (cmp != 0) return cmp; - if (cmp != 0) - { - return cmp; - } + // 2. Sort End events before Start events + bool aIsEnd = !a.isStart; + bool bIsEnd = !b.isStart; + cmp = aIsEnd.CompareTo (bIsEnd); // True (End) comes after False (Start) + if (cmp != 0) return -cmp; // Reverse: End (true) should come before Start (false) - return a.isStart.CompareTo (b.isStart); // Start events before end events at same x + // 3. Tie-breaker: Sort by yTop + cmp = a.yTop.CompareTo (b.yTop); + if (cmp != 0) return cmp; + + // 4. Final Tie-breaker: Sort by yBottom + return a.yBottom.CompareTo (b.yBottom); }); List merged = []; + // Use a dictionary to track active intervals and their overlap counts + Dictionary<(int yTop, int yBottom), int> activeCounts = new (); + // Comparer for sorting intervals when needed + var intervalComparer = Comparer<(int yTop, int yBottom)>.Create ( + (a, b) => + { + int cmp = a.yTop.CompareTo (b.yTop); + return cmp != 0 ? cmp : a.yBottom.CompareTo (b.yBottom); + }); - SortedSet<(int yTop, int yBottom)> active = new ( - Comparer<(int yTop, int yBottom)>.Create ( - (a, b) => - { - int cmp = a.yTop.CompareTo (b.yTop); - - return cmp != 0 ? cmp : a.yBottom.CompareTo (b.yBottom); - })); - int lastX = events [0].x; - - foreach ((int x, bool isStart, int yTop, int yBottom) evt in events) + // Helper to get the current active intervals (where count > 0) as a SortedSet + SortedSet<(int yTop, int yBottom)> GetActiveIntervals () { - // Output rectangles for the previous segment if there are active rectangles - if (active.Count > 0 && evt.x > lastX) + var set = new SortedSet<(int yTop, int yBottom)> (intervalComparer); + foreach (var kvp in activeCounts) { - merged.AddRange (MergeVerticalIntervals (active, lastX, evt.x)); + if (kvp.Value > 0) + { + set.Add (kvp.Key); + } + } + return set; + } + + // Group events by x-coordinate to process all events at a given x together + var groupedEvents = events.GroupBy (e => e.x).OrderBy (g => g.Key); + int lastX = groupedEvents.First ().Key; // Initialize with the first event's x + + foreach (var group in groupedEvents) + { + int currentX = group.Key; + // Get active intervals based on state *before* processing events at currentX + var currentActiveIntervals = GetActiveIntervals (); + + // 1. Output rectangles for the segment ending *before* this x coordinate + if (currentX > lastX && currentActiveIntervals.Count > 0) + { + merged.AddRange (MergeVerticalIntervals (currentActiveIntervals, lastX, currentX)); } - // Process the event - if (evt.isStart) + // 2. Process all events *at* this x coordinate to update counts + foreach (var evt in group) { - active.Add ((evt.yTop, evt.yBottom)); - } - else - { - active.Remove ((evt.yTop, evt.yBottom)); + var interval = (evt.yTop, evt.yBottom); + if (evt.isStart) + { + activeCounts.TryGetValue (interval, out int count); + activeCounts [interval] = count + 1; + } + else + { + // Only decrement/remove if the interval exists + if (activeCounts.TryGetValue (interval, out int count)) + { + if (count - 1 <= 0) + { + activeCounts.Remove (interval); + } + else + { + activeCounts [interval] = count - 1; + } + } + } } - lastX = evt.x; + // 3. Update lastX for the next segment + lastX = currentX; } return minimize ? MinimizeRectangles (merged) : merged; diff --git a/Terminal.Gui/Terminal.Gui.sln b/Terminal.Gui/Terminal.Gui.sln new file mode 100644 index 000000000..7724d3f2e --- /dev/null +++ b/Terminal.Gui/Terminal.Gui.sln @@ -0,0 +1,24 @@ +Microsoft Visual Studio Solution File, Format Version 12.00 +# Visual Studio Version 17 +VisualStudioVersion = 17.5.2.0 +MinimumVisualStudioVersion = 10.0.40219.1 +Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Terminal.Gui", "Terminal.Gui.csproj", "{79692A4F-7704-552C-0EF5-40B81C4F2E81}" +EndProject +Global + GlobalSection(SolutionConfigurationPlatforms) = preSolution + Debug|Any CPU = Debug|Any CPU + Release|Any CPU = Release|Any CPU + EndGlobalSection + GlobalSection(ProjectConfigurationPlatforms) = postSolution + {79692A4F-7704-552C-0EF5-40B81C4F2E81}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {79692A4F-7704-552C-0EF5-40B81C4F2E81}.Debug|Any CPU.Build.0 = Debug|Any CPU + {79692A4F-7704-552C-0EF5-40B81C4F2E81}.Release|Any CPU.ActiveCfg = Release|Any CPU + {79692A4F-7704-552C-0EF5-40B81C4F2E81}.Release|Any CPU.Build.0 = Release|Any CPU + EndGlobalSection + GlobalSection(SolutionProperties) = preSolution + HideSolutionNode = FALSE + EndGlobalSection + GlobalSection(ExtensibilityGlobals) = postSolution + SolutionGuid = {4B162456-436F-4899-B765-26C9DBD2991D} + EndGlobalSection +EndGlobal diff --git a/Tests/UnitTestsParallelizable/Drawing/Region/RegionTests.cs b/Tests/UnitTestsParallelizable/Drawing/Region/RegionTests.cs index cd8f3895b..e471078da 100644 --- a/Tests/UnitTestsParallelizable/Drawing/Region/RegionTests.cs +++ b/Tests/UnitTestsParallelizable/Drawing/Region/RegionTests.cs @@ -783,7 +783,7 @@ public class RegionTests Assert.True (region1.Contains (40, 40)); } - [Fact (Skip = "Union is broken")] + [Fact] public void Union_Third_Rect_Covering_Two_Disjoint_Merges () { var origRegion = new Region (); @@ -791,19 +791,19 @@ public class RegionTests var region1 = new Region (new (0, 0, 1, 1)); var region2 = new Region (new (1, 0, 1, 1)); - origRegion.Union(region1); - origRegion.Union(region2); + origRegion.Union (region1); + origRegion.Union (region2); Assert.Equal (new Rectangle (0, 0, 2, 1), origRegion.GetBounds ()); Assert.Equal (2, origRegion.GetRectangles ().Length); - origRegion.Union(new Region(new (0, 0, 4, 1))); + origRegion.Union (new Region (new (0, 0, 4, 1))); - Assert.Equal (new Rectangle (0, 1, 4, 1), origRegion.GetBounds ()); - Assert.Single (origRegion.GetRectangles ()); + Assert.Equal (new Rectangle (0, 0, 4, 1), origRegion.GetBounds ()); + Assert.Equal (3, origRegion.GetRectangles ().Length); } - [Fact (Skip = "MinimalUnion is broken")] + [Fact] public void MinimalUnion_Third_Rect_Covering_Two_Disjoint_Merges () { var origRegion = new Region (); @@ -819,7 +819,7 @@ public class RegionTests origRegion.MinimalUnion (new Region (new (0, 0, 4, 1))); - Assert.Equal (new Rectangle (0, 1, 4, 1), origRegion.GetBounds ()); + Assert.Equal (new Rectangle (0, 0, 4, 1), origRegion.GetBounds ()); Assert.Single (origRegion.GetRectangles ()); } @@ -928,6 +928,180 @@ public class RegionTests Assert.Contains (new (2, 0, 0, 1), result); } + [Fact] + public void MergeRectangles_Sort_Handles_Coincident_Events_Without_Crashing () + { + // Arrange: Create rectangles designed to produce coincident start/end events + // Rect1 ends at x=10. Rect2 and Rect3 start at x=10. + // Rect4 ends at x=15. Rect5 starts at x=15. + var rect1 = new Rectangle (0, 0, 10, 10); // Ends at x=10 + var rect2 = new Rectangle (10, 0, 10, 5); // Starts at x=10 + var rect3 = new Rectangle (10, 5, 10, 5); // Starts at x=10, adjacent to rect2 vertically + var rect4 = new Rectangle (5, 10, 10, 5); // Ends at x=15 + var rect5 = new Rectangle (15, 10, 5, 5); // Starts at x=15 + var combinedList = new List { rect1, rect2, rect3, rect4, rect5 }; + + // Act & Assert: + // The core assertion is that calling MergeRectangles with this list + // does *not* throw the ArgumentException related to sorting. + var exception = Record.Exception (() => Region.MergeRectangles (combinedList, false)); + + // Assert + Assert.Null (exception); + + // Optional secondary assertion: Check if the merge produced a reasonable number of rectangles + // This isn't strictly necessary for proving the sort fix, but can be useful. + // var merged = Region.MergeRectangles(combinedList, false); + // Assert.True(merged.Count > 0 && merged.Count <= combinedList.Count); + } + + [Fact] + public void MergeRectangles_Sort_Handles_Multiple_Coincident_Starts () + { + // Arrange: Multiple rectangles starting at the same X + var rect1 = new Rectangle (5, 0, 10, 5); + var rect2 = new Rectangle (5, 5, 10, 5); + var rect3 = new Rectangle (5, 10, 10, 5); + var combinedList = new List { rect1, rect2, rect3 }; + + // Act & Assert: Ensure no sorting exception + var exception = Record.Exception (() => Region.MergeRectangles (combinedList, false)); + Assert.Null (exception); + } + + [Fact] + public void MergeRectangles_Sort_Handles_Multiple_Coincident_Ends () + { + // Arrange: Multiple rectangles ending at the same X + var rect1 = new Rectangle (0, 0, 10, 5); + var rect2 = new Rectangle (0, 5, 10, 5); + var rect3 = new Rectangle (0, 10, 10, 5); + var combinedList = new List { rect1, rect2, rect3 }; + + // Act & Assert: Ensure no sorting exception + var exception = Record.Exception (() => Region.MergeRectangles (combinedList, false)); + Assert.Null (exception); + } + + [Fact] + public void MergeRectangles_Sort_Handles_Coincident_Mixed_Events_Without_Crashing () + { + // Arrange: Create rectangles specifically designed to produce multiple + // Start AND End events at the same x-coordinate (e.g., x=10), + // mimicking the pattern observed in the crash log. + var rectA = new Rectangle (0, 0, 10, 5); // Ends at x=10, y=[0, 5) + var rectB = new Rectangle (0, 10, 10, 5); // Ends at x=10, y=[10, 15) + var rectC = new Rectangle (10, 0, 10, 5); // Starts at x=10, y=[0, 5) + var rectD = new Rectangle (10, 10, 10, 5); // Starts at x=10, y=[10, 15) + + // Add another set at a different X to increase complexity + var rectE = new Rectangle (5, 20, 10, 5); // Ends at x=15, y=[20, 25) + var rectF = new Rectangle (5, 30, 10, 5); // Ends at x=15, y=[30, 35) + var rectG = new Rectangle (15, 20, 10, 5); // Starts at x=15, y=[20, 25) + var rectH = new Rectangle (15, 30, 10, 5); // Starts at x=15, y=[30, 35) + + // Add some unrelated rectangles + var rectI = new Rectangle (0, 40, 5, 5); + var rectJ = new Rectangle (100, 100, 5, 5); + + + var combinedList = new List { + rectA, rectB, rectC, rectD, + rectE, rectF, rectG, rectH, + rectI, rectJ + }; + + // Act & Assert: + // Call MergeRectangles with the current code. + // This test *should* fail by throwing ArgumentException due to unstable sort. + var exception = Record.Exception (() => Region.MergeRectangles (combinedList, false)); + + // Assert that no exception was thrown (this assertion will fail with the current code) + Assert.Null (exception); + } + + [Fact] + public void MergeRectangles_Sort_Reproduces_UICatalog_Crash_Pattern_Directly () + { + // Arrange: Rectangles derived *directly* from the events list that caused the crash at x=67 + // This aims to replicate the exact problematic pattern. + var rect_End_67_7_30 = new Rectangle (60, 7, 7, 23); // Ends at x=67, y=[7, 30) -> Event [33] + var rect_Start_67_2_30 = new Rectangle (67, 2, 10, 28); // Starts at x=67, y=[2, 30) -> Event [34] + var rect_Start_67_1_1 = new Rectangle (67, 1, 10, 0); // Starts at x=67, y=[1, 1) -> Event [49] (Height 0) + var rect_End_67_1_1 = new Rectangle (60, 1, 7, 0); // Ends at x=67, y=[1, 1) -> Event [64] (Height 0) + + // Add rectangles for x=94/95 pattern + var rect_End_94_1_30 = new Rectangle (90, 1, 4, 29); // Ends at x=94, y=[1, 30) -> Event [55] + var rect_Start_94_1_1 = new Rectangle (94, 1, 10, 0); // Starts at x=94, y=[1, 1) -> Event [56] + var rect_Start_94_7_30 = new Rectangle (94, 7, 10, 23); // Starts at x=94, y=[7, 30) -> Event [58] + + var rect_End_95_1_1 = new Rectangle (90, 1, 5, 0); // Ends at x=95, y=[1, 1) -> Event [57] + var rect_End_95_7_30 = new Rectangle (90, 7, 5, 23); // Ends at x=95, y=[7, 30) -> Event [59] + var rect_Start_95_0_30 = new Rectangle (95, 0, 10, 30); // Starts at x=95, y=[0, 30) -> Event [60] + + + var combinedList = new List { + rect_End_67_7_30, rect_Start_67_2_30, rect_Start_67_1_1, rect_End_67_1_1, + rect_End_94_1_30, rect_Start_94_1_1, rect_Start_94_7_30, + rect_End_95_1_1, rect_End_95_7_30, rect_Start_95_0_30 + }; + + // Act & Assert: + // Call MergeRectangles. This test is specifically designed to fail with the current code. + var exception = Record.Exception (() => Region.MergeRectangles (combinedList, false)); + + // Assert that no exception was thrown (this assertion *should* fail with the current code) + Assert.Null (exception); + } + + [Fact] + public void MergeRectangles_Sort_Reproduces_UICatalog_Crash_From_Captured_Data () + { + // Arrange: The exact list of rectangles captured during the UICatalog crash + var rectanglesFromCrash = new List { + new Rectangle(38, 7, 1, 11), + new Rectangle(39, 7, 5, 23), + new Rectangle(44, 7, 1, 23), + new Rectangle(45, 7, 6, 23), + new Rectangle(51, 7, 1, 23), + new Rectangle(52, 7, 1, 23), + new Rectangle(53, 7, 1, 23), + new Rectangle(54, 7, 1, 23), + new Rectangle(55, 7, 1, 23), + new Rectangle(56, 7, 1, 23), + new Rectangle(57, 7, 1, 23), + new Rectangle(58, 7, 1, 23), + new Rectangle(59, 7, 1, 23), + new Rectangle(60, 7, 1, 23), + new Rectangle(61, 7, 3, 23), + new Rectangle(64, 7, 1, 23), + new Rectangle(65, 7, 2, 23), + new Rectangle(67, 2, 2, 28), + new Rectangle(69, 2, 3, 28), + new Rectangle(72, 2, 3, 28), + new Rectangle(75, 2, 1, 28), + new Rectangle(76, 2, 2, 28), + new Rectangle(78, 2, 2, 28), + new Rectangle(80, 7, 1, 23), + new Rectangle(81, 1, 7, 29), + new Rectangle(88, 1, 1, 29), + new Rectangle(89, 1, 2, 29), + new Rectangle(91, 1, 3, 29), + new Rectangle(94, 1, 1, 0), // Note: Zero height + new Rectangle(94, 7, 1, 23), + new Rectangle(95, 0, 1, 30), + new Rectangle(96, 0, 23, 30), + new Rectangle(67, 1, 0, 0) // Note: Zero width and height + }; + + // Act & Assert: + // Call MergeRectangles with the current code. + // This test *should* fail by throwing ArgumentException due to unstable sort. + var exception = Record.Exception (() => Region.MergeRectangles (rectanglesFromCrash, false)); + + // Assert that no exception was thrown (this assertion will fail with the current code) + Assert.Null (exception); + } } \ No newline at end of file