From 6f71b093317d2bf54e280316244a31ab3f810583 Mon Sep 17 00:00:00 2001 From: Jon Kazama Date: Tue, 16 Jun 2026 09:04:18 +0200 Subject: [PATCH] fix(closeout): address closure-review findings - gate keybind pill-sync on IsChannelOrExistingLinkshell so an empty linkshell slot no longer desyncs the pill from the real send channel - close manually-popped pop-out windows on logout via an IsOpen filter instead of the PopOut flag (which manual pops never set) - read the router's tell-tab lookup through a lock-wrapped accessor so the framework thread cannot enumerate Config.Tabs mid worker-thread mutation - add a "switch on every tell" toggle (default on) and make the auto-open mode pick the matching layout, so Sidebar vs Top-tab are distinct - comment corrections (stale/contradictory text, TEST-MIRROR path depth) --- HellionChat/AutoTellTabsService.cs | 24 +++++++++++++---- HellionChat/Configuration.cs | 6 +++++ HellionChat/GameFunctions/Chat.cs | 6 ++--- HellionChat/GameFunctions/KeybindManager.cs | 17 ++++++++---- .../SelfTests/ChannelPopoutBindStep.cs | 16 +++++------ HellionChat/Services/TellRouterService.cs | 27 ++++++++++++++++--- .../Components/Settings/Tabs/ChannelsTab.cs | 5 ++++ HellionChat/Ui/Windows/MainWindow.cs | 4 +-- HellionChat/Util/TabLifecycleHelpers.cs | 2 +- 9 files changed, 78 insertions(+), 29 deletions(-) diff --git a/HellionChat/AutoTellTabsService.cs b/HellionChat/AutoTellTabsService.cs index ae1bb20..f66eed5 100644 --- a/HellionChat/AutoTellTabsService.cs +++ b/HellionChat/AutoTellTabsService.cs @@ -239,6 +239,16 @@ internal sealed class AutoTellTabsService : IDisposable ); } + // Lock-protected lookup for the framework-thread caller (TellRouterService). + // Config.Tabs is mutated under _tempTabsLock on the PendingMessage worker thread, + // so a framework-tick reader must take the same lock to avoid enumerating the list + // mid-mutation. + internal Tab? FindTempTabSafe(string name, uint world) + { + lock (_tempTabsLock) + return FindTempTab(name, world); + } + internal void DropOldestTempTab() { // Pinned tabs live in their own bucket (MaxPinnedTempTabs) and are @@ -281,7 +291,8 @@ internal sealed class AutoTellTabsService : IDisposable tab.AddMessage(currentMessage, unread: true); - // Open as pop-out if configured (flag set before Tabs.Add for the next render-tick). + // Flag the tab as a pop-out if configured; the marshalled TryOpen below reads + // that flag to open the real window. if (Plugin.Config.AutoTellTabsOpenAsPopout) { tab.PopOut = true; @@ -438,13 +449,16 @@ internal sealed class AutoTellTabsService : IDisposable var active = _plugin.MainWindow?.ActiveTab; var poppedTempTabIds = Plugin - .Config.Tabs.Where(t => TabLifecycleHelpers.IsInUnpinnedPool(t) && t.PopOut) + .Config.Tabs.Where(t => + TabLifecycleHelpers.IsInUnpinnedPool(t) + && _plugin.ChannelPopoutPool.IsOpen(t.Identifier) + ) .Select(t => t.Identifier) .ToList(); - // Close each popped temp tab's window before the tabs leave the list. - // Logout is a framework-thread event (serialized with Draw), so no - // marshalling is needed here, unlike the worker-thread eviction path. + // Close any pop-out window an unpinned temp tab owns before the tabs leave + // the list. Filtering on the live pool (not the PopOut flag) also catches + // manually right-clicked pop-outs, which never set the flag. foreach (var id in poppedTempTabIds) _plugin.ChannelPopoutPool.TryClose(id); diff --git a/HellionChat/Configuration.cs b/HellionChat/Configuration.cs index 7172db8..3ef8a62 100755 --- a/HellionChat/Configuration.cs +++ b/HellionChat/Configuration.cs @@ -268,6 +268,11 @@ public class Configuration : IPluginConfiguration public bool SettingsWindowOpen; public int MaxParallelPopouts = 8; public TellAutoOpenMode TellAutoOpenMode = TellAutoOpenMode.Sidebar; + + // When true (default) the tell-auto-open router switches the active tab to the + // incoming tell on every message; when false the tab is still created/revealed + // with its unread badge but the active tab is left where the user is reading. + public bool TellAutoOpenSwitchAlways = true; public int SidebarAutoSwitchThresholdPx = 800; // v22 field: MainWindow layout mode (sidebar vs. horizontal top tabs). @@ -428,6 +433,7 @@ public class Configuration : IPluginConfiguration SettingsWindowOpen = other.SettingsWindowOpen; MaxParallelPopouts = other.MaxParallelPopouts; TellAutoOpenMode = other.TellAutoOpenMode; + TellAutoOpenSwitchAlways = other.TellAutoOpenSwitchAlways; SidebarAutoSwitchThresholdPx = other.SidebarAutoSwitchThresholdPx; MainWindowLayoutMode = other.MainWindowLayoutMode; } diff --git a/HellionChat/GameFunctions/Chat.cs b/HellionChat/GameFunctions/Chat.cs index 841e52f..a4e77f1 100755 --- a/HellionChat/GameFunctions/Chat.cs +++ b/HellionChat/GameFunctions/Chat.cs @@ -232,9 +232,9 @@ internal sealed unsafe class Chat : IDisposable if (c != '\0' && !char.IsControl(c)) input = c.ToString(); - // Seed the just-typed character into our input field and focus it, - // the same prefill path the inventory item-link below uses. Prefill- - // only — no tab switch (Flo decision 2026-06-15). + // Seed the just-typed character into our input field and focus it, the + // same InputBar.AppendPending + Activate prefill path inventory item-links + // use. Prefill-only — no tab switch (Flo decision 2026-06-15). if (input != null) { Plugin.InputBar.AppendPending(input); diff --git a/HellionChat/GameFunctions/KeybindManager.cs b/HellionChat/GameFunctions/KeybindManager.cs index e5ee236..1b8f0a0 100644 --- a/HellionChat/GameFunctions/KeybindManager.cs +++ b/HellionChat/GameFunctions/KeybindManager.cs @@ -512,12 +512,19 @@ internal unsafe class KeybindManager : IDisposable // Direct channel-switch binds (CMD_SAY/PARTY/numbered linkshells/…): switch the // game channel AND mirror it onto the active tab so the input pill shows the // real send target (pill-sync, Flo decision 2026-06-15). Rotation binds (REPLY / - // linkshell-cycle, Rotate != None) and the Permanent nuance stay deferred to the - // keybind-routing follow-cycle. + // linkshell-cycle, Rotate != None) are skipped; the temp-vs-permanent distinction + // (v1.5.6's UseTempChannel / info.Permanent) collapses to one permanent-style + // switch here — restoring it is the keybind-routing follow-cycle. if (info.Channel is { } channel && info.Rotate == RotateMode.None) { Plugin.Instance.Functions.Chat.SetChannel(channel); - if (Plugin.Instance.MainWindow?.ActiveTab is { } activeTab) + // Only mirror onto the tab when the game actually accepted the switch — an + // empty linkshell slot leaves the game channel untouched, so the pill must + // stay put rather than show a target the game will not send to. + if ( + Chat.IsChannelOrExistingLinkshell(channel) + && Plugin.Instance.MainWindow?.ActiveTab is { } activeTab + ) { activeTab.CurrentChannel.SetChannel(channel); activeTab.CurrentChannel.TellTarget = null; @@ -530,8 +537,8 @@ internal unsafe class KeybindManager : IDisposable Plugin.Instance.InputBar.SetPendingMessage(text); } - // Cycle the main window's active tab. Pop-out input-bar focus-forward stays - // deferred (no focus contract yet) — main-window tabs only. + // Pop-out input-bar focus-forward stays deferred (no focus contract yet) — + // main-window tabs only. private void DispatchTabDelta(int delta) { Plugin.Instance.MainWindow?.ChangeTabDelta(delta); diff --git a/HellionChat/SelfTests/ChannelPopoutBindStep.cs b/HellionChat/SelfTests/ChannelPopoutBindStep.cs index 8d1b987..9965110 100644 --- a/HellionChat/SelfTests/ChannelPopoutBindStep.cs +++ b/HellionChat/SelfTests/ChannelPopoutBindStep.cs @@ -4,16 +4,12 @@ using Dalamud.Plugin.SelfTest; namespace HellionChat.SelfTests; -// Exercises the ChannelPopoutPool lifecycle in-game (a behavioural step, not a -// non-null-handle check — feedback_hellion_chat_fontmanager_push_trap). Verifies -// pre-alloc == MaxParallelPopouts, unique slot ids, a TryOpen->IsOpen->TryClose -// round-trip, idempotent TryClose, and capacity-exceeded refusal (warn, no throw). -// The pool is a LIVE DI singleton, so a tester may already have real pop-outs open -// when /xlperf runs; the step tests against the FREE slots (not full capacity) and -// only ever closes ids it opened, so it neither false-REDs on a non-empty pool nor -// disturbs real pop-outs. The pure slot-map math is pinned by PopoutSlotMapTests -// (Build-Suite); this step proves the live wiring on top of it. Every slot reserved -// is released before RunStep returns, so no pop-out is left bound. +// In-game behavioural check of the ChannelPopoutPool lifecycle (not a non-null-handle +// check — feedback_hellion_chat_fontmanager_push_trap): pre-alloc count, unique slot +// ids, a TryOpen->IsOpen->TryClose round-trip, idempotent close, and capacity refusal. +// The pool is a live DI singleton, so the step works against the FREE slots (not full +// capacity) and only closes ids it opened — it neither false-REDs on a non-empty pool +// nor disturbs real pop-outs. Pure slot-map math is pinned by PopoutSlotMapTests. internal sealed class ChannelPopoutBindStep : ISelfTestStep { private readonly Plugin _plugin; diff --git a/HellionChat/Services/TellRouterService.cs b/HellionChat/Services/TellRouterService.cs index 5f2b989..ce081d0 100644 --- a/HellionChat/Services/TellRouterService.cs +++ b/HellionChat/Services/TellRouterService.cs @@ -9,7 +9,7 @@ namespace HellionChat.Services; // that service owns tab CREATION + lifecycle; this only REVEALS/pops the tab it // finds. Popout guards on pool.IsOpen so it never double-pops a tab the // AutoTellTabsOpenAsPopout path already opened. Subscribes to the resolved -// MessageManager.MessageProcessed stream (partner already extracted), not the raw +// MessageManager.MessageProcessed stream (a resolved Message), not the raw // IChatGui event, and defers the reveal one tick so the tab exists regardless of // subscriber order. Wired by TellRouterServiceInitHostedService. internal sealed class TellRouterService : IDisposable @@ -69,7 +69,9 @@ internal sealed class TellRouterService : IDisposable // mutation) is serialized with Draw (reference_dalamud_framework_thread). Plugin.Framework.RunOnFrameworkThread(() => { - var tab = AutoTellTabsService.FindTempTab(name, world); + // Lock-safe lookup: AutoTellTabs mutates Config.Tabs under its lock on the + // worker thread, so we read through its guarded accessor, not the static. + var tab = Plugin.Instance.AutoTellTabsService?.FindTempTabSafe(name, world); if (tab == null) return; // nothing to reveal (auto-tell-tabs off -> no tab created) @@ -77,7 +79,26 @@ internal sealed class TellRouterService : IDisposable { case TellAutoOpenMode.Sidebar: case TellAutoOpenMode.TopTab: - Plugin.Instance.MainWindow?.ActivateTab(tab); + // Switching to the tab on every tell is user-gated + // (TellAutoOpenSwitchAlways, default on); when off the tab still + // appears with its unread badge but the active tab is left alone. + // The mode also picks the layout, so Sidebar vs TopTab are actually + // distinct outcomes, not the same ActivateTab. + if (Plugin.Config.TellAutoOpenSwitchAlways) + { + var wantLayout = + mode == TellAutoOpenMode.TopTab + ? MainWindowLayoutMode.TopTabs + : MainWindowLayoutMode.Sidebar; + if (Plugin.Config.MainWindowLayoutMode != wantLayout) + { + Plugin.Config.MainWindowLayoutMode = wantLayout; + Plugin.Instance.SaveConfig(); + } + + Plugin.Instance.MainWindow?.ActivateTab(tab); + } + break; case TellAutoOpenMode.Popout: // IsOpen-guard: don't double-pop a tab the AutoTellTabsOpenAsPopout diff --git a/HellionChat/Ui/Components/Settings/Tabs/ChannelsTab.cs b/HellionChat/Ui/Components/Settings/Tabs/ChannelsTab.cs index 7c3bd2b..b23658d 100644 --- a/HellionChat/Ui/Components/Settings/Tabs/ChannelsTab.cs +++ b/HellionChat/Ui/Components/Settings/Tabs/ChannelsTab.cs @@ -54,6 +54,11 @@ internal sealed class ChannelsTab if (ImGui.CollapsingHeader("Tell auto-open mode", ImGuiTreeNodeFlags.DefaultOpen)) { DrawTellAutoOpenModeCombo(); + DrawToggle( + "Switch to the tab on every tell", + () => Plugin.Config.TellAutoOpenSwitchAlways, + v => Plugin.Config.TellAutoOpenSwitchAlways = v + ); } if (ImGui.CollapsingHeader("Sidebar")) diff --git a/HellionChat/Ui/Windows/MainWindow.cs b/HellionChat/Ui/Windows/MainWindow.cs index 59f095f..524d3ad 100644 --- a/HellionChat/Ui/Windows/MainWindow.cs +++ b/HellionChat/Ui/Windows/MainWindow.cs @@ -161,8 +161,8 @@ internal sealed class MainWindow : Window } // Tab-cycle entry point for the ChatTabForward/Backward keybinds. Empty list is a - // no-op; a null active tab seeds tabs[0]; a single-tab cycle that lands on the - // already-active tab is a no-op (ActivateTab early-returns on the same reference). + // no-op; a null active tab seeds the index to 0; a single-tab cycle that lands on + // the already-active tab is a no-op (ActivateTab early-returns on the same reference). // Routes through ActivateTab so the cycle strips stale tell state + re-derives the // channel exactly like a sidebar/top-tab click. Pop-out focus-forward stays // deferred (no focus contract) — main-window tabs only. diff --git a/HellionChat/Util/TabLifecycleHelpers.cs b/HellionChat/Util/TabLifecycleHelpers.cs index e9edc6f..dc87ae2 100644 --- a/HellionChat/Util/TabLifecycleHelpers.cs +++ b/HellionChat/Util/TabLifecycleHelpers.cs @@ -87,7 +87,7 @@ internal static class TabLifecycleHelpers // Wrap-around tab index for keybind cycling. Pure so the Build-Suite can test the // wrap math without a live window. count == 0 returns 0 (the caller dead-zones // before activating); negative deltas wrap correctly via the double-mod. - // TEST-MIRROR: ../../Hellion Build test/_Helpers/TabLifecycleHelpersTests.cs + // TEST-MIRROR: ../../../Hellion Build test/_Helpers/TabLifecycleHelpersTests.cs internal static int WrapTabIndex(int current, int delta, int count) { if (count <= 0)