diff --git a/docs/mdsource/tray.source.md b/docs/mdsource/tray.source.md index 92383d16..6ad0aeb9 100644 --- a/docs/mdsource/tray.source.md +++ b/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). diff --git a/docs/tray.md b/docs/tray.md index ea306850..dfb025a3 100644 --- a/docs/tray.md +++ b/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). diff --git a/src/DiffEngine/Tray/PendingFiles.cs b/src/DiffEngine/Tray/PendingFiles.cs index 028ce7bd..6241272b 100644 --- a/src/DiffEngine/Tray/PendingFiles.cs +++ b/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; + /// + /// 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. + /// + /// By file name rather than through , 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. + /// + /// + 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 viewerExeNames = ViewerExeNames(); + + static HashSet ViewerExeNames() + { + var support = Definitions.Tools + .Single(_ => _.Tool == DiffTool.DiffEngineViewer) + .OsSupport; + var names = new HashSet(StringComparer.OrdinalIgnoreCase); + foreach (var settings in new[] + { + support.Windows, + support.Linux, + support.Osx + }) + { + if (settings != null) + { + names.Add(settings.ExeName); + } + } + + return names; + } + /// /// How a tracked move is opened again, and whether the window that opens may be killed. /// diff --git a/src/DiffEngineTray.Tests/DebugReportTests.Full.verified.txt b/src/DiffEngineTray.Tests/DebugReportTests.Full.verified.txt index f892e406..359a791a 100644 --- a/src/DiffEngineTray.Tests/DebugReportTests.Full.verified.txt +++ b/src/DiffEngineTray.Tests/DebugReportTests.Full.verified.txt @@ -21,6 +21,7 @@ Moves (1) CanKill: True KillLockingProcess: False Process: none + IsOpen: False Snapshots (3) ------------- diff --git a/src/DiffEngineTray.Tests/TrackerAcceptOpenTest.cs b/src/DiffEngineTray.Tests/TrackerAcceptOpenTest.cs new file mode 100644 index 00000000..433b34b3 --- /dev/null +++ b/src/DiffEngineTray.Tests/TrackerAcceptOpenTest.cs @@ -0,0 +1,73 @@ +/// +/// Which pending moves the "Accept all open" hot key sweeps. +/// +/// 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. +/// +/// +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"); + } + + /// + /// 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. + /// + [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); +} diff --git a/src/DiffEngineTray/DebugReport.cs b/src/DiffEngineTray/DebugReport.cs index 0f161e1f..2e5cec88 100644 --- a/src/DiffEngineTray/DebugReport.cs +++ b/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; diff --git a/src/DiffEngineTray/TrackedMove.cs b/src/DiffEngineTray/TrackedMove.cs index 9b7e6adf..02088e75 100644 --- a/src/DiffEngineTray/TrackedMove.cs +++ b/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; } + + /// + /// Whether the tool showing this pair is the viewer, which is the one tool that opens no + /// process of its own for it. + /// + public bool IsViewer { get; } + + /// + /// Whether something is showing this pair right now, which is what "accept all open" acts on. + /// + /// 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. + /// + /// + public bool IsOpen => + IsViewer || + Process is {HasExited: false}; } \ No newline at end of file diff --git a/src/DiffEngineTray/Tracker.cs b/src/DiffEngineTray/Tracker.cs index 9b762d52..94285b82 100644 --- a/src/DiffEngineTray/Tracker.cs +++ b/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)); } /// @@ -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}'."; + } /// /// On a worker, matching 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(); } -} \ No newline at end of file +}