From 8f571a144b0c02545eff1347ad6a01e1cba4d5dd Mon Sep 17 00:00:00 2001 From: Harry Cordewener Date: Tue, 11 Aug 2026 15:24:05 -0500 Subject: [PATCH 1/3] fix(triggers): a rewrite no longer throws away its own rule's highlight MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reported defect was "highlight colours don't seem to actually work". They work alone — `Highlight_RecoloursMatchedRegion` has always passed — and they were destroyed by the rule's *own* rewrite. `Process` applied the highlight to the matched region and then, four lines later, replaced the whole line with `StyledLine.FromText(text, TextStyle.Default)`: no colour, no attributes, no left rule. That combination is not exotic, it is what a channel rule looks like — route it, tidy it to `» $1`, colour it — and it is the shape of the demo configuration's own headline rule. The F2 screen badged such a rule `H` and painted both swatches, so the client promised a highlight it then threw away, and the only way to find one was to discover that deleting the rewrite brought it back. The rewrite now runs first and the highlight covers the whole of what it produced. It cannot cover the match's own offsets, because after a rewrite those address a string that no longer exists; the rewritten text is the rule's product in its entirety, so colouring all of it is the only reading that means anything. Without a rewrite nothing moves: the highlight covers the match and only the match, as it always has. A *later* rule's rewrite still replaces an earlier rule's highlighted text, and that is correct rather than the same bug one rule over — those characters are gone. Pinned, so the ordering fix is not later generalised into re-colouring text the first rule never saw. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_015nuKnWthnELNkrd86q5KWN --- src/SharpMUTerm.Core/Automation/Trigger.cs | 5 +- .../Automation/TriggerEngine.cs | 38 +++-- .../Automation/HighlightRewriteTests.cs | 148 ++++++++++++++++++ 3 files changed, 181 insertions(+), 10 deletions(-) create mode 100644 tests/SharpMUTerm.Core.Tests/Automation/HighlightRewriteTests.cs diff --git a/src/SharpMUTerm.Core/Automation/Trigger.cs b/src/SharpMUTerm.Core/Automation/Trigger.cs index e512ca4..d11a44b 100644 --- a/src/SharpMUTerm.Core/Automation/Trigger.cs +++ b/src/SharpMUTerm.Core/Automation/Trigger.cs @@ -29,7 +29,10 @@ public sealed class TriggerActions /// /// Replace the whole line's text with this template (supports $1..$9 and - /// ${name} capture references). Rewritten text renders with the default style. Null — which + /// ${name} capture references). Rewritten text starts from the default style — a rewrite is + /// how a server's own colour is dropped as well as how its wording is changed — and then wears + /// whatever , and + /// this same rule asks for, across the whole of it. Null — which /// is what the F2 screen writes for a blank field — means the rule rewrites nothing; settable for /// the same reason is, and with the same absence of any cached state. /// diff --git a/src/SharpMUTerm.Core/Automation/TriggerEngine.cs b/src/SharpMUTerm.Core/Automation/TriggerEngine.cs index d01c5e9..8a97313 100644 --- a/src/SharpMUTerm.Core/Automation/TriggerEngine.cs +++ b/src/SharpMUTerm.Core/Automation/TriggerEngine.cs @@ -253,17 +253,31 @@ public TriggerResult Process(StyledLine line) suppress = true; } - if (actions.HighlightForeground is not null || - actions.HighlightBackground is not null || - actions.AddAttributes != TextAttributes.None) + // The rewrite runs *before* the highlight, and that order is the whole of a reported defect. + // It used to run after, and a rewrite replaces the line wholesale with an unstyled one — so a + // rule that both rewrote and highlighted threw its own colours, attributes and left rule away + // on the very next statement. That is not an exotic combination: it is what a channel rule + // looks like (route it, tidy it to `» $1`, colour it), and it is the shape of the demo + // configuration's own headline rule. The F2 screen badged such a rule `H` and painted its + // swatches, so the client promised a highlight that could never appear. + var rewritten = false; + if (actions.Rewrite is not null) { - current = ApplyHighlight(current, match, actions); + current = StyledLine.FromText(match.Result(actions.Rewrite), TextStyle.Default); + rewritten = true; } - if (actions.Rewrite is not null) + if (actions.HighlightForeground is not null || + actions.HighlightBackground is not null || + actions.AddAttributes != TextAttributes.None) { - var text = match.Result(actions.Rewrite); - current = StyledLine.FromText(text, TextStyle.Default); + // A rewrite makes the match's own offsets meaningless — they described the string the + // rewrite replaced — so the highlight covers the whole of what the rule produced, which + // is the only region of the new line the rule can be said to be talking about. Without a + // rewrite it covers the match and only the match, exactly as it always has. + current = rewritten + ? ApplyHighlight(current, 0, current.Length, actions) + : ApplyHighlight(current, match.Index, match.Length, actions); } if (!string.IsNullOrEmpty(actions.SendResponse)) @@ -382,9 +396,15 @@ actions.HighlightBackground is not null || /// public const int MaxTargetLength = 64; - private static StyledLine ApplyHighlight(StyledLine line, Match match, TriggerActions actions) + /// + /// Recolours characters from and carries the + /// rule's colour onto the whole line so the output pane can draw its left rule. The region is a + /// parameter rather than a because a rewritten line has no match offsets left to + /// speak of — see the call site. + /// + private static StyledLine ApplyHighlight(StyledLine line, int start, int length, TriggerActions actions) { - var restyled = StyledText.Restyle(line, match.Index, match.Length, style => + var restyled = StyledText.Restyle(line, start, length, style => { if (actions.HighlightForeground is not null) { diff --git a/tests/SharpMUTerm.Core.Tests/Automation/HighlightRewriteTests.cs b/tests/SharpMUTerm.Core.Tests/Automation/HighlightRewriteTests.cs new file mode 100644 index 0000000..f39bd1c --- /dev/null +++ b/tests/SharpMUTerm.Core.Tests/Automation/HighlightRewriteTests.cs @@ -0,0 +1,148 @@ +using SharpMUTerm.Core.Automation; +using SharpMUTerm.Core.Text; + +namespace SharpMUTerm.Core.Tests.Automation; + +/// +/// The reported defect: highlight colours don't seem to actually work. They work on their own — +/// has always passed — and they were +/// destroyed by the rule's own rewrite. TriggerEngine.Process applied the highlight to +/// the matched region and then, four lines later, replaced the whole line with +/// StyledLine.FromText(text, TextStyle.Default), which is a line with no colour, no attributes +/// and no left rule on it. +/// +/// That combination is not exotic; it is what a channel rule looks like. Route the line to a capture +/// pane, tidy it up (» $1) and colour it — which is exactly the shape of the demo +/// configuration's own headline rule (DemoScene's public: teal, bold, and +/// Rewrite = "» $1"). The F2 screen badges such a rule H and paints both swatches, so the +/// client promised a highlight it then threw away, and the only way to get one was to discover that +/// deleting the rewrite brought it back. +/// +/// +/// The fix is an ordering one: the rewrite runs first, and the highlight is then applied to +/// the whole rewritten line. It cannot be applied to the match's own offsets, because after a rewrite +/// those address a string that no longer exists — the rewritten text is the rule's product in its +/// entirety, so colouring all of it is the only reading that means anything. +/// +/// +public class HighlightRewriteTests +{ + private static readonly TerminalColor Gold = TerminalColor.FromRgb(0xff, 0xd7, 0x00); + + private static StyledLine Line(string text) => StyledLine.FromText(text, TextStyle.Default); + + private static TriggerResult Run(TriggerActions actions, string pattern, string text) + { + var engine = new TriggerEngine(); + engine.Add(new Trigger { Pattern = pattern, Actions = actions }); + return engine.Process(Line(text)); + } + + /// The headline: a rule that rewrites and highlights does both. + [Test] + public async Task ARewrittenLineStillWearsItsRulesHighlight() + { + var result = Run( + new TriggerActions { HighlightForeground = Gold, Rewrite = "» $1" }, + @"^\[public\] (.+)$", + "[public] hello there"); + + await Assert.That(result.Line.Text).IsEqualTo("» hello there"); + await Assert.That(result.Line.Spans.All(s => s.Style.Foreground == Gold)).IsTrue(); + } + + /// + /// The whole rewritten line, not a fragment of it. The match's offsets described the line the + /// rewrite replaced, so re-using them would colour an arbitrary prefix of the new text — which is + /// the same defect wearing a different mask, and harder to spot. + /// + [Test] + public async Task TheHighlightCoversTheWholeRewrittenLine() + { + // The rewrite is far longer than the region that matched, so a highlight still keyed to + // match.Index/Length would leave the tail of the new text unstyled. + var result = Run( + new TriggerActions { HighlightBackground = Gold, Rewrite = "$1 — and a great deal more text besides" }, + @"^\[(\w+)\]", + "[public] hello there"); + + await Assert.That(result.Line.Spans.All(s => s.Style.Background == Gold)).IsTrue(); + } + + /// Attributes are part of the same promise, and were lost with the colours. + [Test] + public async Task ARewrittenLineKeepsTheAttributesItsRuleAdded() + { + var result = Run( + new TriggerActions { AddAttributes = TextAttributes.Bold, Rewrite = "» $1" }, + @"^\[public\] (.+)$", + "[public] hello there"); + + await Assert.That(result.Line.Spans.All(s => s.Style.HasAttribute(TextAttributes.Bold))).IsTrue(); + } + + /// + /// And the left rule, which is the marker the output pane draws to say a trigger touched this line + /// at all. It went with the colours, so a rewritten line was indistinguishable from an untouched one. + /// + [Test] + public async Task ARewrittenLineKeepsItsLeftRule() + { + var result = Run( + new TriggerActions { HighlightForeground = Gold, Rewrite = "» $1" }, + @"^\[public\] (.+)$", + "[public] hello there"); + + await Assert.That(result.Line.RuleColor).IsEqualTo(Gold); + } + + /// + /// A rule that only rewrites still produces unstyled text. Reordering the two actions must not smuggle + /// a style onto a line whose rule asked for none — the rewritten text is deliberately the default + /// style, so that a rewrite is a way to drop a server's colour as well as to reword it. + /// + [Test] + public async Task ARewriteWithNoHighlightIsStillPlain() + { + var result = Run(new TriggerActions { Rewrite = "» $1" }, @"^\[public\] (.+)$", "[public] hello there"); + + await Assert.That(result.Line.Text).IsEqualTo("» hello there"); + await Assert.That(result.Line.RuleColor).IsNull(); + await Assert.That(result.Line.Spans.All(s => s.Style.Equals(TextStyle.Default))).IsTrue(); + } + + /// + /// Without a rewrite nothing moves: the highlight still covers the matched region and only that. This + /// is the property the reordering could most easily have broken, and it is the behaviour every rule + /// that does not rewrite depends on. + /// + [Test] + public async Task WithoutARewriteTheHighlightStillCoversOnlyTheMatch() + { + var result = Run(new TriggerActions { HighlightForeground = Gold }, "gold", "you find gold today"); + + var gold = result.Line.Spans.Single(s => s.Text == "gold"); + await Assert.That(gold.Style.Foreground).IsEqualTo(Gold); + await Assert.That(result.Line.Spans.Where(s => s.Text != "gold").All(s => + s.Style.Foreground == TerminalColor.Default)).IsTrue(); + } + + /// + /// A later rule's rewrite still replaces an earlier rule's highlighted text, and that is + /// correct rather than the same bug one rule over: the characters the first rule coloured are gone. + /// Pinned so the ordering fix is not later "generalised" into carrying styles across rules, where it + /// would be re-colouring text the first rule never saw. + /// + [Test] + public async Task ALaterRulesRewriteStillReplacesAnEarlierRulesHighlight() + { + var engine = new TriggerEngine(); + engine.Add(new Trigger { Pattern = "gold", Actions = new TriggerActions { HighlightForeground = Gold } }); + engine.Add(new Trigger { Pattern = "^you find (.+)$", Actions = new TriggerActions { Rewrite = "found: $1" } }); + + var result = engine.Process(Line("you find gold today")); + + await Assert.That(result.Line.Text).IsEqualTo("found: gold today"); + await Assert.That(result.Line.Spans.All(s => s.Style.Foreground == TerminalColor.Default)).IsTrue(); + } +} From 4ce384b8ce903bbf61d8ad9cc486ada288a1197a Mon Sep 17 00:00:00 2001 From: Harry Cordewener Date: Tue, 11 Aug 2026 15:24:18 -0500 Subject: [PATCH 2/3] feat(triggers): a rule routes to a window that already exists, not only to a spawn MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "Triggers should not be restricted to only be able to go to spawns." They were, and the restriction was one method deep: `Workspace.RouteSpawn` was the only destination resolver a matched rule had, and it computes `SpawnWindowId(sessionKey, target)` and registers a new `WindowKind.Spawn` window when nothing answers to that id. There is no branch in it that can reach a window that already exists under any other name, so a route naming a window on the screen opened a second one beside it wearing the same label, and the one the user was looking at stayed empty. `Workspace.RouteLine` is the resolver now: a window the target already names wins, and creating a capture pane is what happens when nothing does. `FindRouteTarget` is the same lookup without the side effects, so the shell can still tell "this line opened a pane" from "this line went to one that was already there" without routing twice. What a target may reach is deliberately narrower than "any window with that title": this session's own windows, the windows nobody owns, and another character's *main* window — one alt's channel collected into the pane you actually read. It is not another session's spawn or auxiliary window, because two characters running one capture rule get a pane each and a bare title lookup would collapse them back into one, which is the exact defect `SpawnWindowId` was given an owner to fix. Only a placed window is a destination: appending to a window no pane holds is indistinguishable from the rule not firing. And finding never creates, which is the property that bounds a capture-derived target — that arm can only ever land in a window the user already has. Two consequences in the shell. `OwnerLabel` is stamped on this session's own capture panes only; writing our name onto a window somebody else owns would rename their pane after whoever last routed a line into it. And the F2 `route` field now suggests the workspace's own windows as well as the other rules' targets — while the list was the rules' targets alone, the one place a user reads what a route may say could not name any window they had open. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_015nuKnWthnELNkrd86q5KWN --- src/SharpMUTerm.Core/Workspace/Workspace.cs | 92 +++++++ src/SharpMUTerm.Tui/SharpMUTermApp.cs | 79 ++++-- src/SharpMUTerm.Tui/TriggersScreenRenderer.cs | 38 +-- src/SharpMUTerm.Tui/TriggersScreenView.cs | 6 +- .../Workspace/RouteToExistingWindowTests.cs | 195 +++++++++++++++ .../TriggerRouteDestinationTests.cs | 225 ++++++++++++++++++ .../TriggersScreenRendererTests.cs | 16 +- 7 files changed, 607 insertions(+), 44 deletions(-) create mode 100644 tests/SharpMUTerm.Core.Tests/Workspace/RouteToExistingWindowTests.cs create mode 100644 tests/SharpMUTerm.Tui.Tests/TriggerRouteDestinationTests.cs diff --git a/src/SharpMUTerm.Core/Workspace/Workspace.cs b/src/SharpMUTerm.Core/Workspace/Workspace.cs index fea867b..52316de 100644 --- a/src/SharpMUTerm.Core/Workspace/Workspace.cs +++ b/src/SharpMUTerm.Core/Workspace/Workspace.cs @@ -155,11 +155,103 @@ public WorkspaceWindow OpenWindow( return window; } + /// + /// Routes a matched trigger's line to the window names, on behalf of + /// : a window that already exists wins, and a spawn window is what + /// happens when nothing answers. Counts the line as unread unless the destination is currently + /// being read, and returns it. + /// + /// This is the resolver a routed line goes through, and the finding half of it is the point. A rule's + /// destination used to be and nothing else, which computes a spawn id and + /// registers a new window when nothing answers to it — so "put this in + /// the window I already have open" was not a thing a rule could ask for however it was spelt, and a + /// route naming a window on the screen opened a second one beside it wearing the same label. + /// + /// + /// What a target may reach is deliberately narrower than "any window with that title". It is + /// this session's own windows, the windows nobody owns, and another character's main window + /// — one alt's channel collected into the pane you actually read. It is not another + /// session's spawn or auxiliary window: two characters running one capture rule get a pane each, and + /// a bare title lookup would collapse them back into one and file the second character's channel + /// under the first, which is the exact defect was given + /// an owner to fix. A main window is admitted across that boundary because it is a window the user + /// opened by connecting, rather than one a rule conjured out of a capture. + /// + /// + /// Only a placed window is a destination. Appending to a window no pane holds writes into a + /// buffer nothing can draw, which from the reader's side is indistinguishable from the rule not + /// firing at all; a closed window is passed over and the line goes somewhere visible. + /// + /// + /// Finding never creates. A target is often a template with capture groups in it + /// (Channel $1), so the name can be the server's text — and the security property that keeps + /// that bounded is that this arm can only ever land in a window the user already has. Making one out + /// of a captured name still goes through , which puts the matching session's + /// own key on it. + /// + /// + public WorkspaceWindow RouteLine(string target, string? sessionKey = null) + { + ArgumentException.ThrowIfNullOrEmpty(target); + if (FindRouteTarget(target, sessionKey) is not { } existing) + { + return RouteSpawn(target, sessionKey); + } + + NoteActivity(existing.Id); + return existing; + } + + /// + /// The window already names for , or null when + /// nothing does — the finding half of , with no side effects, so a caller can + /// tell "this line opened a pane" from "this line went to one that was already there" without + /// routing twice. See for what a target may and may not reach. + /// + public WorkspaceWindow? FindRouteTarget(string target, string? sessionKey = null) + { + ArgumentException.ThrowIfNullOrEmpty(target); + + // Preference order, and it has to be total: several windows may carry one title, and a route that + // resolved differently from one line to the next would scatter a channel across panes. This + // session's own first, then the unowned, then another character's main; ties inside a group go to + // the older window, which is the same creation order everything else here numbers windows in. + var best = _windows.Values + .Where(w => string.Equals(w.Title, target, StringComparison.Ordinal)) + .Where(w => Layout.FindWindow(w.Id) is not null) + .Select(w => (Window: w, Rank: RouteRank(w, sessionKey))) + .Where(candidate => candidate.Rank >= 0) + .OrderBy(candidate => candidate.Rank) + .ThenBy(candidate => candidate.Window.Sequence) + .Select(candidate => candidate.Window) + .FirstOrDefault(); + + // A spawn window the user has since renamed answers to no title, and its rule must go on feeding + // it rather than opening a second pane beside it under the old name. + return best ?? _windows.GetValueOrDefault(SpawnWindowId(sessionKey, target)); + } + + /// + /// How willingly takes a line routed by — + /// lower is better, and negative means never. + /// + private static int RouteRank(WorkspaceWindow window, string? sessionKey) => window switch + { + _ when window.SessionKey is not null && string.Equals(window.SessionKey, sessionKey, StringComparison.Ordinal) => 0, + _ when window.SessionKey is null => 1, + _ when window.Kind == WindowKind.Main => 2, + _ => -1, + }; + /// /// Routes trigger-spawned output to 's spawn window named /// , creating and placing the window on first use, and counts the line as /// unread unless the window is currently visible. Returns the destination window. /// + /// This is the creating half only; is what a routed line goes through, + /// and it reaches here when no window the target names already exists. + /// + /// /// The destination is per session, not per workspace. Two connected characters running the /// same capture rule each get a window of their own; the id carries the owner, so the second /// session to match cannot land in the first's window. It used to: the id was the target alone, so diff --git a/src/SharpMUTerm.Tui/SharpMUTermApp.cs b/src/SharpMUTerm.Tui/SharpMUTermApp.cs index a6ac50e..d788e32 100644 --- a/src/SharpMUTerm.Tui/SharpMUTermApp.cs +++ b/src/SharpMUTerm.Tui/SharpMUTermApp.cs @@ -3390,7 +3390,11 @@ private void OnLine(WorldSession session, string windowId, StyledLine line) } /// - /// Routes a trigger-spawned line to its spawn window (creating the tab on first use). + /// Routes a trigger-matched line to the window its rule names — one that already exists when the + /// target names one, and a spawn window created on the spot when it does not + /// (). Routing used to be RouteSpawn and nothing else, so + /// every destination a rule could have was a spawn pane of its own session's; a route naming a window + /// on the screen opened a second one beside it wearing the same label. /// /// The owner recorded on a first-seen window is the whose trigger fired, /// not _active. It used to be the latter, so a background world's capture opened a window @@ -3399,6 +3403,12 @@ private void OnLine(WorldSession session, string windowId, StyledLine line) /// which world a link clicked in a spawn window sends to by it. /// /// + /// It is stamped on this session's own capture panes only. The label prefixes a tab as + /// Owner: Name so a spawn scattered into another pane stays tied to its character, and a + /// destination this session does not own is somebody else's window or nobody's — writing our name + /// onto another character's main window would relabel their pane after whoever routed into it. + /// + /// /// The same session key also picks the window (), /// which is what gives two characters running one capture rule a pane each. While the id was the /// target alone there was one window per workspace: the first session to match created it with its @@ -3409,12 +3419,19 @@ private void OnLine(WorldSession session, string windowId, StyledLine line) /// private void OnSpawnLine(WorldSession session, string target, StyledLine line) { - var existed = _workspace.FindWindow(Workspace.SpawnWindowId(session.SessionKey, target)) is not null; - var window = _workspace.RouteSpawn(target, session.SessionKey); + // Asked before routing rather than after, because routing is what makes the answer false: this is + // "was there already somewhere for this line to go", which is what decides between adding a tab + // and merely refreshing the badges. + var existed = _workspace.FindRouteTarget(target, session.SessionKey) is not null; + var window = _workspace.RouteLine(target, session.SessionKey); // Its owner's own name, which for a session with no character is its world's. It used to fall back on // the *main window's* title, which is a different session's name as soon as more than one is open. - window.OwnerLabel ??= SessionTitle(session); + if (window.Kind == WindowKind.Spawn && + string.Equals(window.SessionKey, session.SessionKey, StringComparison.Ordinal)) + { + window.OwnerLabel ??= SessionTitle(session); + } PaneContentFor(window.Id, window.Title); // ensure the live control exists before buffering // The restore log is fed here as well as in OnLine, and that is the crux of the whole feature: @@ -3424,7 +3441,7 @@ private void OnSpawnLine(WorldSession session, string target, StyledLine line) AppendWindowLine(window.Id, _formatter.ToMarkup(line), stamp); RecordForRestore(session, window.Id, window.Title, line, stamp); - // A first-seen spawn adds a tab to its pane, so rebuild; otherwise just refresh badges. + // A first-seen destination adds a tab to its pane, so rebuild; otherwise just refresh badges. if (existed) { RefreshTabTitles(); @@ -5565,14 +5582,41 @@ private void PersistConfiguration() } } - /// Distinct spawn-window targets referenced by any trigger (for the F2 route-to list). - private IReadOnlyList SpawnTargets() => - _config.TriggerSets.SelectMany(s => s.Triggers) - .Select(t => t.Actions.SpawnTarget) - .Where(t => !string.IsNullOrEmpty(t)) - .Select(t => t!) - .Distinct(StringComparer.Ordinal) - .ToList(); + /// + /// The destinations the F2 route field offers: every window some trigger already routes to, + /// then the windows this workspace actually holds. + /// + /// The second half is what makes routing to an existing window expressible. A rule's + /// destination is resolved by name against the windows that are open + /// (), and while the list was the other rules' targets alone the + /// only names it could offer were spawn panes — so the one place a user reads what a route may say + /// could not name the character's own window, another character's, or any window they had opened. The + /// list is suggestions and not the permitted set, so this widens what is discoverable rather than + /// what is legal. + /// + /// + /// Trigger targets lead, because a rule that has not opened its pane yet names a window nothing else + /// can offer, and because that is the order this list has always been read in. + /// + /// + private IReadOnlyList RouteTargets() + { + var targets = new List(); + foreach (var name in _config.TriggerSets.SelectMany(s => s.Triggers) + .Select(t => t.Actions.SpawnTarget) + .Concat(_workspace.Windows + .Where(w => _workspace.Layout.FindWindow(w.Id) is not null) + .OrderBy(w => w.Sequence) + .Select(w => w.Title))) + { + if (!string.IsNullOrEmpty(name) && !targets.Contains(name, StringComparer.Ordinal)) + { + targets.Add(name); + } + } + + return targets; + } /// Every configured macro across all trigger sets (for the F4 keypad/hotkey list). private IReadOnlyList Macros() => _config.TriggerSets.SelectMany(s => s.Macros).ToList(); @@ -5753,13 +5797,13 @@ private ScreenBinding WorldsScreen(string fkey, bool onCharacters) private ScreenBinding TriggersScreen() { var session = new SettingsSession(selection => - TriggersScreenRenderer.Model(_config.TriggerSets, selection.SelectionIn(0), SpawnTargets()), + TriggersScreenRenderer.Model(_config.TriggerSets, selection.SelectionIn(0), RouteTargets()), SaveConfiguration); return new ScreenBinding(session, () => TriggersScreenView.Build( _config.TriggerSets, session.Selection.SelectionIn(0), - SpawnTargets(), + RouteTargets(), _system.DesktopDimensions.Width, session.Focus(), _system.DesktopDimensions.Height)); @@ -8462,6 +8506,11 @@ internal void OpenUnownedWindowForTest(string id, string title) /// , and it goes stale silently, so it is worth asserting directly. internal string? WindowOwnerOf(string windowId) => _workspace.FindWindow(windowId)?.SessionKey; + /// A window's owner label — the Owner: Name prefix its tab wears. A different + /// fact from and worth asserting separately: a line routed into a window + /// somebody else owns must not stamp the routing character's name onto it. + internal string? WindowOwnerLabelOf(string windowId) => _workspace.FindWindow(windowId)?.OwnerLabel; + /// A pane's visible tab, so a test can say which window a click on a tab strip brought up. internal string? PaneActiveTab(string paneId) => _workspace.Layout.FindPane(paneId)?.ActiveTab; diff --git a/src/SharpMUTerm.Tui/TriggersScreenRenderer.cs b/src/SharpMUTerm.Tui/TriggersScreenRenderer.cs index e7f8abb..f5f5c25 100644 --- a/src/SharpMUTerm.Tui/TriggersScreenRenderer.cs +++ b/src/SharpMUTerm.Tui/TriggersScreenRenderer.cs @@ -119,21 +119,23 @@ internal static class TriggersScreenRenderer private static string Route(Trigger trigger) => trigger.Actions.SpawnTarget ?? MainWindow; /// - /// The windows offered as ↑↓ suggestions on the route field: the main output, every spawn window - /// the workspace knows about, and — always — the one this rule already points at, so a rule - /// routed somewhere the current workspace has no window for still shows its own value. + /// The destinations offered as ↑↓ suggestions on the route field: the main output, every window a + /// rule or the workspace can name (SharpMUTermApp.RouteTargets), and — always — the one this + /// rule already points at, so a rule routed somewhere the current workspace has no window for still + /// shows its own value. /// /// These are suggestions, not the permitted set. Typing a name that isn't here is how a new spawn - /// window comes into existence: the workspace's spawn windows are defined by what triggers route - /// to, so a closed list could only ever re-use one that already exists. + /// window comes into existence: a name nothing answers to is created as a capture pane + /// (Workspace.RouteLine), so a closed list could only ever re-use a destination that already + /// exists. /// /// - internal static IReadOnlyList Routes(Trigger trigger, IReadOnlyList? spawnTargets) + internal static IReadOnlyList Routes(Trigger trigger, IReadOnlyList? routeTargets) { ArgumentNullException.ThrowIfNull(trigger); var routes = new List { MainWindow }; - foreach (var target in (spawnTargets ?? Array.Empty()).Append(Route(trigger))) + foreach (var target in (routeTargets ?? Array.Empty()).Append(Route(trigger))) { if (!string.IsNullOrEmpty(target) && !routes.Contains(target, StringComparer.Ordinal)) { @@ -210,15 +212,15 @@ private static List NamedCallbacks(IReadOnlyList sets) public static List Render( IReadOnlyList sets, int selectedTrigger, - IReadOnlyList spawnTargets) + IReadOnlyList routeTargets) { ArgumentNullException.ThrowIfNull(sets); - ArgumentNullException.ThrowIfNull(spawnTargets); + ArgumentNullException.ThrowIfNull(routeTargets); var left = RulesColumn(sets, selectedTrigger); - var right = EditorColumn(sets, selectedTrigger, spawnTargets); + var right = EditorColumn(sets, selectedTrigger, routeTargets); - var lines = new List { HeaderLine(0, Model(sets, selectedTrigger, spawnTargets)), string.Empty }; + var lines = new List { HeaderLine(0, Model(sets, selectedTrigger, routeTargets)), string.Empty }; var rowCount = Math.Max(left.Count, right.Count); for (var i = 0; i < rowCount; i++) @@ -261,7 +263,7 @@ internal static string HeaderLine(int width, ScreenModel? model = null, ScreenFo /// and a palette are — while the editor keeps drawing them where they are read. /// /// - /// + /// /// The spawn windows a rule may route to, beyond main and its own current target. Optional /// so a caller that only wants the navigable shape (the header hints, the tests) need not know the /// workspace's windows. @@ -269,7 +271,7 @@ internal static string HeaderLine(int width, ScreenModel? model = null, ScreenFo internal static ScreenModel Model( IReadOnlyList sets, int selectedTrigger, - IReadOnlyList? spawnTargets = null) + IReadOnlyList? routeTargets = null) { ArgumentNullException.ThrowIfNull(sets); @@ -284,7 +286,7 @@ internal static ScreenModel Model( "route", () => Route(entry.Trigger), v => entry.Trigger.Actions.SpawnTarget = v == MainWindow ? null : v.Trim(), - Routes(entry.Trigger, spawnTargets)), + Routes(entry.Trigger, routeTargets)), ScreenField.Colour( "highlight fg", () => entry.Trigger.Actions.HighlightForeground, @@ -517,13 +519,13 @@ private static IEnumerable FlagLegend(Trigger? trigger, int width) internal static List EditorColumn( IReadOnlyList sets, int selectedTrigger, - IReadOnlyList spawnTargets, + IReadOnlyList routeTargets, ScreenFocus? focus = null, int width = ColumnWidth, int height = 0) { ArgumentNullException.ThrowIfNull(sets); - ArgumentNullException.ThrowIfNull(spawnTargets); + ArgumentNullException.ThrowIfNull(routeTargets); var cursor = focus ?? ScreenFocus.None; var flattened = Flatten(sets); @@ -531,7 +533,7 @@ internal static List EditorColumn( ? BuildEditor( flattened[selectedTrigger].Trigger, flattened[selectedTrigger].SetName, - spawnTargets, + routeTargets, cursor, selectedTrigger, width, @@ -609,7 +611,7 @@ private static string Flags(TriggerActions actions) private static List BuildEditor( Trigger trigger, string setName, - IReadOnlyList spawnTargets, + IReadOnlyList routeTargets, ScreenFocus cursor, int index, int width = ColumnWidth, diff --git a/src/SharpMUTerm.Tui/TriggersScreenView.cs b/src/SharpMUTerm.Tui/TriggersScreenView.cs index 469684d..90cb703 100644 --- a/src/SharpMUTerm.Tui/TriggersScreenView.cs +++ b/src/SharpMUTerm.Tui/TriggersScreenView.cs @@ -18,14 +18,14 @@ internal static class TriggersScreenView public static IWindowControl Build( IReadOnlyList sets, int selectedTrigger, - IReadOnlyList spawnTargets, + IReadOnlyList routeTargets, int width, ScreenFocus? focus = null, int height = 0) { var header = ScreenChrome.Band( TriggersScreenRenderer.HeaderLine( - width, TriggersScreenRenderer.Model(sets, selectedTrigger, spawnTargets), focus), + width, TriggersScreenRenderer.Model(sets, selectedTrigger, routeTargets), focus), ScreenPalette.HeaderBg); var footer = ScreenChrome.Band( TriggersScreenRenderer.FooterLine(sets, selectedTrigger, width, focus), ScreenPalette.FooterBg); @@ -41,7 +41,7 @@ public static IWindowControl Build( var body = ScreenChrome.Rows(height); var left = TriggersScreenRenderer.RulesColumn(sets, selectedTrigger, focus, rules); var right = TriggersScreenRenderer.EditorColumn( - sets, selectedTrigger, spawnTargets, focus, width <= 0 ? rules : width - rules - ScreenChrome.ColumnDivider, body); + sets, selectedTrigger, routeTargets, focus, width <= 0 ? rules : width - rules - ScreenChrome.ColumnDivider, body); var rulesCol = ScreenChrome.Stretch(new MarkupControl(ScreenChrome.Window(left, body))); var editorCol = ScreenChrome.Stretch(new MarkupControl( diff --git a/tests/SharpMUTerm.Core.Tests/Workspace/RouteToExistingWindowTests.cs b/tests/SharpMUTerm.Core.Tests/Workspace/RouteToExistingWindowTests.cs new file mode 100644 index 0000000..2284791 --- /dev/null +++ b/tests/SharpMUTerm.Core.Tests/Workspace/RouteToExistingWindowTests.cs @@ -0,0 +1,195 @@ +using SharpMUTerm.Core.Workspaces; + +namespace SharpMUTerm.Core.Tests.Workspaces; + +/// +/// The reported defect: a trigger's routing could only ever go to a spawn window. +/// Workspace.RouteSpawn is the one destination resolver a matched rule has, and it computes +/// SpawnWindowId(sessionKey, target) and — when nothing answers to that id — registers a brand +/// new window. There is no branch in it that can reach a window that +/// already exists under any other name, so "route to the window I already have open" was not a thing a +/// rule could ask for however it was spelt. +/// +/// is the resolver now: an existing window the target names wins, and +/// creating a spawn is what happens when nothing does. What that buys is a rule feeding a window +/// somebody opened deliberately — a character's own main window, another character's, or any named +/// auxiliary window — rather than a fourth pane appearing beside them. +/// +/// +/// What it deliberately cannot reach is another session's spawn window, and that is the whole +/// reason resolution is scoped rather than a bare title lookup over the registry. Two characters running +/// one capture rule get a pane each; a title that crossed between them would collapse the two back into +/// one and file the second character's channel under the first, which is exactly the defect +/// SpawnWindowId was given an owner to fix. +/// +/// +public class RouteToExistingWindowTests +{ + private const string Ann = "Convergence.Ann"; + private const string Bob = "Convergence.Bob"; + + /// A workspace whose main window belongs to Ann and is titled the way the shell titles it. + private static Workspace AnnsWorkspace() => new("main", "Ann", Ann); + + // ---- The report ------------------------------------------------------------------------- + + /// + /// The headline. A rule routing to the title of a window that already exists lands in that + /// window; before the fix it opened a second one beside it, of a different kind, with the same name on + /// its tab. + /// + [Test] + public async Task ARuleRoutesToAnExistingWindowRatherThanOpeningASpawnBesideIt() + { + var workspace = AnnsWorkspace(); + workspace.OpenWindow("notes", "Notes", WindowKind.Auxiliary, Ann); + + var destination = workspace.RouteLine("Notes", Ann); + + await Assert.That(destination.Id).IsEqualTo("notes"); + await Assert.That(workspace.Windows.Count(w => w.Title == "Notes")).IsEqualTo(1); + } + + /// + /// Including the character's own main window, which is the destination the F2 route list has always + /// named and the one a rule could least express: main there means do not route, which + /// only reaches the main window for a line the rule does not also gag. + /// + [Test] + public async Task ARuleCanRouteToItsOwnCharactersMainWindow() + { + var workspace = AnnsWorkspace(); + + var destination = workspace.RouteLine("Ann", Ann); + + await Assert.That(destination.Id).IsEqualTo("main"); + await Assert.That(destination.Kind).IsEqualTo(WindowKind.Main); + } + + /// + /// And another character's main window — one alt's channel collected into the pane you actually read. + /// A main window is the one window another session owns that this may reach, because it is a window + /// the user opened by connecting rather than one a capture rule conjured. + /// + [Test] + public async Task ARuleCanRouteToAnotherCharactersMainWindow() + { + var workspace = AnnsWorkspace(); + workspace.OpenWindow("main:bob", "Bob", WindowKind.Main, Bob); + + var destination = workspace.RouteLine("Bob", Ann); + + await Assert.That(destination.Id).IsEqualTo("main:bob"); + } + + /// An unowned window — the web view is the one in this client — is in everybody's reach. + [Test] + public async Task ARuleCanRouteToAnUnownedWindow() + { + var workspace = AnnsWorkspace(); + workspace.OpenWindow("web", "Scratch", WindowKind.Auxiliary); + + await Assert.That(workspace.RouteLine("Scratch", Ann).Id).IsEqualTo("web"); + } + + // ---- What must not move ------------------------------------------------------------------ + + /// + /// The per-session guarantee, which this resolution is scoped to preserve: Bob's rule may not land in + /// Ann's capture pane just because they chose the same channel name. Bob gets his own, as before. + /// + [Test] + public async Task ARuleCannotRouteIntoAnotherSessionsSpawnWindow() + { + var workspace = AnnsWorkspace(); + var anns = workspace.RouteLine("Public", Ann); + + var bobs = workspace.RouteLine("Public", Bob); + + await Assert.That(bobs.Id).IsNotEqualTo(anns.Id); + await Assert.That(bobs.Id).IsEqualTo(Workspace.SpawnWindowId(Bob, "Public")); + await Assert.That(bobs.SessionKey).IsEqualTo(Bob); + } + + /// + /// Nor into another session's auxiliary window. Only a main window crosses the owner + /// boundary: everything else another character owns was created for them, and the two cases a route + /// must never conflate are "the window you meant" and "somebody else's window with the same label". + /// + [Test] + public async Task ARuleCannotRouteIntoAnotherSessionsAuxiliaryWindow() + { + var workspace = AnnsWorkspace(); + workspace.OpenWindow("bobs-notes", "Notes", WindowKind.Auxiliary, Bob); + + var destination = workspace.RouteLine("Notes", Ann); + + await Assert.That(destination.Id).IsEqualTo(Workspace.SpawnWindowId(Ann, "Notes")); + } + + /// + /// Nothing found is still a spawn window, created and placed exactly as it always was. This is the + /// path every existing capture rule takes and it must be untouched. + /// + [Test] + public async Task ATargetNothingAnswersToStillOpensASpawnWindow() + { + var workspace = AnnsWorkspace(); + + var destination = workspace.RouteLine("Chat", Ann); + + await Assert.That(destination.Id).IsEqualTo(Workspace.SpawnWindowId(Ann, "Chat")); + await Assert.That(destination.Kind).IsEqualTo(WindowKind.Spawn); + await Assert.That(destination.Title).IsEqualTo("Chat"); + await Assert.That(workspace.Layout.FindWindow(destination.Id)).IsNotNull(); + } + + /// + /// A rule feeding its own capture pane goes on feeding the same one — the second line of a channel + /// must not find the window by a different route than the first did and end up somewhere else. + /// + [Test] + public async Task TheSecondLineOfACaptureLandsInTheSamePaneAsTheFirst() + { + var workspace = AnnsWorkspace(); + + var first = workspace.RouteLine("Chat", Ann); + var second = workspace.RouteLine("Chat", Ann); + + await Assert.That(second.Id).IsEqualTo(first.Id); + } + + /// + /// A window no pane holds is not a destination. Routing there would append to a buffer nothing can + /// draw, which is indistinguishable from the rule not firing — so a closed window is passed over and + /// the line goes to a spawn pane that can actually be seen. + /// + [Test] + public async Task AClosedWindowIsNotADestination() + { + var workspace = AnnsWorkspace(); + workspace.OpenWindow("notes", "Notes", WindowKind.Auxiliary, Ann); + workspace.CloseWindow("notes"); + + var destination = workspace.RouteLine("Notes", Ann); + + await Assert.That(destination.Id).IsEqualTo(Workspace.SpawnWindowId(Ann, "Notes")); + } + + /// + /// Routing badges the destination unread when it is not the window being read, whichever kind it + /// turned out to be. The badge is the only thing that says a background pane gained a line, and a + /// resolution that reached a new kind of window without it would make the feature silent. + /// + [Test] + public async Task RoutingToAnExistingWindowStillBadgesItUnread() + { + var workspace = AnnsWorkspace(); + workspace.OpenWindow("notes", "Notes", WindowKind.Auxiliary, Ann); + workspace.ActivateWindow("main"); // Notes shares the pane as a tab, and is now the hidden one + + var destination = workspace.RouteLine("Notes", Ann); + + await Assert.That(destination.Unread).IsGreaterThan(0); + } +} diff --git a/tests/SharpMUTerm.Tui.Tests/TriggerRouteDestinationTests.cs b/tests/SharpMUTerm.Tui.Tests/TriggerRouteDestinationTests.cs new file mode 100644 index 0000000..03e0f64 --- /dev/null +++ b/tests/SharpMUTerm.Tui.Tests/TriggerRouteDestinationTests.cs @@ -0,0 +1,225 @@ +using SharpConsoleUI.Drivers; +using SharpMUTerm.Core.Automation; +using SharpMUTerm.Core.Commands; +using SharpMUTerm.Core.Configuration; +using SharpMUTerm.Core.Text; +using SharpMUTerm.Core.Workspaces; +using SharpMUTerm.Graphics; + +namespace SharpMUTerm.Tui.Tests; + +/// +/// The two reported defects, from the shell's side and over a live connection: a trigger could only +/// route to a spawn window, and a highlight colour did not survive the same rule's rewrite. +/// +/// Both are asserted end to end rather than on the engine alone, because both have a second half here. +/// A destination is only a destination if the shell appends to it (OnSpawnLine), and a highlight +/// is only a highlight if the markup a pane is fed carries the colour — a pane holds Spectre markup and +/// nothing else, so a StyledLine that was right on the way in proves nothing about the frame. +/// +/// +/// +/// Serialised with the other end-to-end suites: constructing the app and rendering a frame both touch +/// the process-global console streams. +/// +[NotInParallel] +public class TriggerRouteDestinationTests +{ + private const int Width = 160; + private const int Height = 40; + + private const string Ann = "Convergence.Ann"; + private const string Bob = "Convergence.Bob"; + + private static readonly TerminalCapabilities Headless = + new(GraphicsProtocol.None, supportsTrueColor: true, supportsKittyGraphics: false, supportsSixel: false); + + // ---- Routing somewhere that is not a fresh spawn window ----------------------------------- + + /// + /// The headline. Ann's rule routes to Bob — a window that already exists, of a kind no rule + /// could reach — and the line lands in it. Before the fix routing went through + /// Workspace.RouteSpawn, which can only ever answer with a spawn id it owns, so this opened a + /// fourth pane called Bob beside Bob's own and left his empty. + /// + [Test] + public async Task ARuleRoutesIntoAnotherCharactersWindowThatAlreadyExists() + { + var app = await Two(RouteTo("Bob")); + + Receive(app, AnnWire, " Ann says, \"hello\"\n"); + + await Assert.That(string.Join("\n", app.PaneLines(MainWindowOf(app, Bob)))).Contains("hello"); + await Assert.That(app.WindowIds()).DoesNotContain(Workspace.SpawnWindowId(Ann, "Bob")); + } + + /// + /// And into a character's own main window, which the F2 route list has always been able to name and a + /// rule could not reach: main there means do not route, so it only ever described a + /// line the rule did not also gag. A gagging rule aimed at the main window used to delete the line. + /// + [Test] + public async Task AGaggingRuleCanStillPutItsLineInItsOwnMainWindow() + { + var app = await Two(RouteTo("Ann")); + + Receive(app, AnnWire, " Ann says, \"hello\"\n"); + + await Assert.That(string.Join("\n", app.PaneLines(MainWindowOf(app, Ann)))).Contains("hello"); + } + + /// + /// A target nothing answers to is still a capture pane, created and owned by the matching session — + /// the path every capture rule that ships takes, and the one that must not have moved. + /// + [Test] + public async Task AnUnknownTargetStillOpensACapturePane() + { + var app = await Two(RouteTo("Chat")); + + Receive(app, AnnWire, " Ann says, \"hello\"\n"); + + var id = Workspace.SpawnWindowId(Ann, "Chat"); + await Assert.That(app.WindowIds()).Contains(id); + await Assert.That(app.WindowOwnerOf(id)).IsEqualTo(Ann); + await Assert.That(string.Join("\n", app.PaneLines(id))).Contains("hello"); + } + + /// + /// The per-session guarantee is untouched, and this is the fixture it was bought with: two characters + /// running one capture rule still get a pane each. Resolution admits another character's main + /// window and never their capture panes, so a shared channel name cannot collapse the two back into + /// one and file the second character's channel under the first. + /// + [Test] + public async Task TwoCharactersCapturingOneNameStillGetAPaneEach() + { + var app = await Two(RouteTo("Public")); + + Receive(app, AnnWire, " Ann says, \"first\"\n"); + Receive(app, BobWire, " Bob says, \"second\"\n"); + + var ann = string.Join("\n", app.PaneLines(Workspace.SpawnWindowId(Ann, "Public"))); + var bob = string.Join("\n", app.PaneLines(Workspace.SpawnWindowId(Bob, "Public"))); + + await Assert.That(ann).Contains("first"); + await Assert.That(ann).DoesNotContain("second"); + await Assert.That(bob).Contains("second"); + await Assert.That(bob).DoesNotContain("first"); + } + + /// + /// A window this session does not own is not relabelled by routing into it. OwnerLabel prefixes + /// a tab as Owner: Name to tie a capture pane scattered into another pane back to its + /// character; stamping it on a destination somebody else owns would rename their pane after whoever + /// last routed a line into it. + /// + [Test] + public async Task RoutingIntoAWindowDoesNotRelabelItAfterTheRoutingCharacter() + { + var app = await Two(RouteTo("Bob")); + + Receive(app, AnnWire, " Ann says, \"hello\"\n"); + + await Assert.That(app.WindowOwnerLabelOf(MainWindowOf(app, Bob))).IsNull(); + } + + // ---- The highlight, in the markup a pane is actually fed ------------------------------------ + + /// + /// A rule that rewrites and highlights: the pane's markup carries the colour. On the unfixed build + /// the rewrite ran after the highlight and replaced the line with an unstyled one, so the pane was + /// fed plain text while the F2 screen went on badging that rule H and painting its swatch. + /// + [Test] + public async Task ARewrittenLineReachesThePaneWearingItsHighlight() + { + var app = await Two(Configuration(new TriggerActions + { + HighlightForeground = TerminalColor.FromRgb(0xff, 0xd7, 0x00), + Rewrite = "» $1", + })); + + Receive(app, AnnWire, " Ann says, \"hello\"\n"); + + var line = app.PaneLines(MainWindowOf(app, Ann)).Last(); + await Assert.That(line).Contains("» Ann says"); + await Assert.That(line).Contains("#ffd700"); + } + + // ---- Harness ------------------------------------------------------------------------------ + + private RecordingTelnetSession AnnWire { get; set; } = new(); + + private RecordingTelnetSession BobWire { get; set; } = new(); + + /// The window a character's own output goes to — found by its owner rather than assumed. + private static string MainWindowOf(SharpMUTermApp app, string sessionKey) => + app.WindowIds().Single(id => + app.WindowOwnerOf(id) == sessionKey && !id.StartsWith(Workspace.SpawnPrefix, StringComparison.Ordinal)); + + private static AppConfiguration RouteTo(string target) => + Configuration(new TriggerActions { SpawnTarget = target, Gag = true }); + + private static AppConfiguration Configuration(TriggerActions actions) + { + var config = new AppConfiguration(); + config.TriggerSets.Add(new TriggerSet + { + Name = "Comms", + Triggers = + { + new Trigger { Name = "Public", Pattern = "^ (.+)$", Actions = actions }, + }, + }); + + config.Worlds.Add(new WorldDefinition + { + Name = "Convergence", + Host = "convergence.example.org", + Port = 4201, + Characters = + { + new CharacterDefinition { Name = "Ann", Logging = new LoggingSettings(), TriggerSets = { "Comms" } }, + new CharacterDefinition { Name = "Bob", Logging = new LoggingSettings(), TriggerSets = { "Comms" } }, + }, + }); + + return config; + } + + /// + /// Both characters open and connected. Two, because every destination this is about is a window + /// somebody else has — and because a session that was never connected never runs its receive path, + /// which would make any routing assertion true whatever the code does. + /// + private async Task Two(AppConfiguration config) + { + Console.SetIn(TextReader.Null); + AnnWire = new RecordingTelnetSession(); + BobWire = new RecordingTelnetSession(); + + var app = new SharpMUTermApp(config, Headless, new HeadlessConsoleDriver(Width, Height)); + await Open(app, Ann, AnnWire); + await Open(app, Bob, BobWire); + app.RenderNextFrame(); + return app; + } + + private static async Task Open(SharpMUTermApp app, string sessionKey, RecordingTelnetSession wire) + { + app.TelnetFactory = _ => wire; + if (!app.DispatchCommand(CommandIds.Character(sessionKey))) + { + throw new InvalidOperationException($"the app would not switch to {sessionKey}"); + } + + await app.FindSession(sessionKey)!.ConnectAsync(); + } + + private static void Receive(SharpMUTermApp app, RecordingTelnetSession wire, string text) + { + wire.Receive(text); + app.RenderNextFrame(); + } +} diff --git a/tests/SharpMUTerm.Tui.Tests/TriggersScreenRendererTests.cs b/tests/SharpMUTerm.Tui.Tests/TriggersScreenRendererTests.cs index b87c423..9b42091 100644 --- a/tests/SharpMUTerm.Tui.Tests/TriggersScreenRendererTests.cs +++ b/tests/SharpMUTerm.Tui.Tests/TriggersScreenRendererTests.cs @@ -54,7 +54,7 @@ private static IReadOnlyList Scene() => new[] [Test] public async Task Render_RuleListShowsNamePatternOwningSetAndRoute() { - var lines = TriggersScreenRenderer.Render(Scene(), selectedTrigger: 0, spawnTargets: new[] { "Chat", "Combat log" }); + var lines = TriggersScreenRenderer.Render(Scene(), selectedTrigger: 0, routeTargets: new[] { "Chat", "Combat log" }); var rowIndex = lines.FindIndex(l => l.Contains("Tell") && l.Contains(@"^(\w+) tells you")); await Assert.That(lines[rowIndex]).Contains("→ Chat"); @@ -66,7 +66,7 @@ public async Task Render_RuleListShowsNamePatternOwningSetAndRoute() [Test] public async Task Render_FlagsSummariseGagHighlightAndSpawn() { - var lines = TriggersScreenRenderer.Render(Scene(), selectedTrigger: 0, spawnTargets: new[] { "Chat" }); + var lines = TriggersScreenRenderer.Render(Scene(), selectedTrigger: 0, routeTargets: new[] { "Chat" }); var tellRowIndex = lines.FindIndex(l => l.Contains("Tell") && l.Contains(@"^(\w+) tells you")); var tellSub = lines[tellRowIndex + 1]; @@ -82,7 +82,7 @@ public async Task Render_FlagsSummariseGagHighlightAndSpawn() [Test] public async Task Render_SelectedTriggerEditorShowsPatternAndRoute() { - var lines = TriggersScreenRenderer.Render(Scene(), selectedTrigger: 0, spawnTargets: new[] { "Chat", "Combat log" }); + var lines = TriggersScreenRenderer.Render(Scene(), selectedTrigger: 0, routeTargets: new[] { "Chat", "Combat log" }); await Assert.That(lines.Any(l => l.Contains("match pattern"))).IsTrue(); await Assert.That(lines.Any(l => l.Contains(@"^(\w+) tells you"))).IsTrue(); @@ -99,10 +99,10 @@ public async Task Render_SelectedTriggerEditorShowsPatternAndRoute() [Test] public async Task Render_GagToggleReflectsActionsGag() { - var gagged = TriggersScreenRenderer.Render(Scene(), selectedTrigger: 1, spawnTargets: Array.Empty()); + var gagged = TriggersScreenRenderer.Render(Scene(), selectedTrigger: 1, routeTargets: Array.Empty()); await Assert.That(gagged.Any(l => l.Contains("[[x]] gag line") || l.Contains("#00f5b7][[x]][/] gag line"))).IsTrue(); - var notGagged = TriggersScreenRenderer.Render(Scene(), selectedTrigger: 0, spawnTargets: Array.Empty()); + var notGagged = TriggersScreenRenderer.Render(Scene(), selectedTrigger: 0, routeTargets: Array.Empty()); await Assert.That(notGagged.Any(l => l.Contains("[dim][[ ]] gag line[/]"))).IsTrue(); } @@ -115,7 +115,7 @@ public async Task Render_GagToggleReflectsActionsGag() [Test] public async Task Render_HighlightCaptionAndSwatchAppearWhenColourSet() { - var lines = TriggersScreenRenderer.Render(Scene(), selectedTrigger: 0, spawnTargets: Array.Empty()); + var lines = TriggersScreenRenderer.Render(Scene(), selectedTrigger: 0, routeTargets: Array.Empty()); var heading = lines.Single(l => l.Contains("highlight") && !l.Contains("fg") && !l.Contains("bg")); await Assert.That(heading).Contains("recoloured"); @@ -129,7 +129,7 @@ public async Task Render_HighlightCaptionAndSwatchAppearWhenColourSet() [Test] public async Task Render_EmptySetsShowsNoTriggers() { - var lines = TriggersScreenRenderer.Render(Array.Empty(), selectedTrigger: -1, spawnTargets: Array.Empty()); + var lines = TriggersScreenRenderer.Render(Array.Empty(), selectedTrigger: -1, routeTargets: Array.Empty()); await Assert.That(lines.Any(l => l.Contains("no triggers"))).IsTrue(); await Assert.That(lines.Any(l => l.Contains("Triggers & spawn routing"))).IsTrue(); @@ -150,7 +150,7 @@ public async Task Render_EscapesMarkupBracketsInNamesAndPatterns() }, }; - var lines = TriggersScreenRenderer.Render(sets, selectedTrigger: 0, spawnTargets: Array.Empty()); + var lines = TriggersScreenRenderer.Render(sets, selectedTrigger: 0, routeTargets: Array.Empty()); await Assert.That(lines.Any(l => l.Contains("Br[[acket]]"))).IsTrue(); await Assert.That(lines.Any(l => l.Contains("x[[1]]"))).IsTrue(); await Assert.That(lines.Any(l => l.Contains("Weird[[Set]]"))).IsTrue(); From ee02d8d7e52e435858d2e2513aa20f7bea09c4fa Mon Sep 17 00:00:00 2001 From: Harry Cordewener Date: Tue, 11 Aug 2026 15:59:05 -0500 Subject: [PATCH 3/3] fix(triggers): a closed spawn window is placed again, not fed invisibly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review catch. The title lookup already required a *placed* window, and the renamed-spawn fallback beside it did not — so a spawn window whose pane the user closed could be returned as a destination and the channel written into a buffer nothing draws. That is reachable rather than theoretical: the registry outlives the layout, and a restored workspace registers windows a saved layout no longer places. It also contradicted RouteLine's own doc comment, which says a closed window is passed over and the line goes somewhere visible. Both halves, because guarding only the fallback moves the same defect one call deeper: RouteSpawn returned a registered-but-unplaced window untouched, since it only placed one it had just created. It now places on the way past whether or not the window is new — which is what lets FindRouteTarget decline a closed window and fall through, reopening the pane under the same id with its history in it. The regression test was checked against the unfixed code and fails there. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_015nuKnWthnELNkrd86q5KWN --- src/SharpMUTerm.Core/Workspace/Workspace.cs | 22 +++++++++++++++--- .../Workspace/RouteToExistingWindowTests.cs | 23 +++++++++++++++++++ 2 files changed, 42 insertions(+), 3 deletions(-) diff --git a/src/SharpMUTerm.Core/Workspace/Workspace.cs b/src/SharpMUTerm.Core/Workspace/Workspace.cs index 52316de..85a4cb1 100644 --- a/src/SharpMUTerm.Core/Workspace/Workspace.cs +++ b/src/SharpMUTerm.Core/Workspace/Workspace.cs @@ -228,7 +228,15 @@ public WorkspaceWindow RouteLine(string target, string? sessionKey = null) // A spawn window the user has since renamed answers to no title, and its rule must go on feeding // it rather than opening a second pane beside it under the old name. - return best ?? _windows.GetValueOrDefault(SpawnWindowId(sessionKey, target)); + // + // Placed, like the title lookup above it, and for the same reason: a window the registry still + // knows and no pane holds is *closed* (see the numbering remarks), and routing to one writes the + // channel into a buffer nobody can see. The registry outlives the layout in two ways — a restored + // workspace registers windows a saved layout no longer places — so this is reachable rather than + // theoretical. Falling through instead is not a loss: RouteLine then reaches RouteSpawn, which + // places this very window again under the same id, so the pane reopens with its history in it. + var renamed = _windows.GetValueOrDefault(SpawnWindowId(sessionKey, target)); + return best ?? (renamed is not null && Layout.FindWindow(renamed.Id) is not null ? renamed : null); } /// @@ -265,9 +273,17 @@ public WorkspaceWindow RouteSpawn(string target, string? sessionKey = null) { ArgumentException.ThrowIfNullOrEmpty(target); var id = SpawnWindowId(sessionKey, target); - if (!_windows.TryGetValue(id, out var window)) + var window = _windows.TryGetValue(id, out var existing) + ? existing + : Register(new WorkspaceWindow(id, target, WindowKind.Spawn, sessionKey)); + + // Placed on the way past, and *not* only when the window is new. The registry outlives the layout + // — a restored workspace registers windows a saved layout no longer places — so a window can be + // known and closed at once, and returning that from a route writes the channel into a buffer + // nobody can see. Making this total is what lets FindRouteTarget decline a closed window and fall + // through here: the pane reopens under the same id, with its history already in it. + if (Layout.FindWindow(id) is null) { - window = Register(new WorkspaceWindow(id, target, WindowKind.Spawn, sessionKey)); Layout.AddWindow(id, activate: false); // spawns open in the background and accrue unread } diff --git a/tests/SharpMUTerm.Core.Tests/Workspace/RouteToExistingWindowTests.cs b/tests/SharpMUTerm.Core.Tests/Workspace/RouteToExistingWindowTests.cs index 2284791..7671909 100644 --- a/tests/SharpMUTerm.Core.Tests/Workspace/RouteToExistingWindowTests.cs +++ b/tests/SharpMUTerm.Core.Tests/Workspace/RouteToExistingWindowTests.cs @@ -176,6 +176,29 @@ public async Task AClosedWindowIsNotADestination() await Assert.That(destination.Id).IsEqualTo(Workspace.SpawnWindowId(Ann, "Notes")); } + /// + /// The same rule through the renamed-spawn fallback, which is the one arm that does not go by title. + /// A spawn window whose pane the user closed is still in the registry — the registry outlives the + /// layout, and a restored workspace can register windows a saved layout no longer places — so the + /// fallback would hand back a window nothing draws. It has to place the pane again instead, under + /// the same id, so the channel comes back with its history rather than going somewhere invisible. + /// + [Test] + public async Task ARenamedSpawnWindowWhosePaneWasClosedIsPlacedAgainRatherThanFedInvisibly() + { + var workspace = AnnsWorkspace(); + var spawned = workspace.RouteLine("Chat", Ann); + spawned.Title = "Tells"; // the user renames it, so no title answers to "Chat" any more + workspace.Layout.RemoveWindow(spawned.Id); // and closes its pane, leaving it registered + + await Assert.That(workspace.FindRouteTarget("Chat", Ann)).IsNull(); + + var destination = workspace.RouteLine("Chat", Ann); + + await Assert.That(destination.Id).IsEqualTo(spawned.Id); + await Assert.That(workspace.Layout.FindWindow(destination.Id)).IsNotNull(); + } + /// /// Routing badges the destination unread when it is not the window being read, whichever kind it /// turned out to be. The badge is the only thing that says a background pane gained a line, and a