From dc9228157e944388c6717b7ab5b132416724822f Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sun, 23 Aug 2026 21:50:28 +1000 Subject: [PATCH 1/6] Show every failing pair in one viewer window The viewer is not MDI, so DiffRunner gave it a process per pair the way it does every other tool: a run failing several image snapshots opened a window each. That shape was not incidental. Kill closes a pair's window by matching the command line it was launched with, and it skips MDI tools entirely, so a shared window would have left a test that started passing nothing to close - or closed the window every other pending pair was drawn in. The pair was already a row somewhere. DiffRunner tracks a pending move for every launch, and with no tray that move goes to the queue owner, so the entry existed and a second window was drawn beside it. Diff is Move plus the window Move deliberately withholds: every other tool has already opened its own for the pair by the time its move arrives, and when the viewer is the tool there is none. So the verb tracks the pair and raises the owner over it - a viewer that holds a window selects the entry, and a tray that does not starts one and hands it the same selection. DiffRunner takes that route as soon as it knows the tool is the viewer, before the process bookkeeping that has nothing to work on: no instance showing this pair to find, no window to replace, no MaxInstance slot to spend on a window that already exists, and no process id for the tray to kill on accept. Kill sends Settle for the move key instead, and a tracked key settles by being dropped, on both owners: the received file is already gone by then, so accepting or discarding the row would only fail on it. An owner too old to know the verb gets a plain Move rather than a second process it would refuse as well. Rows follow their files now, which they did not before and had not needed to. An owned queue is push only - a socket message or a launch argument puts an entry in it and nothing revisits it - so a row described the moment it arrived and nothing after: content a re-run had already replaced, and a received file that was no longer there. OwnerLink has always done the equivalent for a displayed queue, so TrackedWatch is the same stat, the same FileStamp test and the same cadence for an owned one. It stops short of the tray's rule about a pair whose files became byte equal, because that check is there for an external diff tool converging them and here the viewer is the diff tool. Three things change for a reader. Accepting moves the received file over the target rather than copying it, which is what accepting a move has always meant. Received is always the left pane, so the target-on-left ordering no longer applies to these. And the window closes once the last row goes, which was already the rule for an emptied queue and is now reachable by the files themselves going away. A hand run DiffEngineViewer left right is untouched: its own window, no port, no queue, since a queue nothing can join is the whole intent there. --- claude.md | 24 ++- docs/inline.md | 6 +- docs/mdsource/inline.source.md | 6 +- docs/mdsource/viewer.source.md | 12 +- docs/viewer.md | 12 +- src/DiffEngine.Tests/ViewerProtocolTests.cs | 51 +++++- src/DiffEngine/DiffRunner.cs | 14 ++ src/DiffEngine/DiffRunner_Kill.cs | 10 ++ .../Protocol/ViewerMessageHandler.cs | 20 +++ src/DiffEngine/Protocol/ViewerVerb.cs | 13 ++ src/DiffEngine/Tray/PendingFiles.cs | 86 ++++++++++ src/DiffEngine/Viewer/ViewerLauncher.cs | 16 ++ .../OwnedInlineHostTest.cs | 32 ++++ .../TrackerTrackedFilesTest.cs | 34 ++++ src/DiffEngineTray/ITrackedFiles.cs | 8 + src/DiffEngineTray/OwnedInlineHost.cs | 16 ++ src/DiffEngineTray/Tracker.cs | 11 ++ .../CommandLineTests.Attach.verified.txt | 3 +- .../CommandLineTests.Diff.verified.txt | 8 + .../CommandLineTests.Files.verified.txt | 3 +- .../CommandLineTests.Inline.verified.txt | 3 +- ...ests.InlineArgumentsReordered.verified.txt | 3 +- ...ts.Rejected_DiffWithoutTarget.verified.txt | 1 + .../CommandLineTests.cs | 9 + src/DiffEngineViewer.Tests/EngineDiffTests.cs | 133 +++++++++++++++ .../EngineInlineTests.cs | 34 ---- src/DiffEngineViewer.Tests/EngineScope.cs | 46 +++++ .../TrackedFileTests.cs | 82 +++++++++ .../TrackedWatchTests.cs | 143 ++++++++++++++++ src/DiffEngineViewer/CommandLine.cs | 17 ++ src/DiffEngineViewer/TrackedWatch.cs | 157 ++++++++++++++++++ src/DiffEngineViewer/ViewerProgram.cs | 49 ++++++ src/DiffEngineViewer/ViewerRequest.cs | 8 + src/DiffEngineViewer/ViewerSession.cs | 68 ++++++++ 34 files changed, 1086 insertions(+), 52 deletions(-) create mode 100644 src/DiffEngineViewer.Tests/CommandLineTests.Diff.verified.txt create mode 100644 src/DiffEngineViewer.Tests/CommandLineTests.Rejected_DiffWithoutTarget.verified.txt create mode 100644 src/DiffEngineViewer.Tests/EngineDiffTests.cs create mode 100644 src/DiffEngineViewer.Tests/EngineScope.cs create mode 100644 src/DiffEngineViewer.Tests/TrackedWatchTests.cs create mode 100644 src/DiffEngineViewer/TrackedWatch.cs diff --git a/claude.md b/claude.md index a26730e5..9ca47bbd 100644 --- a/claude.md +++ b/claude.md @@ -58,8 +58,8 @@ flowchart LR Files[("source files and
staged patch files")] Engine -->|"3492 moves, deletes (one way),
when a tray is running"| Tray - Engine -->|"3493 inline, settle, and
moves and deletes with no tray"| Owner - Engine -.->|"launch with patch on stdin, or
with a delete, when nothing owns 3493"| Window + Engine -->|"3493 inline, settle, and diff,
moves and deletes with no tray"| Owner + Engine -.->|"launch with patch on stdin, or with
a delete or a pair, when nothing owns 3493"| Window Tray <-->|"3493 list, accept, focus"| Owner Window <-->|"3493 listfull, accept, discard"| Owner Plugin -->|"3493 settle, after accepting"| Owner @@ -171,7 +171,17 @@ apart. `Tray/TrayDetector.cs` as source, because DiffEngine publishes and embeds the heads and a reference back would be a cycle. - Holds pending moves and deletes itself when it owns the queue, which is what happens with no - tray installed. They are ordinary `QueueEntryKind.Move`/`Delete` entries — the same ones an + tray installed, and every failing pair DiffEngine resolved it for, whether a tray is running or + not. +- `TrackedWatch` is what keeps those rows honest, and only runs for a queue this process owns. An + owned queue is otherwise push only — a socket message or a launch argument puts an entry in it + and nothing ever revisits it — so rows described the moment they arrived and nothing after. + `OwnerLink.ReadChanges` has always done the equivalent for a displayed queue, on the same 200ms + cadence and the same `FileStamp` test, which is why the two are worth reading together. A pass + that finds nothing must return the identical `SessionState`, or the open context menu closes + five times a second. It stops short of the tray's third rule, dropping a pair whose two files + became byte equal: that check exists because an external diff tool might have converged them, + and here the viewer is the diff tool. They are ordinary `QueueEntryKind.Move`/`Delete` entries — the same ones an attached viewer draws for the tray's — so nothing about how they look or what their menu offers is per arrangement. Only who applies them differs: `ViewerActions.MoveFile`/`DeleteFile` here, a forwarded key there. @@ -242,6 +252,14 @@ apart. window — the diff tool DiffRunner just launched for that pair — and a delete has no second file to compare against, so no tool ever opens for it. `--delete ` is the launch, on the command line rather than stdin because a path fits where snapshot content does not. +- Unless that diff tool is the viewer, which is the `Diff` verb and `--diff `. + Then the premise above is false — there is no window for the pair yet — so it is tracked exactly + as a move and a window is raised over the entry, and `DiffRunner` skips the whole process per + pair path: nothing to find already showing it, no window to replace, no `MaxInstance` slot to + spend, and no process for the tray to kill on accept. `DiffRunner.Kill` sends `Settle` for the + move key rather than killing anything, since the row is drawn in a window shared with every other + pending pair. That is what makes ten failing image snapshots one window instead of ten, and it is + only available to the viewer because no other tool can be told to drop one pair. - The catch that shape creates: every inline transition rebuilds its half of the queue from `InlineQueue`, so `ViewerSession.Rebuild` carries the tracked entries across it. Without that, accepting one snapshot silently drops the files pending beside it. `Sync` is the one caller that diff --git a/docs/inline.md b/docs/inline.md index 94b63ef1..b69f1a06 100644 --- a/docs/inline.md +++ b/docs/inline.md @@ -26,8 +26,8 @@ flowchart LR Files[("source files and
staged patch files")] Engine -->|"3492 moves, deletes (one way),
when a tray is running"| Tray - Engine -->|"3493 inline, settle, and
moves and deletes with no tray"| Owner - Engine -.->|"launch with patch on stdin, or with
a delete, when nothing owns 3493"| Window + Engine -->|"3493 inline, settle, and diff,
moves and deletes with no tray"| Owner + Engine -.->|"launch with patch on stdin, or with
a delete or a pair, when nothing owns 3493"| Window Tray <-->|"3493 list, accept, focus"| Owner Window <-->|"3493 listfull (with the owner's moves
and deletes), accept, discard"| Owner Plugin <-->|"3493 listfull, accept, discard,
focus, via InlineQueueClient"| Owner @@ -37,7 +37,7 @@ flowchart LR The queue of pending snapshots has exactly one owner per session: whichever process bound port 3493 first, decided once and never transferred. When the tray owns it, its edges to the owner above are in-process calls; when a viewer owns it, the tray drives that viewer over the same verbs. Either way both hosts run the same `InlineQueue` implementation, so they cannot disagree on what accepting or settling means. [DiffEngineViewer](/docs/viewer.md) and [DiffEngineTray](/docs/tray.md) cover the two arrangements in detail. -Pending file moves and deletes follow the same rule. They go to the tray when one is running, over the port they have always used, and to the queue owner when one is not — so with no tray installed they are reviewed in the viewer rather than going nowhere. A delete starts a viewer if nothing owns the queue, because it has no second file to compare against and so no diff tool ever opens for it. A move does not: DiffEngine has already opened a diff tool for that file pair. +Pending file moves and deletes follow the same rule. They go to the tray when one is running, over the port they have always used, and to the queue owner when one is not — so with no tray installed they are reviewed in the viewer rather than going nowhere. A delete starts a viewer if nothing owns the queue, because it has no second file to compare against and so no diff tool ever opens for it. A move normally does not, because DiffEngine has already opened a diff tool for that file pair — unless that tool is the viewer itself, in which case there is no separate window to compete with and the pair is queued and raised the way a snapshot is. That is what makes a run failing several file comparisons produce one window rather than one per pair. ## When a test fails diff --git a/docs/mdsource/inline.source.md b/docs/mdsource/inline.source.md index 5af482da..370fabb7 100644 --- a/docs/mdsource/inline.source.md +++ b/docs/mdsource/inline.source.md @@ -19,8 +19,8 @@ flowchart LR Files[("source files and
staged patch files")] Engine -->|"3492 moves, deletes (one way),
when a tray is running"| Tray - Engine -->|"3493 inline, settle, and
moves and deletes with no tray"| Owner - Engine -.->|"launch with patch on stdin, or with
a delete, when nothing owns 3493"| Window + Engine -->|"3493 inline, settle, and diff,
moves and deletes with no tray"| Owner + Engine -.->|"launch with patch on stdin, or with
a delete or a pair, when nothing owns 3493"| Window Tray <-->|"3493 list, accept, focus"| Owner Window <-->|"3493 listfull (with the owner's moves
and deletes), accept, discard"| Owner Plugin <-->|"3493 listfull, accept, discard,
focus, via InlineQueueClient"| Owner @@ -30,7 +30,7 @@ flowchart LR The queue of pending snapshots has exactly one owner per session: whichever process bound port 3493 first, decided once and never transferred. When the tray owns it, its edges to the owner above are in-process calls; when a viewer owns it, the tray drives that viewer over the same verbs. Either way both hosts run the same `InlineQueue` implementation, so they cannot disagree on what accepting or settling means. [DiffEngineViewer](/docs/viewer.md) and [DiffEngineTray](/docs/tray.md) cover the two arrangements in detail. -Pending file moves and deletes follow the same rule. They go to the tray when one is running, over the port they have always used, and to the queue owner when one is not — so with no tray installed they are reviewed in the viewer rather than going nowhere. A delete starts a viewer if nothing owns the queue, because it has no second file to compare against and so no diff tool ever opens for it. A move does not: DiffEngine has already opened a diff tool for that file pair. +Pending file moves and deletes follow the same rule. They go to the tray when one is running, over the port they have always used, and to the queue owner when one is not — so with no tray installed they are reviewed in the viewer rather than going nowhere. A delete starts a viewer if nothing owns the queue, because it has no second file to compare against and so no diff tool ever opens for it. A move normally does not, because DiffEngine has already opened a diff tool for that file pair — unless that tool is the viewer itself, in which case there is no separate window to compete with and the pair is queued and raised the way a snapshot is. That is what makes a run failing several file comparisons produce one window rather than one per pair. ## When a test fails diff --git a/docs/mdsource/viewer.source.md b/docs/mdsource/viewer.source.md index ee1c2235..09ab66ee 100644 --- a/docs/mdsource/viewer.source.md +++ b/docs/mdsource/viewer.source.md @@ -32,12 +32,18 @@ One package per operating system rather than one for all of them, because WinFor ## Usage -Comparing two files: +Comparing two files, in a window of its own: ``` DiffEngineViewer ``` +Comparing a failing pair, which DiffEngine sends when the diff tool it resolved for that pair is the viewer. Queued rather than given its own window, so later pairs join it: + +``` +DiffEngineViewer --diff +``` + Reviewing an inline snapshot, where the patch payload arrives on stdin: ``` @@ -77,6 +83,10 @@ Nothing is written to disk for inline review. The patch travels over stdin, or o A test run that fails several inline snapshots produces one window, not several. Whichever process binds the loopback port holds the queue; everything else hands its patch to that one. The window lists everything pending and offers **Accept all**. +Failing file comparisons join the same queue, so a run that fails ten snapshots opens one window whether they are inline or on disk. Every other diff tool gets a process per pair, and DiffEngine closes each one as its test starts passing; the viewer is told to drop that row instead. + +Rows that came from files follow those files. A re-run that rewrites a received file shows the rewrite, a verified file that appears fills in the other pane, and a row whose received file goes away leaves with it — so nothing is offered for a file that is no longer there, however it went. The window closes once the last row does. + The list sits in a column on the left. Drag the divider beside it to widen the column when the file names are longer than it is. When the list outgrows the window it follows the selection, keeping the selected row visible. Row labels are the shortest thing that tells one entry from another, so hovering one fills in what it left out: the whole path, the test behind a call site, every framework behind a conflict, and the failure behind a `!`. A row with nothing to add shows no tooltip at all. diff --git a/docs/viewer.md b/docs/viewer.md index 3f1c9f51..ab4c2504 100644 --- a/docs/viewer.md +++ b/docs/viewer.md @@ -39,12 +39,18 @@ One package per operating system rather than one for all of them, because WinFor ## Usage -Comparing two files: +Comparing two files, in a window of its own: ``` DiffEngineViewer ``` +Comparing a failing pair, which DiffEngine sends when the diff tool it resolved for that pair is the viewer. Queued rather than given its own window, so later pairs join it: + +``` +DiffEngineViewer --diff +``` + Reviewing an inline snapshot, where the patch payload arrives on stdin: ``` @@ -84,6 +90,10 @@ Nothing is written to disk for inline review. The patch travels over stdin, or o A test run that fails several inline snapshots produces one window, not several. Whichever process binds the loopback port holds the queue; everything else hands its patch to that one. The window lists everything pending and offers **Accept all**. +Failing file comparisons join the same queue, so a run that fails ten snapshots opens one window whether they are inline or on disk. Every other diff tool gets a process per pair, and DiffEngine closes each one as its test starts passing; the viewer is told to drop that row instead. + +Rows that came from files follow those files. A re-run that rewrites a received file shows the rewrite, a verified file that appears fills in the other pane, and a row whose received file goes away leaves with it — so nothing is offered for a file that is no longer there, however it went. The window closes once the last row does. + The list sits in a column on the left. Drag the divider beside it to widen the column when the file names are longer than it is. When the list outgrows the window it follows the selection, keeping the selected row visible. Row labels are the shortest thing that tells one entry from another, so hovering one fills in what it left out: the whole path, the test behind a call site, every framework behind a conflict, and the failure behind a `!`. A row with nothing to add shows no tooltip at all. diff --git a/src/DiffEngine.Tests/ViewerProtocolTests.cs b/src/DiffEngine.Tests/ViewerProtocolTests.cs index ab1f05a5..c3737295 100644 --- a/src/DiffEngine.Tests/ViewerProtocolTests.cs +++ b/src/DiffEngine.Tests/ViewerProtocolTests.cs @@ -468,6 +468,50 @@ await Assert.That(owner.Tracked).IsEquivalentTo( ]); } + /// + /// The pair whose diff tool is the viewer itself: tracked exactly as a move, and then raised, + /// which is the whole difference between the two verbs. The focus names the entry just + /// tracked, so an owner with a window selects it and one without starts a viewer over it. + /// + [Test] + public async Task ADiffTracksThePairAndRaisesAWindow() + { + var owner = new FakeOwner((true, null)); + + var response = ViewerMessageHandler.Handle(owner, new(ViewerVerb.Diff, @"c:\temp\a.received.txt", @"c:\code\a.verified.txt")); + + await Assert.That(response.Ok).IsTrue(); + await Assert.That(owner.Tracked).IsEquivalentTo([@"move c:\temp\a.received.txt > c:\code\a.verified.txt"]); + await Assert.That(owner.Windowed).IsEquivalentTo([$"{WindowCommand.Focus} {TrackedKeys.ForMove(@"c:\temp\a.received.txt")}"]); + } + + /// + /// And a move stays silent, which is what lets the two coexist: every other tool has already + /// opened its own window for the pair by the time its move arrives. + /// + [Test] + public async Task AMoveRaisesNothing() + { + var owner = new FakeOwner((true, null)); + + ViewerMessageHandler.Handle(owner, new(ViewerVerb.Move, @"c:\temp\a.received.txt", @"c:\code\a.verified.txt")); + + await Assert.That(owner.Windowed).IsEmpty(); + } + + [Test] + public async Task ADiffWithoutBothPathsIsRefused() + { + var owner = new FakeOwner((true, null)); + + var noTarget = ViewerMessageHandler.Handle(owner, new(ViewerVerb.Diff, @"c:\temp\a.received.txt")); + + await Assert.That(noTarget.Ok).IsFalse(); + await Assert.That(noTarget.Message).IsEqualTo("Diff requires a key and a body"); + await Assert.That(owner.Tracked).IsEmpty(); + await Assert.That(owner.Windowed).IsEmpty(); + } + [Test] public async Task AMoveWithoutBothPathsIsRefused() { @@ -530,9 +574,10 @@ public void TrackDelete(string file) => public string? DiscardAll() => null; - public void Window(WindowCommand command, string? key) - { - } + public List Windowed { get; } = []; + + public void Window(WindowCommand command, string? key) => + Windowed.Add($"{command} {key}"); } /// diff --git a/src/DiffEngine/DiffRunner.cs b/src/DiffEngine/DiffRunner.cs index cef9327e..ec5af74c 100644 --- a/src/DiffEngine/DiffRunner.cs +++ b/src/DiffEngine/DiffRunner.cs @@ -180,6 +180,14 @@ static LaunchResult InnerLaunch(TryResolveTool tryResolveTool, string tempFile, return result.Value; } + // The viewer queues rather than opening a window per pair, so none of the process + // bookkeeping below applies to it: there is no instance showing this pair to find, no + // window to replace, and no slot to spend on a window that already exists. + if (PendingFiles.IsViewer(tool)) + { + return PendingFiles.AddDiff(tempFile, targetFile, tool.ExePath); + } + tool.CommandAndArguments(tempFile, targetFile, out var arguments, out var command); var canKill = !tool.IsMdi; @@ -220,6 +228,12 @@ static async Task InnerLaunchAsync(TryResolveTool tryResolveTool, return result.Value; } + // As above: the viewer has no window of its own for this pair to reason about. + if (PendingFiles.IsViewer(tool)) + { + return await PendingFiles.AddDiffAsync(tempFile, targetFile, tool.ExePath, Cancel.None); + } + tool.CommandAndArguments(tempFile, targetFile, out var arguments, out var command); var canKill = !tool.IsMdi; diff --git a/src/DiffEngine/DiffRunner_Kill.cs b/src/DiffEngine/DiffRunner_Kill.cs index d347262a..05fcfd11 100644 --- a/src/DiffEngine/DiffRunner_Kill.cs +++ b/src/DiffEngine/DiffRunner_Kill.cs @@ -22,6 +22,16 @@ public static void Kill(string tempFile, string targetFile) return; } + // The viewer holds this pair as a row in a queue some other process owns, not as a window + // of its own, so there is no process here to kill - and killing the one it is drawn in + // would take every other pending pair with it. Settling drops the row instead, which is + // what killing the window meant for a tool that had one per pair. + if (PendingFiles.IsViewer(diffTool)) + { + ViewerClient.TrySend(new(ViewerVerb.Settle, TrackedKeys.ForMove(tempFile))); + return; + } + if (diffTool.IsMdi) { Logging.Write($"DiffTool is Mdi so not killing. diffTool: {diffTool.ExePath}"); diff --git a/src/DiffEngine/Protocol/ViewerMessageHandler.cs b/src/DiffEngine/Protocol/ViewerMessageHandler.cs index aa095c75..6d6a80b9 100644 --- a/src/DiffEngine/Protocol/ViewerMessageHandler.cs +++ b/src/DiffEngine/Protocol/ViewerMessageHandler.cs @@ -19,6 +19,8 @@ public static ViewerResponse Handle(IQueueOwner owner, ViewerMessage message) return Settle(owner, message.Key, message.Body, message.Member); case ViewerVerb.Move: return Move(owner, message.Key, message.Body); + case ViewerVerb.Diff: + return Diff(owner, message.Key, message.Body); case ViewerVerb.Delete: return Delete(owner, message.Key); case ViewerVerb.List: @@ -103,6 +105,24 @@ static ViewerResponse Move(IQueueOwner owner, string? temp, string? target) return ViewerResponse.Success(); } + /// + /// The same tracking performs, plus the window it deliberately withholds. + /// The focus names the entry just tracked, so an owner with a window selects it and an owner + /// without one - a tray - starts a viewer and hands it the same selection. + /// + static ViewerResponse Diff(IQueueOwner owner, string? temp, string? target) + { + if (temp is null || + target is null) + { + return ViewerResponse.Error("Diff requires a key and a body"); + } + + owner.TrackMove(temp, target); + owner.Window(WindowCommand.Focus, TrackedKeys.ForMove(temp)); + return ViewerResponse.Success(); + } + static ViewerResponse Delete(IQueueOwner owner, string? file) { if (file is null) diff --git a/src/DiffEngine/Protocol/ViewerVerb.cs b/src/DiffEngine/Protocol/ViewerVerb.cs index 732e5976..04c3f047 100644 --- a/src/DiffEngine/Protocol/ViewerVerb.cs +++ b/src/DiffEngine/Protocol/ViewerVerb.cs @@ -23,6 +23,19 @@ enum ViewerVerb /// Move, + /// + /// Show a two file comparison, tracking it as a pending move at the same time: key is + /// the received file, body the target it belongs at. From DiffEngine when the diff tool + /// it resolved for the pair is the viewer itself. + /// + /// with a window, and the split is the whole reason both exist. A move for + /// some other tool must not raise anything, because that tool has just opened its own window + /// for the pair. When the viewer is the tool there is no such window, and one queue entry with + /// a window raised over it is what replaces the process per pair every other tool needs. + /// + /// + Diff, + /// /// Track a pending file delete: key is the file. From DiffEngine when no tray is /// running, and unlike this one does start a viewer when nothing owns the diff --git a/src/DiffEngine/Tray/PendingFiles.cs b/src/DiffEngine/Tray/PendingFiles.cs index 3272c00c..f0cc6170 100644 --- a/src/DiffEngine/Tray/PendingFiles.cs +++ b/src/DiffEngine/Tray/PendingFiles.cs @@ -66,6 +66,92 @@ await PiperClient.SendDeleteAsync(file, cancel)) ViewerLauncher.LaunchDelete(file); } + /// + /// A failing pair whose resolved diff tool is the viewer itself. + /// + /// Tracked exactly as any other move is - the tray when one is running, the queue owner + /// otherwise - and then shown, which is the part withholds. + /// Every other tool's move arrives with that tool's window already open for the pair; this one + /// has no window until something raises one over the entry. + /// + /// + /// The window is a when the tray took the move, because the + /// tray tracks it and the queue owner - normally that same tray - only has to raise something + /// over it. In the arrangement where a viewer owns the queue while a tray runs, that viewer + /// does not know the tray's files, so the focus finds nothing and the pair stays what it was + /// before any of this: an entry in the tray menu. + /// + /// + public static LaunchResult AddDiff(string tempFile, string targetFile, string exe) + { + // CanKill false and no process: there is no window of its own to kill, and killing the + // shared one would take every other pair in it away as well. The arguments are stored all + // the same, because the tray re-runs them for "Open diff tool". + if (DiffEngineTray.IsRunning && + PiperClient.SendMove(tempFile, targetFile, exe, ViewerLauncher.DiffArguments(tempFile, targetFile), false, null)) + { + ViewerClient.TrySend(new(ViewerVerb.Focus, TrackedKeys.ForMove(tempFile))); + return LaunchResult.AlreadyRunningAndSupportsRefresh; + } + + if (ViewerClient.TrySend(new(ViewerVerb.Diff, tempFile, targetFile), out var response)) + { + return response.Ok + ? LaunchResult.AlreadyRunningAndSupportsRefresh + : Refused(tempFile, targetFile); + } + + return ViewerLauncher.LaunchDiff(tempFile, targetFile) + ? LaunchResult.StartedNewInstance + : LaunchResult.NoDiffToolFound; + } + + /// + /// An owner that is there and said no, which is an owner too old to know the verb. Launching a + /// second viewer cannot change that answer and would bind nothing, so the pair goes over as a + /// plain move: a row with nothing raised over it, which every owner has always understood. + /// + static LaunchResult Refused(string tempFile, string targetFile) => + ViewerClient.TrySend(new(ViewerVerb.Move, tempFile, targetFile)) + ? LaunchResult.AlreadyRunningAndSupportsRefresh + : LaunchResult.NoDiffToolFound; + + /// + public static async Task AddDiffAsync(string tempFile, string targetFile, string exe, Cancel cancel) + { + if (DiffEngineTray.IsRunning && + await PiperClient.SendMoveAsync(tempFile, targetFile, exe, ViewerLauncher.DiffArguments(tempFile, targetFile), false, null, cancel)) + { + await ViewerClient.TrySendAsync(new(ViewerVerb.Focus, TrackedKeys.ForMove(tempFile)), cancel); + return LaunchResult.AlreadyRunningAndSupportsRefresh; + } + + var outcome = await ViewerClient.SendAsync(new(ViewerVerb.Diff, tempFile, targetFile), cancel); + if (outcome == SendOutcome.Accepted) + { + return LaunchResult.AlreadyRunningAndSupportsRefresh; + } + + if (outcome == SendOutcome.Refused) + { + return await ViewerClient.TrySendAsync(new(ViewerVerb.Move, tempFile, targetFile), cancel) + ? LaunchResult.AlreadyRunningAndSupportsRefresh + : LaunchResult.NoDiffToolFound; + } + + return ViewerLauncher.LaunchDiff(tempFile, targetFile) + ? LaunchResult.StartedNewInstance + : LaunchResult.NoDiffToolFound; + } + + /// + /// Whether a pending file should take the route rather than the plain + /// tracking one, which is exactly whether the tool that would have opened a window for it is + /// the viewer. + /// + public static bool IsViewer(ResolvedTool tool) => + tool.Tool == DiffTool.DiffEngineViewer; + public static void AddMove( string tempFile, string targetFile, diff --git a/src/DiffEngine/Viewer/ViewerLauncher.cs b/src/DiffEngine/Viewer/ViewerLauncher.cs index fa73c8f0..f09a455b 100644 --- a/src/DiffEngine/Viewer/ViewerLauncher.cs +++ b/src/DiffEngine/Viewer/ViewerLauncher.cs @@ -63,6 +63,22 @@ public static async Task LaunchAsync(InlinePatch patch, string payload, Ca public static bool LaunchDelete(string file) => Start($"--delete \"{file}\"") is not null; + /// + /// Starts a viewer holding one failing pair, for when the tool resolved for that pair is the + /// viewer itself and nothing owns the queue. The same launch makes, + /// for the same reason: the pair joins a queue that later pairs can join too. + /// + public static bool LaunchDiff(string temp, string target) => + Start(DiffArguments(temp, target)) is not null; + + /// + /// Built here rather than at each caller, because the tray stores these arguments against the + /// tracked move and re-runs them for "Open diff tool". A relaunch that did not say --diff would + /// open a window of its own instead of raising the queue the pair is already in. + /// + public static string DiffArguments(string temp, string target) => + $"--diff \"{temp}\" \"{target}\""; + static Process? Start(InlinePatch patch) => // The source and line go on the command line, not just in the payload, so each launch is // distinguishable: ProcessCleanup matches on command line, and it makes the process diff --git a/src/DiffEngineTray.Tests/OwnedInlineHostTest.cs b/src/DiffEngineTray.Tests/OwnedInlineHostTest.cs index 33e69604..ff4c72d9 100644 --- a/src/DiffEngineTray.Tests/OwnedInlineHostTest.cs +++ b/src/DiffEngineTray.Tests/OwnedInlineHostTest.cs @@ -630,6 +630,14 @@ public void AddMove(string temp, string target) => public void AddDelete(string file) => Added.Add($"delete {file}"); + + public List Untracked { get; } = []; + + public bool Untrack(string key) + { + Untracked.Add(key); + return Has(key); + } } [Test] @@ -670,6 +678,30 @@ public async Task ATrackedKeyAcceptDispatchesToTrackedFiles() await Assert.That(owner.Host.List()).HasSingleItem(); } + /// + /// A settle for a tracked key drops the pair rather than reaching the inline queue, which is + /// how a viewer-as-diff-tool pair leaves when its test starts passing: there is no window to + /// kill for it, only a row that should stop describing a file DiffEngine has removed. + /// + [Test] + public async Task ATrackedKeySettleUntracksThePair() + { + using var owner = new Owner(_ => throw new("the inline queue must not be touched")); + var tracked = new FakeTracked + { + MoveList = [new(@"move:c:\temp\a.txt", "Sample.Test (txt)", null, @"c:\temp\a.txt", @"c:\code\a.txt")] + }; + owner.Host.TrackedFiles = tracked; + owner.Queue(); + + var response = owner.Send(new(ViewerVerb.Settle, @"move:c:\temp\a.txt")); + + await Assert.That(response.Ok).IsTrue(); + await Assert.That(tracked.Untracked).IsEquivalentTo([@"move:c:\temp\a.txt"]); + // The snapshot queued beside it is untouched: a settle names one entry. + await Assert.That(owner.Host.List()).HasSingleItem(); + } + [Test] public async Task ATrackedKeyWithNoTrackedFilesIsUnknown() { diff --git a/src/DiffEngineTray.Tests/TrackerTrackedFilesTest.cs b/src/DiffEngineTray.Tests/TrackerTrackedFilesTest.cs index cf5f4c62..deae12ea 100644 --- a/src/DiffEngineTray.Tests/TrackerTrackedFilesTest.cs +++ b/src/DiffEngineTray.Tests/TrackerTrackedFilesTest.cs @@ -100,6 +100,40 @@ public async Task ALockedMoveIsRefusedWithoutPrompting() } } + /// + /// Settling a pair whose test started passing. Neither accept nor discard: DiffEngine has + /// already taken the received file away, and both of those would act on disk. + /// + [Test] + public async Task UntrackingLeavesBothFilesAlone() + { + await using var tracker = new RecordingTracker(); + ITrackedFiles tracked = tracker; + tracker.AddDelete(file); + await File.WriteAllTextAsync(temp, "content"); + await File.WriteAllTextAsync(target, "verified"); + tracker.AddMove(temp, target, null, null, false, null); + + await Assert.That(tracked.Untrack(TrackedKeys.ForMove(temp))).IsTrue(); + await Assert.That(tracked.Untrack(TrackedKeys.ForDelete(file))).IsTrue(); + + await Assert.That(tracker.Moves).IsEmpty(); + await Assert.That(tracker.Deletes).IsEmpty(); + await Assert.That(File.Exists(temp)).IsTrue(); + await Assert.That(await File.ReadAllTextAsync(target)).IsEqualTo("verified"); + await Assert.That(File.Exists(file)).IsTrue(); + } + + [Test] + public async Task UntrackingSomethingUntrackedSaysSo() + { + await using var tracker = new RecordingTracker(); + ITrackedFiles tracked = tracker; + + await Assert.That(tracked.Untrack(TrackedKeys.ForMove("nothing"))).IsFalse(); + await Assert.That(tracked.Untrack("not a tracked key")).IsFalse(); + } + [Test] public async Task AnUnknownTrackedKeyIsUnknown() { diff --git a/src/DiffEngineTray/ITrackedFiles.cs b/src/DiffEngineTray/ITrackedFiles.cs index 18bc8409..72ca19a1 100644 --- a/src/DiffEngineTray/ITrackedFiles.cs +++ b/src/DiffEngineTray/ITrackedFiles.cs @@ -40,5 +40,13 @@ interface ITrackedFiles void AddDelete(string file); + /// + /// Drop a tracked move or delete without touching the file, for a test that started passing. + /// Neither nor , because both of those act on disk + /// and DiffEngine has already dealt with the file by the time this arrives. False when the key + /// was not tracked here, which is the goal state either way. + /// + bool Untrack(string key); + int DiscardAll(); } diff --git a/src/DiffEngineTray/OwnedInlineHost.cs b/src/DiffEngineTray/OwnedInlineHost.cs index 4d73ee78..4c9841da 100644 --- a/src/DiffEngineTray/OwnedInlineHost.cs +++ b/src/DiffEngineTray/OwnedInlineHost.cs @@ -174,8 +174,24 @@ int IQueueOwner.Enqueue(InlinePatch patch) return count; } + /// + /// A tracked key settles by being dropped: the pair belongs to a test that now passes, and + /// DiffEngine has already removed the received file, so there is nothing here to accept or + /// discard - only an entry that would otherwise sit on the queue describing a file that is + /// gone. + /// void IQueueOwner.Settle(string key, string? origin, string? member) { + if (TrackedKeys.IsTracked(key)) + { + if (TrackedFiles?.Untrack(key) == true) + { + Changed?.Invoke(); + } + + return; + } + lock (gate) { var settled = queue.Settle(key, origin, member); diff --git a/src/DiffEngineTray/Tracker.cs b/src/DiffEngineTray/Tracker.cs index 044f1029..877c4c65 100644 --- a/src/DiffEngineTray/Tracker.cs +++ b/src/DiffEngineTray/Tracker.cs @@ -780,6 +780,17 @@ bool ITrackedFiles.Has(string key) deletes.ContainsKey(file); } + bool ITrackedFiles.Untrack(string key) + { + if (TrackedKeys.TryStrip(key, TrackedKeys.MovePrefix, out var temp)) + { + return moves.TryRemove(temp, out _); + } + + return TrackedKeys.TryStrip(key, TrackedKeys.DeletePrefix, out var file) && + deletes.TryRemove(file, out _); + } + (bool ok, string? message) ITrackedFiles.Accept(string key) { if (TrackedKeys.TryStrip(key, TrackedKeys.MovePrefix, out var temp)) diff --git a/src/DiffEngineViewer.Tests/CommandLineTests.Attach.verified.txt b/src/DiffEngineViewer.Tests/CommandLineTests.Attach.verified.txt index 6ebda751..79180aef 100644 --- a/src/DiffEngineViewer.Tests/CommandLineTests.Attach.verified.txt +++ b/src/DiffEngineViewer.Tests/CommandLineTests.Attach.verified.txt @@ -1,5 +1,6 @@ { Mode: Inline, Attach: true, - Delete: false + Delete: false, + Diff: false } \ No newline at end of file diff --git a/src/DiffEngineViewer.Tests/CommandLineTests.Diff.verified.txt b/src/DiffEngineViewer.Tests/CommandLineTests.Diff.verified.txt new file mode 100644 index 00000000..beea6c00 --- /dev/null +++ b/src/DiffEngineViewer.Tests/CommandLineTests.Diff.verified.txt @@ -0,0 +1,8 @@ +{ + Mode: Inline, + Left: received.txt, + Right: target.txt, + Attach: false, + Delete: false, + Diff: true +} \ No newline at end of file diff --git a/src/DiffEngineViewer.Tests/CommandLineTests.Files.verified.txt b/src/DiffEngineViewer.Tests/CommandLineTests.Files.verified.txt index eb6430d9..b8a34b69 100644 --- a/src/DiffEngineViewer.Tests/CommandLineTests.Files.verified.txt +++ b/src/DiffEngineViewer.Tests/CommandLineTests.Files.verified.txt @@ -2,5 +2,6 @@ Left: left.txt, Right: right.txt, Attach: false, - Delete: false + Delete: false, + Diff: false } \ No newline at end of file diff --git a/src/DiffEngineViewer.Tests/CommandLineTests.Inline.verified.txt b/src/DiffEngineViewer.Tests/CommandLineTests.Inline.verified.txt index 7cb17c40..b2e2ff01 100644 --- a/src/DiffEngineViewer.Tests/CommandLineTests.Inline.verified.txt +++ b/src/DiffEngineViewer.Tests/CommandLineTests.Inline.verified.txt @@ -3,5 +3,6 @@ Source: Tests.cs, Line: 42, Attach: false, - Delete: false + Delete: false, + Diff: false } \ No newline at end of file diff --git a/src/DiffEngineViewer.Tests/CommandLineTests.InlineArgumentsReordered.verified.txt b/src/DiffEngineViewer.Tests/CommandLineTests.InlineArgumentsReordered.verified.txt index 7cb17c40..b2e2ff01 100644 --- a/src/DiffEngineViewer.Tests/CommandLineTests.InlineArgumentsReordered.verified.txt +++ b/src/DiffEngineViewer.Tests/CommandLineTests.InlineArgumentsReordered.verified.txt @@ -3,5 +3,6 @@ Source: Tests.cs, Line: 42, Attach: false, - Delete: false + Delete: false, + Diff: false } \ No newline at end of file diff --git a/src/DiffEngineViewer.Tests/CommandLineTests.Rejected_DiffWithoutTarget.verified.txt b/src/DiffEngineViewer.Tests/CommandLineTests.Rejected_DiffWithoutTarget.verified.txt new file mode 100644 index 00000000..9baffbee --- /dev/null +++ b/src/DiffEngineViewer.Tests/CommandLineTests.Rejected_DiffWithoutTarget.verified.txt @@ -0,0 +1 @@ +--diff takes a received file and a target. \ No newline at end of file diff --git a/src/DiffEngineViewer.Tests/CommandLineTests.cs b/src/DiffEngineViewer.Tests/CommandLineTests.cs index 7e59687e..ed324a62 100644 --- a/src/DiffEngineViewer.Tests/CommandLineTests.cs +++ b/src/DiffEngineViewer.Tests/CommandLineTests.cs @@ -16,6 +16,14 @@ public Task InlineArgumentsReordered() => public Task Attach() => Verify(CommandLine.Parse(["--attach"])); + /// + /// Two paths as well, but queue mode rather than file mode: this is the pair DiffEngine sends + /// when the viewer is the diff tool, and every later pair has to be able to join it. + /// + [Test] + public Task Diff() => + Verify(CommandLine.Parse(["--diff", "received.txt", "target.txt"])); + [Test] [Arguments("NoArguments")] [Arguments("AttachWithMore", "--attach", "--source", "Tests.cs")] @@ -27,6 +35,7 @@ public Task Attach() => [Arguments("LineIsZero", "--inline", "--source", "Tests.cs", "--line", "0")] [Arguments("UnknownArgument", "--inline", "--wat", "1")] [Arguments("MissingValue", "--inline", "--source")] + [Arguments("DiffWithoutTarget", "--diff", "received.txt")] public async Task Rejected(string name, params string[] args) { var request = CommandLine.Parse(args); diff --git a/src/DiffEngineViewer.Tests/EngineDiffTests.cs b/src/DiffEngineViewer.Tests/EngineDiffTests.cs new file mode 100644 index 00000000..337346ba --- /dev/null +++ b/src/DiffEngineViewer.Tests/EngineDiffTests.cs @@ -0,0 +1,133 @@ +extern alias engine; + +using EngineArguments = engine::DiffEngine.LaunchArguments; +using EngineLaunchResult = engine::DiffEngine.LaunchResult; +using EngineRunner = engine::DiffEngine.DiffRunner; +using EngineTool = engine::DiffEngine.DiffTool; +using EngineResolvedTool = engine::DiffEngine.ResolvedTool; + +/// +/// A failing pair whose resolved diff tool is the viewer itself, driven through DiffEngine's +/// public launch and a real socket into a real . +/// +/// The behaviour under test is that no window is launched per pair: the pair is queued with +/// whoever owns the port and a window is raised over the queue. Every other tool gets a process +/// of its own for every pair, which is what this replaces. +/// +/// +/// The tool is constructed rather than resolved, because resolution depends on a viewer being +/// installed or bundled beside the test run, and what is being covered is the route DiffEngine +/// takes once it knows the tool is the viewer. +/// +/// +[NotInParallel] +public class EngineDiffTests : + IDisposable +{ + [Test] + public async Task APairJoinsTheQueueRatherThanTakingAWindow() + { + using var scope = new EngineScope(); + var (received, target) = Pair("Sample.Test"); + + var result = await EngineRunner.LaunchAsync(Viewer(), received, target); + + await Assert.That(result).IsEqualTo(EngineLaunchResult.AlreadyRunningAndSupportsRefresh); + var entry = scope.Fixture.Host.State.Queue.Single(); + await Assert.That(entry.Kind).IsEqualTo(QueueEntryKind.Move); + await Assert.That(entry.Key).IsEqualTo(TrackedKeys.ForMove(received)); + await Assert.That(entry.LeftText).IsEqualTo("received"); + await Assert.That(entry.RightText).IsEqualTo("verified"); + // Raised over the entry that arrived, which is what a per pair window used to do by + // existing at all. + await Assert.That(scope.Fixture.Windows).IsEquivalentTo([WindowCommand.Focus]); + } + + /// + /// The whole point: the second pair is a second row, not a second window. + /// + [Test] + public async Task ASecondPairJoinsTheSameQueue() + { + using var scope = new EngineScope(); + var first = Pair("First.Test"); + var second = Pair("Second.Test"); + + await EngineRunner.LaunchAsync(Viewer(), first.Received, first.Target); + await EngineRunner.LaunchAsync(Viewer(), second.Received, second.Target); + + await Assert.That(scope.Fixture.Host.State.Queue.Select(_ => _.Key)) + .IsEquivalentTo([TrackedKeys.ForMove(first.Received), TrackedKeys.ForMove(second.Received)]); + } + + /// + /// A re-run of the same failing test stages the same received file again, and a second row for + /// it would be a duplicate rather than news. + /// + [Test] + public async Task ARepeatOfThePairReplacesIt() + { + using var scope = new EngineScope(); + var (received, target) = Pair("Sample.Test"); + + await EngineRunner.LaunchAsync(Viewer(), received, target); + await File.WriteAllTextAsync(received, "changed"); + await EngineRunner.LaunchAsync(Viewer(), received, target); + + var entry = scope.Fixture.Host.State.Queue.Single(); + await Assert.That(entry.LeftText).IsEqualTo("changed"); + } + + /// + /// Settling is what replaces killing the window for a tool that had one per pair, and it names + /// one entry: the pair beside it stays. + /// + [Test] + public async Task SettlingAPairLeavesTheRest() + { + using var scope = new EngineScope(); + var first = Pair("First.Test"); + var second = Pair("Second.Test"); + await EngineRunner.LaunchAsync(Viewer(), first.Received, first.Target); + await EngineRunner.LaunchAsync(Viewer(), second.Received, second.Target); + + var response = scope.Fixture.Send(new(ViewerVerb.Settle, TrackedKeys.ForMove(first.Received))); + + await Assert.That(response.Ok).IsTrue(); + await Assert.That(scope.Fixture.Host.State.Queue.Single().Key) + .IsEqualTo(TrackedKeys.ForMove(second.Received)); + } + + static EngineResolvedTool Viewer() => + new( + EngineTool.DiffEngineViewer.ToString(), + EngineTool.DiffEngineViewer, + // Guarded as existing, and never started: an owner answers on the port every time. + Environment.ProcessPath!, + new( + (temp, target) => $"\"{target}\" \"{temp}\"", + (temp, target) => $"\"{temp}\" \"{target}\""), + isMdi: false, + autoRefresh: false, + binaryExtensions: [], + requiresTarget: true, + supportsText: true, + useShellExecute: false); + + (string Received, string Target) Pair(string name) + { + var received = Path.Combine(directory, $"{name}.received.txt"); + var target = Path.Combine(directory, $"{name}.verified.txt"); + File.WriteAllText(received, "received"); + File.WriteAllText(target, "verified"); + return (received, target); + } + + readonly string directory = Path.Combine(Path.GetTempPath(), $"EngineDiffTests_{Guid.NewGuid():N}"); + + public EngineDiffTests() => + Directory.CreateDirectory(directory); + + public void Dispose() => + Directory.Delete(directory, true); +} diff --git a/src/DiffEngineViewer.Tests/EngineInlineTests.cs b/src/DiffEngineViewer.Tests/EngineInlineTests.cs index b022fb9c..39c2487d 100644 --- a/src/DiffEngineViewer.Tests/EngineInlineTests.cs +++ b/src/DiffEngineViewer.Tests/EngineInlineTests.cs @@ -128,38 +128,4 @@ public async Task TheOptOutDoesNotReachTheViewer() await Assert.That(result).IsEqualTo(EngineResult.NoViewerFound); await Assert.That(scope.Fixture.Host.State.Queue).IsEmpty(); } - - /// - /// Points DiffEngine at a real viewer on an ephemeral port and restores every piece of global - /// state it touches. - /// - sealed class EngineScope : IDisposable - { - readonly string? previousPort; - readonly string? previousOptOut; - readonly bool previousDisabled; - - public EngineScope(bool disabled = false, bool optOut = false) - { - Fixture = new(); - previousPort = Environment.GetEnvironmentVariable(EngineViewerClient.PortVariable); - previousOptOut = Environment.GetEnvironmentVariable(EngineRunner.InlineViewerVariable); - previousDisabled = EngineRunner.Disabled; - - Environment.SetEnvironmentVariable(EngineViewerClient.PortVariable, Fixture.Server.Port.ToString()); - Environment.SetEnvironmentVariable(EngineRunner.InlineViewerVariable, optOut ? "false" : null); - // Off by default in this process, because an AI CLI counts as disabled. - EngineRunner.Disabled = disabled; - } - - public ServerFixture Fixture { get; } - - public void Dispose() - { - Fixture.Dispose(); - EngineRunner.Disabled = previousDisabled; - Environment.SetEnvironmentVariable(EngineViewerClient.PortVariable, previousPort); - Environment.SetEnvironmentVariable(EngineRunner.InlineViewerVariable, previousOptOut); - } - } } diff --git a/src/DiffEngineViewer.Tests/EngineScope.cs b/src/DiffEngineViewer.Tests/EngineScope.cs new file mode 100644 index 00000000..39882b4b --- /dev/null +++ b/src/DiffEngineViewer.Tests/EngineScope.cs @@ -0,0 +1,46 @@ +extern alias engine; + +using EngineRunner = engine::DiffEngine.DiffRunner; +using EngineTray = engine::DiffEngine.DiffEngineTray; +using EngineViewerClient = engine::DiffEngine.ViewerClient; + +/// +/// Points DiffEngine at a real viewer on an ephemeral port and restores every piece of global +/// state it touches. +/// +sealed class EngineScope : IDisposable +{ + readonly string? previousPort; + readonly string? previousOptOut; + readonly bool previousDisabled; + readonly bool previousTray; + + public EngineScope(bool disabled = false, bool optOut = false, bool tray = false) + { + Fixture = new(); + previousPort = Environment.GetEnvironmentVariable(EngineViewerClient.PortVariable); + previousOptOut = Environment.GetEnvironmentVariable(EngineRunner.InlineViewerVariable); + previousDisabled = EngineRunner.Disabled; + previousTray = EngineTray.IsRunning; + + Environment.SetEnvironmentVariable(EngineViewerClient.PortVariable, Fixture.Server.Port.ToString()); + Environment.SetEnvironmentVariable(EngineRunner.InlineViewerVariable, optOut ? "false" : null); + // Off by default in this process, because an AI CLI counts as disabled. + EngineRunner.Disabled = disabled; + // Stated rather than detected: the answer is cached at type initialisation from a mutex + // this process does not control, so a developer with a tray in their notification area + // would otherwise route pending files down the piper port and read as a failure here. + EngineTray.IsRunning = tray; + } + + public ServerFixture Fixture { get; } + + public void Dispose() + { + Fixture.Dispose(); + EngineRunner.Disabled = previousDisabled; + EngineTray.IsRunning = previousTray; + Environment.SetEnvironmentVariable(EngineViewerClient.PortVariable, previousPort); + Environment.SetEnvironmentVariable(EngineRunner.InlineViewerVariable, previousOptOut); + } +} diff --git a/src/DiffEngineViewer.Tests/TrackedFileTests.cs b/src/DiffEngineViewer.Tests/TrackedFileTests.cs index 42a22a07..38e9bad7 100644 --- a/src/DiffEngineViewer.Tests/TrackedFileTests.cs +++ b/src/DiffEngineViewer.Tests/TrackedFileTests.cs @@ -94,6 +94,88 @@ public async Task SettlingASnapshotKeepsTheFilesPendingBesideIt() await Assert.That(settled.Queue.Single().Kind).IsEqualTo(QueueEntryKind.Delete); } + /// + /// A pair leaving because its test started passing. Settling drops it without touching disk, + /// which is what accepting and discarding both do and what neither should do here: DiffEngine + /// has already taken the received file away by the time this arrives. + /// + [Test] + public async Task SettlingAPairDropsItWithoutTouchingTheFiles() + { + var done = new List(); + var state = Owned(Fixtures.Move(), Fixtures.Delete()); + state = ViewerSession.EnqueueInline(state, Fixtures.Patch()); + + var settled = ViewerSession.Settle(state, Fixtures.Move().Key); + + await Assert.That(settled.Queue.Select(_ => _.Kind)) + .IsEquivalentTo([QueueEntryKind.Inline, QueueEntryKind.Delete]); + await Assert.That(done).IsEmpty(); + } + + [Test] + public async Task SettlingAPairThatIsNotQueuedChangesNothing() + { + var state = Owned(Fixtures.Delete()); + + var settled = ViewerSession.Settle(state, Fixtures.Move().Key); + + await Assert.That(settled).IsSameReferenceAs(state); + } + + /// + /// The keys a watch pass reports gone leave, and the rest of the queue is untouched. + /// + [Test] + public async Task RefreshDropsWhatWentAndKeepsWhatDidNot() + { + var state = Owned(Fixtures.Move(), Fixtures.Delete()); + state = ViewerSession.EnqueueInline(state, Fixtures.Patch()); + + var refreshed = ViewerSession.Refresh(state, [Fixtures.Move().Key], []); + + await Assert.That(refreshed.Queue.Select(_ => _.Kind)) + .IsEquivalentTo([QueueEntryKind.Inline, QueueEntryKind.Delete]); + } + + [Test] + public async Task RefreshReplacesAnEntryByKey() + { + var state = Owned(Fixtures.Move()); + var fresh = Fixtures.Move(left: "rewritten"); + + var refreshed = ViewerSession.Refresh(state, [], [fresh]); + + await Assert.That(refreshed.Queue.Single().LeftText).IsEqualTo("rewritten"); + } + + /// + /// A pass runs several times a second, so one that found nothing has to be free: a new state + /// every time would close the open context menu and rebuild the screen forever. + /// + [Test] + public async Task RefreshFindingNothingChangesNothing() + { + var state = Owned(Fixtures.Move()); + + await Assert.That(ViewerSession.Refresh(state, [], [])).IsSameReferenceAs(state); + await Assert.That(ViewerSession.Refresh(state, ["not queued"], [])).IsSameReferenceAs(state); + } + + /// + /// Housekeeping does not get to speak in the status line: the reader is still owed the answer + /// to whatever they last did. + /// + [Test] + public async Task RefreshKeepsTheMessage() + { + var state = Owned(Fixtures.Move(), Fixtures.Delete()) with { Message = "Accepted something" }; + + var refreshed = ViewerSession.Refresh(state, [Fixtures.Move().Key], []); + + await Assert.That(refreshed.Message).IsEqualTo("Accepted something"); + } + /// /// Worded the way an owning tray words its own sweep, so the same click reads the same /// whichever process is holding the files. diff --git a/src/DiffEngineViewer.Tests/TrackedWatchTests.cs b/src/DiffEngineViewer.Tests/TrackedWatchTests.cs new file mode 100644 index 00000000..df814ad6 --- /dev/null +++ b/src/DiffEngineViewer.Tests/TrackedWatchTests.cs @@ -0,0 +1,143 @@ +/// +/// The pass that keeps an owned queue in step with the disk, over real files, because what it is +/// for is entirely about what the file system says: an entry whose received file has gone stops +/// being pending, and one whose file was rewritten shows the rewrite. +/// +/// The rewrites here change the length as well as the content. A stamp is the write time and the +/// length, and a file system's write time granularity is coarse enough that two writes inside one +/// test can share one - so a same-length rewrite is a test that passes or fails on how fast the +/// machine is. +/// +/// +public class TrackedWatchTests : + IDisposable +{ + [Test] + public async Task AVanishedReceivedFileDropsThePair() + { + var (temp, target) = Pair("Sample.Test"); + var host = Owned(TrackedEntry.ForMove(temp, target)); + File.Delete(temp); + + new TrackedWatch(host).Pump(); + + await Assert.That(host.State.Queue).IsEmpty(); + } + + /// + /// The other side is not the same thing. A brand new snapshot has no verified file at all, and + /// offering to create it is the whole point of the entry. + /// + [Test] + public async Task AVanishedTargetKeepsThePair() + { + var (temp, target) = Pair("Sample.Test"); + var host = Owned(TrackedEntry.ForMove(temp, target)); + File.Delete(target); + + new TrackedWatch(host).Pump(); + + var entry = host.State.Queue.Single(); + await Assert.That(entry.Kind).IsEqualTo(QueueEntryKind.Move); + await Assert.That(entry.RightText).IsEmpty(); + } + + [Test] + public async Task ARewrittenReceivedFileReachesThePane() + { + var (temp, target) = Pair("Sample.Test"); + var host = Owned(TrackedEntry.ForMove(temp, target)); + await File.WriteAllTextAsync(temp, "rewritten by a later run"); + + new TrackedWatch(host).Pump(); + + await Assert.That(host.State.Queue.Single().LeftText).IsEqualTo("rewritten by a later run"); + } + + /// + /// Accepting the pair elsewhere - the tray menu, an IDE, a hand copy - creates the target, and + /// a window still offering the old empty side is describing a comparison nobody has. + /// + [Test] + public async Task ACreatedTargetReachesThePane() + { + var temp = Path.Combine(directory, "New.Test.received.txt"); + var target = Path.Combine(directory, "New.Test.verified.txt"); + await File.WriteAllTextAsync(temp, "received"); + var host = Owned(TrackedEntry.ForMove(temp, target)); + await File.WriteAllTextAsync(target, "now verified"); + + new TrackedWatch(host).Pump(); + + await Assert.That(host.State.Queue.Single().RightText).IsEqualTo("now verified"); + } + + [Test] + public async Task AVanishedDeleteFileDropsTheEntry() + { + var file = Path.Combine(directory, "Extra.verified.txt"); + await File.WriteAllTextAsync(file, "doomed"); + var host = Owned(TrackedEntry.ForDelete(file)); + File.Delete(file); + + new TrackedWatch(host).Pump(); + + await Assert.That(host.State.Queue).IsEmpty(); + } + + /// + /// The pass runs several times a second for as long as the window is up, so one that found + /// nothing has to leave the state alone rather than replace it with an equal one. + /// + [Test] + public async Task APassOverUnchangedFilesChangesNothing() + { + var (temp, target) = Pair("Sample.Test"); + var host = Owned(TrackedEntry.ForMove(temp, target)); + var before = host.State; + + new TrackedWatch(host).Pump(); + + await Assert.That(host.State).IsSameReferenceAs(before); + } + + /// + /// An inline entry has no file on disk to follow - its content came over the socket - so a + /// pass has to walk straight past it rather than reading its null paths. + /// + [Test] + public async Task InlineEntriesAreLeftAlone() + { + var (temp, target) = Pair("Sample.Test"); + var host = Owned(TrackedEntry.ForMove(temp, target)); + host.Mutate(_ => ViewerSession.EnqueueInline(_, Fixtures.Patch())); + File.Delete(temp); + + new TrackedWatch(host).Pump(); + + await Assert.That(host.State.Queue.Single().Kind).IsEqualTo(QueueEntryKind.Inline); + } + + static SessionHost Owned(QueueEntry entry) => + new( + ViewerSession.EnqueueTracked( + SessionState.Start(ViewerMode.Inline, Fixtures.Columns, Fixtures.Rows), + entry)); + + (string Temp, string Target) Pair(string name) + { + var temp = Path.Combine(directory, $"{name}.received.txt"); + var target = Path.Combine(directory, $"{name}.verified.txt"); + File.WriteAllText(temp, "received"); + File.WriteAllText(target, "verified"); + return (temp, target); + } + + readonly string directory = Path.Combine(Path.GetTempPath(), $"TrackedWatchTests_{Guid.NewGuid():N}"); + + public TrackedWatchTests() => + Directory.CreateDirectory(directory); + + public void Dispose() => + Directory.Delete(directory, true); +} diff --git a/src/DiffEngineViewer/CommandLine.cs b/src/DiffEngineViewer/CommandLine.cs index 67d9e695..ac4b1da1 100644 --- a/src/DiffEngineViewer/CommandLine.cs +++ b/src/DiffEngineViewer/CommandLine.cs @@ -4,10 +4,12 @@ static class CommandLine DiffEngineViewer DiffEngineViewer --inline --source --line DiffEngineViewer --delete + DiffEngineViewer --diff DiffEngineViewer --attach Inline mode reads the patch payload from stdin. Delete mode takes a file that a passing test no longer produces. + Diff mode takes a failing pair, and queues it rather than taking a window of its own. Attach mode reads nothing, and displays the queue of whoever owns the port. """; @@ -48,6 +50,21 @@ public static ViewerRequest Parse(IReadOnlyList args) }; } + if (args[0] == "--diff") + { + if (args.Count != 3) + { + return Error("--diff takes a received file and a target."); + } + + // Queue mode for the same reason --delete is: DiffEngine sends these one pair at a + // time, and every pair after the first has to join what is already on screen. + return new(ViewerMode.Inline, args[1], args[2], null, 0, null) + { + Diff = true + }; + } + if (args.Count != 2) { return Error($"Expected two file paths, got {args.Count} arguments."); diff --git a/src/DiffEngineViewer/TrackedWatch.cs b/src/DiffEngineViewer/TrackedWatch.cs new file mode 100644 index 00000000..d3eb6aae --- /dev/null +++ b/src/DiffEngineViewer/TrackedWatch.cs @@ -0,0 +1,157 @@ +/// +/// Keeps the tracked files a viewer owns in step with the disk: a stat per entry per pass, +/// dropping the entries whose received file has gone and re-reading the ones that changed. +/// +/// The owned counterpart of 's read seam, which has always done this for a +/// queue held elsewhere. An owned queue is only ever pushed to - a socket message or a launch +/// argument puts an entry in it and nothing ever revisits it - so its rows described the moment +/// they arrived and nothing after. That was survivable while an owning viewer only held files with +/// no tray running; it stopped being survivable when every failing pair started arriving this way. +/// +/// +/// Its own thread, like 's and for the same reason: re-reading a queue of +/// image snapshots is not work to do between two frames. +/// +/// +/// Deliberately not a file system watcher. The stat is what the attached path already pays, it +/// needs no handle per directory and no debounce, and a queue is small enough that the difference +/// is not measurable. +/// +/// +sealed class TrackedWatch(SessionHost host) +{ + /// + /// The same cadence an attached viewer reads at, so a re-run that rewrites a received file + /// reaches the pane at the same speed whichever process is holding it. + /// + public static TimeSpan Interval { get; set; } = TimeSpan.FromMilliseconds(200); + + public void Run(Cancel cancel) + { + while (!cancel.IsCancellationRequested) + { + cancel.WaitHandle.WaitOne(Interval); + if (cancel.IsCancellationRequested) + { + return; + } + + try + { + Pump(); + } + catch (Exception exception) + { + // Nothing below is expected to throw - both reads swallow their own IO failures - + // but this runs on a task nothing awaits, so an unobserved fault here would leave + // a live window quietly no longer following its files. Said out loud, and the + // queue stays usable, which is why this does not exit the way a lost owner does. + host.Mutate(_ => _ with + { + Message = $"Could not re-read the pending files: {exception.Message}" + }); + return; + } + } + } + + /// + /// One pass. Public for the tests, which drive it directly rather than waiting on a thread. + /// + public void Pump() + { + var gone = new List(); + var changed = new List(); + foreach (var entry in host.State.Queue) + { + if (entry.Kind == QueueEntryKind.Move) + { + Move(entry, gone, changed); + continue; + } + + if (entry.Kind == QueueEntryKind.Delete) + { + Delete(entry, gone, changed); + } + } + + if (gone.Count == 0 && + changed.Count == 0) + { + return; + } + + host.Mutate(_ => ViewerSession.Refresh(_, gone, changed)); + } + + static void Move(QueueEntry entry, List gone, List changed) + { + var temp = entry.LeftFile!; + var target = entry.TargetFile!; + if (FileSide.StampOf(temp) is not { } tempStamp) + { + // The received file is what the pair exists for, so its absence ends the entry. A + // target that is not there is not the same thing at all: a brand new snapshot never + // has one, and an entry offering to create it is the whole point. + gone.Add(entry.Key); + return; + } + + if (entry.LeftStamp == tempStamp && + entry.RightStamp == FileSide.StampOf(target)) + { + return; + } + + Changed( + entry, + QueueEntry.ForMove( + entry.Key, + entry.Name, + entry.Solution, + temp, + target, + FileSide.Read(temp), + FileSide.Read(target)), + changed); + } + + static void Delete(QueueEntry entry, List gone, List changed) + { + var file = entry.LeftFile!; + if (FileSide.StampOf(file) is not { } stamp) + { + // Already gone, so there is nothing left to offer to delete. + gone.Add(entry.Key); + return; + } + + if (entry.LeftStamp == stamp) + { + return; + } + + Changed( + entry, + QueueEntry.ForDelete(entry.Key, entry.Name, entry.Solution, file, FileSide.Read(file)), + changed); + } + + /// + /// Stamped again after the read rather than trusted from before it, because a file that exists + /// but cannot be opened stamps and does not read: it comes back with no stamp at all, which + /// differs from the stat every time and would rebuild and re-diff the entry on every pass for + /// as long as whatever holds the file holds it. + /// + static void Changed(QueueEntry entry, QueueEntry fresh, List changed) + { + if (fresh.LeftStamp == entry.LeftStamp && + fresh.RightStamp == entry.RightStamp) + { + return; + } + + changed.Add(fresh); + } +} diff --git a/src/DiffEngineViewer/ViewerProgram.cs b/src/DiffEngineViewer/ViewerProgram.cs index eaffab32..1f2ea096 100644 --- a/src/DiffEngineViewer/ViewerProgram.cs +++ b/src/DiffEngineViewer/ViewerProgram.cs @@ -26,6 +26,11 @@ public static int Run(string[] args, OpenWindow open) return RunDelete(request.Left!, open); } + if (request.Diff) + { + return RunDiff(request.Left!, request.Right!, open); + } + if (request.Mode == ViewerMode.Inline) { return RunInline(open); @@ -115,6 +120,44 @@ static int RunDelete(string file, OpenWindow open) } } + /// + /// One failing pair, owning the queue so more can join it. + /// + /// Launched by DiffEngine when the diff tool it resolved for the pair is this viewer and + /// nothing answered on the port. Tracked as a pending move, which is what the pair already + /// is everywhere else: the same entry a tray's move produces, accepted by putting the + /// received file where the target is. + /// + /// + /// Deliberately not , which owns no port and so cannot be joined. + /// That mode is what a hand run DiffEngineViewer left right still gets, where a queue + /// nothing else can add to is the whole intent. + /// + /// + static int RunDiff(string temp, string target, OpenWindow open) + { + var port = ViewerClient.Port; + if (!ViewerServer.TryBind(port, out var server)) + { + if (!ViewerClient.TrySend(new(ViewerVerb.Diff, temp, target), out var response, port) || + !response.Ok) + { + Console.Error.WriteLine("A viewer holds the port but did not accept the pair."); + return 1; + } + + return 0; + } + + using (server) + { + var start = ViewerSession.EnqueueTracked( + SessionState.Start(ViewerMode.Inline), + TrackedEntry.ForMove(temp, target)); + return Run(new(start), server, null, open); + } + } + /// /// Display only: the queue belongs to whoever holds the port, and this process just draws it /// and forwards commands. Launched this way by DiffEngineTray, which owns the queue itself and @@ -180,6 +223,11 @@ static int Run(SessionHost host, ViewerServer? server, OwnerLink? link, OpenWind var polling = link is null ? null : Task.Run(() => link.Run(cancel.Token), Cancel.None); + // Only for a queue this process owns. A displayed one is re-read by OwnerLink already, and + // its files belong to the owner, which is what decides when an entry stops being pending. + var watching = server is null + ? null + : Task.Run(() => new TrackedWatch(host).Run(cancel.Token), Cancel.None); using (window) { @@ -191,6 +239,7 @@ static int Run(SessionHost host, ViewerServer? server, OwnerLink? link, OpenWind { listening?.Wait(TimeSpan.FromSeconds(2)); polling?.Wait(TimeSpan.FromSeconds(2)); + watching?.Wait(TimeSpan.FromSeconds(2)); } catch (AggregateException) { diff --git a/src/DiffEngineViewer/ViewerRequest.cs b/src/DiffEngineViewer/ViewerRequest.cs index 68614fe2..fdf42249 100644 --- a/src/DiffEngineViewer/ViewerRequest.cs +++ b/src/DiffEngineViewer/ViewerRequest.cs @@ -21,4 +21,12 @@ record ViewerRequest( /// only surface it can have. /// public bool Delete { get; init; } + + /// + /// Own the queue seeded with one pending move, from to . + /// From DiffEngine when the diff tool it resolved for a pair is this viewer: the pair joins + /// the queue beside everything else rather than taking a window of its own, which is what + /// would give it. + /// + public bool Diff { get; init; } } diff --git a/src/DiffEngineViewer/ViewerSession.cs b/src/DiffEngineViewer/ViewerSession.cs index f9f7abac..ae082001 100644 --- a/src/DiffEngineViewer/ViewerSession.cs +++ b/src/DiffEngineViewer/ViewerSession.cs @@ -82,6 +82,20 @@ public static SessionState Settle(SessionState state, string key, string? origin return state; } + // A tracked key settles by being dropped. The pair is a test that now passes, so + // DiffEngine has taken the received file away already and neither accepting nor + // discarding this has anything left to act on - both would fail on a file that is gone. + if (TrackedKeys.IsTracked(key)) + { + var kept = state.Queue.Where(_ => _.Key != key).ToList(); + if (kept.Count == state.Queue.Count) + { + return state; + } + + return Remove(state, kept, null); + } + var pending = Pending(state); var settled = pending.Settle(key, origin, member); if (ReferenceEquals(settled, pending)) @@ -148,6 +162,60 @@ public static SessionState Sync( }); } + /// + /// A pass over the tracked files this process owns: entries whose file has gone drop out, and + /// entries whose file changed underneath the window are replaced by the re-read one. + /// + /// The owning half of what does for a displaying viewer. An attached one + /// re-reads the owner's files on every pump and so has always followed them; an owned queue is + /// only ever pushed to, so its rows stayed frozen at the moment they arrived - showing content + /// a re-run had already replaced, and offering a received file that was no longer there. + /// + /// + /// Both arguments name keys, and anything they name that is no longer queued is skipped: the + /// read that produced them ran outside the lock, so a patch or a pair can have arrived since. + /// A pass that changes nothing returns the same state, because this runs several times a + /// second and rebuilding the queue - or clearing the open menu - on every one of them is not + /// housekeeping the reader should be able to feel. + /// + /// + public static SessionState Refresh( + SessionState state, + IReadOnlyCollection gone, + IReadOnlyList changed) + { + var replacements = changed.ToDictionary(_ => _.Key); + var queue = new List(state.Queue.Count); + var any = false; + foreach (var entry in state.Queue) + { + if (gone.Contains(entry.Key)) + { + any = true; + continue; + } + + if (replacements.TryGetValue(entry.Key, out var fresh)) + { + any = true; + queue.Add(fresh); + continue; + } + + queue.Add(entry); + } + + if (!any) + { + return state; + } + + // The message is carried rather than cleared, unlike every other path through Remove: this + // is not something the reader did, and "Accepted Foo" disappearing because an unrelated + // file went away reads as the accept having been undone. + return Remove(state, queue, state.Message); + } + /// /// Selects by key rather than index, for a queue owner asking that a particular item be the /// one on screen. A key that is not here leaves the selection alone, because a listing and the From f4ee7e2449cc92e909d3858a0745afe8a56b564b Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sun, 23 Aug 2026 22:27:10 +1000 Subject: [PATCH 2/6] Pin the relaunch arguments the tray stores A pair whose diff tool is the viewer goes to a running tray as an ordinary move, carrying the exe and arguments the tray re-runs for "Open diff tool". Those arguments now say --diff, so the relaunch joins the queue the pair is already in rather than opening a window of its own beside it - which is the arrangement the whole route exists to remove. Nothing covered that, and it is a string built in one assembly and parsed in another, which is the shape that rots quietly. Asserted by putting it back through both parsers it actually has to survive: CommandLineToArgvW, which is what Windows will split it with, and then the viewer's own CommandLine. The received file is staged under a directory with a space in its name, because that is the case where the quoting has to be right rather than merely present. The move is also pinned as not killable and carrying no process id. There is no window of its own to kill, and the one it is drawn in is holding every other pending pair, so a tray that killed it on accept would take them all. --- .../DiffRunnerViewerMoveTest.cs | 177 ++++++++++++++++++ 1 file changed, 177 insertions(+) create mode 100644 src/DiffEngineTray.Tests/DiffRunnerViewerMoveTest.cs diff --git a/src/DiffEngineTray.Tests/DiffRunnerViewerMoveTest.cs b/src/DiffEngineTray.Tests/DiffRunnerViewerMoveTest.cs new file mode 100644 index 00000000..751a9a27 --- /dev/null +++ b/src/DiffEngineTray.Tests/DiffRunnerViewerMoveTest.cs @@ -0,0 +1,177 @@ +extern alias viewer; + +using System.Runtime.InteropServices; + +using ViewerCommandLine = viewer::CommandLine; +using ViewerMode = viewer::ViewerMode; + +#pragma warning disable CS0618 // DiffEngineTray is obsolete; the test drives it directly to enable the send path. + +/// +/// What a tray is told about a pair whose diff tool is the viewer itself. +/// +/// The tray stores the exe and the arguments against the tracked move and re-runs them for "Open +/// diff tool", so they have to name the queue. The plain two path form would open a window of its +/// own beside the one the pair is already in, which is the arrangement this whole route exists to +/// remove. +/// +/// +/// Parsed back through Windows' own splitter and then the viewer's own command line, rather than +/// compared against a string, because those two are what the arguments actually have to survive - +/// and a received file staged under a path with a space in it is the case where surviving them is +/// not free. The tray is Windows only, so CommandLineToArgvW is the splitter that will be used. +/// +/// +/// The move must also not be killable: there is no window of its own to kill, and the one it is +/// drawn in is holding every other pending pair. +/// +/// +public class DiffRunnerViewerMoveTest : + IDisposable +{ + [Test] + public async Task A_sync_launch_tells_the_tray_how_to_reopen_the_queue() => + await AssertReopens(await CaptureMove(() => Task.FromResult(DiffRunner.Launch(Viewer(), temp, target)))); + + [Test] + public async Task An_async_launch_tells_the_tray_how_to_reopen_the_queue() => + await AssertReopens(await CaptureMove(() => DiffRunner.LaunchAsync(Viewer(), temp, target))); + + async Task AssertReopens(MovePayload received) + { + await Assert.That(received.Temp).IsEqualTo(temp); + await Assert.That(received.Target).IsEqualTo(target); + await Assert.That(received.Exe).IsEqualTo(Environment.ProcessPath); + await Assert.That(received.CanKill).IsFalse(); + await Assert.That(received.ProcessId).IsNull(); + + var request = ViewerCommandLine.Parse(Split(received.Arguments!)); + await Assert.That(request.Error).IsNull(); + await Assert.That(request.Diff).IsTrue(); + // Queue mode, which is the whole point of the relaunch naming --diff. + await Assert.That(request.Mode).IsEqualTo(ViewerMode.Inline); + await Assert.That(request.Left).IsEqualTo(temp); + await Assert.That(request.Right).IsEqualTo(target); + } + + static async Task CaptureMove(Func> launch) + { + MovePayload? received = null; + var source = new CancelSource(); + var server = PiperServer.Start(move => received = move, _ => { }, source.Token); + try + { + var result = await launch(); + // The tray took it, so nothing was launched and no window was opened for the pair. + await Assert.That(result).IsEqualTo(LaunchResult.AlreadyRunningAndSupportsRefresh); + + for (var i = 0; received == null && i < 50; i++) + { + await Task.Delay(100, source.Token); + } + } + finally + { + await source.CancelAsync(); + await server; + } + + await Assert.That(received).IsNotNull(); + return received!; + } + + static ResolvedTool Viewer() => + new( + name: DiffTool.DiffEngineViewer.ToString(), + tool: DiffTool.DiffEngineViewer, + // Guarded as existing, and never started: the tray takes the move, so nothing here + // reaches a launch. + exePath: Environment.ProcessPath!, + launchArguments: new( + Left: (t, target) => $"\"{target}\" \"{t}\"", + Right: (t, target) => $"\"{t}\" \"{target}\""), + isMdi: false, + autoRefresh: false, + binaryExtensions: [], + requiresTarget: false, + supportsText: true, + useShellExecute: false); + + /// + /// The splitter the OS will use on this string, so the quoting is asserted as Windows reads it + /// rather than as this test would like to read it. + /// + static string[] Split(string arguments) + { + var pointer = CommandLineToArgvW("exe " + arguments, out var count); + if (pointer == IntPtr.Zero) + { + throw new("Could not split the arguments."); + } + + try + { + var split = new string[count]; + for (var index = 0; index < count; index++) + { + split[index] = Marshal.PtrToStringUni(Marshal.ReadIntPtr(pointer, index * IntPtr.Size))!; + } + + // The exe stands in for argv[0], which the process would consume. + return split[1..]; + } + finally + { + LocalFree(pointer); + } + } + + [DllImport("shell32.dll", SetLastError = true, CharSet = CharSet.Unicode)] + static extern IntPtr CommandLineToArgvW(string commandLine, out int count); + + [DllImport("kernel32.dll")] + static extern IntPtr LocalFree(IntPtr handle); + + static int GetFreePort() + { + var probe = new TcpListener(IPAddress.Loopback, 0); + probe.Start(); + try + { + return ((IPEndPoint) probe.LocalEndpoint).Port; + } + finally + { + probe.Stop(); + } + } + + // A space in the path, because that is where the quoting the tray stores has to earn itself. + readonly string directory = Path.Combine(Path.GetTempPath(), $"Viewer Move {Guid.NewGuid():N}"); + readonly string temp; + readonly string target; + readonly bool originalDisabled = DiffRunner.Disabled; + readonly string? originalViewerPort = Environment.GetEnvironmentVariable("DiffEngine_ViewerPort"); + + public DiffRunnerViewerMoveTest() + { + Directory.CreateDirectory(directory); + temp = Path.Combine(directory, "Sample.Test.received.txt"); + target = Path.Combine(directory, "Sample.Test.verified.txt"); + File.WriteAllText(temp, "received"); + PiperClient.Port = GetFreePort(); + // The route sends a focus to whoever owns the queue after the tray has taken the move. + // Pointed at a free port so a live viewer on the machine running these is not raised. + Environment.SetEnvironmentVariable("DiffEngine_ViewerPort", GetFreePort().ToString()); + DiffEngine.DiffEngineTray.IsRunning = true; + DiffRunner.Disabled = false; + } + + public void Dispose() + { + DiffEngine.DiffEngineTray.IsRunning = false; + DiffRunner.Disabled = originalDisabled; + Environment.SetEnvironmentVariable("DiffEngine_ViewerPort", originalViewerPort); + Directory.Delete(directory, true); + } +} From c7e4418e5e5b14b406dff99e79c685b972a48cd1 Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sun, 23 Aug 2026 22:38:00 +1000 Subject: [PATCH 3/6] Cover the settle a passing pair sends, and stop the table lying DiffRunner.Kill's viewer branch had nothing on it. What it does - address the queue owner rather than a process, and drop the row rather than acting on a file that has already gone - now lives beside the launch half in PendingFiles.SettleDiff, where a test can drive it over a real socket. The dispatch in Kill stays uncovered, because tool resolution cannot be steered to a DiffTool identity from a test: the public AddTool overloads all register with a null one. It is the same predicate the launch side branches on two lines away. The refusal fallback was reasoned about and never run. An owner that answers and says no is one too old to know the verb, and the pair goes over as a plain move instead - a row with nothing raised over it. Both the sync and async routes are pinned now, which matters because they take different client calls to tell a refusal from an absence. The auto-refresh and MDI table describes what DiffEngine does with a tool it has launched before: relaunch it, kill it, or leave it to refresh itself. None of those happen for the viewer any more, so a reader working out what to expect from "Is MDI: False" was being told the wrong thing. Said on the tool and once under the table. ITrackedFiles said a viewer that owns the queue never has tracked files, which stopped being true when moves and deletes started going to the queue owner with no tray running, and is now how every failing pair arrives. --- docs/diff-tool.md | 6 + docs/mdsource/diff-tool.source.md | 2 + src/DiffEngine.Tests/PendingFilesDiffTests.cs | 204 ++++++++++++++++++ src/DiffEngine.Tests/diffTools.include.md | 4 + src/DiffEngine/DiffRunner_Kill.cs | 2 +- .../Implementation/DiffEngineViewer.cs | 4 + src/DiffEngine/Tray/PendingFiles.cs | 16 ++ src/DiffEngineTray/ITrackedFiles.cs | 5 +- 8 files changed, 240 insertions(+), 3 deletions(-) create mode 100644 src/DiffEngine.Tests/PendingFilesDiffTests.cs diff --git a/docs/diff-tool.md b/docs/diff-tool.md index 24163de9..112a4d4c 100644 --- a/docs/diff-tool.md +++ b/docs/diff-tool.md @@ -39,6 +39,8 @@ If a tool is running for the comparison of the current verification (per test), | false | true | Open new instance. Previous instance must be manually closed | | false | false | Kill current and open new instance | +The bundled [DiffEngineViewer](/docs/viewer.md) is the exception: it queues every failing pair into one window, so none of the four rows describes it. + This allows, in most cases, for no manual closing of the tool to be required. @@ -284,6 +286,10 @@ DiffTools.UseOrder(DiffTool.DiffEngineViewer); #### Notes: + * The one tool DiffEngine does not open per pair. Every failing pair joins one + window, so the auto-refresh and MDI table above does not describe it: nothing + is relaunched, nothing is killed, and a test that starts passing has its entry + dropped instead * Bundled inside the DiffEngine package, so it needs no install * Also available standalone as `DiffEngineViewer.Windows`, `.Mac` or `.Linux` * Renders natively per platform: WinForms on Windows, AppKit and Core Text on diff --git a/docs/mdsource/diff-tool.source.md b/docs/mdsource/diff-tool.source.md index adf051b1..0c0e17a3 100644 --- a/docs/mdsource/diff-tool.source.md +++ b/docs/mdsource/diff-tool.source.md @@ -32,6 +32,8 @@ If a tool is running for the comparison of the current verification (per test), | false | true | Open new instance. Previous instance must be manually closed | | false | false | Kill current and open new instance | +The bundled [DiffEngineViewer](/docs/viewer.md) is the exception: it queues every failing pair into one window, so none of the four rows describes it. + include: diffToolCleanup diff --git a/src/DiffEngine.Tests/PendingFilesDiffTests.cs b/src/DiffEngine.Tests/PendingFilesDiffTests.cs new file mode 100644 index 00000000..ca19ae58 --- /dev/null +++ b/src/DiffEngine.Tests/PendingFilesDiffTests.cs @@ -0,0 +1,204 @@ +// DiffEngineTray is the obsolete public shim, but its IsRunning is still where the tray check +// lives, and these tests have to hold it down. +#pragma warning disable CS0618 + +/// +/// The route a pair takes when the diff tool resolved for it is the viewer itself: queued with +/// whoever owns the queue rather than given a process and a window of its own. +/// +/// Driven over a real socket against a server that only records, so what is asserted is the wire +/// - which is all DiffEngine controls. What an owner then does with a Diff is the viewer's +/// half and is covered where the viewer's own handler is. +/// +/// +[NotInParallel] +public class PendingFilesDiffTests +{ + [Test] + public async Task ADiffReachesTheOwnerWithBothPaths() + { + using var owner = new Recording(); + + var result = await PendingFiles.AddDiffAsync(Temp, Target, Exe, Cancel.None); + + await Assert.That(result).IsEqualTo(LaunchResult.AlreadyRunningAndSupportsRefresh); + await Assert.That(owner.Heard).IsEquivalentTo([$"{ViewerVerb.Diff}:{Temp}:{Target}"]); + } + + [Test] + public async Task ASyncDiffReachesTheOwnerToo() + { + using var owner = new Recording(); + + var result = PendingFiles.AddDiff(Temp, Target, Exe); + + await Assert.That(result).IsEqualTo(LaunchResult.AlreadyRunningAndSupportsRefresh); + await Assert.That(owner.Heard).IsEquivalentTo([$"{ViewerVerb.Diff}:{Temp}:{Target}"]); + } + + /// + /// An owner that answers and says no is one too old to know the verb. Launching a second + /// viewer cannot change that answer and would bind nothing, so the pair goes over as a plain + /// move: a row with nothing raised over it, which every owner has always understood. + /// + [Test] + public async Task ARefusedDiffFallsBackToAPlainMove() + { + using var owner = new Recording {Refuse = ViewerVerb.Diff}; + + var result = await PendingFiles.AddDiffAsync(Temp, Target, Exe, Cancel.None); + + await Assert.That(result).IsEqualTo(LaunchResult.AlreadyRunningAndSupportsRefresh); + await Assert.That(owner.Heard).IsEquivalentTo( + [ + $"{ViewerVerb.Diff}:{Temp}:{Target}", + $"{ViewerVerb.Move}:{Temp}:{Target}" + ]); + } + + [Test] + public async Task ASyncRefusedDiffFallsBackToAPlainMove() + { + using var owner = new Recording {Refuse = ViewerVerb.Diff}; + + PendingFiles.AddDiff(Temp, Target, Exe); + + await Assert.That(owner.Heard).IsEquivalentTo( + [ + $"{ViewerVerb.Diff}:{Temp}:{Target}", + $"{ViewerVerb.Move}:{Temp}:{Target}" + ]); + } + + /// + /// The other end: the pair's test started passing, so the row it took goes. A settle rather + /// than a kill, because there is no process of its own to kill, and rather than a discard, + /// because the received file a discard would delete is one DiffEngine has already removed. + /// + [Test] + public async Task SettlingSendsTheMoveKey() + { + using var owner = new Recording(); + + PendingFiles.SettleDiff(Temp); + + await Assert.That(owner.Heard).IsEquivalentTo([$"{ViewerVerb.Settle}:{TrackedKeys.ForMove(Temp)}:"]); + } + + /// + /// Nobody owning the queue means no row to drop, which is the goal state already. Silent + /// rather than reported, the same bargain a pending delete with no surface makes. + /// + [Test] + public async Task SettlingWithNoOwnerIsSilent() + { + using var absent = new NoOwner(); + + await Assert.That(() => PendingFiles.SettleDiff(Temp)).ThrowsNothing(); + } + + const string Temp = @"c:\temp\Sample.Test.received.png"; + const string Target = @"c:\code\Sample.Test.verified.png"; + const string Exe = @"c:\tools\DiffEngineViewer.exe"; + + /// + /// A queue owner that only writes down what it was asked, so the assertions are about the + /// wire rather than about anything a real owner would go on to do. + /// + sealed class Recording : + IDisposable + { + readonly ViewerServer server; + readonly CancelSource cancel = new(); + readonly Task listening; + readonly string? previousPort; + readonly bool previousRunning; + + public List Heard { get; } = []; + + /// + /// The verb this owner is too old to understand. + /// + public ViewerVerb? Refuse { get; init; } + + public Recording() + { + if (!ViewerServer.TryBind(0, out var bound)) + { + throw new("Could not bind an ephemeral port."); + } + + server = bound; + previousPort = Environment.GetEnvironmentVariable(ViewerClient.PortVariable); + previousRunning = DiffEngineTray.IsRunning; + Environment.SetEnvironmentVariable(ViewerClient.PortVariable, server.Port.ToString()); + // No tray, so the queue owner is where a pending file goes. + DiffEngineTray.IsRunning = false; + listening = server.Listen( + message => + { + lock (Heard) + { + Heard.Add($"{message.Verb}:{message.Key}:{message.Body}"); + } + + if (message.Verb == Refuse) + { + return ViewerResponse.Error($"Unsupported verb: {message.Verb}"); + } + + return ViewerResponse.Success(); + }, + cancel.Token); + } + + public void Dispose() + { + Environment.SetEnvironmentVariable(ViewerClient.PortVariable, previousPort); + DiffEngineTray.IsRunning = previousRunning; + cancel.Cancel(); + server.Dispose(); + try + { + listening.Wait(TimeSpan.FromSeconds(5)); + } + catch (AggregateException) + { + // Cancellation unwinds through the listener; nothing to report. + } + + cancel.Dispose(); + } + } + + /// + /// A port that was free and was let go, so nothing can answer on it. + /// + sealed class NoOwner : + IDisposable + { + readonly string? previousPort; + readonly bool previousRunning; + + public NoOwner() + { + if (!ViewerServer.TryBind(0, out var bound)) + { + throw new("Could not bind an ephemeral port."); + } + + var port = bound.Port; + bound.Dispose(); + previousPort = Environment.GetEnvironmentVariable(ViewerClient.PortVariable); + previousRunning = DiffEngineTray.IsRunning; + Environment.SetEnvironmentVariable(ViewerClient.PortVariable, port.ToString()); + DiffEngineTray.IsRunning = false; + } + + public void Dispose() + { + Environment.SetEnvironmentVariable(ViewerClient.PortVariable, previousPort); + DiffEngineTray.IsRunning = previousRunning; + } + } +} diff --git a/src/DiffEngine.Tests/diffTools.include.md b/src/DiffEngine.Tests/diffTools.include.md index e681b6c4..d26b4263 100644 --- a/src/DiffEngine.Tests/diffTools.include.md +++ b/src/DiffEngine.Tests/diffTools.include.md @@ -149,6 +149,10 @@ DiffTools.UseOrder(DiffTool.DiffEngineViewer); #### Notes: + * The one tool DiffEngine does not open per pair. Every failing pair joins one + window, so the auto-refresh and MDI table above does not describe it: nothing + is relaunched, nothing is killed, and a test that starts passing has its entry + dropped instead * Bundled inside the DiffEngine package, so it needs no install * Also available standalone as `DiffEngineViewer.Windows`, `.Mac` or `.Linux` * Renders natively per platform: WinForms on Windows, AppKit and Core Text on diff --git a/src/DiffEngine/DiffRunner_Kill.cs b/src/DiffEngine/DiffRunner_Kill.cs index 05fcfd11..23875fce 100644 --- a/src/DiffEngine/DiffRunner_Kill.cs +++ b/src/DiffEngine/DiffRunner_Kill.cs @@ -28,7 +28,7 @@ public static void Kill(string tempFile, string targetFile) // what killing the window meant for a tool that had one per pair. if (PendingFiles.IsViewer(diffTool)) { - ViewerClient.TrySend(new(ViewerVerb.Settle, TrackedKeys.ForMove(tempFile))); + PendingFiles.SettleDiff(tempFile); return; } diff --git a/src/DiffEngine/Implementation/DiffEngineViewer.cs b/src/DiffEngine/Implementation/DiffEngineViewer.cs index 88627735..d05aff81 100644 --- a/src/DiffEngine/Implementation/DiffEngineViewer.cs +++ b/src/DiffEngine/Implementation/DiffEngineViewer.cs @@ -32,6 +32,10 @@ public static Definition DiffEngineViewer() // Console subsystem, so without this a window flashes on every launch. CreateNoWindow: true, Notes: """ + * The one tool DiffEngine does not open per pair. Every failing pair joins one + window, so the auto-refresh and MDI table above does not describe it: nothing + is relaunched, nothing is killed, and a test that starts passing has its entry + dropped instead * Bundled inside the DiffEngine package, so it needs no install * Also available standalone as `DiffEngineViewer.Windows`, `.Mac` or `.Linux` * Renders natively per platform: WinForms on Windows, AppKit and Core Text on diff --git a/src/DiffEngine/Tray/PendingFiles.cs b/src/DiffEngine/Tray/PendingFiles.cs index f0cc6170..77077ff6 100644 --- a/src/DiffEngine/Tray/PendingFiles.cs +++ b/src/DiffEngine/Tray/PendingFiles.cs @@ -144,6 +144,22 @@ await PiperClient.SendMoveAsync(tempFile, targetFile, exe, ViewerLauncher.DiffAr : LaunchResult.NoDiffToolFound; } + /// + /// The other end of : the pair's test started passing, so the row it + /// took goes. + /// + /// A settle rather than a kill, because there is no process of its own to kill and the window + /// it is drawn in holds every other pending pair. And rather than a discard, because the + /// received file a discard would delete is one DiffEngine has already removed. + /// + /// + /// Silent when nobody answers, the same bargain a pending file with no surface makes: no + /// owner means no row, which is the state this was asking for. + /// + /// + public static void SettleDiff(string tempFile) => + ViewerClient.TrySend(new(ViewerVerb.Settle, TrackedKeys.ForMove(tempFile))); + /// /// Whether a pending file should take the route rather than the plain /// tracking one, which is exactly whether the tool that would have opened a window for it is diff --git a/src/DiffEngineTray/ITrackedFiles.cs b/src/DiffEngineTray/ITrackedFiles.cs index 72ca19a1..616496f9 100644 --- a/src/DiffEngineTray/ITrackedFiles.cs +++ b/src/DiffEngineTray/ITrackedFiles.cs @@ -1,8 +1,9 @@ /// /// The tray's tracked moves and deletes, as the inline queue owner reaches them to answer the /// wire: listed into a full listing, and accepted or discarded by their prefixed keys. Tray only — -/// a viewer that owns the queue never has any, because DiffEngine only sends moves and deletes to -/// a running tray. +/// the interface a tray owner reaches its own tracker through. A viewer that owns the queue holds +/// the equivalent entries in its session instead, which is where DiffEngine's moves, deletes and +/// pairs go when no tray is running. /// /// Everything here can run on a listener thread, so nothing behind it may raise UI: a locked move /// is refused with a message pointing at the tray menu instead of prompting. From 8ba0ca5c5202645f22a7f4f0251252837409e23f Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sun, 23 Aug 2026 23:04:19 +1000 Subject: [PATCH 4/6] Start one viewer for a run, not one per failing snapshot Nothing owns the port at the start of a failing run, so every caller that finds it free is entitled to start a viewer, and a parallel run reaches that point once per failing snapshot. Twenty pairs failing at once measured as twenty processes: one bound the port, and the other nineteen handed their work over and exited. The queue came out right - the racing resolution is what makes it right - but it was twenty process starts to open one window, and every one of them reported StartedNewInstance when there was one instance. MaxInstance used to hold this down to five, and stopped applying when the viewer stopped opening a window per pair, so nothing was capping it any more. The gate replaces it for this tool: the first caller through starts a viewer and holds the gate until that viewer answers, and everyone behind it finds an owner and never launches. Fifty pairs failing at once now measures as one process and one StartedNewInstance, against fifty of each before. What the gate holds is the decision, not the work. Inside it is a connect that asks whether anyone is there; the send that hands the payload over happens outside, so the callers that find an owner still reach it at once. Sending inside instead turned twenty process starts into nineteen serialised round trips, which measured slower than the problem. The inline and delete routes go through it too. The race is not new there and was never as visible - a run failing twenty inline snapshots has always started twenty viewers - but it is the same race, and leaving one of the three unfixed would only mean rediscovering it. The gate's ownership probe is a parameter rather than a call into ViewerClient, because the alternative is a test that arranges twenty concurrent connects against a port being bound underneath them and then reads the answer out of the operating system's timing. That test existed for a while and was not reliable. One test still uses the real probe against a real bound port, so the default the call sites take is not only ever exercised through a stand-in. --- src/DiffEngine.Tests/ViewerLaunchGateTests.cs | 193 ++++++++++++++++++ src/DiffEngine/DiffRunner_Inline.cs | 9 +- src/DiffEngine/Protocol/ViewerClient.cs | 19 ++ src/DiffEngine/Tray/PendingFiles.cs | 37 +++- src/DiffEngine/Viewer/ViewerLaunchGate.cs | 183 +++++++++++++++++ 5 files changed, 431 insertions(+), 10 deletions(-) create mode 100644 src/DiffEngine.Tests/ViewerLaunchGateTests.cs create mode 100644 src/DiffEngine/Viewer/ViewerLaunchGate.cs diff --git a/src/DiffEngine.Tests/ViewerLaunchGateTests.cs b/src/DiffEngine.Tests/ViewerLaunchGateTests.cs new file mode 100644 index 00000000..0abf47ed --- /dev/null +++ b/src/DiffEngine.Tests/ViewerLaunchGateTests.cs @@ -0,0 +1,193 @@ +/// +/// The gate that keeps a parallel run from starting a viewer per failing snapshot. +/// +/// The ownership probe is supplied rather than the real one, so what is asserted is the gate's own +/// decision making. Arranging the real thing means twenty concurrent connects to a port nothing is +/// listening on, and then reading the answer back out of the operating system's timing; the one +/// test below that does use it is the one where the answer is not in doubt. +/// +/// +[NotInParallel] +public class ViewerLaunchGateTests +{ + /// + /// The whole point: twenty callers, one viewer. The nineteen behind the first find the queue + /// owned and hand their work over instead of starting anything. + /// + [Test] + public async Task ManyCallersAtOnceLaunchOnce() + { + var viewer = new FakeViewer(); + + var outcomes = await Task.WhenAll( + Enumerable.Range(0, 20) + .Select(_ => Task.Run(() => ViewerLaunchGate.Launch( + retry: () => true, + launch: viewer.Start, + isOwned: viewer.IsUp)))); + + await Assert.That(viewer.Starts).IsEqualTo(1); + await Assert.That(outcomes.Count(_ => _ == ViewerLaunchOutcome.Launched)).IsEqualTo(1); + await Assert.That(outcomes.Count(_ => _ == ViewerLaunchOutcome.Taken)).IsEqualTo(19); + } + + /// + /// A viewer takes most of a second to bind, so the gate has to be held across that wait. A + /// gate let go the moment the process started would let the next caller in while the port was + /// still free, and start another. + /// + [Test] + public async Task TheGateIsHeldUntilTheLaunchedViewerAnswers() + { + var viewer = new FakeViewer {BindDelay = TimeSpan.FromMilliseconds(300)}; + + var outcomes = await Task.WhenAll( + Enumerable.Range(0, 10) + .Select(_ => Task.Run(() => ViewerLaunchGate.Launch( + retry: () => true, + launch: viewer.Start, + isOwned: viewer.IsUp)))); + + await Assert.That(viewer.Starts).IsEqualTo(1); + await Assert.That(outcomes.Count(_ => _ == ViewerLaunchOutcome.Taken)).IsEqualTo(9); + } + + /// + /// One that never answers costs the caller holding the gate the wait, and then the next caller + /// is free to try again rather than the queue being stuck behind a viewer that is not there. + /// + [Test] + public async Task AViewerThatNeverAnswersDoesNotHoldTheGateForever() + { + var previous = ViewerLaunchGate.BindWait; + ViewerLaunchGate.BindWait = TimeSpan.FromMilliseconds(200); + try + { + var starts = 0; + + var outcomes = await Task.WhenAll( + Enumerable.Range(0, 3) + .Select(_ => Task.Run(() => ViewerLaunchGate.Launch( + retry: () => true, + launch: () => + { + Interlocked.Increment(ref starts); + return true; + }, + isOwned: () => false)))); + + await Assert.That(starts).IsEqualTo(3); + await Assert.That(outcomes.All(_ => _ == ViewerLaunchOutcome.Launched)).IsTrue(); + } + finally + { + ViewerLaunchGate.BindWait = previous; + } + } + + [Test] + public async Task ALaunchThatCouldNotStartIsReportedRatherThanWaitedOn() + { + var previous = ViewerLaunchGate.BindWait; + // Long enough that waiting on it would show in this test's duration. + ViewerLaunchGate.BindWait = TimeSpan.FromSeconds(30); + try + { + var outcome = ViewerLaunchGate.Launch( + retry: () => true, + launch: () => false, + isOwned: () => false); + + await Assert.That(outcome).IsEqualTo(ViewerLaunchOutcome.Failed); + } + finally + { + ViewerLaunchGate.BindWait = previous; + } + } + + /// + /// An owner that is there and refuses the payload is not answered by launching another, which + /// would bind nothing and be refused in its turn. + /// + [Test] + public async Task ARefusingOwnerIsNotLaunchedOver() + { + var launches = 0; + + var outcome = await ViewerLaunchGate.LaunchAsync( + retry: () => Task.FromResult(false), + launch: () => + { + launches++; + return Task.FromResult(true); + }, + Cancel.None, + isOwned: () => true); + + await Assert.That(outcome).IsEqualTo(ViewerLaunchOutcome.Failed); + await Assert.That(launches).IsEqualTo(0); + } + + /// + /// The real probe, against a real bound port, so the default the call sites rely on is not + /// only ever exercised through a stand-in. + /// + [Test] + public async Task TheDefaultProbeReadsTheRealPort() + { + var previousPort = Environment.GetEnvironmentVariable(ViewerClient.PortVariable); + try + { + if (!ViewerServer.TryBind(0, out var bound)) + { + throw new("Could not bind an ephemeral port."); + } + + using var server = bound; + Environment.SetEnvironmentVariable(ViewerClient.PortVariable, server.Port.ToString()); + var launches = 0; + + var outcome = ViewerLaunchGate.Launch( + retry: () => true, + launch: () => + { + launches++; + return true; + }); + + await Assert.That(outcome).IsEqualTo(ViewerLaunchOutcome.Taken); + await Assert.That(launches).IsEqualTo(0); + } + finally + { + Environment.SetEnvironmentVariable(ViewerClient.PortVariable, previousPort); + } + } + + /// + /// Stands in for the process a launch starts: not up until it has been started, and then only + /// after , which is the gap a real one spends between being started + /// and answering on the port. + /// + sealed class FakeViewer + { + int starts; + long upAt = long.MaxValue; + readonly Stopwatch elapsed = Stopwatch.StartNew(); + + public TimeSpan BindDelay { get; init; } + + public int Starts => starts; + + public bool Start() + { + Interlocked.Increment(ref starts); + Interlocked.Exchange(ref upAt, (elapsed.Elapsed + BindDelay).Ticks); + return true; + } + + public bool IsUp() => + elapsed.Elapsed.Ticks >= Interlocked.Read(ref upAt); + } +} diff --git a/src/DiffEngine/DiffRunner_Inline.cs b/src/DiffEngine/DiffRunner_Inline.cs index 6c247b25..7c7d57c1 100644 --- a/src/DiffEngine/DiffRunner_Inline.cs +++ b/src/DiffEngine/DiffRunner_Inline.cs @@ -79,8 +79,13 @@ public static async Task AddInlineAsync(InlinePatch patch, Cancel return InlineResult.NoViewerFound; } - var launched = await ViewerLauncher.LaunchAsync(patch, payload, cancel); - return launched ? InlineResult.Queued : InlineResult.NoViewerFound; + // Through the gate, because a parallel run reaches here once per failing snapshot with + // nothing owning the port, and every one of them used to start a viewer of its own. + var launched = await ViewerLaunchGate.LaunchAsync( + async () => await ViewerClient.SendAsync(new(ViewerVerb.Inline, Body: payload), cancel) == SendOutcome.Accepted, + () => ViewerLauncher.LaunchAsync(patch, payload, cancel), + cancel); + return launched == ViewerLaunchOutcome.Failed ? InlineResult.NoViewerFound : InlineResult.Queued; } /// diff --git a/src/DiffEngine/Protocol/ViewerClient.cs b/src/DiffEngine/Protocol/ViewerClient.cs index 65b73841..fc84e416 100644 --- a/src/DiffEngine/Protocol/ViewerClient.cs +++ b/src/DiffEngine/Protocol/ViewerClient.cs @@ -80,6 +80,25 @@ public static int Port /// public static readonly TimeSpan ShortTimeout = TimeSpan.FromMilliseconds(500); + /// + /// Whether anything is listening, without sending it anything. For a caller that has just + /// started a viewer and wants to know when it can be talked to, which a send cannot answer + /// without also handing over work. + /// + public static bool IsOwned(int? port = null) + { + try + { + using var client = new TcpClient(); + return client.ConnectAsync(IPAddress.Loopback, port ?? Port).Wait(ShortTimeout); + } + catch (Exception exception) + when (Ignorable(exception)) + { + return false; + } + } + /// /// True when the owner acknowledged. A refused connection means nobody owns the queue. /// diff --git a/src/DiffEngine/Tray/PendingFiles.cs b/src/DiffEngine/Tray/PendingFiles.cs index 77077ff6..ca068d50 100644 --- a/src/DiffEngine/Tray/PendingFiles.cs +++ b/src/DiffEngine/Tray/PendingFiles.cs @@ -47,7 +47,9 @@ public static void AddDelete(string file) return; } - ViewerLauncher.LaunchDelete(file); + ViewerLaunchGate.Launch( + () => ViewerClient.TrySend(new(ViewerVerb.Delete, file)), + () => ViewerLauncher.LaunchDelete(file)); } public static async Task AddDeleteAsync(string file, Cancel cancel) @@ -63,7 +65,10 @@ await PiperClient.SendDeleteAsync(file, cancel)) return; } - ViewerLauncher.LaunchDelete(file); + await ViewerLaunchGate.LaunchAsync( + async () => await ViewerClient.TrySendAsync(new(ViewerVerb.Delete, file), cancel), + () => Task.FromResult(ViewerLauncher.LaunchDelete(file)), + cancel); } /// @@ -101,11 +106,25 @@ public static LaunchResult AddDiff(string tempFile, string targetFile, string ex : Refused(tempFile, targetFile); } - return ViewerLauncher.LaunchDiff(tempFile, targetFile) - ? LaunchResult.StartedNewInstance - : LaunchResult.NoDiffToolFound; + return Launched( + ViewerLaunchGate.Launch( + () => ViewerClient.TrySend(new(ViewerVerb.Diff, tempFile, targetFile)), + () => ViewerLauncher.LaunchDiff(tempFile, targetFile))); } + /// + /// A launch that turned out not to be one is not reported as one. Twenty pairs failing at once + /// put twenty callers on the gate and one viewer on the screen, and calling that twenty new + /// instances is how the count stopped meaning anything. + /// + static LaunchResult Launched(ViewerLaunchOutcome outcome) => + outcome switch + { + ViewerLaunchOutcome.Launched => LaunchResult.StartedNewInstance, + ViewerLaunchOutcome.Taken => LaunchResult.AlreadyRunningAndSupportsRefresh, + _ => LaunchResult.NoDiffToolFound + }; + /// /// An owner that is there and said no, which is an owner too old to know the verb. Launching a /// second viewer cannot change that answer and would bind nothing, so the pair goes over as a @@ -139,9 +158,11 @@ await PiperClient.SendMoveAsync(tempFile, targetFile, exe, ViewerLauncher.DiffAr : LaunchResult.NoDiffToolFound; } - return ViewerLauncher.LaunchDiff(tempFile, targetFile) - ? LaunchResult.StartedNewInstance - : LaunchResult.NoDiffToolFound; + return Launched( + await ViewerLaunchGate.LaunchAsync( + async () => await ViewerClient.TrySendAsync(new(ViewerVerb.Diff, tempFile, targetFile), cancel), + () => Task.FromResult(ViewerLauncher.LaunchDiff(tempFile, targetFile)), + cancel)); } /// diff --git a/src/DiffEngine/Viewer/ViewerLaunchGate.cs b/src/DiffEngine/Viewer/ViewerLaunchGate.cs new file mode 100644 index 00000000..eddbf259 --- /dev/null +++ b/src/DiffEngine/Viewer/ViewerLaunchGate.cs @@ -0,0 +1,183 @@ +namespace DiffEngine; + +/// +/// What a gated launch settled on. +/// +enum ViewerLaunchOutcome +{ + /// + /// An owner appeared while this call was queued behind the gate and has taken the work, so + /// nothing was started. + /// + Taken, + + /// + /// This call started the viewer. + /// + Launched, + + /// + /// Nothing could be started, and nobody was there to take it. + /// + Failed +} + +/// +/// One viewer launch at a time, per process, with the send retried inside the gate. +/// +/// A parallel run reaches the launch path once per failing snapshot, and while nothing owns the +/// port every one of them is entitled to start a viewer. Twenty failing pairs meant twenty +/// processes: one bound the port, and the other nineteen handed their work over and exited. The +/// outcome is correct - that racing resolution is what makes it correct - but it is twenty process +/// starts to open one window, and it reported twenty new instances when there was one. +/// MaxInstance caps this for every tool that opens a window per pair, and does not apply to +/// the one that does not. +/// +/// +/// So the first caller through starts a viewer and holds the gate until that viewer answers, and +/// everyone behind it finds an owner and never launches at all. Held across the wait rather than +/// released at the start, because a viewer takes most of a second to bind and a gate let go before +/// then only lets the next caller start a second one. +/// +/// +/// What the gate holds is the decision, not the work. Inside it is a connect that asks whether +/// anyone is there; the send that hands the payload over happens outside, so the callers that find +/// an owner still reach it at once. Sending inside instead turned twenty process starts into +/// nineteen serialised round trips, which was slower than the problem. +/// +/// +/// Per process rather than per machine. Two test assemblies running at once still race, which is +/// the case the bind resolution was written for and still handles - and a named mutex would put a +/// cross process wait on the failing path of every run to save a handful of starts in the rarer +/// arrangement. +/// +/// +static class ViewerLaunchGate +{ + static readonly SemaphoreSlim gate = new(1, 1); + + /// + /// How long the caller that launched holds the gate waiting for its viewer to answer. Long + /// enough for a cold start with an antivirus in the way; one that never binds costs a single + /// caller this wait, and then the next tries again. + /// + internal static TimeSpan BindWait { get; set; } = TimeSpan.FromSeconds(5); + + /// + /// The send, run again once an owner exists. Outside the gate, because it carries a payload + /// and takes a round trip: nineteen of those queued behind one another cost more than the + /// nineteen processes this exists to avoid. + /// + /// Starts a viewer. False when nothing could be started. + /// + /// How the gate asks whether anyone holds the queue, which is also what it waits on after a + /// launch. Defaults to the real port. Supplied by the tests, which otherwise have to arrange + /// twenty concurrent connects to a port nothing is listening on and read the answer back out + /// of the operating system. + /// + public static ViewerLaunchOutcome Launch(Func retry, Func launch, Func? isOwned = null) + { + isOwned ??= () => ViewerClient.IsOwned(); + bool owned; + gate.Wait(); + try + { + // Asked rather than sent, so the decision to launch costs a connect rather than a + // round trip with a payload on it. + owned = isOwned(); + if (!owned) + { + if (!launch()) + { + return ViewerLaunchOutcome.Failed; + } + + WaitForBind(isOwned); + } + } + finally + { + gate.Release(); + } + + if (!owned) + { + return ViewerLaunchOutcome.Launched; + } + + return retry() ? ViewerLaunchOutcome.Taken : ViewerLaunchOutcome.Failed; + } + + /// + public static async Task LaunchAsync( + Func> retry, + Func> launch, + Cancel cancel, + Func? isOwned = null) + { + isOwned ??= () => ViewerClient.IsOwned(); + bool owned; + await gate.WaitAsync(cancel); + try + { + owned = isOwned(); + if (!owned) + { + if (!await launch()) + { + return ViewerLaunchOutcome.Failed; + } + + await WaitForBindAsync(isOwned, cancel); + } + } + finally + { + gate.Release(); + } + + if (!owned) + { + return ViewerLaunchOutcome.Launched; + } + + return await retry() ? ViewerLaunchOutcome.Taken : ViewerLaunchOutcome.Failed; + } + + /// + /// Waits for the launched viewer to be answerable, so the next caller through the gate finds + /// an owner rather than starting another. Gives up after and reports + /// the launch all the same, because it did happen: the work went over on the command line or + /// on stdin, and the cost of giving up early is one more viewer, which is where this began. + /// + static void WaitForBind(Func isOwned) + { + var elapsed = Stopwatch.StartNew(); + while (elapsed.Elapsed < BindWait) + { + if (isOwned()) + { + return; + } + + Thread.Sleep(Poll); + } + } + + /// + static async Task WaitForBindAsync(Func isOwned, Cancel cancel) + { + var elapsed = Stopwatch.StartNew(); + while (elapsed.Elapsed < BindWait) + { + if (isOwned()) + { + return; + } + + await Task.Delay(Poll, cancel); + } + } + + static readonly TimeSpan Poll = TimeSpan.FromMilliseconds(50); +} From a7a44e91ccddcda0458cb0753bfdf2896bcf4582 Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sun, 23 Aug 2026 23:12:42 +1000 Subject: [PATCH 5/6] Answer "how is this move reopened" in one place The tray works out the exe and arguments for itself whenever a tracked move arrives without them, which is what a move over the viewer port does - the wire carries two paths and nothing else. It took them off the tool definition, and the viewer's definition still declares the plain two path form, so a pair reopened that way got a window of its own while the queue it belongs to was on screen behind it. It also got CanKill from IsMdi, which for the viewer means the tray would kill the window every other pending pair is drawn in. Neither is reachable often - it needs a tray owning the inline queue and a move that did not come through AddDiff, which the refusal fallback is - but the two callers disagreeing about how to reopen the same pair is the kind of thing that only ever gets worse. PendingFiles.RelaunchFor answers it once, and both ask. The tray's own call is not covered, for the reason the Kill dispatch is not: it goes through resolution by extension, and which tool that finds depends on what is installed. What it now calls is covered with a constructed viewer, an ordinary tool and an MDI one. --- src/DiffEngine.Tests/PendingFilesDiffTests.cs | 73 +++++++++++++++++-- src/DiffEngine/DiffRunner.cs | 4 +- src/DiffEngine/Tray/PendingFiles.cs | 39 ++++++++-- src/DiffEngineTray/Tracker.cs | 7 +- 4 files changed, 107 insertions(+), 16 deletions(-) diff --git a/src/DiffEngine.Tests/PendingFilesDiffTests.cs b/src/DiffEngine.Tests/PendingFilesDiffTests.cs index ca19ae58..f42e7287 100644 --- a/src/DiffEngine.Tests/PendingFilesDiffTests.cs +++ b/src/DiffEngine.Tests/PendingFilesDiffTests.cs @@ -19,7 +19,7 @@ public async Task ADiffReachesTheOwnerWithBothPaths() { using var owner = new Recording(); - var result = await PendingFiles.AddDiffAsync(Temp, Target, Exe, Cancel.None); + var result = await PendingFiles.AddDiffAsync(Viewer(), Temp, Target, Cancel.None); await Assert.That(result).IsEqualTo(LaunchResult.AlreadyRunningAndSupportsRefresh); await Assert.That(owner.Heard).IsEquivalentTo([$"{ViewerVerb.Diff}:{Temp}:{Target}"]); @@ -30,7 +30,7 @@ public async Task ASyncDiffReachesTheOwnerToo() { using var owner = new Recording(); - var result = PendingFiles.AddDiff(Temp, Target, Exe); + var result = PendingFiles.AddDiff(Viewer(), Temp, Target); await Assert.That(result).IsEqualTo(LaunchResult.AlreadyRunningAndSupportsRefresh); await Assert.That(owner.Heard).IsEquivalentTo([$"{ViewerVerb.Diff}:{Temp}:{Target}"]); @@ -46,7 +46,7 @@ public async Task ARefusedDiffFallsBackToAPlainMove() { using var owner = new Recording {Refuse = ViewerVerb.Diff}; - var result = await PendingFiles.AddDiffAsync(Temp, Target, Exe, Cancel.None); + var result = await PendingFiles.AddDiffAsync(Viewer(), Temp, Target, Cancel.None); await Assert.That(result).IsEqualTo(LaunchResult.AlreadyRunningAndSupportsRefresh); await Assert.That(owner.Heard).IsEquivalentTo( @@ -61,7 +61,7 @@ public async Task ASyncRefusedDiffFallsBackToAPlainMove() { using var owner = new Recording {Refuse = ViewerVerb.Diff}; - PendingFiles.AddDiff(Temp, Target, Exe); + PendingFiles.AddDiff(Viewer(), Temp, Target); await Assert.That(owner.Heard).IsEquivalentTo( [ @@ -97,9 +97,72 @@ public async Task SettlingWithNoOwnerIsSilent() await Assert.That(() => PendingFiles.SettleDiff(Temp)).ThrowsNothing(); } + /// + /// The tray works the arguments out for itself when a move arrives without them, and used to + /// take the viewer's declared ones - two plain paths, which open a window of its own for a + /// pair whose queue is already on screen. Both callers ask this instead. + /// + [Test] + public async Task AViewerIsReopenedIntoItsQueueAndNeverKilled() + { + var (arguments, canKill) = PendingFiles.RelaunchFor(Viewer(), Temp, Target); + + await Assert.That(arguments).IsEqualTo($"--diff \"{Temp}\" \"{Target}\""); + // One window holds every pending pair, so killing it takes the rest with it. + await Assert.That(canKill).IsFalse(); + } + + [Test] + public async Task AnOrdinaryToolKeepsItsOwnArgumentsAndStaysKillable() + { + var (arguments, canKill) = PendingFiles.RelaunchFor(Other(isMdi: false), Temp, Target); + + await Assert.That(arguments).IsEqualTo($"\"{Temp}\" \"{Target}\""); + await Assert.That(canKill).IsTrue(); + } + + [Test] + public async Task AnMdiToolIsNotKillableEither() + { + var (_, canKill) = PendingFiles.RelaunchFor(Other(isMdi: true), Temp, Target); + + await Assert.That(canKill).IsFalse(); + } + + static ResolvedTool Other(bool isMdi) => + new( + name: "Fake", + exePath: Environment.ProcessPath!, + launchArguments: new( + Left: (temp, target) => $"\"{target}\" \"{temp}\"", + Right: (temp, target) => $"\"{temp}\" \"{target}\""), + isMdi: isMdi, + autoRefresh: false, + binaryExtensions: [], + requiresTarget: false, + supportsText: true, + useShellExecute: false); + const string Temp = @"c:\temp\Sample.Test.received.png"; const string Target = @"c:\code\Sample.Test.verified.png"; - const string Exe = @"c:\tools\DiffEngineViewer.exe"; + + /// + /// Carries the identity the route branches on. Never started: an owner answers every time. + /// + static ResolvedTool Viewer() => + new( + name: DiffTool.DiffEngineViewer.ToString(), + tool: DiffTool.DiffEngineViewer, + exePath: Environment.ProcessPath!, + launchArguments: new( + Left: (temp, target) => $"\"{target}\" \"{temp}\"", + Right: (temp, target) => $"\"{temp}\" \"{target}\""), + isMdi: false, + autoRefresh: false, + binaryExtensions: [], + requiresTarget: false, + supportsText: true, + useShellExecute: false); /// /// A queue owner that only writes down what it was asked, so the assertions are about the diff --git a/src/DiffEngine/DiffRunner.cs b/src/DiffEngine/DiffRunner.cs index ec5af74c..de3fe608 100644 --- a/src/DiffEngine/DiffRunner.cs +++ b/src/DiffEngine/DiffRunner.cs @@ -185,7 +185,7 @@ static LaunchResult InnerLaunch(TryResolveTool tryResolveTool, string tempFile, // window to replace, and no slot to spend on a window that already exists. if (PendingFiles.IsViewer(tool)) { - return PendingFiles.AddDiff(tempFile, targetFile, tool.ExePath); + return PendingFiles.AddDiff(tool, tempFile, targetFile); } tool.CommandAndArguments(tempFile, targetFile, out var arguments, out var command); @@ -231,7 +231,7 @@ static async Task InnerLaunchAsync(TryResolveTool tryResolveTool, // As above: the viewer has no window of its own for this pair to reason about. if (PendingFiles.IsViewer(tool)) { - return await PendingFiles.AddDiffAsync(tempFile, targetFile, tool.ExePath, Cancel.None); + return await PendingFiles.AddDiffAsync(tool, tempFile, targetFile, Cancel.None); } tool.CommandAndArguments(tempFile, targetFile, out var arguments, out var command); diff --git a/src/DiffEngine/Tray/PendingFiles.cs b/src/DiffEngine/Tray/PendingFiles.cs index ca068d50..d9290aca 100644 --- a/src/DiffEngine/Tray/PendingFiles.cs +++ b/src/DiffEngine/Tray/PendingFiles.cs @@ -87,13 +87,13 @@ await ViewerLaunchGate.LaunchAsync( /// before any of this: an entry in the tray menu. /// /// - public static LaunchResult AddDiff(string tempFile, string targetFile, string exe) + public static LaunchResult AddDiff(ResolvedTool tool, string tempFile, string targetFile) { - // CanKill false and no process: there is no window of its own to kill, and killing the - // shared one would take every other pair in it away as well. The arguments are stored all - // the same, because the tray re-runs them for "Open diff tool". + // No process, and the arguments and CanKill from the one place that answers that, because + // the tray works out the same two values for itself when a move arrives without them. + var (arguments, canKill) = RelaunchFor(tool, tempFile, targetFile); if (DiffEngineTray.IsRunning && - PiperClient.SendMove(tempFile, targetFile, exe, ViewerLauncher.DiffArguments(tempFile, targetFile), false, null)) + PiperClient.SendMove(tempFile, targetFile, tool.ExePath, arguments, canKill, null)) { ViewerClient.TrySend(new(ViewerVerb.Focus, TrackedKeys.ForMove(tempFile))); return LaunchResult.AlreadyRunningAndSupportsRefresh; @@ -136,10 +136,11 @@ static LaunchResult Refused(string tempFile, string targetFile) => : LaunchResult.NoDiffToolFound; /// - public static async Task AddDiffAsync(string tempFile, string targetFile, string exe, Cancel cancel) + public static async Task AddDiffAsync(ResolvedTool tool, string tempFile, string targetFile, Cancel cancel) { + var (arguments, canKill) = RelaunchFor(tool, tempFile, targetFile); if (DiffEngineTray.IsRunning && - await PiperClient.SendMoveAsync(tempFile, targetFile, exe, ViewerLauncher.DiffArguments(tempFile, targetFile), false, null, cancel)) + await PiperClient.SendMoveAsync(tempFile, targetFile, tool.ExePath, arguments, canKill, null, cancel)) { await ViewerClient.TrySendAsync(new(ViewerVerb.Focus, TrackedKeys.ForMove(tempFile)), cancel); return LaunchResult.AlreadyRunningAndSupportsRefresh; @@ -189,6 +190,30 @@ public static void SettleDiff(string tempFile) => public static bool IsViewer(ResolvedTool tool) => tool.Tool == DiffTool.DiffEngineViewer; + /// + /// How a tracked move is opened again, and whether the window that opens may be killed. + /// + /// Answered here rather than at each caller, because there are two: this file, sending the + /// move to a tray, and the tray itself, working them out from the extension for a move that + /// arrived without them. The two disagreeing is not theoretical - the viewer's declared + /// arguments are still the plain two path form, so the tray's answer reopened a pair in a + /// window of its own while the queue it belongs to was on screen behind it. + /// + /// + /// A viewer is never killable. It draws every pending pair in one window, so killing the one + /// a pair was opened from takes the rest with it. + /// + /// + public static (string arguments, bool canKill) RelaunchFor(ResolvedTool tool, string temp, string target) + { + if (IsViewer(tool)) + { + return (ViewerLauncher.DiffArguments(temp, target), false); + } + + return (tool.GetArguments(temp, target), !tool.IsMdi); + } + public static void AddMove( string tempFile, string targetFile, diff --git a/src/DiffEngineTray/Tracker.cs b/src/DiffEngineTray/Tracker.cs index 877c4c65..65c331d2 100644 --- a/src/DiffEngineTray/Tracker.cs +++ b/src/DiffEngineTray/Tracker.cs @@ -192,9 +192,12 @@ static TrackedMove BuildTrackedMove(string temp, string? exe, string? arguments, { if (DiffTools.TryFindByExtension(extension, out var tool)) { - arguments = tool.GetArguments(temp, target); + // Through DiffEngine's own answer rather than straight off the definition, because + // the viewer's declared arguments still name two paths and running those opens a + // window of its own for a pair whose queue is already on screen. + (arguments, var killable) = PendingFiles.RelaunchFor(tool, temp, target); + canKill = killable; exe = tool.ExePath; - canKill = !tool.IsMdi; killLockingProcess = tool.KillLockingProcess; } } From 56bbbf9839888a438af5f2c57bbfde8b3f418f53 Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sun, 23 Aug 2026 23:17:26 +1000 Subject: [PATCH 6/6] Say why file mode is still here Nothing in DiffEngine reaches ViewerMode.File any more, which makes it look like something left behind rather than something kept. It is kept: it is the blocking one-pair-per-invocation shape a difftool caller needs, where queue mode's second invocation forwards and exits and the caller races on through the rest of the files; it is the only place accepting means copy rather than move, which is what two arbitrary files a person named deserve; and Fixtures.File() is the state around thirty test call sites are built on, so collapsing it would re-approve every renderer, scroll and pixel snapshot with a pending column those tests are not about. Written down because the next person to read ScreenBuilder will see four mode checks guarding a path nothing takes, and the reasons are not in the code. --- claude.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/claude.md b/claude.md index 9ca47bbd..930a6bb8 100644 --- a/claude.md +++ b/claude.md @@ -185,6 +185,15 @@ apart. attached viewer draws for the tray's — so nothing about how they look or what their menu offers is per arrangement. Only who applies them differs: `ViewerActions.MoveFile`/`DeleteFile` here, a forwarded key there. +- `ViewerMode.File` — two paths on the command line, one window, no port — is reached by nothing in + DiffEngine any more, and is kept deliberately rather than left behind. It is the blocking + one-pair-per-invocation shape a `git difftool` style caller needs, where queue mode's second + invocation forwards and exits and the caller races ahead; it is the only place accepting means + copy rather than move, which is what two arbitrary files a person named deserve; and + `Fixtures.File()` is the "one entry, no queue chrome" state around thirty test call sites are + built on, so collapsing it would re-approve every renderer, scroll and pixel snapshot with a + pending column those tests are not about. It costs a handful of `if`s in `ScreenBuilder`, + `QueueProjection` and `Settle`. Do not delete it because it looks unreachable. - Single instance by socket bind on 3493 (`DiffEngine_ViewerPort`): whoever binds owns the queue, and a process that fails to bind talks to the owner instead. A viewer that does not own one runs with `--attach`: it polls `listfull`, derives every pane from the patches that come back, and