Accept all open sweeps viewer backed pairs

docs/mdsource/tray.source.md

@@ -141,7 +141,7 @@ Registers a system wide HotKey to accept pending:
 Registers a system wide HotKey to accept pending:
 
  * Deletes
- * Moves that are currently open in a diff tool
+ * Moves that are currently open in a diff tool. A pair whose tool is the viewer counts: it is drawn as a row in the one window every pending pair shares, rather than in a process of its own
  * Inline snapshots, all of which are open by definition: the viewer only stays running while it has something to show
 
 To limit impact on system resources, the [default max concurrent open tool instances is limited to 5](/docs/diff-tool.md#maxinstancestolaunch).

docs/tray.md

@@ -148,7 +148,7 @@ Registers a system wide HotKey to accept pending:
 Registers a system wide HotKey to accept pending:
 
  * Deletes
- * Moves that are currently open in a diff tool
+ * Moves that are currently open in a diff tool. A pair whose tool is the viewer counts: it is drawn as a row in the one window every pending pair shares, rather than in a process of its own
  * Inline snapshots, all of which are open by definition: the viewer only stays running while it has something to show
 
 To limit impact on system resources, the [default max concurrent open tool instances is limited to 5](/docs/diff-tool.md#maxinstancestolaunch).

src/DiffEngine/Tray/PendingFiles.cs

@@ -209,6 +209,46 @@ public static void SettleDiff(string tempFile) =>
     public static bool IsViewer(ResolvedTool tool) =>
         tool.Tool == DiffTool.DiffEngineViewer;
 
+    /// <summary>
+    /// The same question asked of a move that is already tracked, where all that survives of the
+    /// tool is the executable it was recorded with.
+    /// <para>
+    /// By file name rather than through <see cref="DiffTools.TryFindByPath"/>, which is an exact
+    /// path lookup: the sender resolved the viewer bundled inside its own DiffEngine package and
+    /// a tray carries a copy of its own, so the two paths are never the same string.
+    /// </para>
+    /// </summary>
+    public static bool IsViewerExe(string? exe) =>
+        exe != null &&
+        viewerExeNames.Contains(Path.GetFileName(exe));
+
+    // Read off the definition rather than spelled again here, so renaming the executable cannot
+    // leave this matching the old name. Every OS's name, because the string being tested arrived
+    // from another process rather than from this one.
+    static readonly HashSet<string> viewerExeNames = ViewerExeNames();
+
+    static HashSet<string> ViewerExeNames()
+    {
+        var support = Definitions.Tools
+            .Single(_ => _.Tool == DiffTool.DiffEngineViewer)
+            .OsSupport;
+        var names = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
+        foreach (var settings in new[]
+                 {
+                     support.Windows,
+                     support.Linux,
+                     support.Osx
+                 })
+        {
+            if (settings != null)
+            {
+                names.Add(settings.ExeName);
+            }
+        }
+
+        return names;
+    }
+
     /// <summary>
     /// How a tracked move is opened again, and whether the window that opens may be killed.
     /// <para>

src/DiffEngineTray.Tests/DebugReportTests.Full.verified.txt

@@ -21,6 +21,7 @@ Moves (1)
     CanKill:            True
     KillLockingProcess: False
     Process:            none
+    IsOpen:             False
 
 Snapshots (3)
 -------------

src/DiffEngineTray.Tests/TrackerAcceptOpenTest.cs

@@ -0,0 +1,73 @@
+/// <summary>
+/// Which pending moves the "Accept all open" hot key sweeps.
+/// <para>
+/// The rule is "a window is showing this pair", and for every tool but one that is a live process
+/// DiffRunner started for it. The viewer is the exception: it draws every pending pair as a row in
+/// one shared window, so no process id is ever sent for one and none may be killed. Testing the
+/// process alone therefore left exactly the pairs that were on screen out of the sweep, and the
+/// hot key looked dead to anyone whose diff tool is the viewer.
+/// </para>
+/// </summary>
+public class TrackerAcceptOpenTest :
+    IDisposable
+{
+    [Test]
+    public async Task AViewerPairIsOpenEvenWithNoProcess()
+    {
+        await using var tracker = new RecordingTracker(inline: new StubInlineHost());
+        tracker.AddMove(temp, target, viewerExe, "--diff", false, null);
+
+        await tracker.AcceptOpen();
+
+        await tracker.AssertEmpty();
+        await Assert.That(File.Exists(temp)).IsFalse();
+        await Assert.That(await File.ReadAllTextAsync(target)).IsEqualTo("received");
+    }
+
+    /// <summary>
+    /// The other half of the rule, so the fix for the viewer does not quietly turn "accept all
+    /// open" into "accept all": a pair whose own window has gone is still not open.
+    /// </summary>
+    [Test]
+    public async Task AnotherToolWithNoProcessIsNotOpen()
+    {
+        await using var tracker = new RecordingTracker(inline: new StubInlineHost());
+        tracker.AddMove(temp, target, "theExe", "theArguments", true, null);
+
+        await tracker.AcceptOpen();
+
+        await Assert.That(tracker.Moves).HasSingleItem();
+    }
+
+    [Test]
+    public async Task TheViewerIsRecognisedByNameRatherThanByPath()
+    {
+        // The sender resolved the copy bundled in its own DiffEngine package, so the path is one
+        // this process has never seen and the tool lookup finds nothing for it.
+        await Assert.That(PendingFiles.IsViewerExe(viewerExe)).IsTrue();
+        await Assert.That(PendingFiles.IsViewerExe("theExe")).IsFalse();
+        await Assert.That(PendingFiles.IsViewerExe(null)).IsFalse();
+    }
+
+    static readonly string viewerExe = Path.Combine(
+        Path.GetTempPath(),
+        "some-other-package",
+        "viewer",
+        OperatingSystem.IsWindows() ? "DiffEngineViewer.exe" : "DiffEngineViewer");
+
+    string directory = Path.Combine(Path.GetTempPath(), $"AcceptOpen {Guid.NewGuid():N}");
+    string temp;
+    string target;
+
+    public TrackerAcceptOpenTest()
+    {
+        Directory.CreateDirectory(directory);
+        temp = Path.Combine(directory, "Sample.Test.received.txt");
+        target = Path.Combine(directory, "Sample.Test.verified.txt");
+        File.WriteAllText(temp, "received");
+        File.WriteAllText(target, "verified");
+    }
+
+    public void Dispose() =>
+        Directory.Delete(directory, true);
+}

src/DiffEngineTray/DebugReport.cs

@@ -64,6 +64,8 @@ public static string Build(Tracker tracker, DateTime now)
             AppendField(builder, "CanKill", move.CanKill);
             AppendField(builder, "KillLockingProcess", move.KillLockingProcess);
             AppendField(builder, "Process", Describe(move.Process));
+            // What "accept all open" acts on, which is not the process for a viewer backed pair.
+            AppendField(builder, "IsOpen", move.IsOpen);
         }
 
         var queued = tracker.QueuedPatches;

src/DiffEngineTray/TrackedMove.cs

@@ -8,7 +8,8 @@ public TrackedMove(string temp,
         Process? process,
         string? group,
         string extension,
-        bool killLockingProcess = false)
+        bool killLockingProcess = false,
+        bool isViewer = false)
     {
         Temp = temp;
         Target = target;
@@ -20,6 +21,7 @@ public TrackedMove(string temp,
         Process = process;
         Group = group;
         KillLockingProcess = killLockingProcess;
+        IsViewer = isViewer;
     }
 
     public string Extension { get; }
@@ -32,4 +34,24 @@ public TrackedMove(string temp,
     public Process? Process { get; set; }
     public string? Group { get; }
     public bool KillLockingProcess { get; }
+
+    /// <summary>
+    /// Whether the tool showing this pair is the viewer, which is the one tool that opens no
+    /// process of its own for it.
+    /// </summary>
+    public bool IsViewer { get; }
+
+    /// <summary>
+    /// Whether something is showing this pair right now, which is what "accept all open" acts on.
+    /// <para>
+    /// For every other tool that is a live process, because DiffRunner started one window per
+    /// pair and recorded it. A viewer backed pair has none by construction - it is drawn as a row
+    /// in the one window holding every pending pair, which is why nothing may kill it and why no
+    /// process id is sent - so the process test alone left those rows out of every "accept all
+    /// open" while the window they were drawn in sat on the screen.
+    /// </para>
+    /// </summary>
+    public bool IsOpen =>
+        IsViewer ||
+        Process is {HasExited: false};
 }

src/DiffEngineTray/Tracker.cs

@@ -227,7 +227,20 @@ static TrackedMove BuildTrackedMove(string temp, string? exe, string? arguments,
             }
         }
 
-        return new(temp, target, exe, arguments, canKill.GetValueOrDefault(false), process, solution, extension, killLockingProcess);
+        // Off the resolved executable rather than the resolved tool, because the sender's viewer
+        // and this tray's are different copies at different paths, so the path lookup above finds
+        // nothing for the one case that matters most here.
+        return new(
+            temp,
+            target,
+            exe,
+            arguments,
+            canKill.GetValueOrDefault(false),
+            process,
+            solution,
+            extension,
+            killLockingProcess,
+            PendingFiles.IsViewerExe(exe));
     }
 
     /// <summary>
@@ -293,10 +306,15 @@ bool TryAcceptOne(PendingSnapshot snapshot, out string? message)
 
     // The owner does not always have something to add, and a balloon ending in a bare full stop
     // and a space reads as a message that went missing
-    static string CouldNotAccept(string name, string? message) =>
-        message is { Length: > 0 }
-            ? $"Could not accept the snapshot for '{name}'. {message}"
-            : $"Could not accept the snapshot for '{name}'.";
+    static string CouldNotAccept(string name, string? message)
+    {
+        if (message is { Length: > 0 })
+        {
+            return $"Could not accept the snapshot for '{name}'. {message}";
+        }
+
+        return $"Could not accept the snapshot for '{name}'.";
+    }
 
     /// <summary>
     /// On a worker, matching <see cref="Accept(PendingSnapshot)"/> and for the same reason. Against
@@ -712,7 +730,7 @@ public Task AcceptOpen()
 
         AcceptMoves(
             moves.Values
-                .Where(_ => _.Process is { HasExited: false })
+                .Where(_ => _.IsOpen)
                 .ToList());
 
         // Every pending snapshot is open by definition: the viewer only stays running while it
@@ -804,16 +822,17 @@ bool ITrackedFiles.Untrack(string key)
     {
         if (TrackedKeys.TryStrip(key, TrackedKeys.MovePrefix, out var temp))
         {
-            return moves.TryGetValue(temp, out var move)
-                ? AcceptWithoutPrompting(move)
-                : (false, null);
+            if (moves.TryGetValue(temp, out var move))
+            {
+                return AcceptWithoutPrompting(move);
+            }
         }
-
-        if (TrackedKeys.TryStrip(key, TrackedKeys.DeletePrefix, out var file))
+        else if (TrackedKeys.TryStrip(key, TrackedKeys.DeletePrefix, out var file))
         {
-            return deletes.TryGetValue(file, out var delete)
-                ? AcceptTracked(delete)
-                : (false, null);
+            if (deletes.TryGetValue(file, out var delete))
+            {
+                return AcceptTracked(delete);
+            }
         }
 
         return (false, null);
@@ -979,4 +998,4 @@ public ValueTask DisposeAsync()
         snapshots = [];
         return timer.DisposeAsync();
     }
-}
+}