From 11f285a8a8445fda5038624304c5c1822a9a97ed Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Tue, 1 Sep 2026 09:46:29 +1000 Subject: [PATCH 1/2] 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. --- docs/mdsource/tray.source.md | 2 +- docs/tray.md | 2 +- src/DiffEngine/Tray/PendingFiles.cs | 40 ++++++++++ .../DebugReportTests.Full.verified.txt | 1 + .../TrackerAcceptOpenTest.cs | 73 +++++++++++++++++++ src/DiffEngineTray/DebugReport.cs | 2 + src/DiffEngineTray/TrackedMove.cs | 24 +++++- src/DiffEngineTray/Tracker.cs | 7 +- 8 files changed, 146 insertions(+), 5 deletions(-) create mode 100644 src/DiffEngineTray.Tests/TrackerAcceptOpenTest.cs 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..267f4d05 --- /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(File.ReadAllText(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"); + + readonly string directory = Path.Combine(Path.GetTempPath(), $"AcceptOpen {Guid.NewGuid():N}"); + readonly string temp; + readonly 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..c6afb505 100644 --- a/src/DiffEngineTray/Tracker.cs +++ b/src/DiffEngineTray/Tracker.cs @@ -227,7 +227,10 @@ 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)); } /// @@ -712,7 +715,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 From 656e9ad44c339b6aaaab8e0a9e5eefc5b66ba3b4 Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Tue, 1 Sep 2026 09:50:23 +1000 Subject: [PATCH 2/2] cleanup --- .../TrackerAcceptOpenTest.cs | 8 ++-- src/DiffEngineTray/Tracker.cs | 44 +++++++++++++------ 2 files changed, 34 insertions(+), 18 deletions(-) diff --git a/src/DiffEngineTray.Tests/TrackerAcceptOpenTest.cs b/src/DiffEngineTray.Tests/TrackerAcceptOpenTest.cs index 267f4d05..433b34b3 100644 --- a/src/DiffEngineTray.Tests/TrackerAcceptOpenTest.cs +++ b/src/DiffEngineTray.Tests/TrackerAcceptOpenTest.cs @@ -21,7 +21,7 @@ public async Task AViewerPairIsOpenEvenWithNoProcess() await tracker.AssertEmpty(); await Assert.That(File.Exists(temp)).IsFalse(); - await Assert.That(File.ReadAllText(target)).IsEqualTo("received"); + await Assert.That(await File.ReadAllTextAsync(target)).IsEqualTo("received"); } /// @@ -55,9 +55,9 @@ public async Task TheViewerIsRecognisedByNameRatherThanByPath() "viewer", OperatingSystem.IsWindows() ? "DiffEngineViewer.exe" : "DiffEngineViewer"); - readonly string directory = Path.Combine(Path.GetTempPath(), $"AcceptOpen {Guid.NewGuid():N}"); - readonly string temp; - readonly string target; + string directory = Path.Combine(Path.GetTempPath(), $"AcceptOpen {Guid.NewGuid():N}"); + string temp; + string target; public TrackerAcceptOpenTest() { diff --git a/src/DiffEngineTray/Tracker.cs b/src/DiffEngineTray/Tracker.cs index c6afb505..94285b82 100644 --- a/src/DiffEngineTray/Tracker.cs +++ b/src/DiffEngineTray/Tracker.cs @@ -230,7 +230,17 @@ static TrackedMove BuildTrackedMove(string temp, string? exe, string? arguments, // 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)); + return new( + temp, + target, + exe, + arguments, + canKill.GetValueOrDefault(false), + process, + solution, + extension, + killLockingProcess, + PendingFiles.IsViewerExe(exe)); } /// @@ -296,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 @@ -807,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); @@ -982,4 +998,4 @@ public ValueTask DisposeAsync() snapshots = []; return timer.DisposeAsync(); } -} \ No newline at end of file +}