From 66a288c8212385c06531f4bb73825812790c8dda Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Fri, 14 Aug 2026 13:11:01 +1000 Subject: [PATCH] Review pending files in the viewer when no tray is running A pending move or delete used to be reported to nothing at all without a tray: DiffRunner checked DiffEngineTray.IsRunning and skipped the send, so a received file waiting to be promoted, or a verified file a passing test no longer produces, was pending in no surface whatsoever. PendingFiles is now the router: the piper port when a tray is running, and the inline queue's owner when one is not. 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, since DiffRunner has already opened one for that pair. An owning viewer holds them as ordinary Move/Delete queue entries, the same ones an attached viewer already draws for the tray's, so the rows, the context menu, grouping and Accept all are unchanged. Only who applies them differs. A tray that owns the queue answers the new verbs too, which is load bearing rather than defensive: the tray check is cached at type init, so a test process that started first addresses the queue owner for good. That surfaced a latent bug. Every inline transition rebuilds its half of the queue from InlineQueue, and the projection returned only inline entries, so accepting one snapshot would have dropped every file pending beside it. ViewerSession.Rebuild carries them across. Also in this change, from auditing what the tray and the viewer do to the same item: * RemoteInlineHost decided applied versus failed from the wire's ok, which carries no apply status. An owner keeps a failed entry pending, so the tray reported a snapshot as accepted while the viewer still showed it, and the menu offered it again a scan later. It now re-reads the listing. * The tray menu's "Discard (n)" swept its dictionaries directly, leaving received files behind that the per item discard and the viewer's both delete. It goes through the same discard now, which in turn forced DisposeAsync to stop being a discard: exiting is not discarding. * Bulk and group accepts sweep tracked files, worded as the tray words its own, so "Accept all in ..." cannot quietly mean "the snapshots in ...". TrayViewerSyncTest is the new coverage for all of it: a real tray and a real viewer session over a real socket, in both ownership arrangements, asserting that an accept, discard, sweep or settle from either surface leaves the other showing the same thing. --- claude.md | 34 +- docs/inline.md | 14 +- docs/mdsource/inline.source.md | 14 +- docs/mdsource/tray.source.md | 4 +- docs/mdsource/viewer.source.md | 22 + docs/tray.md | 4 +- docs/viewer.md | 22 + src/DiffEngine.Tests/ViewerProtocolTests.cs | 44 + src/DiffEngine/Protocol/IQueueOwner.cs | 21 +- .../Protocol/ViewerMessageHandler.cs | 33 + src/DiffEngine/Protocol/ViewerVerb.cs | 19 + src/DiffEngine/Tray/DiffEngineTray.cs | 46 +- src/DiffEngine/Tray/PendingFiles.cs | 96 ++ src/DiffEngine/Viewer/ViewerLauncher.cs | 16 + .../DiffEngineTray.Tests.csproj | 6 + src/DiffEngineTray.Tests/FakeLauncher.cs | 19 + .../OwnedInlineHostTest.cs | 24 +- src/DiffEngineTray.Tests/TrackerDeleteTest.cs | 4 +- src/DiffEngineTray.Tests/TrackerMoveTest.cs | 4 +- .../TrayViewerSyncTest.cs | 935 ++++++++++++++++++ src/DiffEngineTray/ITrackedFiles.cs | 10 + src/DiffEngineTray/MenuBuilder.cs | 2 +- src/DiffEngineTray/OwnedInlineHost.cs | 17 + src/DiffEngineTray/Program.cs | 4 +- src/DiffEngineTray/RemoteInlineHost.cs | 42 +- .../Settings/OptionsFormLauncher.cs | 4 +- src/DiffEngineTray/Tracker.cs | 69 +- .../CommandLineTests.Attach.verified.txt | 3 +- .../CommandLineTests.Files.verified.txt | 3 +- .../CommandLineTests.Inline.verified.txt | 3 +- ...ests.InlineArgumentsReordered.verified.txt | 3 +- .../TrackedFileTests.cs | 225 +++++ src/DiffEngineViewer/CommandLine.cs | 17 + src/DiffEngineViewer/InternalsVisibleTo.cs | 4 + src/DiffEngineViewer/Ipc/MessageHandler.cs | 33 +- src/DiffEngineViewer/TrackedEntry.cs | 42 + src/DiffEngineViewer/ViewerActions.cs | 64 +- src/DiffEngineViewer/ViewerProgram.cs | 37 + src/DiffEngineViewer/ViewerRequest.cs | 10 +- src/DiffEngineViewer/ViewerSession.cs | 258 ++++- 40 files changed, 2096 insertions(+), 135 deletions(-) create mode 100644 src/DiffEngine/Tray/PendingFiles.cs create mode 100644 src/DiffEngineTray.Tests/FakeLauncher.cs create mode 100644 src/DiffEngineTray.Tests/TrayViewerSyncTest.cs create mode 100644 src/DiffEngineViewer.Tests/TrackedFileTests.cs create mode 100644 src/DiffEngineViewer/TrackedEntry.cs 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