Accept all open sweeps viewer backed pairs (#862)

* Accept all open sweeps viewer backed pairs

"Accept all open" matched a move by its live process, which is what
DiffRunner records for every tool that opens one window per pair. The
viewer opens none: a pair whose tool is the viewer is tracked with no
process id, because it is drawn as a row in the one window every pending
pair shares and so nothing may kill it. The one kind of move that is
unambiguously on screen was therefore the one kind the hot key could
never match, and with the viewer as the resolved tool the key looked
dead. The process test predates the viewer becoming a diff tool and was
not revisited then.

TrackedMove now answers IsOpen for itself - the viewer, or a live
process - and AcceptOpen asks that instead. Whether a tracked move is a
viewer one is read off its executable name rather than through
DiffTools.TryFindByPath, which is an exact path lookup: the sender
resolves the copy bundled in its own DiffEngine package and a tray
carries one of its own, so those two paths are never the same string.

IsOpen joins the debug view, since it is the field this turned on.

* cleanup

Author: SimonCropp

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();
     }
-}
+}