diff --git a/claude.md b/claude.md index 57acae09..492624ae 100644 --- a/claude.md +++ b/claude.md @@ -53,9 +53,9 @@ flowchart LR Owner{{"inline queue owner: whoever bound 3493
first — the tray at login, else a viewer"}} Files[("source files and
staged patch files")] - Engine -->|"3492 moves, deletes (one way)"| Tray - Engine -->|"3493 inline, settle"| Owner - Engine -.->|"launch with patch on stdin,
when nothing owns 3493"| Window + 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 Tray <-->|"3493 list, accept, focus"| Owner Window <-->|"3493 listfull, accept, discard"| Owner Plugin -->|"3493 settle, after accepting"| Owner @@ -139,6 +139,11 @@ keeps offering a snapshot that is already in the source. - Does **not** reference DiffEngine. It links `Inline/*.cs`, `Protocol/*.cs` and `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 + 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. - 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 @@ -187,6 +192,29 @@ keeps offering a snapshot that is already in the source. - Either host runs the same `InlineQueue` from DiffEngine, so the two cannot differ on what accepting or settling means. Owning it means accepting runs on a listener thread rather than on a render loop, which is where `InlineApplier`'s ten second mutex wait used to sit. +- Which arrangement is live decides *which process applies an accept*, so both are pinned by + `TrayViewerSyncTest` — a real tray and a real `SessionState` over a real socket, asserting that + an accept, discard, sweep or settle from either surface leaves the other showing the same thing. + It sits in DiffEngineTray.Tests because that is the only project that can reference both halves + (the viewer aliased, since it links DiffEngine's sources and so declares the same type names). + The wire carries `ok` and a message, not an apply status, so `RemoteInlineHost` decides applied + versus failed by re-reading the listing: an owner keeps a failed entry pending, and taking `ok` + at face value used to report it as accepted while the viewer was still showing it. +- A viewer that owns the queue answers about the tray's snapshots and about its **own** pending + files. Moves and deletes go to the tray when one is running and to the queue owner when one is + not (`PendingFiles`), because the alternative was that with no tray they went nowhere at all — + the send was skipped and the file was pending in nothing. A tray that owns the queue answers + those verbs too, routing them into the same tracked files the piper port fills, which is + load bearing rather than defensive: `DiffEngineTray.IsRunning` is cached at type init, so a test + process that started before the tray addresses the queue owner for the rest of its life. +- A delete starts a viewer when nothing owns the queue; a move does not. A move already has a + 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. +- 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 + must not, since it is replacing them with what the owner just reported. - `DebugReport` / `DebugForm` - the menu's "Debug view": every field of every tracked move, delete and snapshot as text, plus the queued patches when this tray owns the queue. The report is a string so it can be copied into an issue and snapshot tested without rendering a window. diff --git a/docs/inline.md b/docs/inline.md index a99c82f8..d77a76c7 100644 --- a/docs/inline.md +++ b/docs/inline.md @@ -30,11 +30,11 @@ flowchart LR Owner{{"inline queue owner: whoever bound 3493
first — the tray at login, else a viewer"}} Files[("source files and
staged patch files")] - Engine -->|"3492 moves, deletes (one way)"| Tray - Engine -->|"3493 inline, settle"| Owner - Engine -.->|"launch with patch on stdin,
when nothing owns 3493"| Window + 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 Tray <-->|"3493 list, accept, focus"| Owner - Window <-->|"3493 listfull (with the tray's moves
and deletes), accept, discard"| Owner + Window <-->|"3493 listfull (with the owner's moves
and deletes), accept, discard"| Owner Plugin -->|"3493 settle, after accepting"| Owner Owner -->|"InlineApplier"| Files Plugin -->|"InlineApplier"| Files @@ -47,6 +47,12 @@ verbs. Either way both hosts run the same `InlineQueue` implementation, so they 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. + ## When a test fails diff --git a/docs/mdsource/inline.source.md b/docs/mdsource/inline.source.md index 3842a260..13f3aeff 100644 --- a/docs/mdsource/inline.source.md +++ b/docs/mdsource/inline.source.md @@ -23,11 +23,11 @@ flowchart LR Owner{{"inline queue owner: whoever bound 3493
first — the tray at login, else a viewer"}} Files[("source files and
staged patch files")] - Engine -->|"3492 moves, deletes (one way)"| Tray - Engine -->|"3493 inline, settle"| Owner - Engine -.->|"launch with patch on stdin,
when nothing owns 3493"| Window + 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 Tray <-->|"3493 list, accept, focus"| Owner - Window <-->|"3493 listfull (with the tray's moves
and deletes), accept, discard"| Owner + Window <-->|"3493 listfull (with the owner's moves
and deletes), accept, discard"| Owner Plugin -->|"3493 settle, after accepting"| Owner Owner -->|"InlineApplier"| Files Plugin -->|"InlineApplier"| Files @@ -40,6 +40,12 @@ verbs. Either way both hosts run the same `InlineQueue` implementation, so they 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. + ## When a test fails diff --git a/docs/mdsource/tray.source.md b/docs/mdsource/tray.source.md index 9206ec23..ff63f83b 100644 --- a/docs/mdsource/tray.source.md +++ b/docs/mdsource/tray.source.md @@ -81,7 +81,9 @@ If accepting a move fails because the files are locked by another process (for e ### Discard -Discard will clear all currently tracked items. +Discard will clear all currently tracked items. It is the same discard as the per item menu and as +the one in [DiffEngineViewer](/docs/viewer.md): a move loses its received file, a pending delete +keeps its file and is untracked, and every pending inline snapshot is dropped. ### Purge verified files diff --git a/docs/mdsource/viewer.source.md b/docs/mdsource/viewer.source.md index 927e9813..f27602d5 100644 --- a/docs/mdsource/viewer.source.md +++ b/docs/mdsource/viewer.source.md @@ -52,6 +52,12 @@ Reviewing an inline snapshot, where the patch payload arrives on stdin: DiffEngineViewer --inline --source --line ``` +Reviewing a file a passing test no longer produces, which DiffEngine sends when no tray is running: + +``` +DiffEngineViewer --delete +``` + Displaying a queue held by another process, which is how [DiffEngineTray](/docs/tray.md) opens one: ``` @@ -225,6 +231,22 @@ a particular snapshot, and close the viewer. A tray restart loses the queue, as it loses pending file moves and deletes. Re-run the tests. +## With no tray + +Pending file moves and deletes go to the tray when one is running. When one is not, they go to the +viewer, which holds and applies them itself — so a received file waiting to be promoted, or a +verified file a passing test no longer produces, is reviewable rather than invisible. + +A pending delete starts a viewer if none is running. It is the one change with no second file to +compare against, so no diff tool ever opens for it, and a window is the only surface it can have. A pending move does not start one: DiffEngine has already opened a diff tool for that file +pair, and a second window competing with it is not an improvement. A move joins a window that is +already open. + +Both look and behave exactly as they do when the tray owns them — same rows, same context menu, +same **Accept all** — because which process is holding a pending file depends only on whether a +tray happened to be running. + + ## Disabling Set `DiffEngine_InlineViewer` to `false` to stop inline snapshots opening a window. The viewer also diff --git a/docs/tray.md b/docs/tray.md index 1ba64c42..e0d03e56 100644 --- a/docs/tray.md +++ b/docs/tray.md @@ -88,7 +88,9 @@ If accepting a move fails because the files are locked by another process (for e ### Discard -Discard will clear all currently tracked items. +Discard will clear all currently tracked items. It is the same discard as the per item menu and as +the one in [DiffEngineViewer](/docs/viewer.md): a move loses its received file, a pending delete +keeps its file and is untracked, and every pending inline snapshot is dropped. ### Purge verified files diff --git a/docs/viewer.md b/docs/viewer.md index 3ef110e4..cd3ea767 100644 --- a/docs/viewer.md +++ b/docs/viewer.md @@ -59,6 +59,12 @@ Reviewing an inline snapshot, where the patch payload arrives on stdin: DiffEngineViewer --inline --source --line ``` +Reviewing a file a passing test no longer produces, which DiffEngine sends when no tray is running: + +``` +DiffEngineViewer --delete +``` + Displaying a queue held by another process, which is how [DiffEngineTray](/docs/tray.md) opens one: ``` @@ -232,6 +238,22 @@ a particular snapshot, and close the viewer. A tray restart loses the queue, as it loses pending file moves and deletes. Re-run the tests. +## With no tray + +Pending file moves and deletes go to the tray when one is running. When one is not, they go to the +viewer, which holds and applies them itself — so a received file waiting to be promoted, or a +verified file a passing test no longer produces, is reviewable rather than invisible. + +A pending delete starts a viewer if none is running. It is the one change with no second file to +compare against, so no diff tool ever opens for it, and a window is the only surface it can have. A pending move does not start one: DiffEngine has already opened a diff tool for that file +pair, and a second window competing with it is not an improvement. A move joins a window that is +already open. + +Both look and behave exactly as they do when the tray owns them — same rows, same context menu, +same **Accept all** — because which process is holding a pending file depends only on whether a +tray happened to be running. + + ## Disabling Set `DiffEngine_InlineViewer` to `false` to stop inline snapshots opening a window. The viewer also diff --git a/src/DiffEngine.Tests/ViewerProtocolTests.cs b/src/DiffEngine.Tests/ViewerProtocolTests.cs index 54a78cf2..dfffcd05 100644 --- a/src/DiffEngine.Tests/ViewerProtocolTests.cs +++ b/src/DiffEngine.Tests/ViewerProtocolTests.cs @@ -373,6 +373,42 @@ public async Task ARefusedAcceptGoesOnTheWireAsAnError() await Assert.That(done.Message).IsEqualTo("Applied Tests.cs:42"); } + /// + /// A pending file with no tray running. The paths ride key and body rather than an encoded + /// payload, because that is all a tracked move or delete is. + /// + [Test] + public async Task MoveAndDeleteReachTheOwner() + { + var owner = new FakeOwner((true, null)); + + var move = ViewerMessageHandler.Handle(owner, new(ViewerVerb.Move, @"c:\temp\a.received.txt", @"c:\code\a.verified.txt")); + var delete = ViewerMessageHandler.Handle(owner, new(ViewerVerb.Delete, @"c:\code\b.verified.txt")); + + await Assert.That(move.Ok).IsTrue(); + await Assert.That(delete.Ok).IsTrue(); + await Assert.That(owner.Tracked).IsEquivalentTo( + [ + @"move c:\temp\a.received.txt > c:\code\a.verified.txt", + @"delete c:\code\b.verified.txt" + ]); + } + + [Test] + public async Task AMoveWithoutBothPathsIsRefused() + { + var owner = new FakeOwner((true, null)); + + var noTarget = ViewerMessageHandler.Handle(owner, new(ViewerVerb.Move, @"c:\temp\a.received.txt")); + var noFile = ViewerMessageHandler.Handle(owner, new(ViewerVerb.Delete)); + + await Assert.That(noTarget.Ok).IsFalse(); + await Assert.That(noTarget.Message).IsEqualTo("Move requires a key and a body"); + await Assert.That(noFile.Ok).IsFalse(); + await Assert.That(noFile.Message).IsEqualTo("Delete requires a key"); + await Assert.That(owner.Tracked).IsEmpty(); + } + /// /// The accept body is the variant origin a reviewer picked, and it has to reach the owner. /// @@ -396,6 +432,14 @@ public void Settle(string key, string? origin) { } + public List Tracked { get; } = []; + + public void TrackMove(string temp, string target) => + Tracked.Add($"move {temp} > {target}"); + + public void TrackDelete(string file) => + Tracked.Add($"delete {file}"); + public ViewerResponse Listing(bool withPatches) => ViewerResponse.Listing([]); public bool Has(string key) => true; diff --git a/src/DiffEngine/Protocol/IQueueOwner.cs b/src/DiffEngine/Protocol/IQueueOwner.cs index a350e52b..9c4dff26 100644 --- a/src/DiffEngine/Protocol/IQueueOwner.cs +++ b/src/DiffEngine/Protocol/IQueueOwner.cs @@ -20,9 +20,24 @@ interface IQueueOwner void Settle(string key, string? origin); /// - /// The whole listing response rather than just its items, because the tray answers with the - /// window command it has stashed plus its tracked moves and deletes, and the viewer has - /// nothing to add to the items. + /// Track a pending file move, replacing the entry for the same received file — a re-run + /// produces the same pair, and a second entry for it is a duplicate rather than news. + /// + /// Only reaches an owner when no tray was running in the sending process, so it is normally + /// the viewer that answers this. A tray owner routes it into the same tracked moves the piper + /// port fills, which is what a tray started after the test process needs: that process's + /// tray check is cached, so its moves come here for the rest of its life. + /// + /// + void TrackMove(string temp, string target); + + /// + void TrackDelete(string file); + + /// + /// The whole listing response rather than just its items, because an owner answers with its + /// tracked moves and deletes beside the queue, and a tray adds the window command it has + /// stashed. /// ViewerResponse Listing(bool withPatches); diff --git a/src/DiffEngine/Protocol/ViewerMessageHandler.cs b/src/DiffEngine/Protocol/ViewerMessageHandler.cs index 46c6b31f..b3ccbbdd 100644 --- a/src/DiffEngine/Protocol/ViewerMessageHandler.cs +++ b/src/DiffEngine/Protocol/ViewerMessageHandler.cs @@ -17,6 +17,10 @@ public static ViewerResponse Handle(IQueueOwner owner, ViewerMessage message) return Inline(owner, message.Body); case ViewerVerb.Settle: return Settle(owner, message.Key, message.Body); + case ViewerVerb.Move: + return Move(owner, message.Key, message.Body); + case ViewerVerb.Delete: + return Delete(owner, message.Key); case ViewerVerb.List: return owner.Listing(false); case ViewerVerb.ListFull: @@ -81,6 +85,35 @@ static ViewerResponse Settle(IQueueOwner owner, string? key, string? origin) return ViewerResponse.Success(); } + /// + /// The paths ride key and body rather than an encoded payload, because that is all a tracked + /// move is. What DiffEngine knows beside them — the diff tool it launched and that tool's + /// process id — is the tray's kill machinery and means nothing to an owner that does not have + /// any, so it is not sent. + /// + static ViewerResponse Move(IQueueOwner owner, string? temp, string? target) + { + if (temp is null || + target is null) + { + return ViewerResponse.Error("Move requires a key and a body"); + } + + owner.TrackMove(temp, target); + return ViewerResponse.Success(); + } + + static ViewerResponse Delete(IQueueOwner owner, string? file) + { + if (file is null) + { + return ViewerResponse.Error("Delete requires a key"); + } + + owner.TrackDelete(file); + return ViewerResponse.Success(); + } + static ViewerResponse Act(IQueueOwner owner, string? key, string? body, ViewerVerb verb) { if (key is null) diff --git a/src/DiffEngine/Protocol/ViewerVerb.cs b/src/DiffEngine/Protocol/ViewerVerb.cs index f45e670c..732e5976 100644 --- a/src/DiffEngine/Protocol/ViewerVerb.cs +++ b/src/DiffEngine/Protocol/ViewerVerb.cs @@ -12,6 +12,25 @@ enum ViewerVerb /// Settle, + /// + /// Track a pending file move: key is the received file, body the target it + /// belongs at. From DiffEngine when no tray is running, so the pair still has somewhere to be + /// pending rather than nowhere at all. + /// + /// Never launches an owner. DiffRunner has already opened a diff tool for that file pair, and + /// a second window competing with it is not an improvement. + /// + /// + Move, + + /// + /// 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 + /// queue — a delete has no second file to compare against and so no diff tool to open, which + /// left it with no surface whatsoever. + /// + Delete, + /// /// Return the pending entries as key, name and status. Enough to build a menu. From the tray. /// diff --git a/src/DiffEngine/Tray/DiffEngineTray.cs b/src/DiffEngine/Tray/DiffEngineTray.cs index f582d78e..7a265016 100644 --- a/src/DiffEngine/Tray/DiffEngineTray.cs +++ b/src/DiffEngine/Tray/DiffEngineTray.cs @@ -21,15 +21,10 @@ static DiffEngineTray() public static bool IsRunning { get; internal set; } - public static void AddDelete(string file) - { - if (!IsRunning) - { - return; - } - - PiperClient.SendDelete(file); - } + // No IsRunning gate any more. PendingFiles is the router: the piper port when a tray is + // running, and the inline queue's owner when one is not, rather than nothing at all. + public static void AddDelete(string file) => + PendingFiles.AddDelete(file); public static void AddMove( string tempFile, @@ -37,25 +32,11 @@ public static void AddMove( string? exe, string? arguments, bool canKill, - int? processId) - { - if (!IsRunning) - { - return; - } - - PiperClient.SendMove(tempFile, targetFile, exe, arguments, canKill, processId); - } + int? processId) => + PendingFiles.AddMove(tempFile, targetFile, exe, arguments, canKill, processId); - public static Task AddDeleteAsync(string file, Cancel cancel = default) - { - if (!IsRunning) - { - return Task.CompletedTask; - } - - return PiperClient.SendDeleteAsync(file, cancel); - } + public static Task AddDeleteAsync(string file, Cancel cancel = default) => + PendingFiles.AddDeleteAsync(file, cancel); public static Task AddMoveAsync( string tempFile, @@ -64,13 +45,6 @@ public static Task AddMoveAsync( string? arguments, bool canKill, int? processId, - Cancel cancel = default) - { - if (!IsRunning) - { - return Task.CompletedTask; - } - - return PiperClient.SendMoveAsync(tempFile, targetFile, exe, arguments, canKill, processId, cancel); - } + Cancel cancel = default) => + PendingFiles.AddMoveAsync(tempFile, targetFile, exe, arguments, canKill, processId, cancel); } diff --git a/src/DiffEngine/Tray/PendingFiles.cs b/src/DiffEngine/Tray/PendingFiles.cs new file mode 100644 index 00000000..ba936813 --- /dev/null +++ b/src/DiffEngine/Tray/PendingFiles.cs @@ -0,0 +1,96 @@ +// DiffEngineTray is the obsolete public shim, but its IsRunning is still where the tray check +// lives, and tests still set it. +#pragma warning disable CS0618 // Type or member is obsolete + +namespace DiffEngine; + +/// +/// Where a pending move or delete goes. +/// +/// The tray when one is running, over the piper port it has always used. Otherwise the process +/// that owns the inline queue, which is normally a viewer — so a pending file has a surface with +/// no tray installed. Before this it had none: the send was skipped outright, and a received file +/// or a stale verified file was pending in nothing at all. +/// +/// +/// A delete starts a viewer when nothing owns the queue. A move does not, and the asymmetry is the +/// point: has already opened a diff tool for that file pair, so a move +/// has a window, and a second one competing with it is not an improvement. A delete has no second +/// file to compare against and so no tool to open. +/// +/// +/// The tray check is , read once when that type initialises. +/// A tray started after the test process therefore never sees the piper port for the rest of that +/// process's life, and its moves and deletes arrive here instead — which a tray that owns the +/// queue answers, so they end up tracked either way. +/// +/// +static class PendingFiles +{ + public static void AddDelete(string file) + { + if (DiffEngineTray.IsRunning) + { + PiperClient.SendDelete(file); + return; + } + + if (ViewerClient.TrySend(new(ViewerVerb.Delete, file))) + { + return; + } + + ViewerLauncher.LaunchDelete(file); + } + + public static async Task AddDeleteAsync(string file, Cancel cancel) + { + if (DiffEngineTray.IsRunning) + { + await PiperClient.SendDeleteAsync(file, cancel); + return; + } + + if (await ViewerClient.TrySendAsync(new(ViewerVerb.Delete, file), cancel)) + { + return; + } + + ViewerLauncher.LaunchDelete(file); + } + + public static void AddMove( + string tempFile, + string targetFile, + string? exe, + string? arguments, + bool canKill, + int? processId) + { + if (DiffEngineTray.IsRunning) + { + PiperClient.SendMove(tempFile, targetFile, exe, arguments, canKill, processId); + return; + } + + ViewerClient.TrySend(new(ViewerVerb.Move, tempFile, targetFile)); + } + + public static async Task AddMoveAsync( + string tempFile, + string targetFile, + string? exe, + string? arguments, + bool canKill, + int? processId, + Cancel cancel) + { + if (DiffEngineTray.IsRunning) + { + await PiperClient.SendMoveAsync(tempFile, targetFile, exe, arguments, canKill, processId, cancel); + return; + } + + await ViewerClient.TrySendAsync(new(ViewerVerb.Move, tempFile, targetFile), cancel); + } +} diff --git a/src/DiffEngine/Viewer/ViewerLauncher.cs b/src/DiffEngine/Viewer/ViewerLauncher.cs index 1eaa44b6..fa73c8f0 100644 --- a/src/DiffEngine/Viewer/ViewerLauncher.cs +++ b/src/DiffEngine/Viewer/ViewerLauncher.cs @@ -47,6 +47,22 @@ public static async Task LaunchAsync(InlinePatch patch, string payload, Ca public static Process? LaunchAttached() => Start("--attach"); + /// + /// Starts a viewer holding one pending delete, for when no tray is running and nothing owns + /// the queue. + /// + /// On the command line rather than on stdin, which is what an inline patch needs: a path fits + /// inside the length limit where snapshot content does not. It also keeps each launch + /// distinguishable, which is what ProcessCleanup matches on. + /// + /// + /// Two deletes racing both launch. Only one binds the port; the other forwards its delete to + /// the winner and exits, which is the same resolution a second inline viewer reaches. + /// + /// + public static bool LaunchDelete(string file) => + Start($"--delete \"{file}\"") is not null; + 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/DiffEngineTray.Tests.csproj b/src/DiffEngineTray.Tests/DiffEngineTray.Tests.csproj index a1047f82..10d25849 100644 --- a/src/DiffEngineTray.Tests/DiffEngineTray.Tests.csproj +++ b/src/DiffEngineTray.Tests/DiffEngineTray.Tests.csproj @@ -16,6 +16,12 @@ + + diff --git a/src/DiffEngineTray.Tests/FakeLauncher.cs b/src/DiffEngineTray.Tests/FakeLauncher.cs new file mode 100644 index 00000000..33c13ca3 --- /dev/null +++ b/src/DiffEngineTray.Tests/FakeLauncher.cs @@ -0,0 +1,19 @@ +/// +/// A viewer that is never actually started. launches one whenever a +/// patch arrives with nothing displaying the queue, and the tests supply their own display instead. +/// +sealed class FakeLauncher : IViewerLauncher +{ + public int Launches { get; private set; } + + public bool Running { get; set; } + + public bool Succeed { get; set; } = true; + + public bool Launch() + { + Launches++; + Running = Succeed; + return Succeed; + } +} diff --git a/src/DiffEngineTray.Tests/OwnedInlineHostTest.cs b/src/DiffEngineTray.Tests/OwnedInlineHostTest.cs index 8c59f128..983eed14 100644 --- a/src/DiffEngineTray.Tests/OwnedInlineHostTest.cs +++ b/src/DiffEngineTray.Tests/OwnedInlineHostTest.cs @@ -6,22 +6,6 @@ /// public class OwnedInlineHostTest { - sealed class FakeLauncher : IViewerLauncher - { - public int Launches { get; private set; } - - public bool Running { get; set; } - - public bool Succeed { get; set; } = true; - - public bool Launch() - { - Launches++; - Running = Succeed; - return Succeed; - } - } - sealed class Owner : IDisposable { public Owner(Func? applier = null) @@ -490,6 +474,14 @@ public bool Has(string key) => public (int accepted, int kept) AcceptAll() => SweepResult; public int DiscardAll() => 0; + + public List Added { get; } = []; + + public void AddMove(string temp, string target) => + Added.Add($"move {temp} > {target}"); + + public void AddDelete(string file) => + Added.Add($"delete {file}"); } [Test] diff --git a/src/DiffEngineTray.Tests/TrackerDeleteTest.cs b/src/DiffEngineTray.Tests/TrackerDeleteTest.cs index 5c320c47..3d760f86 100644 --- a/src/DiffEngineTray.Tests/TrackerDeleteTest.cs +++ b/src/DiffEngineTray.Tests/TrackerDeleteTest.cs @@ -46,7 +46,7 @@ public async Task AcceptAllSingle() { await using var tracker = new RecordingTracker(); tracker.AddDelete(file1); - tracker.AcceptAll(); + await tracker.AcceptAll(); await tracker.AssertEmpty(); } @@ -56,7 +56,7 @@ public async Task AcceptAllMultiple() await using var tracker = new RecordingTracker(); tracker.AddDelete(file1); tracker.AddDelete(file2); - tracker.AcceptAll(); + await tracker.AcceptAll(); await tracker.AssertEmpty(); } diff --git a/src/DiffEngineTray.Tests/TrackerMoveTest.cs b/src/DiffEngineTray.Tests/TrackerMoveTest.cs index 3040482f..583b2a76 100644 --- a/src/DiffEngineTray.Tests/TrackerMoveTest.cs +++ b/src/DiffEngineTray.Tests/TrackerMoveTest.cs @@ -38,7 +38,7 @@ public async Task AcceptAllSingle() { await using var tracker = new RecordingTracker(); tracker.AddMove(file1, file1, "theExe", "theArguments", true, null); - tracker.AcceptAll(); + await tracker.AcceptAll(); await tracker.AssertEmpty(); } @@ -48,7 +48,7 @@ public async Task AcceptAllMultiple() await using var tracker = new RecordingTracker(); tracker.AddMove(file1, file1, "theExe", "theArguments", true, null); tracker.AddMove(file2, file2, "theExe", "theArguments", true, null); - tracker.AcceptAll(); + await tracker.AcceptAll(); await tracker.AssertEmpty(); } diff --git a/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs b/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs new file mode 100644 index 00000000..abf3d61a --- /dev/null +++ b/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs @@ -0,0 +1,935 @@ +extern alias viewer; + +using System.Collections.Concurrent; + +// The viewer's own copies of the protocol types. DiffEngineViewer links DiffEngine's Inline and +// Protocol sources rather than referencing them, so the same names exist in both assemblies and +// only the alias tells them apart. That duplication is the point of these tests: the two halves +// are compiled separately and only ever meet over a socket. +using ViewerSideApplyResult = viewer::DiffEngine.InlineApplyResult; +using ViewerSidePatch = viewer::DiffEngine.InlinePatch; +using ViewerSideServer = viewer::DiffEngine.ViewerServer; +using ViewerSideVerb = viewer::DiffEngine.ViewerVerb; +using ViewerSideWindowCommand = viewer::DiffEngine.WindowCommand; + +// The viewer's own half of the app. +using CommandKind = viewer::CommandKind; +using OwnerLink = viewer::OwnerLink; +using SessionHost = viewer::SessionHost; +using SessionMessageHandler = viewer::MessageHandler; +using SessionState = viewer::SessionState; +using ViewerActions = viewer::ViewerActions; +using ViewerMode = viewer::ViewerMode; +using ViewerSession = viewer::ViewerSession; + +/// +/// The tray and the viewer as a pair, over a real socket, with a real on +/// one side and a real on the other. Every other test in this repo holds +/// one half still — drives the tray's queue with raw messages, +/// drives the tray against a — so +/// nothing until now could catch the two agreeing on a verb but disagreeing on what the user is +/// left looking at. +/// +/// Both ownership arrangements are covered, because which process owns the queue is decided by +/// whichever bound the port and the answer changes which code applies an accept: +/// +/// +/// Tray owned: the usual case, since the tray starts at login. The viewer is +/// display only, polls listfull, and forwards every acting command. +/// Viewer owned: a viewer bound the port before the tray started. The viewer applies +/// locally and the tray is the remote control. +/// +/// +/// Tracked moves and deletes are only in the pair for the tray owned case, and deliberately: they +/// live in the tray, and a tray that does not own the queue has no way to publish them to the +/// process that does. +/// +/// +public class TrayViewerSyncTest +{ + #region tray owned + + [Test] + public async Task TrayAcceptAllEmptiesTheAttachedViewer() + { + await using var pair = new TrayOwned(); + pair.Queue(sample, 1); + pair.Queue(other, 7); + var move = pair.AddMove(); + var delete = pair.AddDelete(); + await Assert.That(pair.Pump().Keys()).IsEquivalentTo([Key(sample, 1), Key(other, 7), move.Key, delete.Key]); + + await pair.Tracker.AcceptAll(); + + var viewer = pair.Pump(); + await Assert.That(viewer.Queue).IsEmpty(); + // Nothing left to show, and this window is not what holds the queue, so it closes itself. + await Assert.That(viewer.Exit).IsTrue(); + await Assert.That(pair.Tracker.TrackingAny).IsFalse(); + await Assert.That(File.Exists(move.Target)).IsTrue(); + await Assert.That(File.Exists(delete.File)).IsFalse(); + } + + [Test] + public async Task TrayAcceptOfOneSnapshotLeavesTheRestInTheAttachedViewer() + { + await using var pair = new TrayOwned(); + var snapshot = pair.Queue(sample, 1); + pair.Queue(other, 7); + pair.Pump(); + + await pair.Tracker.Accept(snapshot); + + await Assert.That(pair.Pump().Keys()).IsEquivalentTo([Key(other, 7)]); + await Assert.That(pair.Tracker.Snapshots.Select(_ => _.Key)).IsEquivalentTo([Key(other, 7)]); + } + + [Test] + public async Task TrayDiscardOfOneSnapshotReachesTheAttachedViewer() + { + await using var pair = new TrayOwned(); + var snapshot = pair.Queue(sample, 1); + pair.Queue(other, 7); + pair.Pump(); + + pair.Tracker.Discard(snapshot); + + await Assert.That(pair.Pump().Keys()).IsEquivalentTo([Key(other, 7)]); + } + + /// + /// The tray menu's "Discard (n)", which counts snapshots and tracked files together and so has + /// to sweep both. + /// + [Test] + public async Task TrayDiscardAllEmptiesTheAttachedViewer() + { + await using var pair = new TrayOwned(); + pair.Queue(sample, 1); + var move = pair.AddMove(); + pair.Pump(); + + pair.Tracker.Clear(); + + var viewer = pair.Pump(); + await Assert.That(viewer.Queue).IsEmpty(); + await Assert.That(viewer.Exit).IsTrue(); + // Discarding a move throws its received file away, wherever the discard came from. + await Assert.That(File.Exists(move.Temp)).IsFalse(); + await Assert.That(File.Exists(move.Target)).IsFalse(); + } + + [Test] + public async Task TrayAcceptOfATrackedMoveReachesTheAttachedViewer() + { + await using var pair = new TrayOwned(); + pair.Queue(sample, 1); + var move = pair.AddMove(); + await Assert.That(pair.Pump().Keys()).IsEquivalentTo([Key(sample, 1), move.Key]); + + pair.Tracker.Accept(pair.Tracker.Moves.Single()); + + await Assert.That(pair.Pump().Keys()).IsEquivalentTo([Key(sample, 1)]); + await Assert.That(File.ReadAllText(move.Target)).IsEqualTo("received"); + } + + [Test] + public async Task TrayAcceptOfATrackedDeleteReachesTheAttachedViewer() + { + await using var pair = new TrayOwned(); + pair.Queue(sample, 1); + var delete = pair.AddDelete(); + pair.Pump(); + + pair.Tracker.Accept(pair.Tracker.Deletes.Single()); + + await Assert.That(pair.Pump().Keys()).IsEquivalentTo([Key(sample, 1)]); + await Assert.That(File.Exists(delete.File)).IsFalse(); + } + + /// + /// A passing re-run settles the entry wherever the queue lives, so the window stops offering a + /// snapshot that is already in the source. + /// + [Test] + public async Task ASettleReachesTheAttachedViewer() + { + await using var pair = new TrayOwned(); + pair.Queue(sample, 1); + pair.Queue(other, 7); + pair.Pump(); + + pair.Send(new(ViewerVerb.Settle, Key(sample, 1))); + + await Assert.That(pair.Pump().Keys()).IsEquivalentTo([Key(other, 7)]); + await Assert.That(pair.Tracker.Snapshots.Select(_ => _.Key)).IsEquivalentTo([Key(other, 7)]); + } + + /// + /// The attached viewer's "Accept all" sweeps what it was shown, which is the tray's tracked + /// files as well as the snapshots. + /// + [Test] + public async Task ViewerAcceptAllEmptiesTheTray() + { + await using var pair = new TrayOwned(); + pair.Queue(sample, 1); + pair.Queue(other, 7); + var move = pair.AddMove(); + var delete = pair.AddDelete(); + pair.Pump(); + + pair.Link.Post(ViewerSideVerb.AcceptAll, null); + + var viewer = pair.Pump(); + await Assert.That(viewer.Queue).IsEmpty(); + await Assert.That(pair.Tracker.Snapshots).IsEmpty(); + await Assert.That(pair.Tracker.Moves).IsEmpty(); + await Assert.That(pair.Tracker.Deletes).IsEmpty(); + await Assert.That(File.ReadAllText(move.Target)).IsEqualTo("received"); + await Assert.That(File.Exists(delete.File)).IsFalse(); + } + + [Test] + public async Task ViewerAcceptOfOneSnapshotReachesTheTray() + { + await using var pair = new TrayOwned(); + pair.Queue(sample, 1); + pair.Queue(other, 7); + pair.Pump(); + + pair.Link.Post(ViewerSideVerb.Accept, Key(sample, 1)); + + await Assert.That(pair.Pump().Keys()).IsEquivalentTo([Key(other, 7)]); + await Assert.That(pair.Tracker.Snapshots.Select(_ => _.Key)).IsEquivalentTo([Key(other, 7)]); + await Assert.That(pair.Applied.Select(_ => _.LineHint)).IsEquivalentTo([1]); + } + + [Test] + public async Task ViewerDiscardOfOneSnapshotReachesTheTray() + { + await using var pair = new TrayOwned(); + pair.Queue(sample, 1); + pair.Queue(other, 7); + pair.Pump(); + + pair.Link.Post(ViewerSideVerb.Discard, Key(sample, 1)); + + await Assert.That(pair.Pump().Keys()).IsEquivalentTo([Key(other, 7)]); + await Assert.That(pair.Tracker.Snapshots.Select(_ => _.Key)).IsEquivalentTo([Key(other, 7)]); + await Assert.That(pair.Applied).IsEmpty(); + } + + [Test] + public async Task ViewerDiscardAllEmptiesTheTray() + { + await using var pair = new TrayOwned(); + pair.Queue(sample, 1); + var move = pair.AddMove(); + var delete = pair.AddDelete(); + pair.Pump(); + + pair.Link.Post(ViewerSideVerb.DiscardAll, null); + + await Assert.That(pair.Pump().Queue).IsEmpty(); + await Assert.That(pair.Tracker.TrackingAny).IsFalse(); + await Assert.That(File.Exists(move.Temp)).IsFalse(); + // Discarding a pending delete has always meant leaving the file; the next run re-tracks it. + await Assert.That(File.Exists(delete.File)).IsTrue(); + } + + [Test] + public async Task ViewerAcceptOfATrackedMoveReachesTheTray() + { + await using var pair = new TrayOwned(); + pair.Queue(sample, 1); + var move = pair.AddMove(); + pair.Pump(); + + pair.Link.Post(ViewerSideVerb.Accept, move.Key); + + await Assert.That(pair.Pump().Keys()).IsEquivalentTo([Key(sample, 1)]); + await Assert.That(pair.Tracker.Moves).IsEmpty(); + await Assert.That(File.ReadAllText(move.Target)).IsEqualTo("received"); + } + + /// + /// A failed apply keeps its entry so it can be retried, and both surfaces have to say the same + /// thing about it — the tray in a balloon, the viewer in the entry's status. + /// + [Test] + public async Task AFailedAcceptStaysPendingOnBothSides() + { + await using var pair = new TrayOwned(_ => InlineApplyResult.Failed("the file is locked")); + var snapshot = pair.Queue(sample, 1); + pair.Pump(); + + await pair.Tracker.Accept(snapshot); + + var viewer = pair.Pump(); + await Assert.That(viewer.Queue.Single().Status).IsEqualTo("the file is locked"); + await Assert.That(viewer.Exit).IsFalse(); + await Assert.That(pair.Tracker.Snapshots.Single().Status).IsEqualTo("the file is locked"); + await Assert.That(pair.Failures.Single()).Contains("the file is locked"); + } + + /// + /// A bulk accept that could not apply everything must not report success on either side: what + /// failed stays pending, the window keeps showing it, and the tray says so rather than leaving + /// the menu to offer it again a scan later. + /// + [Test] + public async Task TrayAcceptAllReportsWhatStayedPending() + { + await using var pair = new TrayOwned( + patch => patch.SourceFile == sample + ? InlineApplyResult.Failed("the file is locked") + : InlineApplyResult.Applied); + pair.Queue(sample, 1); + pair.Queue(other, 7); + pair.Pump(); + + await pair.Tracker.AcceptAll(); + + var viewer = pair.Pump(); + await Assert.That(viewer.Keys()).IsEquivalentTo([Key(sample, 1)]); + await Assert.That(viewer.Queue.Single().Status).IsEqualTo("the file is locked"); + await Assert.That(pair.Tracker.Snapshots.Single().Status).IsEqualTo("the file is locked"); + await Assert.That(pair.Failures.Single()).Contains("the file is locked"); + } + + /// + /// Neither surface picks a side of a conflict silently, so a bulk accept from either leaves the + /// entry exactly where the other one would have. + /// + [Test] + public async Task AConflictSurvivesABulkAcceptFromEitherSide() + { + await using var pair = new TrayOwned(); + pair.Queue(sample, 1, "eight", "net8.0"); + pair.Queue(sample, 1, "nine", "net9.0"); + pair.Queue(other, 7); + pair.Pump(); + + await pair.Tracker.AcceptAll(); + await Assert.That(pair.Pump().Keys()).IsEquivalentTo([Key(sample, 1)]); + + pair.Link.Post(ViewerSideVerb.AcceptAll, null); + + var viewer = pair.Pump(); + await Assert.That(viewer.Keys()).IsEquivalentTo([Key(sample, 1)]); + await Assert.That(viewer.Queue.Single().Conflicted).IsTrue(); + await Assert.That(pair.Applied.Select(_ => _.NewContent)).IsEquivalentTo(["new"]); + } + + #endregion + + #region viewer owned + + [Test] + public async Task TrayAcceptAllEmptiesTheOwningViewer() + { + await using var pair = new ViewerOwned(); + pair.Queue(sample, 1); + pair.Queue(other, 7); + await Assert.That(pair.Viewer.Keys()).IsEquivalentTo([Key(sample, 1), Key(other, 7)]); + + await pair.Tracker.AcceptAll(); + + await Assert.That(pair.Viewer.Queue).IsEmpty(); + await Assert.That(pair.Tracker.Snapshots).IsEmpty(); + await Assert.That(pair.Applied.Count).IsEqualTo(2); + } + + [Test] + public async Task TrayAcceptOfOneSnapshotLeavesTheRestInTheOwningViewer() + { + await using var pair = new ViewerOwned(); + var snapshot = pair.Snapshot(sample, 1); + pair.Queue(other, 7); + + await pair.Tracker.Accept(snapshot); + + await Assert.That(pair.Viewer.Keys()).IsEquivalentTo([Key(other, 7)]); + await Assert.That(pair.Tracker.Snapshots.Select(_ => _.Key)).IsEquivalentTo([Key(other, 7)]); + } + + [Test] + public async Task TrayDiscardReachesTheOwningViewer() + { + await using var pair = new ViewerOwned(); + var snapshot = pair.Snapshot(sample, 1); + pair.Queue(other, 7); + + pair.Tracker.Discard(snapshot); + + await Assert.That(pair.Viewer.Keys()).IsEquivalentTo([Key(other, 7)]); + await Assert.That(pair.Applied).IsEmpty(); + } + + [Test] + public async Task TrayDiscardAllEmptiesTheOwningViewer() + { + await using var pair = new ViewerOwned(); + pair.Queue(sample, 1); + pair.Queue(other, 7); + + pair.Tracker.Clear(); + + await Assert.That(pair.Viewer.Queue).IsEmpty(); + await Assert.That(pair.Tracker.Snapshots).IsEmpty(); + await Assert.That(pair.Applied).IsEmpty(); + } + + [Test] + public async Task ViewerAcceptReachesTheTrayListing() + { + await using var pair = new ViewerOwned(); + pair.Queue(sample, 1); + pair.Queue(other, 7); + + pair.Act(CommandKind.Accept, Key(sample, 1)); + + await Assert.That(pair.Viewer.Keys()).IsEquivalentTo([Key(other, 7)]); + await Assert.That(pair.Tracker.Snapshots.Select(_ => _.Key)).IsEquivalentTo([Key(other, 7)]); + } + + [Test] + public async Task ViewerAcceptAllEmptiesTheTrayListing() + { + await using var pair = new ViewerOwned(); + pair.Queue(sample, 1); + pair.Queue(other, 7); + + pair.Act(CommandKind.AcceptAll, null); + + await Assert.That(pair.Viewer.Queue).IsEmpty(); + await Assert.That(pair.Tracker.Snapshots).IsEmpty(); + await Assert.That(pair.Tracker.TrackingAny).IsFalse(); + } + + [Test] + public async Task ViewerDiscardReachesTheTrayListing() + { + await using var pair = new ViewerOwned(); + pair.Queue(sample, 1); + pair.Queue(other, 7); + + pair.Act(CommandKind.Discard, Key(sample, 1)); + + await Assert.That(pair.Viewer.Keys()).IsEquivalentTo([Key(other, 7)]); + await Assert.That(pair.Tracker.Snapshots.Select(_ => _.Key)).IsEquivalentTo([Key(other, 7)]); + await Assert.That(pair.Applied).IsEmpty(); + } + + /// + /// Same as , with the queue on the other side + /// of the socket: the tray reads the failure off the wire rather than out of its own queue. + /// + [Test] + public async Task AFailedAcceptStaysPendingOnBothSidesOfAnOwningViewer() + { + await using var pair = new ViewerOwned(_ => ViewerSideApplyResult.Failed("the file is locked")); + var snapshot = pair.Snapshot(sample, 1); + + await pair.Tracker.Accept(snapshot); + + await Assert.That(pair.Viewer.Queue.Single().Status).IsEqualTo("the file is locked"); + await Assert.That(pair.Tracker.Snapshots.Single().Status).IsEqualTo("the file is locked"); + await Assert.That(pair.Failures.Single()).Contains("the file is locked"); + } + + /// + [Test] + public async Task TrayAcceptAllReportsWhatTheOwningViewerKept() + { + await using var pair = new ViewerOwned( + patch => patch.SourceFile == sample + ? ViewerSideApplyResult.Failed("the file is locked") + : ViewerSideApplyResult.Applied); + pair.Queue(sample, 1); + pair.Queue(other, 7); + + await pair.Tracker.AcceptAll(); + + await Assert.That(pair.Viewer.Keys()).IsEquivalentTo([Key(sample, 1)]); + await Assert.That(pair.Viewer.Queue.Single().Status).IsEqualTo("the file is locked"); + await Assert.That(pair.Tracker.Snapshots.Single().Status).IsEqualTo("the file is locked"); + await Assert.That(pair.Failures.Single()).Contains("the file is locked"); + } + + [Test] + public async Task AConflictIsRefusedTheSameWayFromTheTray() + { + await using var pair = new ViewerOwned(); + pair.Queue(sample, 1, "eight", "net8.0"); + var snapshot = pair.Snapshot(sample, 1, "nine", "net9.0"); + + await pair.Tracker.Accept(snapshot); + + await Assert.That(pair.Viewer.Queue.Single().Conflicted).IsTrue(); + await Assert.That(pair.Applied).IsEmpty(); + await Assert.That(pair.Failures.Single()).Contains("Conflicting snapshots (net8.0 / net9.0), resolve in the viewer"); + } + + #endregion + + #region no tray running + + /// + /// The whole point of the delete verb. With no tray, a stale verified file used to be reported + /// to nothing at all: DiffEngine skipped the send outright, and a delete has no second file to + /// compare against, so no diff tool ever opened for it either. + /// + /// End to end from the public API: DiffEngine, over a real socket, into a real session, onto + /// disk. Nothing is launched here because this viewer already owns the port, which is the same + /// branch a second failing test takes. + /// + /// + [Test] + public async Task ADeleteWithNoTrayReachesTheViewerAndTheFileGoes() + { + await using var pair = new ViewerOwned(); + using var noTray = new NoTray(); + var file = pair.StageStaleFile(); + + DiffRunner.AddDelete(file); + + await Assert.That(pair.Viewer.Keys()).IsEquivalentTo([TrackedKeys.ForDelete(file)]); + + pair.Act(CommandKind.Accept, TrackedKeys.ForDelete(file)); + + await Assert.That(File.Exists(file)).IsFalse(); + await Assert.That(pair.Viewer.Queue).IsEmpty(); + } + + [Test] + public async Task ADiscardedDeleteWithNoTrayLeavesTheFile() + { + await using var pair = new ViewerOwned(); + using var noTray = new NoTray(); + var file = pair.StageStaleFile(); + await DiffRunner.AddDeleteAsync(file); + + pair.Act(CommandKind.Discard, TrackedKeys.ForDelete(file)); + + await Assert.That(File.Exists(file)).IsTrue(); + await Assert.That(pair.Viewer.Queue).IsEmpty(); + } + + /// + /// A move rides along in a window that is already open rather than starting one: DiffRunner has + /// already opened a diff tool for that file pair, and a second window competing with it is not + /// an improvement. + /// + [Test] + public async Task AMoveWithNoTrayReachesTheViewerAndTheFileMoves() + { + await using var pair = new ViewerOwned(); + using var noTray = new NoTray(); + var move = pair.StageMove(); + + PendingFiles.AddMove(move.Temp, move.Target, null, null, false, null); + + await Assert.That(pair.Viewer.Keys()).IsEquivalentTo([move.Key]); + + pair.Act(CommandKind.Accept, move.Key); + + await Assert.That(File.ReadAllText(move.Target)).IsEqualTo("received"); + await Assert.That(File.Exists(move.Temp)).IsFalse(); + } + + [Test] + public async Task ADiscardedMoveWithNoTrayThrowsTheReceivedFileAway() + { + await using var pair = new ViewerOwned(); + using var noTray = new NoTray(); + var move = pair.StageMove(); + await PendingFiles.AddMoveAsync(move.Temp, move.Target, null, null, false, null, Cancel.None); + + pair.Act(CommandKind.Discard, move.Key); + + await Assert.That(File.Exists(move.Temp)).IsFalse(); + await Assert.That(File.Exists(move.Target)).IsFalse(); + } + + /// + /// A re-run stages the same received file again, and one entry is what a reviewer should see. + /// + [Test] + public async Task AResentMoveReplacesItsEntry() + { + await using var pair = new ViewerOwned(); + using var noTray = new NoTray(); + var move = pair.StageMove(); + PendingFiles.AddMove(move.Temp, move.Target, null, null, false, null); + + File.WriteAllText(move.Temp, "second run"); + PendingFiles.AddMove(move.Temp, move.Target, null, null, false, null); + + await Assert.That(pair.Viewer.Queue).HasSingleItem(); + await Assert.That(pair.Viewer.Queue.Single().LeftText).IsEqualTo("second run"); + } + + /// + /// Whoever holds the pending files lists them, so a second viewer attaching to this one shows + /// what it is holding — the same answer a tray owner gives for its own. + /// + [Test] + public async Task AFullListingCarriesTheViewersOwnPendingFiles() + { + await using var pair = new ViewerOwned(); + using var noTray = new NoTray(); + var file = pair.StageStaleFile(); + DiffRunner.AddDelete(file); + + var full = pair.Send(new(ViewerVerb.ListFull)); + var plain = pair.Send(new(ViewerVerb.List)); + + await Assert.That(full.Deletes.Single().File).IsEqualTo(file); + // The plain listing drives the tray menu, which reads its own tracker for these. + await Assert.That(plain.Deletes).IsEmpty(); + } + + /// + /// A tray that owns the queue answers these too, and routes them into the same tracked files + /// the piper port fills. That is not theoretical: a test process that started before the tray + /// has its tray check cached false for good, so its pending files arrive this way for the rest + /// of its life, and were dropped before. + /// + [Test] + public async Task AnOwningTrayTracksWhatArrivesOnTheViewerPort() + { + await using var pair = new TrayOwned(); + var delete = pair.AddStaleFile(); + + pair.Send(new(ViewerVerb.Delete, delete)); + + await Assert.That(pair.Tracker.Deletes.Select(_ => _.File)).IsEquivalentTo([delete]); + await Assert.That(pair.Pump().Keys()).IsEquivalentTo([TrackedKeys.ForDelete(delete)]); + } + + /// + /// The tray check is cached, so this is the state a test process is in, not a property of the + /// machine. Set explicitly rather than assumed, because another test in this project sets it + /// true. + /// +#pragma warning disable CS0618 // DiffEngineTray is the obsolete shim, but its IsRunning is still where the tray check lives. + sealed class NoTray : IDisposable + { + readonly bool previousTray = DiffEngine.DiffEngineTray.IsRunning; + readonly bool previousDisabled = DiffRunner.Disabled; + + public NoTray() + { + DiffEngine.DiffEngineTray.IsRunning = false; + // DisabledChecker turns this on for build servers, and these tests drive the real + // DiffRunner entry points. + DiffRunner.Disabled = false; + } + + public void Dispose() + { + DiffEngine.DiffEngineTray.IsRunning = previousTray; + DiffRunner.Disabled = previousDisabled; + } + } +#pragma warning restore CS0618 + + #endregion + + const string sample = @"c:\repo\SampleTests.cs"; + const string other = @"c:\repo\OtherTests.cs"; + + static string Key(string source, int line) => + InlineKey.For(source, line); + + static string Payload(string source, int line, string content, string? framework) => + InlinePatchFile.Build( + new(source, line, "\"old\"", content) + { + Framework = framework + }); + + /// + /// The temp directory a pair stages its tracked files in, plus the paths a test asserts over. + /// + record TrackedMoveFiles(string Key, string Temp, string Target); + + record TrackedDeleteFile(string Key, string File); + + /// + /// The usual arrangement: this tray bound the port, holds the queue, and a display only viewer + /// polls it. Nothing is launched — stands in for the process the + /// host would otherwise start, and is the window it stands for. + /// + sealed class TrayOwned : IAsyncDisposable + { + public TrayOwned(Func? applier = null) + { + Host = OwnedInlineHost.TryOwn( + Warnings.Add, + new FakeLauncher(), + 0, + patch => + { + Applied.Add(patch); + return applier?.Invoke(patch) ?? InlineApplyResult.Applied; + }) ?? + throw new("Could not bind an ephemeral port."); + Tracker = new(inlineFailed: Failures.Add, inline: Host); + // Wired the way Program does, and before serving starts: a queue change arriving over + // the socket has to reach the listing the tray menu and the icon read, not wait for the + // next two second scan. + Host.Changed = Tracker.Refresh; + Host.TrackedFiles = Tracker; + Host.Start(); + Window = new(SessionState.Start(ViewerMode.Inline)); + Link = new(Window, Host.Port); + Directory.CreateDirectory(root); + } + + public OwnedInlineHost Host { get; } + public RecordingTracker Tracker { get; } + public SessionHost Window { get; } + public OwnerLink Link { get; } + public List Applied { get; } = []; + public List Warnings { get; } = []; + public List Failures { get; } = []; + + readonly string root = TempRoot(); + + /// + /// One turn of the attached viewer's polling thread: post whatever the window queued, then + /// read the owner's queue back into the session. + /// + public SessionState Pump() + { + if (!Link.Pump()) + { + throw new("The queue owner did not answer."); + } + + return Window.State; + } + + public ViewerResponse Send(ViewerMessage message) + { + if (!ViewerClient.TrySend(message, out var response, Host.Port)) + { + throw new($"No response for {message.Verb}."); + } + + return response; + } + + /// + /// A failing inline snapshot arriving from a test process. + /// + public PendingSnapshot Queue(string source, int line, string content = "new", string? framework = null) + { + var response = Send(new(ViewerVerb.Inline, Body: Payload(source, line, content, framework))); + if (!response.Ok) + { + throw new($"The owner refused the patch. {response.Message}"); + } + + return Tracker.Snapshots.Single(_ => _.Key == Key(source, line)); + } + + public TrackedMoveFiles AddMove() + { + // Its own directory, the way DiffEngine stages received files, because accepting a move + // deletes that directory. + var directory = Path.Combine(root, $"move_{Guid.NewGuid():N}"); + Directory.CreateDirectory(directory); + var temp = Path.Combine(directory, "Sample.Test.received.txt"); + File.WriteAllText(temp, "received"); + var target = Path.Combine(root, $"Sample.Test.{Guid.NewGuid():N}.verified.txt"); + Tracker.AddMove(temp, target, null, null, false, null); + return new(TrackedKeys.ForMove(temp), temp, target); + } + + public TrackedDeleteFile AddDelete() + { + var file = AddStaleFile(); + Tracker.AddDelete(file); + return new(TrackedKeys.ForDelete(file), file); + } + + /// + /// Staged but not tracked, for the paths that arrive over a socket rather than being added + /// to the tracker directly. + /// + public string AddStaleFile() + { + var file = Path.Combine(root, $"Stale.{Guid.NewGuid():N}.verified.txt"); + File.WriteAllText(file, "stale"); + return file; + } + + public async ValueTask DisposeAsync() + { + await Tracker.DisposeAsync(); + await Host.DisposeAsync(); + FileEx.SafeDeleteDirectory(root); + } + } + + /// + /// A viewer that bound the port before this tray started, so it owns the queue and applies + /// locally while the tray drives it over the wire. + /// + /// Also the shape a machine with no tray installed is in, which is why this harness holds the + /// pending files too: DiffEngine addresses the queue owner when no tray answered its startup + /// check, so the moves and deletes land here rather than in a tracker. + /// + /// + sealed class ViewerOwned : IAsyncDisposable + { + public ViewerOwned(Func? applier = null) + { + if (!ViewerSideServer.TryBind(0, out var bound)) + { + throw new("Could not bind an ephemeral port."); + } + + server = bound; + // Points the tray's RemoteInlineHost, and DiffEngine's own sends, here — and keeps a + // viewer that happens to be running on this machine out of the way. + previousPort = Environment.GetEnvironmentVariable(ViewerClient.PortVariable); + Environment.SetEnvironmentVariable(ViewerClient.PortVariable, server.Port.ToString()); + Window = new(SessionState.Start(ViewerMode.Inline)); + actions = new ViewerActions( + patch => + { + Applied.Add(patch); + return applier?.Invoke(patch) ?? ViewerSideApplyResult.Applied; + }, + (_, _) => throw new("A queued snapshot is never accepted by copying a file."), + _ => + { + }) + { + // The real ones, so accepting a pending file here is the file operation itself + // rather than a recording of one. + MoveFile = ViewerActions.Real.MoveFile, + DeleteFile = ViewerActions.Real.DeleteFile + }; + var handler = new SessionMessageHandler(Window, actions, Windows.Enqueue); + listening = server.Listen(handler.Handle, cancel.Token); + Tracker = new(inlineFailed: Failures.Add); + Directory.CreateDirectory(root); + } + + readonly ViewerSideServer server; + readonly CancelSource cancel = new(); + readonly Task listening; + readonly string? previousPort; + readonly ViewerActions actions; + readonly string root = TempRoot(); + + /// + /// A verified file a passing test no longer produces, which is what DiffEngine reports as + /// a pending delete. + /// + public string StageStaleFile() + { + var file = Path.Combine(root, $"Stale.{Guid.NewGuid():N}.verified.txt"); + File.WriteAllText(file, "stale"); + return file; + } + + public TrackedMoveFiles StageMove() + { + var directory = Path.Combine(root, $"move_{Guid.NewGuid():N}"); + Directory.CreateDirectory(directory); + var temp = Path.Combine(directory, "Sample.Test.received.txt"); + File.WriteAllText(temp, "received"); + var target = Path.Combine(root, $"Sample.Test.{Guid.NewGuid():N}.verified.txt"); + return new(TrackedKeys.ForMove(temp), temp, target); + } + + public SessionHost Window { get; } + public RecordingTracker Tracker { get; } + public List Applied { get; } = []; + public List Failures { get; } = []; + public ConcurrentQueue Windows { get; } = new(); + + public SessionState Viewer => Window.State; + + public void Queue(string source, int line, string content = "new", string? framework = null) + { + var message = new ViewerMessage(ViewerVerb.Inline, Body: Payload(source, line, content, framework)); + if (!ViewerClient.TrySend(message, out var response, server.Port) || + !response.Ok) + { + throw new($"The owner refused the patch. {response?.Message}"); + } + } + + public PendingSnapshot Snapshot(string source, int line, string content = "new", string? framework = null) + { + Queue(source, line, content, framework); + return Tracker.Snapshots.Single(_ => _.Key == Key(source, line)); + } + + public ViewerResponse Send(ViewerMessage message) + { + if (!ViewerClient.TrySend(message, out var response, server.Port)) + { + throw new($"No response for {message.Verb}."); + } + + return response; + } + + /// + /// What the window does with a command when it owns the queue: applies it here, selecting + /// the entry first the way every acting path does. The forwarding branch is the other + /// arrangement, and covers it. + /// + public void Act(CommandKind command, string? key) => + Window.Mutate(state => + { + if (key is not null) + { + state = ViewerSession.SelectKey(state, key); + } + + return ViewerSession.Apply(state, command, actions); + }); + + public async ValueTask DisposeAsync() + { + await Tracker.DisposeAsync(); + await cancel.CancelAsync(); + server.Dispose(); + try + { + await listening.WaitAsync(TimeSpan.FromSeconds(5)); + } + catch (Exception exception) + when (exception is OperationCanceledException or TimeoutException) + { + // Cancellation unwinds through the listener; nothing to report. + } + + cancel.Dispose(); + Environment.SetEnvironmentVariable(ViewerClient.PortVariable, previousPort); + FileEx.SafeDeleteDirectory(root); + } + } + + static string TempRoot() => + Path.Combine(Path.GetTempPath(), $"TrayViewerSync_{Guid.NewGuid():N}"); +} + +static class TrayViewerSyncExtensions +{ + /// + /// What the window is showing, in display order, as the keys both sides address entries by. + /// + public static IReadOnlyList Keys(this SessionState state) => + state.Queue.Select(_ => _.Key).ToList(); +} diff --git a/src/DiffEngineTray/ITrackedFiles.cs b/src/DiffEngineTray/ITrackedFiles.cs index b5492c4e..18bc8409 100644 --- a/src/DiffEngineTray/ITrackedFiles.cs +++ b/src/DiffEngineTray/ITrackedFiles.cs @@ -30,5 +30,15 @@ interface ITrackedFiles /// (int accepted, int kept) AcceptAll(); + /// + /// Track a pending move or delete that arrived over the viewer port rather than the piper one. + /// That happens when the sending process saw no tray at startup and this tray started after + /// it: that check is cached for the life of the sender, so its files come the other way for + /// good, and dropping them would lose them. + /// + void AddMove(string temp, string target); + + void AddDelete(string file); + int DiscardAll(); } diff --git a/src/DiffEngineTray/MenuBuilder.cs b/src/DiffEngineTray/MenuBuilder.cs index 5eb2965c..f6438dad 100644 --- a/src/DiffEngineTray/MenuBuilder.cs +++ b/src/DiffEngineTray/MenuBuilder.cs @@ -111,7 +111,7 @@ static IEnumerable BuildTrackingMenuItems(Tracker tracker) } yield return new MenuButton($"Discard ({count})", tracker.Clear, Images.Discard); - yield return new MenuButton($"Accept all ({count})", tracker.AcceptAll, Images.AcceptAll); + yield return new MenuButton($"Accept all ({count})", () => tracker.AcceptAll(), Images.AcceptAll); } static IEnumerable BuildGroupedMenuItems( diff --git a/src/DiffEngineTray/OwnedInlineHost.cs b/src/DiffEngineTray/OwnedInlineHost.cs index dff5739a..2d946683 100644 --- a/src/DiffEngineTray/OwnedInlineHost.cs +++ b/src/DiffEngineTray/OwnedInlineHost.cs @@ -185,6 +185,23 @@ void IQueueOwner.Settle(string key, string? origin) Changed?.Invoke(); } + /// + /// Straight into the tracked files, which is where a piper move would have landed. Reaches + /// this tray when the sending process saw no tray as it started and so addressed the queue + /// owner instead — and this tray is the queue owner. + /// + void IQueueOwner.TrackMove(string temp, string target) + { + TrackedFiles?.AddMove(temp, target); + Changed?.Invoke(); + } + + void IQueueOwner.TrackDelete(string file) + { + TrackedFiles?.AddDelete(file); + Changed?.Invoke(); + } + ViewerResponse IQueueOwner.Listing(bool withPatches) { // Read outside the gate: the tracked collections are concurrent, and only the full diff --git a/src/DiffEngineTray/Program.cs b/src/DiffEngineTray/Program.cs index 824b6c34..1bb2ab9c 100644 --- a/src/DiffEngineTray/Program.cs +++ b/src/DiffEngineTray/Program.cs @@ -155,12 +155,12 @@ internal static IEnumerable BuildKeyBindings(Settings settings, Trac if (settings.AcceptAllHotKey is { } acceptAll) { - yield return new(KeyBindingIds.AcceptAll, acceptAll, tracker.AcceptAll); + yield return new(KeyBindingIds.AcceptAll, acceptAll, () => tracker.AcceptAll()); } if (settings.AcceptOpenHotKey is { } acceptOpen) { - yield return new(KeyBindingIds.AcceptOpen, acceptOpen, tracker.AcceptOpen); + yield return new(KeyBindingIds.AcceptOpen, acceptOpen, () => tracker.AcceptOpen()); } } diff --git a/src/DiffEngineTray/RemoteInlineHost.cs b/src/DiffEngineTray/RemoteInlineHost.cs index e464da21..31197ddd 100644 --- a/src/DiffEngineTray/RemoteInlineHost.cs +++ b/src/DiffEngineTray/RemoteInlineHost.cs @@ -31,20 +31,46 @@ public IReadOnlyList List() null; /// - /// Applied or failed only. The wire carries ok and a message, not an apply status, so a - /// stale patch reads as applied here. It costs nothing: the owner is a viewer, and it is - /// showing that message in its own footer. + /// Applied or failed, decided by whether the entry is still there afterwards rather than by + /// ok. + /// + /// The wire carries ok and a message, not an apply status, and every owner keeps a + /// failed entry pending so it can be retried — an accept that could not write the file is + /// still an accept that was attempted. Taking ok at face value reported that snapshot + /// as applied while the viewer was still showing it, and the menu offered it again on the next + /// scan. A tray that owns the queue has never had that problem, because it reads the outcome + /// out of its own , so the two arrangements disagreed about the same + /// click. + /// + /// + /// A stale patch still reads as applied: it is dropped rather than kept, and from here that is + /// indistinguishable. It costs nothing, because the owner is a viewer and it is showing that + /// message in its own footer. + /// /// - public AcceptOutcome Accept(PendingSnapshot snapshot, out string? message) => - Send(ViewerVerb.Accept, snapshot.Key, out message) - ? AcceptOutcome.Applied - : AcceptOutcome.Failed; + public AcceptOutcome Accept(PendingSnapshot snapshot, out string? message) + { + if (!Send(ViewerVerb.Accept, snapshot.Key, out message)) + { + return AcceptOutcome.Failed; + } + + return List().Any(_ => _.Key == snapshot.Key) + ? AcceptOutcome.Failed + : AcceptOutcome.Applied; + } public bool Discard(PendingSnapshot snapshot, out string? message) => Send(ViewerVerb.Discard, snapshot.Key, out message); + /// + /// True only when the queue is empty afterwards, for the reason gives — + /// and matching what an owning tray reports, which is also "is anything still pending". A + /// conflict counts as not accepted, which is right: it is what a reviewer still has to resolve. + /// public bool AcceptAll(out string? message) => - Send(ViewerVerb.AcceptAll, null, out message); + Send(ViewerVerb.AcceptAll, null, out message) && + List().Count == 0; public void DiscardAll() => Send(ViewerVerb.DiscardAll, null, out _); diff --git a/src/DiffEngineTray/Settings/OptionsFormLauncher.cs b/src/DiffEngineTray/Settings/OptionsFormLauncher.cs index fd9470a5..1b40bdde 100644 --- a/src/DiffEngineTray/Settings/OptionsFormLauncher.cs +++ b/src/DiffEngineTray/Settings/OptionsFormLauncher.cs @@ -28,9 +28,9 @@ static async Task> Save(KeyRegister keyRegister, Tra var saveErrors = new List(); - AddHotKey(keyRegister, settings.AcceptAllHotKey, KeyBindingIds.AcceptAll, tracker.AcceptAll, saveErrors); + AddHotKey(keyRegister, settings.AcceptAllHotKey, KeyBindingIds.AcceptAll, () => tracker.AcceptAll(), saveErrors); AddHotKey(keyRegister, settings.DiscardAllHotKey, KeyBindingIds.DiscardAll, tracker.Clear, saveErrors); - AddHotKey(keyRegister, settings.AcceptOpenHotKey, KeyBindingIds.AcceptOpen, tracker.AcceptOpen, saveErrors); + AddHotKey(keyRegister, settings.AcceptOpenHotKey, KeyBindingIds.AcceptOpen, () => tracker.AcceptOpen(), saveErrors); if (saveErrors.Count != 0) { diff --git a/src/DiffEngineTray/Tracker.cs b/src/DiffEngineTray/Tracker.cs index 15948f7e..fe60f541 100644 --- a/src/DiffEngineTray/Tracker.cs +++ b/src/DiffEngineTray/Tracker.cs @@ -615,25 +615,34 @@ static void KillProcesses(TrackedMove move) move.Process.KillAndDispose(); } + /// + /// The menu's "Discard (n)". Everything pending goes, on every surface. + /// + /// Through the same discard the wire uses, so the two surfaces cannot mean different things by + /// it. Discarding a move throws its received file away — has + /// always done that, and so does a discard arriving from the viewer — and sweeping the + /// dictionary directly left the temps behind for a button that said it had discarded them. + /// + /// + /// The snapshots go too: the menu counts them in "Discard (n)". Clearing only the cache used to + /// make the button lie twice over — it discarded fewer things than it said, and the ones it + /// skipped came back on the next scan two seconds later. + /// + /// public void Clear() { - deletes.Clear(); + ((ITrackedFiles) this).DiscardAll(); - foreach (var move in moves.Values) - { - KillProcesses(move); - } - - moves.Clear(); - - // The menu counts snapshots in "Discard (n)", so discarding has to include them. Clearing - // only the cache used to make the button lie twice over: it discarded fewer things than it - // said, and the ones it skipped came back on the next scan two seconds later. inline.DiscardAll(); snapshots = []; } - public void AcceptOpen() + /// + /// The returned task covers the snapshot half, which runs on a worker for the reason + /// gives. The menu and the hot keys discard it; tests + /// await it so what the other surface should now be showing is settled rather than in flight. + /// + public Task AcceptOpen() { AcceptAllDeletes(); @@ -644,16 +653,17 @@ public void AcceptOpen() // Every pending snapshot is open by definition: the viewer only stays running while it // has something to show. - AcceptAllSnapshots(); + return AcceptAllSnapshots(); } - public void AcceptAll() + /// + public Task AcceptAll() { AcceptAllDeletes(); AcceptMoves(moves.Values); - AcceptAllSnapshots(); + return AcceptAllSnapshots(); } void AcceptAllDeletes() @@ -689,6 +699,20 @@ IReadOnlyList ITrackedFiles.Deletes() => _.File)) .ToList(); + void ITrackedFiles.AddMove(string temp, string target) + { + // No exe, arguments or process: the sender's diff tool details do not cross the viewer + // port, so this is resolved from the extension exactly as a piper move with no exe is. + AddMove(temp, target, null, null, false, null); + Refresh(); + } + + void ITrackedFiles.AddDelete(string file) + { + AddDelete(file); + Refresh(); + } + bool ITrackedFiles.Has(string key) { if (TrackedKeys.TryStrip(key, TrackedKeys.MovePrefix, out var temp)) @@ -854,9 +878,22 @@ public IReadOnlyList Snapshots } } + /// + /// Deliberately not : exiting is not discarding. The diff tools this tray + /// started are killed, and everything pending stays where it is — the received files on disk + /// for the next tray to re-track, and the inline queue with whoever owns it, which outlives + /// this process whenever that is a viewer. + /// public ValueTask DisposeAsync() { - Clear(); + foreach (var move in moves.Values) + { + KillProcesses(move); + } + + moves.Clear(); + deletes.Clear(); + snapshots = []; return timer.DisposeAsync(); } } \ No newline at end of file diff --git a/src/DiffEngineViewer.Tests/CommandLineTests.Attach.verified.txt b/src/DiffEngineViewer.Tests/CommandLineTests.Attach.verified.txt index 7febb4ab..6ebda751 100644 --- a/src/DiffEngineViewer.Tests/CommandLineTests.Attach.verified.txt +++ b/src/DiffEngineViewer.Tests/CommandLineTests.Attach.verified.txt @@ -1,4 +1,5 @@ { Mode: Inline, - Attach: true + Attach: true, + Delete: false } \ 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 dd64547a..eb6430d9 100644 --- a/src/DiffEngineViewer.Tests/CommandLineTests.Files.verified.txt +++ b/src/DiffEngineViewer.Tests/CommandLineTests.Files.verified.txt @@ -1,5 +1,6 @@ { Left: left.txt, Right: right.txt, - Attach: false + Attach: false, + Delete: 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 66e0c670..7cb17c40 100644 --- a/src/DiffEngineViewer.Tests/CommandLineTests.Inline.verified.txt +++ b/src/DiffEngineViewer.Tests/CommandLineTests.Inline.verified.txt @@ -2,5 +2,6 @@ Mode: Inline, Source: Tests.cs, Line: 42, - Attach: false + Attach: false, + Delete: 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 66e0c670..7cb17c40 100644 --- a/src/DiffEngineViewer.Tests/CommandLineTests.InlineArgumentsReordered.verified.txt +++ b/src/DiffEngineViewer.Tests/CommandLineTests.InlineArgumentsReordered.verified.txt @@ -2,5 +2,6 @@ Mode: Inline, Source: Tests.cs, Line: 42, - Attach: false + Attach: false, + Delete: false } \ No newline at end of file diff --git a/src/DiffEngineViewer.Tests/TrackedFileTests.cs b/src/DiffEngineViewer.Tests/TrackedFileTests.cs new file mode 100644 index 00000000..eceb0aed --- /dev/null +++ b/src/DiffEngineViewer.Tests/TrackedFileTests.cs @@ -0,0 +1,225 @@ +/// +/// Pending moves and deletes in a viewer that owns them, which is what happens with no tray +/// running: DiffEngine addresses the queue owner instead of the piper port, and a delete even +/// starts this window, because it has no diff tool to open. +/// +/// The attached case is the other half and behaves differently by design — there the entries +/// belong to the tray and every command is forwarded rather than applied, which +/// TrayViewerSyncTest covers. +/// +/// +public class TrackedFileTests +{ + [Test] + public async Task AcceptingADeleteRemovesTheFile() + { + var done = new List(); + var state = Owned(Fixtures.Delete()); + + var accepted = Act(state, CommandKind.Accept, Tracking(done)); + + await Assert.That(done).IsEquivalentTo(["delete code/extra.verified.txt"]); + await Assert.That(accepted.Queue).IsEmpty(); + await Assert.That(accepted.Message).IsEqualTo("Accepted extra.verified.txt"); + } + + /// + /// Discarding a pending delete has always meant leaving the file and only untracking it. The + /// next run re-tracks it. + /// + [Test] + public async Task DiscardingADeleteLeavesTheFile() + { + var done = new List(); + var state = Owned(Fixtures.Delete()); + + var discarded = Act(state, CommandKind.Discard, Tracking(done)); + + await Assert.That(done).IsEmpty(); + await Assert.That(discarded.Queue).IsEmpty(); + await Assert.That(discarded.Message).IsEqualTo("Discarded extra.verified.txt"); + } + + [Test] + public async Task AcceptingAMoveMovesTheReceivedFileOverTheTarget() + { + var done = new List(); + var state = Owned(Fixtures.Move()); + + var accepted = Act(state, CommandKind.Accept, Tracking(done)); + + await Assert.That(done).IsEquivalentTo(["move temp/sample.received.txt > code/sample.verified.txt"]); + await Assert.That(accepted.Queue).IsEmpty(); + } + + [Test] + public async Task DiscardingAMoveThrowsTheReceivedFileAway() + { + var done = new List(); + var state = Owned(Fixtures.Move()); + + var discarded = Act(state, CommandKind.Discard, Tracking(done)); + + await Assert.That(done).IsEquivalentTo(["delete temp/sample.received.txt"]); + await Assert.That(discarded.Queue).IsEmpty(); + } + + /// + /// The one that matters most. Every inline command rebuilds its half of the queue from + /// , and an owning viewer holds tracked files beside the snapshots, so + /// accepting one snapshot must not take the pending files with it. + /// + [Test] + public async Task AcceptingASnapshotKeepsTheFilesPendingBesideIt() + { + var state = Owned(Fixtures.Move(), Fixtures.Delete()); + state = ViewerSession.EnqueueInline(state, Fixtures.Patch()); + state = ViewerSession.SelectKey(state, QueueEntry.KeyForInline("SampleTests.cs", 42)); + + var accepted = ViewerSession.Apply(state, CommandKind.Accept, Fixtures.Applied); + + await Assert.That(accepted.Queue.Select(_ => _.Kind)) + .IsEquivalentTo([QueueEntryKind.Move, QueueEntryKind.Delete]); + await Assert.That(accepted.Exit).IsFalse(); + } + + [Test] + public async Task SettlingASnapshotKeepsTheFilesPendingBesideIt() + { + var state = Owned(Fixtures.Delete()); + state = ViewerSession.EnqueueInline(state, Fixtures.Patch()); + + var settled = ViewerSession.Settle(state, QueueEntry.KeyForInline("SampleTests.cs", 42)); + + await Assert.That(settled.Queue.Single().Kind).IsEqualTo(QueueEntryKind.Delete); + } + + /// + /// Worded the way an owning tray words its own sweep, so the same click reads the same + /// whichever process is holding the files. + /// + [Test] + public async Task AcceptAllSweepsTheSnapshotsAndTheFiles() + { + var done = new List(); + var state = Owned(Fixtures.Move(), Fixtures.Delete()); + state = ViewerSession.EnqueueInline(state, Fixtures.Patch()); + + var accepted = ViewerSession.Apply(state, CommandKind.AcceptAll, Tracking(done)); + + await Assert.That(done.Count).IsEqualTo(2); + await Assert.That(accepted.Queue).IsEmpty(); + await Assert.That(accepted.Message).IsEqualTo("Accepted 1, plus 2 files"); + } + + [Test] + public async Task DiscardAllSweepsTheSnapshotsAndTheFiles() + { + var done = new List(); + var state = Owned(Fixtures.Move(), Fixtures.Delete()); + state = ViewerSession.EnqueueInline(state, Fixtures.Patch()); + + var discarded = ViewerSession.Apply(state, CommandKind.DiscardAll, Tracking(done)); + + // The move's received file goes, the pending delete's file stays. + await Assert.That(done).IsEquivalentTo(["delete temp/sample.received.txt"]); + await Assert.That(discarded.Queue).IsEmpty(); + await Assert.That(discarded.Message).IsEqualTo("Discarded 1, plus 2 files"); + } + + [Test] + public async Task ASweepCountsWhatItCouldNotApply() + { + var state = Owned(Fixtures.Move(), Fixtures.Delete()); + + var accepted = ViewerSession.Apply(state, CommandKind.AcceptAll, Failing("the file is locked")); + + await Assert.That(accepted.Message).IsEqualTo("Accepted 0, plus 0 files (2 kept)"); + await Assert.That(accepted.Queue.All(_ => _.Status == "the file is locked")).IsTrue(); + } + + /// + /// Kept pending carrying the reason, so it can be retried once whatever holds the file is + /// gone — the same bargain a failed inline apply makes. + /// + [Test] + public async Task AFailedAcceptKeepsItsEntryAndSaysWhy() + { + var state = Owned(Fixtures.Delete()); + + var accepted = Act(state, CommandKind.Accept, Failing("the file is locked")); + + await Assert.That(accepted.Queue.Single().Status).IsEqualTo("the file is locked"); + await Assert.That(accepted.Message).IsEqualTo("the file is locked"); + await Assert.That(accepted.Exit).IsFalse(); + } + + /// + /// A re-run stages the same received file again, and a second entry for it would be a + /// duplicate rather than news. + /// + [Test] + public async Task TrackingTheSameFileAgainReplacesItsEntry() + { + var state = Owned(Fixtures.Move(left: "first")); + + var again = ViewerSession.EnqueueTracked(state, Fixtures.Move(left: "second")); + + await Assert.That(again.Queue).HasSingleItem(); + await Assert.That(again.Queue.Single().LeftText).IsEqualTo("second"); + } + + /// + /// A solution header spans tracked files as well as snapshots, so its sweep has to as well: + /// "Accept all in ..." must not quietly mean "accept the snapshots in ...". + /// + [Test] + public async Task AGroupAcceptSweepsThatGroupsFiles() + { + var done = new List(); + var state = Owned( + Fixtures.Move(solution: "Alpha"), + Fixtures.Delete(solution: "Beta")); + + var menu = ViewerSession.OpenMenu(state, 0); + var accepted = ViewerSession.Apply(menu, CommandKind.AcceptGroup, Tracking(done)); + + await Assert.That(done).IsEquivalentTo(["move temp/sample.received.txt > code/sample.verified.txt"]); + await Assert.That(accepted.Queue.Single().Kind).IsEqualTo(QueueEntryKind.Delete); + } + + static SessionState Owned(params QueueEntry[] tracked) + { + var state = SessionState.Start(ViewerMode.Inline, Fixtures.Columns, Fixtures.Rows); + foreach (var entry in tracked) + { + state = ViewerSession.EnqueueTracked(state, entry); + } + + return state; + } + + /// + /// Acts on the first entry, which is what the window does with the selection it is showing. + /// + static SessionState Act(SessionState state, CommandKind command, ViewerActions actions) => + ViewerSession.Apply(ViewerSession.Apply(state, Command.Select(0)), command, actions); + + /// + /// Records rather than performs, so accepting a pending file is reachable without staging one + /// on disk — the same thing does for patches. + /// + static ViewerActions Tracking(List done) => + Fixtures.Applied with + { + MoveFile = (temp, target) => done.Add($"move {temp} > {target}"), + DeleteFile = file => done.Add($"delete {file}") + }; + + static ViewerActions Failing(string message) => + Fixtures.Applied with + { + MoveFile = (_, _) => throw new IOException(message), + DeleteFile = _ => throw new IOException(message) + }; +} diff --git a/src/DiffEngineViewer/CommandLine.cs b/src/DiffEngineViewer/CommandLine.cs index 2f491efe..1ce092ae 100644 --- a/src/DiffEngineViewer/CommandLine.cs +++ b/src/DiffEngineViewer/CommandLine.cs @@ -3,9 +3,11 @@ static class CommandLine public const string Usage = """ DiffEngineViewer DiffEngineViewer --inline --source --line + DiffEngineViewer --delete DiffEngineViewer --attach Inline mode reads the patch payload from stdin. + Delete mode takes a file that a passing test no longer produces. Attach mode reads nothing, and displays the queue of whoever owns the port. """; @@ -31,6 +33,21 @@ public static ViewerRequest Parse(IReadOnlyList args) return ParseInline(args); } + if (args[0] == "--delete") + { + if (args.Count != 2) + { + return Error("--delete takes one file."); + } + + // Queue mode, not file mode: a delete owns the port and more can arrive after it, + // which is the whole difference between the two modes. + return new(ViewerMode.Inline, args[1], null, null, 0, null) + { + Delete = true + }; + } + if (args.Count != 2) { return Error($"Expected two file paths, got {args.Count} arguments."); diff --git a/src/DiffEngineViewer/InternalsVisibleTo.cs b/src/DiffEngineViewer/InternalsVisibleTo.cs index 42b2c4a2..ae24ed56 100644 --- a/src/DiffEngineViewer/InternalsVisibleTo.cs +++ b/src/DiffEngineViewer/InternalsVisibleTo.cs @@ -1,4 +1,8 @@ [assembly: InternalsVisibleTo("DiffEngineViewer.Tests, PublicKey=00240000048000009400000006020000002400005253413100040000010001000f0a8e4bf1639dce01be6592384e7dfc621915b7759fb5cee42ec5d351bcc43460432da1659ee618ca6cab6b8b8e56a5deb5d4ee1a49783d5c2690752502d31ccbfee9b2c697e20359b55ad100cc9370c8e983fd9496f01d761a060d0435bac7243b1832ba95757aa5adbb67df38c213d717b6751e1217cea9fa5c61e9b799dd")] +// TrayViewerSyncTest drives a real viewer session against a real tray over a socket, which is the +// only place both halves of the pair exist in one process. It lives in the tray's test project +// because the tray is Windows only and the viewer is not. +[assembly: InternalsVisibleTo("DiffEngineTray.Tests, PublicKey=00240000048000009400000006020000002400005253413100040000010001000f0a8e4bf1639dce01be6592384e7dfc621915b7759fb5cee42ec5d351bcc43460432da1659ee618ca6cab6b8b8e56a5deb5d4ee1a49783d5c2690752502d31ccbfee9b2c697e20359b55ad100cc9370c8e983fd9496f01d761a060d0435bac7243b1832ba95757aa5adbb67df38c213d717b6751e1217cea9fa5c61e9b799dd")] [assembly: InternalsVisibleTo("DiffEngineViewer.Windows.Tests, PublicKey=00240000048000009400000006020000002400005253413100040000010001000f0a8e4bf1639dce01be6592384e7dfc621915b7759fb5cee42ec5d351bcc43460432da1659ee618ca6cab6b8b8e56a5deb5d4ee1a49783d5c2690752502d31ccbfee9b2c697e20359b55ad100cc9370c8e983fd9496f01d761a060d0435bac7243b1832ba95757aa5adbb67df38c213d717b6751e1217cea9fa5c61e9b799dd")] // One entry covers all three platform heads, because all three take the DiffEngineViewer assembly diff --git a/src/DiffEngineViewer/Ipc/MessageHandler.cs b/src/DiffEngineViewer/Ipc/MessageHandler.cs index d2472433..f699702e 100644 --- a/src/DiffEngineViewer/Ipc/MessageHandler.cs +++ b/src/DiffEngineViewer/Ipc/MessageHandler.cs @@ -16,20 +16,49 @@ int IQueueOwner.Enqueue(InlinePatch patch) => void IQueueOwner.Settle(string key, string? origin) => host.Mutate(_ => ViewerSession.Settle(_, key, origin)); + /// + /// The files are read here, on the listener thread, so the session stays IO free — the same + /// seam materializes the tray's tracked files through. + /// + void IQueueOwner.TrackMove(string temp, string target) => + host.Mutate(_ => ViewerSession.EnqueueTracked(_, TrackedEntry.ForMove(temp, target))); + + void IQueueOwner.TrackDelete(string file) => + host.Mutate(_ => ViewerSession.EnqueueTracked(_, TrackedEntry.ForDelete(file))); + /// /// With patches, each item carries the payloads it was queued from — every variant of it — /// so a viewer showing someone else's queue can rebuild every pane locally and no diff has /// to cross the wire. Through the shared projection, so a conflicted entry lists identically /// whichever process owns the queue. + /// + /// The tracked moves and deletes ride a full listing only, matching a tray owner: the plain + /// listing drives the tray menu, which reads its own tracker rather than the wire for those. + /// /// ViewerResponse IQueueOwner.Listing(bool withPatches) { + var queue = host.State.Queue; var items = ViewerListing.Items( - host.State.Queue + queue .Where(_ => _.Kind == QueueEntryKind.Inline) .Select(_ => new PendingInline(_.Variants, _.Status)), withPatches); - return ViewerResponse.Listing(items); + if (!withPatches) + { + return ViewerResponse.Listing(items); + } + + return ViewerResponse.Listing( + items, + moves: queue + .Where(_ => _.Kind == QueueEntryKind.Move) + .Select(_ => new ViewerResponseMove(_.Key, _.Name, _.Solution, _.LeftFile!, _.TargetFile!)) + .ToList(), + deletes: queue + .Where(_ => _.Kind == QueueEntryKind.Delete) + .Select(_ => new ViewerResponseDelete(_.Key, _.Name, _.Solution, _.LeftFile!)) + .ToList()); } bool IQueueOwner.Has(string key) => diff --git a/src/DiffEngineViewer/TrackedEntry.cs b/src/DiffEngineViewer/TrackedEntry.cs new file mode 100644 index 00000000..6863f5cd --- /dev/null +++ b/src/DiffEngineViewer/TrackedEntry.cs @@ -0,0 +1,42 @@ +/// +/// The queue entry for a pending move or delete this viewer owns. +/// +/// Derives the same key, name and group DiffEngineTray derives for the ones it owns, so a file +/// pending here is indistinguishable in the window from one pending there — which matters because +/// which process is tracking it depends only on whether a tray happened to be running. +/// +/// +/// Reads the files, so this is called on the listener thread rather than inside +/// , the same seam uses for the tray's. +/// +/// +static class TrackedEntry +{ + public static QueueEntry ForMove(string temp, string target) => + QueueEntry.ForMove( + TrackedKeys.ForMove(temp), + $"{Name(target)} ({Extension(target)})", + SolutionDirectoryFinder.Find(target), + temp, + target, + FileSide.Read(temp), + FileSide.Read(target)); + + public static QueueEntry ForDelete(string file) => + QueueEntry.ForDelete( + TrackedKeys.ForDelete(file), + Path.GetFileName(file), + SolutionDirectoryFinder.Find(file), + file, + FileSide.Read(file)); + + /// + /// Twice, because a verified file carries two: Sample.Test.verified.txt is the test + /// Sample.Test. Exactly what TrackedMove does with the same path. + /// + static string Name(string target) => + Path.GetFileNameWithoutExtension(Path.GetFileNameWithoutExtension(target)); + + static string Extension(string target) => + Path.GetExtension(target).TrimStart('.'); +} diff --git a/src/DiffEngineViewer/ViewerActions.cs b/src/DiffEngineViewer/ViewerActions.cs index d170fab0..e0f7c5fc 100644 --- a/src/DiffEngineViewer/ViewerActions.cs +++ b/src/DiffEngineViewer/ViewerActions.cs @@ -7,17 +7,73 @@ record ViewerActions( Action CopyFile, Action Reveal) { + /// + /// Accepting a tracked move: the received file over the target. + /// + /// Init rather than positional, and throwing by default, because only a viewer that owns the + /// queue ever reaches it — one displaying someone else's forwards the key instead, and file + /// mode has no tracked entries at all. A caller that turns out to need it and did not supply + /// one fails loudly, the same bargain makes for the rest. + /// + /// + public Action MoveFile { get; init; } = Missing; + + /// + /// Accepting a tracked delete, and discarding a tracked move — the two cases where a pending + /// file is the thing that goes. + /// + public Action DeleteFile { get; init; } = Missing; + public static readonly ViewerActions Real = new( InlineApplier.Apply, static (source, destination) => File.Copy(source, destination, true), - RevealFile.Show); + RevealFile.Show) + { + MoveFile = Move, + DeleteFile = File.Delete + }; /// /// Refuses everything. Held by the view only Apply overload, so a command that turns /// out to need IO fails loudly rather than quietly doing nothing. /// public static readonly ViewerActions None = new( - static _ => throw new("This command was applied as view only, but needs real actions."), - static (_, _) => throw new("This command was applied as view only, but needs real actions."), - static _ => throw new("This command was applied as view only, but needs real actions.")); + static _ => throw new(missing), + static (_, _) => throw new(missing), + static _ => throw new(missing)); + + const string missing = "This command was applied as view only, but needs real actions."; + + static void Missing(string file) => + throw new(missing); + + static void Missing(string temp, string target) => + throw new(missing); + + /// + /// Then the directory the received file sat in, when nothing is left in it. DiffEngine stages + /// received files in their own directory for some flows, and the tray has always swept it, so + /// accepting here leaves behind what accepting there leaves behind. + /// + static void Move(string temp, string target) + { + File.Move(temp, target, true); + + var directory = Path.GetDirectoryName(temp); + if (directory is null || + Directory.EnumerateFileSystemEntries(directory).Any()) + { + return; + } + + try + { + Directory.Delete(directory); + } + catch (IOException) + { + // Raced by something writing into it. The move itself succeeded, which is what the + // caller is reporting on. + } + } } diff --git a/src/DiffEngineViewer/ViewerProgram.cs b/src/DiffEngineViewer/ViewerProgram.cs index f569c114..20c818c6 100644 --- a/src/DiffEngineViewer/ViewerProgram.cs +++ b/src/DiffEngineViewer/ViewerProgram.cs @@ -21,6 +21,11 @@ public static int Run(string[] args, OpenWindow open) return RunAttached(open); } + if (request.Delete) + { + return RunDelete(request.Left!, open); + } + if (request.Mode == ViewerMode.Inline) { return RunInline(open); @@ -78,6 +83,38 @@ static int RunInline(OpenWindow open) } } + /// + /// One pending delete, owning the queue so more can join it. + /// + /// Launched by DiffEngine when no tray is running and nothing answered on the port. Two + /// deletes racing both launch, and the loser hands its file to the winner and exits, which is + /// the same resolution reaches for a second patch. + /// + /// + static int RunDelete(string file, OpenWindow open) + { + var port = ViewerClient.Port; + if (!ViewerServer.TryBind(port, out var server)) + { + if (!ViewerClient.TrySend(new(ViewerVerb.Delete, file), out var response, port) || + !response.Ok) + { + Console.Error.WriteLine("A viewer holds the port but did not accept the delete."); + return 1; + } + + return 0; + } + + using (server) + { + var start = ViewerSession.EnqueueTracked( + SessionState.Start(ViewerMode.Inline), + TrackedEntry.ForDelete(file)); + 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 diff --git a/src/DiffEngineViewer/ViewerRequest.cs b/src/DiffEngineViewer/ViewerRequest.cs index 29b74082..68614fe2 100644 --- a/src/DiffEngineViewer/ViewerRequest.cs +++ b/src/DiffEngineViewer/ViewerRequest.cs @@ -13,4 +13,12 @@ record ViewerRequest( string? Source, int Line, string? Error, - bool Attach = false); + bool Attach = false) +{ + /// + /// Own the queue seeded with one pending delete, whose file is . From + /// DiffEngine when no tray is running: a delete has no diff tool to open, so a viewer is the + /// only surface it can have. + /// + public bool Delete { get; init; } +} diff --git a/src/DiffEngineViewer/ViewerSession.cs b/src/DiffEngineViewer/ViewerSession.cs index 418bfee8..6f254380 100644 --- a/src/DiffEngineViewer/ViewerSession.cs +++ b/src/DiffEngineViewer/ViewerSession.cs @@ -30,7 +30,7 @@ public static SessionState EnqueueInline(SessionState state, InlinePatch patch) { var key = InlineKey.For(patch.SourceFile, patch.LineHint); var replacedCurrent = state.Current?.Key == key; - var queue = Project(state, Pending(state).Enqueue(patch)); + var queue = Rebuild(state, Pending(state).Enqueue(patch)); // Grouping can reorder the list, so the selection follows its key rather than its index. var currentKey = state.Current?.Key; var selected = currentKey is null ? 0 : IndexOf(queue, currentKey); @@ -79,7 +79,32 @@ public static SessionState Settle(SessionState state, string key, string? origin return state; } - return Remove(state, Project(state, settled), null); + return Remove(state, Rebuild(state, settled), null); + } + + /// + /// Adds a pending move or delete, or replaces the entry for the same file: a re-run stages the + /// same received file again, and a second entry for it would be a duplicate rather than news. + /// + /// Takes a built entry rather than paths, because building one reads both files. + /// does that on the listener thread, which is the same seam + /// takes the tray's through. + /// + /// + public static SessionState EnqueueTracked(SessionState state, QueueEntry entry) + { + var replacedCurrent = state.Current?.Key == entry.Key; + var kept = state.Queue.Where(_ => _.Key != entry.Key); + var queue = QueueProjection.Order([..kept, entry]); + var currentKey = state.Current?.Key; + var selected = currentKey is null ? 0 : IndexOf(queue, currentKey); + return Clamp(state with + { + Queue = queue, + Selected = selected < 0 ? 0 : selected, + ScrollTop = replacedCurrent ? 0 : state.ScrollTop, + Menu = null + }); } /// @@ -208,7 +233,7 @@ public static SessionState Apply(SessionState state, Command command, ViewerActi case CommandKind.AcceptGroup: return menu is null || !inline ? state : AcceptGroup(state, menu, actions); case CommandKind.DiscardGroup: - return menu is null || !inline ? state : DiscardGroup(state, menu); + return menu is null || !inline ? state : DiscardGroup(state, menu, actions); case CommandKind.ToggleGroup: return menu?.GroupKey is not { } key ? state : Toggle(state, key); case CommandKind.RevealSource: @@ -238,18 +263,33 @@ public static SessionState Apply(SessionState state, Command command, ViewerActi case CommandKind.SelectItem: return Select(state, command.Index); case CommandKind.Accept: - return inline ? AcceptInline(state, actions) : AcceptFile(state, actions); + if (!inline) + { + return AcceptFile(state, actions); + } + + // A move or a delete is applied by whoever holds it, and in queue mode that is + // either this process or the owner this one forwards to. Reaching here means the + // former, because forwarding never gets this far. + return state.Current is { Kind: QueueEntryKind.Move or QueueEntryKind.Delete } accepting + ? AcceptTracked(state, accepting, actions) + : AcceptInline(state, actions); case CommandKind.AcceptAll: // File mode shows one comparison and cannot grow, so accepting all of it is // accepting it. Reachable through shift+A even though the button is disabled for // a single item, so it behaves rather than being a hole. return inline ? AcceptAllInline(state, actions) : AcceptFile(state, actions); case CommandKind.Discard: - return inline ? DiscardInline(state) : DiscardFile(state); + if (!inline) + { + return DiscardFile(state); + } + + return state.Current is { Kind: QueueEntryKind.Move or QueueEntryKind.Delete } discarding + ? DiscardTracked(state, discarding, actions) + : DiscardInline(state); case CommandKind.DiscardAll: - return inline - ? Remove(state, Project(state, Pending(state).DiscardAll(out var summary)), summary) - : DiscardFile(state); + return inline ? DiscardAllInline(state, actions) : DiscardFile(state); case CommandKind.NextVariant: return NextVariant(state); case CommandKind.Quit: @@ -283,7 +323,7 @@ static SessionState AcceptInline(SessionState state, ViewerActions actions) accepted = pending.Accept(current.Key, actions.ApplyInline, out message); } - var queue = Project(state, accepted); + var queue = Rebuild(state, accepted); if (accepted.Count < pending.Count) { return Remove(state, queue, message); @@ -298,15 +338,19 @@ static SessionState AcceptInline(SessionState state, ViewerActions actions) } /// - /// Accepts every inline member of the group a header's menu described, skipping conflicted - /// entries the way accept-all does. By key rather than index, because each accept rebuilds - /// the queue underneath the next. + /// Accepts every member of the group a header's menu described, skipping conflicted entries + /// the way accept-all does. By key rather than index, because each accept rebuilds the queue + /// underneath the next. + /// + /// A solution header spans tracked moves and deletes as well as snapshots, so the sweep does + /// too. Skipping them would make "Accept all in ..." quietly mean "accept the snapshots in + /// ...", which is the divergence the unqualified accept-all already avoids. + /// /// static SessionState AcceptGroup(SessionState state, MenuState menu, ViewerActions actions) { - var members = menu.Members - .Where(_ => _ >= 0 && _ < state.Queue.Count) - .Select(_ => state.Queue[_]) + var all = Members(state, menu); + var members = all .Where(_ => _.Kind == QueueEntryKind.Inline) .ToList(); var queue = Pending(state); @@ -356,14 +400,19 @@ static SessionState AcceptGroup(SessionState state, MenuState menu, ViewerAction builder.Append($". {failure}"); } - return Remove(state, Project(state, queue), builder.ToString()); + return SweepTracked( + state, + Rebuild(state, queue), + builder.ToString(), + actions, + discarding: false, + TrackedKeysOf(all)); } - static SessionState DiscardGroup(SessionState state, MenuState menu) + static SessionState DiscardGroup(SessionState state, MenuState menu, ViewerActions actions) { - var keys = menu.Members - .Where(_ => _ >= 0 && _ < state.Queue.Count) - .Select(_ => state.Queue[_]) + var all = Members(state, menu); + var keys = all .Where(_ => _.Kind == QueueEntryKind.Inline) .Select(_ => _.Key) .ToList(); @@ -373,9 +422,27 @@ static SessionState DiscardGroup(SessionState state, MenuState menu) queue = queue.Discard(key, out _); } - return Remove(state, Project(state, queue), $"Discarded {keys.Count}"); + return SweepTracked( + state, + Rebuild(state, queue), + $"Discarded {keys.Count}", + actions, + discarding: true, + TrackedKeysOf(all)); } + static List Members(SessionState state, MenuState menu) => + menu.Members + .Where(_ => _ >= 0 && _ < state.Queue.Count) + .Select(_ => state.Queue[_]) + .ToList(); + + static List TrackedKeysOf(IEnumerable entries) => + entries + .Where(_ => _.Kind is QueueEntryKind.Move or QueueEntryKind.Delete) + .Select(_ => _.Key) + .ToList(); + /// /// Shows the current entry's file in the platform's file manager: the source for an inline /// entry, the target for a move, the doomed file for a delete, the left file in file mode. @@ -462,7 +529,132 @@ public static SessionState SelectVariant(SessionState state, string origin) static SessionState AcceptAllInline(SessionState state, ViewerActions actions) { var accepted = Pending(state).AcceptAll(actions.ApplyInline, out var message); - return Remove(state, Project(state, accepted), message); + return SweepTracked(state, Rebuild(state, accepted), message, actions, discarding: false); + } + + static SessionState DiscardAllInline(SessionState state, ViewerActions actions) + { + var discarded = Pending(state).DiscardAll(out var message); + return SweepTracked(state, Rebuild(state, discarded), message, actions, discarding: true); + } + + /// + /// The tracked half of a bulk command, worded the way an owning tray words its own: the inline + /// summary, then ", plus n files" with what stayed pending counted rather than hidden. Both + /// sweeps say the same thing about the same files, whichever process is holding them. + /// + /// + /// The keys to sweep, for a group header acting on its own members. Null sweeps every tracked + /// entry, which is what the unqualified bulk commands mean. + /// + static SessionState SweepTracked( + SessionState state, + IReadOnlyList queue, + string message, + ViewerActions actions, + bool discarding, + IReadOnlyCollection? only = null) + { + var remaining = new List(queue.Count); + var swept = 0; + var kept = 0; + foreach (var entry in queue) + { + if (entry.Kind is not (QueueEntryKind.Move or QueueEntryKind.Delete) || + (only is not null && !only.Contains(entry.Key))) + { + remaining.Add(entry); + continue; + } + + if (TryApplyTracked(entry, actions, discarding) is not { } failure) + { + swept++; + continue; + } + + kept++; + remaining.Add(entry with { Status = failure }); + } + + if (swept == 0 && + kept == 0) + { + return Remove(state, remaining, message); + } + + var clause = kept == 0 ? $"{swept} files" : $"{swept} files ({kept} kept)"; + return Remove(state, remaining, $"{message}, plus {clause}"); + } + + /// + /// Accepting a tracked entry is the file operation it describes; discarding one is throwing + /// the received file away, or, for a delete, leaving the file alone and only untracking it — + /// which is what discarding a pending delete has always meant. + /// + /// Returns null when it went, and the failure otherwise. A failed entry stays pending carrying + /// the reason, so it can be retried once whatever holds the file is gone, exactly as a failed + /// inline apply does. + /// + /// + static string? TryApplyTracked(QueueEntry entry, ViewerActions actions, bool discarding) + { + try + { + if (discarding) + { + if (entry.Kind == QueueEntryKind.Move) + { + actions.DeleteFile(entry.LeftFile!); + } + + return null; + } + + if (entry.Kind == QueueEntryKind.Move) + { + actions.MoveFile(entry.LeftFile!, entry.TargetFile!); + } + else + { + actions.DeleteFile(entry.LeftFile!); + } + + return null; + } + catch (Exception exception) + { + return exception.Message; + } + } + + static SessionState AcceptTracked(SessionState state, QueueEntry entry, ViewerActions actions) => + ApplyTracked(state, entry, actions, discarding: false, $"Accepted {entry.Name}"); + + static SessionState DiscardTracked(SessionState state, QueueEntry entry, ViewerActions actions) => + ApplyTracked(state, entry, actions, discarding: true, $"Discarded {entry.Name}"); + + static SessionState ApplyTracked( + SessionState state, + QueueEntry entry, + ViewerActions actions, + bool discarding, + string done) + { + if (TryApplyTracked(entry, actions, discarding) is { } failure) + { + var queue = state.Queue + .Select(_ => _.Key == entry.Key ? _ with { Status = failure } : _) + .ToList(); + return Clamp(state with + { + Queue = queue, + Message = failure, + Menu = null + }); + } + + return Remove(state, state.Queue.Where(_ => _.Key != entry.Key).ToList(), done); } static SessionState DiscardInline(SessionState state) @@ -474,7 +666,7 @@ static SessionState DiscardInline(SessionState state) } var discarded = Pending(state).Discard(current.Key, out var message); - return Remove(state, Project(state, discarded), message); + return Remove(state, Rebuild(state, discarded), message); } /// @@ -532,6 +724,26 @@ static InlineQueue Pending(SessionState state) => .Where(_ => _.Kind == QueueEntryKind.Inline) .Select(_ => new PendingInline(_.Variants, _.Status))); + /// + /// The display list after an inline transition: the queue projected back, plus the tracked + /// moves and deletes carried over untouched. + /// + /// Every inline command rebuilds its half of the list from , which is + /// what keeps the two from disagreeing. The tracked half has to survive that. An owning viewer + /// holds both — a delete arrives with no tray running and sits beside the snapshots — so + /// accepting a snapshot must not take the files pending next to it with it. + /// + /// + /// is the one caller that does not use this: it is replacing the tracked + /// entries with what the owner just reported, so carrying the old ones over would double them. + /// + /// + static IReadOnlyList Rebuild(SessionState state, InlineQueue queue) => + QueueProjection.Order([..Project(state, queue), ..Tracked(state)]); + + static IEnumerable Tracked(SessionState state) => + state.Queue.Where(_ => _.Kind is QueueEntryKind.Move or QueueEntryKind.Delete); + /// /// And back onto the display list, in display order. Building an entry runs the diff, so an /// entry already built for the same variants is reused, keeping its selected variant, and