From 2b4243599e37d0109ea3b6a59279208eb80d82bc Mon Sep 17 00:00:00 2001 From: Jon Kazama Date: Mon, 17 Aug 2026 07:27:34 +0200 Subject: [PATCH] fix(ui): render each frame from one tab-list snapshot, key widgets by identity Sidebar, top tabs and status bar each read Config.Tabs on their own, unlocked, while the worker added or evicted tabs. That gave three independent views of a moving list: an index built in one place could resolve to a different tab a few lines later, which showed up either as an out-of-range crash on the draw thread or -- worse, because it is silent -- as a click landing in someone else's tell. MainWindow now takes one snapshot under the lock and passes it through the whole frame. Deliberately a shallow copy: tab identity is compared by reference all over the draw path, so cloning would break every ReferenceEquals and Contains. ChangeTabDelta and ResetActiveTabIfRemoved run on the framework thread and keep their own locked reads instead; ThemeQuickPicker locks its own copy, since reaching it would mean threading a parameter through InputBar, which popouts share and which has no tab list. Widget IDs move from list position to tab.Identifier. ImGui carries popup and widget state across frames under that ID, so a position-based one re-binds an open context menu to a different tab as soon as the list shifts -- a snapshot cannot fix that, it spans frames. This also resolves top tabs visually merging into each other when the list changed. The sidebar section headers counted over the live list while the rows came from BuildRenderOrder, which skips popped-out tabs. Both sides take the same predicate now, so the count matches what is drawn. --- HellionChat/Ui/Components/Sidebar.cs | 13 +++--- HellionChat/Ui/Components/StatusBar.cs | 8 ++-- HellionChat/Ui/Components/ThemeQuickPicker.cs | 7 ++- HellionChat/Ui/Components/TopTabBar.cs | 6 +-- HellionChat/Ui/Windows/MainWindow.cs | 46 ++++++++++++------- HellionChat/Util/TabLifecycleHelpers.cs | 27 ++++++++++- 6 files changed, 75 insertions(+), 32 deletions(-) diff --git a/HellionChat/Ui/Components/Sidebar.cs b/HellionChat/Ui/Components/Sidebar.cs index 19b255b..d8b05f4 100644 --- a/HellionChat/Ui/Components/Sidebar.cs +++ b/HellionChat/Ui/Components/Sidebar.cs @@ -103,7 +103,7 @@ internal sealed class Sidebar Plugin.Instance.AutoTellTabsService.MarkGreeted(tab); } - public void Draw(float windowWidth, IList tabs, ref Tab? activeTab) + public void Draw(float windowWidth, IReadOnlyList tabs, ref Tab? activeTab) { LastRenderedGreetedGlyphCount = 0; LastRenderedUnreadDotCount = 0; @@ -147,7 +147,7 @@ internal sealed class Sidebar { DrawSectionHeader( HellionStrings.PinTab_SectionHeader, - Plugin.Instance.AutoTellTabsService.PinnedTempTabCount + TabLifecycleHelpers.CountPinnedPool(tabs, t => _pool.IsOpen(t.Identifier)) ); pinnedHeaderRendered = true; } @@ -155,14 +155,13 @@ internal sealed class Sidebar { DrawSectionHeader( HellionStrings.AutoTellTabs_SectionHeader, - Plugin.Instance.AutoTellTabsService.ActiveTempTabCount + TabLifecycleHelpers.CountUnpinnedPool(tabs, t => _pool.IsOpen(t.Identifier)) ); unpinnedHeaderRendered = true; } DrawRow( tab, - i, expanded, accentRgba, textAbgr, @@ -191,7 +190,6 @@ internal sealed class Sidebar private void DrawRow( Tab tab, - int index, bool expanded, uint accentRgba, uint textAbgr, @@ -202,7 +200,10 @@ internal sealed class Sidebar ref Tab? activeTab ) { - ImGui.PushID(index); + // Identity, not position: ImGui keeps popup state across frames under this + // ID, so an index would re-bind an open context menu to a different tab as + // soon as the list shifts. String, not GetHashCode — hashes collide. + ImGui.PushID(tab.Identifier.ToString()); var origin = ImGui.GetCursorScreenPos(); var avail = ImGui.GetContentRegionAvail().X; diff --git a/HellionChat/Ui/Components/StatusBar.cs b/HellionChat/Ui/Components/StatusBar.cs index 9342c5b..3b867b8 100644 --- a/HellionChat/Ui/Components/StatusBar.cs +++ b/HellionChat/Ui/Components/StatusBar.cs @@ -60,7 +60,7 @@ internal sealed class StatusBar // Single-pass aggregator — same shape as the previous helper so the // build-suite test continues to pin the contract. - internal static (int messages, int tells) AggregateForStatusBar(IList tabs) + internal static (int messages, int tells) AggregateForStatusBar(IReadOnlyList tabs) { int messages = 0, tells = 0; @@ -93,7 +93,7 @@ internal sealed class StatusBar _lastUpdateMs = now; } - public void Draw(Tab? activeTab) + public void Draw(Tab? activeTab, IReadOnlyList tabs) { if (!_fonts.FontsReady) { @@ -105,8 +105,8 @@ internal sealed class StatusBar var now = Environment.TickCount64; if (now - _lastUpdateMs >= UpdateIntervalMs) { - var (messages, tells) = AggregateForStatusBar(Plugin.Config.Tabs); - UpdateCacheIfDue(now, Plugin.Config.Tabs.Count, messages, tells); + var (messages, tells) = AggregateForStatusBar(tabs); + UpdateCacheIfDue(now, tabs.Count, messages, tells); } // Top border via DrawList — ImGui.Separator has too much padding for diff --git a/HellionChat/Ui/Components/ThemeQuickPicker.cs b/HellionChat/Ui/Components/ThemeQuickPicker.cs index 3cb763d..105a76a 100644 --- a/HellionChat/Ui/Components/ThemeQuickPicker.cs +++ b/HellionChat/Ui/Components/ThemeQuickPicker.cs @@ -86,7 +86,12 @@ internal sealed class ThemeQuickPicker ImGui.Separator(); // Snapshot so a worker-thread temp-tab strip can't shift the list mid-loop. - var tabs = Plugin.Config.Tabs.ToList(); + // The copy itself needs the lock, otherwise it tears the same way. Not the + // frame snapshot from MainWindow: reaching it would mean threading a + // parameter through InputBar, which popouts share and which has no tab list. + List tabs; + lock (_plugin.TabsListLock) + tabs = Plugin.Config.Tabs.ToList(); var height = MathF.Min(tabs.Count * RowHeight, MaxSectionHeight); using var child = ImRaii.Child( "##hellion-quick-picker-tabs", diff --git a/HellionChat/Ui/Components/TopTabBar.cs b/HellionChat/Ui/Components/TopTabBar.cs index c7c4e00..59e10c9 100644 --- a/HellionChat/Ui/Components/TopTabBar.cs +++ b/HellionChat/Ui/Components/TopTabBar.cs @@ -16,7 +16,7 @@ internal sealed class TopTabBar _pool = pool; } - public void Draw(IList tabs, ref Tab? activeTab) + public void Draw(IReadOnlyList tabs, ref Tab? activeTab) { var firstDrawn = true; for (var i = 0; i < tabs.Count; i++) @@ -39,7 +39,7 @@ internal sealed class TopTabBar var tabWidth = ImGui.CalcTextSize(tab.Name).X; if ( ImGui.Selectable( - $"{tab.Name}###hellion_toptab_{i}", + $"{tab.Name}###hellion_toptab_{tab.Identifier}", selected, ImGuiSelectableFlags.None, new Vector2(tabWidth, 0) @@ -71,7 +71,7 @@ internal sealed class TopTabBar .AddCircleFilled(new Vector2(max.X - 4f, min.Y + 4f), 3.5f, danger, 12); } - TabContextMenu.Draw(tab, $"toptab_ctx_{i}", _pool); + TabContextMenu.Draw(tab, $"toptab_ctx_{tab.Identifier}", _pool); } ImGui.Separator(); diff --git a/HellionChat/Ui/Windows/MainWindow.cs b/HellionChat/Ui/Windows/MainWindow.cs index 6c5dcac..a89982a 100644 --- a/HellionChat/Ui/Windows/MainWindow.cs +++ b/HellionChat/Ui/Windows/MainWindow.cs @@ -144,7 +144,11 @@ internal sealed class MainWindow : Window, IFocusableChatWindow if (!ReferenceEquals(_activeTab, removed)) return; - var next = Plugin.Config.Tabs.Count > 0 ? Plugin.Config.Tabs[0] : null; + // Framework thread, not the draw frame: needs the current truth, so it takes + // its own lock instead of using the frame snapshot. + Tab? next; + lock (Plugin.Instance.TabsListLock) + next = Plugin.Config.Tabs.Count > 0 ? Plugin.Config.Tabs[0] : null; _activeTab = next; if (next is not null) TabLifecycleHelpers.OnTabActivated(next, removed); @@ -171,7 +175,11 @@ internal sealed class MainWindow : Window, IFocusableChatWindow // deferred (no focus contract) — main-window tabs only. internal void ChangeTabDelta(int delta) { - var tabs = Plugin.Config.Tabs; + // Runs on Framework.Update via the keybind dispatch, not on the draw frame — + // own lock, own copy. Stays a List so IndexOf below keeps working. + List tabs; + lock (Plugin.Instance.TabsListLock) + tabs = Plugin.Config.Tabs.ToList(); if (tabs.Count == 0) return; @@ -256,24 +264,30 @@ internal sealed class MainWindow : Window, IFocusableChatWindow // Primary pool-reset path; InputPreview has a defensive fallback for the MainWindow-closed edge case. _handlerLender.ResetCounter(); + // One snapshot for the whole frame. Everything below reads this instead of + // Config.Tabs, so sidebar, top tabs and status bar see the same list even if + // the worker adds or evicts a tab mid-frame. Deliberately a SHALLOW copy: + // tab identity is compared by reference all over the draw path, so cloning + // would break every ReferenceEquals and Contains. + List tabs; + lock (Plugin.Instance.TabsListLock) + tabs = Plugin.Config.Tabs.ToList(); + // First-frame seed: the active tab defaults to the first persisted // tab so the message list isn't empty on a clean session. - if (_activeTab is null && Plugin.Config.Tabs.Count > 0) + if (_activeTab is null && tabs.Count > 0) { - var seeded = Plugin.Config.Tabs[0]; + var seeded = tabs[0]; _activeTab = seeded; // The seeded Tabs[0] is the likeliest legacy stale-tell carrier // (pre-coupling the detour wrote here); strip it like any activation. TabLifecycleHelpers.OnTabActivated(seeded, null); } - else if (_activeTab is { } active && !Plugin.Config.Tabs.Contains(active)) + else if (_activeTab is { } active && !tabs.Contains(active)) { // Active tab is no longer in the list (e.g. a wholesale config import - // the service repair paths never see). Re-seed on the Draw thread. The - // Contains read shares the pre-existing unsynchronized-Tabs-list - // exposure that spec §6 defers (SaveConfig also strips from the worker - // thread); this adds one more racing read, not a new hazard class. - var reseed = Plugin.Config.Tabs.Count > 0 ? Plugin.Config.Tabs[0] : null; + // the service repair paths never see). Re-seed on the Draw thread. + var reseed = tabs.Count > 0 ? tabs[0] : null; _activeTab = reseed; if (reseed is not null) TabLifecycleHelpers.OnTabActivated(reseed, active); @@ -287,7 +301,7 @@ internal sealed class MainWindow : Window, IFocusableChatWindow // settled, so OnTabActivated fires only on the pop frame. var visibleActive = TabLifecycleHelpers.PickMainActiveTab( _activeTab, - Plugin.Config.Tabs, + tabs, t => _pool.IsOpen(t.Identifier) ); if (!ReferenceEquals(visibleActive, _activeTab)) @@ -309,20 +323,20 @@ internal sealed class MainWindow : Window, IFocusableChatWindow using (var body = ImRaii.Child("##hellion-body", new Vector2(-1f, -statusHeight))) { if (body.Success) - DrawBody(); + DrawBody(tabs); } - _status.Draw(_activeTab); + _status.Draw(_activeTab, tabs); } - private void DrawBody() + private void DrawBody(IReadOnlyList tabs) { var bodyWidth = ImGui.GetContentRegionAvail().X; _honorific.Draw(bodyWidth); if (Plugin.Config.MainWindowLayoutMode == MainWindowLayoutMode.TopTabs) { - _topTabs.Draw(Plugin.Config.Tabs, ref _activeTab); + _topTabs.Draw(tabs, ref _activeTab); using (ImRaii.Group()) { DrawMainArea(); @@ -333,7 +347,7 @@ internal sealed class MainWindow : Window, IFocusableChatWindow // Sidebar layout (default). using (ImRaii.Group()) { - _sidebar.Draw(bodyWidth, Plugin.Config.Tabs, ref _activeTab); + _sidebar.Draw(bodyWidth, tabs, ref _activeTab); } ImGui.SameLine(); diff --git a/HellionChat/Util/TabLifecycleHelpers.cs b/HellionChat/Util/TabLifecycleHelpers.cs index 103f482..b8fb729 100644 --- a/HellionChat/Util/TabLifecycleHelpers.cs +++ b/HellionChat/Util/TabLifecycleHelpers.cs @@ -115,7 +115,30 @@ internal static class TabLifecycleHelpers // row and its pool's section header gates on the first tab actually reached. Pure // + Dalamud-free so the Build-Suite can pin it. // TEST-MIRROR: ../../../Hellion Build test/_Helpers/SidebarRenderOrderTests.cs - internal static List BuildRenderOrder(IList tabs, Func isPoppedOut) + // Section-header counts for the sidebar. They take the same isPoppedOut + // predicate as BuildRenderOrder, which skips popped-out tabs — without it the + // header would claim "(3)" above two rendered rows. AutoTellTabsService keeps + // its own live properties: those gate the pool limits and must not see a + // snapshot. + internal static int CountUnpinnedPool(IReadOnlyList tabs, Func isPoppedOut) + { + var n = 0; + for (var i = 0; i < tabs.Count; i++) + if (IsInUnpinnedPool(tabs[i]) && !isPoppedOut(tabs[i])) + n++; + return n; + } + + internal static int CountPinnedPool(IReadOnlyList tabs, Func isPoppedOut) + { + var n = 0; + for (var i = 0; i < tabs.Count; i++) + if (IsInPinnedPool(tabs[i]) && !isPoppedOut(tabs[i])) + n++; + return n; + } + + internal static List BuildRenderOrder(IReadOnlyList tabs, Func isPoppedOut) { var persistent = new List(tabs.Count); var pinned = new List(); @@ -145,7 +168,7 @@ internal static class TabLifecycleHelpers // TEST-MIRROR: ../../../Hellion Build test/_Helpers/PickMainActiveTabTests.cs internal static Tab? PickMainActiveTab( Tab? current, - IList tabs, + IReadOnlyList tabs, Func isPoppedOut ) {