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.
This commit is contained in:
2026-08-17 07:27:34 +02:00
parent 24dff3cc2e
commit 2b4243599e
6 changed files with 75 additions and 32 deletions
+7 -6
View File
@@ -103,7 +103,7 @@ internal sealed class Sidebar
Plugin.Instance.AutoTellTabsService.MarkGreeted(tab);
}
public void Draw(float windowWidth, IList<Tab> tabs, ref Tab? activeTab)
public void Draw(float windowWidth, IReadOnlyList<Tab> 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;
+4 -4
View File
@@ -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<Tab> tabs)
internal static (int messages, int tells) AggregateForStatusBar(IReadOnlyList<Tab> 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<Tab> 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
@@ -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<Tab> 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",
+3 -3
View File
@@ -16,7 +16,7 @@ internal sealed class TopTabBar
_pool = pool;
}
public void Draw(IList<Tab> tabs, ref Tab? activeTab)
public void Draw(IReadOnlyList<Tab> 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();
+30 -16
View File
@@ -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<Tab> 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<Tab> 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<Tab> 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();
+25 -2
View File
@@ -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<int> BuildRenderOrder(IList<Tab> tabs, Func<Tab, bool> 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<Tab> tabs, Func<Tab, bool> 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<Tab> tabs, Func<Tab, bool> 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<int> BuildRenderOrder(IReadOnlyList<Tab> tabs, Func<Tab, bool> isPoppedOut)
{
var persistent = new List<int>(tabs.Count);
var pinned = new List<int>();
@@ -145,7 +168,7 @@ internal static class TabLifecycleHelpers
// TEST-MIRROR: ../../../Hellion Build test/_Helpers/PickMainActiveTabTests.cs
internal static Tab? PickMainActiveTab(
Tab? current,
IList<Tab> tabs,
IReadOnlyList<Tab> tabs,
Func<Tab, bool> isPoppedOut
)
{