From 918cdc81117cc8bc5bda29e3ed51410238b7e2d4 Mon Sep 17 00:00:00 2001 From: Jon Kazama Date: Mon, 17 Aug 2026 23:59:17 +0200 Subject: [PATCH] fix(style): stop the hover registry from churning on idle rows Review of block B found Query allocating an entry for every element it was asked about, hovered or not. The cycle: Query creates the entry, the next BeginFrame steps it to zero, evicts it, and the next Query creates it again. With fifteen tabs that is fifteen allocations plus fifteen dictionary inserts and removes per frame, permanently, with the mouse nowhere near the window. That is exactly the property master spec 7.5 asks for and the one this block claimed to improve, so it ate the two string allocations 31fa410 had just saved. An unhovered element with no entry now returns zero without creating one. The footprint self-test only ever queried with hovered: true, which is why it could not see this. It now runs an idle phase as well. Four smaller items from the same review: Advance skipped clearing the hover flags when deltaTime was zero, so such a frame carried the previous frame's state forward. Metrics reads GlobalScaleSafe now. The unsafe variant throws while the interface manager is still coming up, and block F pulls Metrics into more call sites. Badge.CalcSize returned a full-size box for a count of zero while Draw drew nothing, so a caller that reserves and then draws left a badge-shaped hole on every tab without unread messages -- the normal case. IconButton caches its glyph strings; ToIconString allocates on every call and keeps no cache of its own. And the widget gallery clamps its own row width, since asserting on a zero-width button in the window that demonstrates the clamp would be a poor look. --- .../SelfTests/HoverStateFootprintStep.cs | 31 ++++++++++++++++--- HellionChat/Ui/StyleEngine/HoverState.cs | 15 +++++++++ HellionChat/Ui/StyleEngine/Metrics.cs | 4 ++- HellionChat/Ui/StyleEngine/Widgets/Badge.cs | 6 ++++ .../Ui/StyleEngine/Widgets/IconButton.cs | 16 +++++++++- HellionChat/Ui/Windows/WidgetGalleryWindow.cs | 10 ++++-- 6 files changed, 74 insertions(+), 8 deletions(-) diff --git a/HellionChat/SelfTests/HoverStateFootprintStep.cs b/HellionChat/SelfTests/HoverStateFootprintStep.cs index e726155..a8c9d43 100644 --- a/HellionChat/SelfTests/HoverStateFootprintStep.cs +++ b/HellionChat/SelfTests/HoverStateFootprintStep.cs @@ -59,13 +59,36 @@ internal sealed class HoverStateFootprintStep : ISelfTestStep if (HoverState.TrackedCount != 0) { - var msg = $"Registry leaked {HoverState.TrackedCount} entries after fade-out"; - ImGui.Text(msg); - SelfTestReport.Append(Name, "FAIL", new[] { msg }); + var leak = $"Registry leaked {HoverState.TrackedCount} entries after fade-out"; + ImGui.Text(leak); + SelfTestReport.Append(Name, "FAIL", new[] { leak }); return SelfTestStepResult.Fail; } - SelfTestReport.Append(Name, "PASS", new[] { "registry empty after fade-out" }); + // The case a hovered-only probe cannot see: querying an element that + // is NOT hovered must not create an entry. Otherwise every row in + // the window allocates one per frame and loses it again in the next + // BeginFrame, forever. + for (var frame = 0; frame < 10; frame++) + { + foreach (var id in ids) + HoverState.Query(id, hovered: false); + HoverState.AdvanceForTest(Frame); + + if (HoverState.TrackedCount == 0) + continue; + + var churn = $"Unhovered query created {HoverState.TrackedCount} entries"; + ImGui.Text(churn); + SelfTestReport.Append(Name, "FAIL", new[] { churn }); + return SelfTestStepResult.Fail; + } + + SelfTestReport.Append( + Name, + "PASS", + new[] { "no growth while hovered, empty after fade-out, no churn when idle" } + ); return SelfTestStepResult.Pass; } finally diff --git a/HellionChat/Ui/StyleEngine/HoverState.cs b/HellionChat/Ui/StyleEngine/HoverState.cs index f3d194e..15775ad 100644 --- a/HellionChat/Ui/StyleEngine/HoverState.cs +++ b/HellionChat/Ui/StyleEngine/HoverState.cs @@ -48,6 +48,12 @@ internal static class HoverState if (!Entries.TryGetValue(id, out var entry)) { + // Nothing to fade from, so nothing to track. Without this a + // never-hovered element allocates an entry in Query and loses it + // again in the next BeginFrame, every frame, for every row. + if (!hovered) + return 0f; + entry = new Entry(); Entries[id] = entry; } @@ -58,10 +64,19 @@ internal static class HoverState return entry.Value; } + // Never re-entrant: BeginFrame runs once from the draw thread, AdvanceForTest + // only from a self-test step. Evicted is shared, so overlapping calls would + // corrupt it. private static void Advance(float deltaTime) { if (deltaTime <= 0f) + { + // Still clear the flags: a zero-delta frame otherwise carries the + // previous frame's hover state forward. + foreach (var entry in Entries.Values) + entry.Hovered = false; return; + } Evicted.Clear(); foreach (var (id, entry) in Entries) diff --git a/HellionChat/Ui/StyleEngine/Metrics.cs b/HellionChat/Ui/StyleEngine/Metrics.cs index 849d45c..5a1ef30 100644 --- a/HellionChat/Ui/StyleEngine/Metrics.cs +++ b/HellionChat/Ui/StyleEngine/Metrics.cs @@ -66,8 +66,10 @@ internal static class Metrics if (frame == _cachedFrame) return _cachedScale; + // Safe variant: GlobalScale throws while the interface manager is + // still coming up, and Block F pulls Metrics into more call sites. + _cachedScale = ImGuiHelpers.GlobalScaleSafe; _cachedFrame = frame; - _cachedScale = ImGuiHelpers.GlobalScale; return _cachedScale; } } diff --git a/HellionChat/Ui/StyleEngine/Widgets/Badge.cs b/HellionChat/Ui/StyleEngine/Widgets/Badge.cs index 44f29cb..a639201 100644 --- a/HellionChat/Ui/StyleEngine/Widgets/Badge.cs +++ b/HellionChat/Ui/StyleEngine/Widgets/Badge.cs @@ -21,8 +21,14 @@ internal static class Badge internal static string Format(int count, int maxCount) => count > maxCount ? $"{maxCount}+" : count.ToString(); + // Zero for a count of zero, matching Draw: a caller that reserves space and + // then draws would otherwise leave a badge-shaped hole on every tab without + // unread messages, which is the normal case. internal static Vector2 CalcSize(int count, BadgeStyle? styleOverride = null) { + if (count <= 0) + return Vector2.Zero; + var style = styleOverride ?? new BadgeStyle(); var text = Format(count, style.MaxCount); return WidgetGeometry.Badge( diff --git a/HellionChat/Ui/StyleEngine/Widgets/IconButton.cs b/HellionChat/Ui/StyleEngine/Widgets/IconButton.cs index 488c5e9..2bd23b8 100644 --- a/HellionChat/Ui/StyleEngine/Widgets/IconButton.cs +++ b/HellionChat/Ui/StyleEngine/Widgets/IconButton.cs @@ -20,9 +20,23 @@ internal readonly record struct IconButtonStyle // the hit area plus the glyph. internal static class IconButton { + // FontAwesomeExtensions.ToIconString allocates on every call and holds no + // cache of its own, so a per-frame glyph would allocate per button per frame. + private static readonly Dictionary GlyphCache = []; + internal static Vector2 CalcSize(float width, float height) => WidgetGeometry.IconButton(width, height); + private static string Glyph(FontAwesomeIcon icon) + { + if (GlyphCache.TryGetValue(icon, out var s)) + return s; + + s = icon.ToIconString(); + GlyphCache[icon] = s; + return s; + } + internal static (bool Clicked, bool Hovered) Draw( uint id, Vector2 size, @@ -59,7 +73,7 @@ internal static class IconButton { using (font.Push()) { - var text = icon.ToIconString(); + var text = Glyph(icon); var textSize = ImGui.CalcTextSize(text); dl.AddText(origin + (clamped - textSize) * 0.5f, glyphAbgr, text); } diff --git a/HellionChat/Ui/Windows/WidgetGalleryWindow.cs b/HellionChat/Ui/Windows/WidgetGalleryWindow.cs index ba17a31..3663d4e 100644 --- a/HellionChat/Ui/Windows/WidgetGalleryWindow.cs +++ b/HellionChat/Ui/Windows/WidgetGalleryWindow.cs @@ -55,8 +55,14 @@ internal sealed class WidgetGalleryWindow : Window ImGui.TextUnformatted("Row"); ImGui.Checkbox("active##row", ref _rowActive); - var width = ImGui.GetContentRegionAvail().X; - var height = Metrics.SidebarRowHeight; + // Clamped: this window demonstrates the guard, it should not be the one + // that asserts when dragged narrow. + var rowSize = HellionChat.Util.WidgetGeometry.IconButton( + ImGui.GetContentRegionAvail().X, + Metrics.SidebarRowHeight + ); + var width = rowSize.X; + var height = rowSize.Y; for (var i = 0; i < 3; i++) {