From 6ed70d4eb851f6b9556c4b6c5a81434c8fb16d81 Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sun, 23 Aug 2026 23:32:59 +1000 Subject: [PATCH] Relaunch a tracked move the way the tool says to DiffRunner starts a diff tool with the UseShellExecute and CreateNoWindow the tool declares. The tray's "Open diff tool" hard coded ShellExecute and left CreateNoWindow off, so the same tool was started two different ways depending on which surface asked - and the flag a console subsystem tool sets specifically to keep a window off the screen was the one being dropped. Measured before changing anything, because the comment that flag carries says a window flashes without it. On Windows 11 nothing flashes: the console window the hard coded pair produces is zero sized, WS_EX_TOOLWINDOW and WS_EX_NOACTIVATE, so nobody sees it, and a conhost child is attached either way. What the tool's own flags change here is that the window object is not created at all. The reason to do it is still the first paragraph: one launch contract rather than two, on a machine where that comment may well have been earned. The flags are resolved rather than carried, because the payload has no room for them - PiperServer's format is frozen, and every stable DiffEngine embeds the client that writes it. By path first, and then by the executable's name, which is what the bundled viewer needs: the path a move carries is the sending process's, and that one is inside that project's package folder, somewhere the tray has never looked. An exe that resolves to neither keeps the pair this always used. Note that following the tool now also means following UseShellExecute, so a viewer reopened from the tray inherits the tray's handles rather than being detached from them. The tray is a long lived process whose output nobody is reading, which is not the case the ShellExecute default was there to protect. --- .../DiffToolLauncherFlagsTest.cs | 77 +++++++++++++++++++ src/DiffEngineTray/DiffToolLauncher.cs | 48 +++++++++++- 2 files changed, 121 insertions(+), 4 deletions(-) create mode 100644 src/DiffEngineTray.Tests/DiffToolLauncherFlagsTest.cs diff --git a/src/DiffEngineTray.Tests/DiffToolLauncherFlagsTest.cs b/src/DiffEngineTray.Tests/DiffToolLauncherFlagsTest.cs new file mode 100644 index 00000000..0aea8f40 --- /dev/null +++ b/src/DiffEngineTray.Tests/DiffToolLauncherFlagsTest.cs @@ -0,0 +1,77 @@ +/// +/// The start flags the tray relaunches a tracked move with, which have to be the tool's own rather +/// than a fixed pair. +/// +/// They were fixed at ShellExecute with no CreateNoWindow, so a console subsystem tool - the +/// bundled viewer is one - came up from "Open diff tool" with a console attached, while +/// DiffEngine's own launch of the very same tool did not. The window that console puts on screen +/// is zero sized and never activates, so nobody saw it; what it left behind was a conhost process +/// per relaunch and two launch paths that disagreed. +/// +/// +public class DiffToolLauncherFlagsTest : + IDisposable +{ + [Test] + public async Task AToolsOwnFlagsAreUsed() + { + var registered = DiffTools.AddTool( + name: "FakeConsoleTool", + autoRefresh: false, + isMdi: false, + supportsText: true, + requiresTarget: false, + useShellExecute: false, + launchArguments: new( + Left: (temp, target) => $"\"{target}\" \"{temp}\"", + Right: (temp, target) => $"\"{temp}\" \"{target}\""), + exePath: exe, + binaryExtensions: [], + createNoWindow: true); + + await Assert.That(registered).IsNotNull(); + await Assert.That(DiffToolLauncher.FlagsFor(exe)).IsEqualTo((false, true)); + } + + /// + /// The path a move carries is the sending process's. For the bundled viewer that is inside + /// that project's package folder, which this process has never looked in, so the exact path + /// misses and the executable's name is what is left to go on. + /// + [Test] + public async Task TheSameToolAtAnotherPathIsStillThatTool() + { + DiffTools.AddTool( + name: "FakeConsoleTool", + autoRefresh: false, + isMdi: false, + supportsText: true, + requiresTarget: false, + useShellExecute: false, + launchArguments: new( + Left: (temp, target) => $"\"{target}\" \"{temp}\"", + Right: (temp, target) => $"\"{temp}\" \"{target}\""), + exePath: exe, + binaryExtensions: [], + createNoWindow: true); + + var elsewhere = Path.Combine(@"c:\somewhere\else", Path.GetFileName(exe)); + + await Assert.That(DiffToolLauncher.FlagsFor(elsewhere)).IsEqualTo((false, true)); + } + + /// + /// Nothing resolves for a tool that is no longer installed, or an exe a payload named that + /// never was one. That keeps what this has always done, and ShellExecute is the safe end of + /// it: without it the launched tool inherits the launching process's handles. + /// + [Test] + public async Task AnUnknownExeKeepsTheOldPair() => + await Assert.That(DiffToolLauncher.FlagsFor(@"c:\nothing\here.exe")).IsEqualTo((true, false)); + + readonly string exe = Environment.ProcessPath!; + + public void Dispose() => + // Registered into the static lookup, so the rest of the run has to get it back. + DiffTools.Reset(); +} diff --git a/src/DiffEngineTray/DiffToolLauncher.cs b/src/DiffEngineTray/DiffToolLauncher.cs index 9931dcb2..b5b3a45c 100644 --- a/src/DiffEngineTray/DiffToolLauncher.cs +++ b/src/DiffEngineTray/DiffToolLauncher.cs @@ -6,6 +6,47 @@ static class DiffToolLauncher public static void Launch(TrackedMove move) => Launch(move.Exe!, move.Arguments!, move.CanKill, move.Process, _ => move.Process = _); + /// + /// The two start flags the tool itself declares, which is what DiffRunner.LaunchProcess + /// launches it with. Hard coded here before, so a console subsystem tool - the bundled viewer + /// is one - was started without CreateNoWindow and came up with a console attached, which + /// DiffEngine's own launch of the same tool does not do. + /// + /// Resolved by path rather than carried on the move, because the payload the tray receives has + /// no room for them: PiperServer's format is frozen, every stable DiffEngine embeds the client + /// that writes it, and a new field would be read as nothing by all of them. + /// + /// + /// By name when the path misses, which for the bundled viewer it does: the path a move carries + /// is the sending process's, and that one sits inside that project's package folder, somewhere + /// this process has never looked. The same tool found somewhere else is still that tool, and + /// its start flags belong to the executable rather than to where it was installed. + /// + /// + /// An exe that resolves to neither keeps what this always did. ShellExecute is the safe end of + /// that: a tool started without it inherits the launching process's handles, which is what + /// is about. + /// + /// + internal static (bool useShellExecute, bool createNoWindow) FlagsFor(string exe) + { + if (DiffTools.TryFindByPath(exe, out var tool)) + { + return (tool.UseShellExecute, tool.CreateNoWindow); + } + + var name = Path.GetFileName(exe); + foreach (var candidate in DiffTools.Resolved) + { + if (string.Equals(Path.GetFileName(candidate.ExePath), name, StringComparison.OrdinalIgnoreCase)) + { + return (candidate.UseShellExecute, candidate.CreateNoWindow); + } + } + + return (true, false); + } + static void Launch(string exe, string arguments, bool canKill, Process? process, Action assign) { if (process is { HasExited: false }) @@ -24,12 +65,11 @@ static void Launch(string exe, string arguments, bool canKill, Process? process, process?.Dispose(); assign(null); + var (useShellExecute, createNoWindow) = FlagsFor(exe); var startInfo = new ProcessStartInfo(exe, arguments) { - // Given the full exe path is known we dont need UseShellExecute https://stackoverflow.com/a/5255335 - // however UseShellExecute allows the test running to not block when the difftool is launched - // https://github.com/VerifyTests/Verify/issues/1229 - UseShellExecute = true + UseShellExecute = useShellExecute, + CreateNoWindow = createNoWindow }; try