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)
This commit is contained in:
@@ -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);
|
||||
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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"))
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user