Relaunch a tracked move the way the tool says to (#849)

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.

Author: SimonCropp

src/DiffEngineTray.Tests/DiffToolLauncherFlagsTest.cs

@@ -0,0 +1,77 @@
+/// <summary>
+/// The start flags the tray relaunches a tracked move with, which have to be the tool's own rather
+/// than a fixed pair.
+/// <para>
+/// 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.
+/// </para>
+/// </summary>
+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));
+    }
+
+    /// <summary>
+    /// 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.
+    /// </summary>
+    [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));
+    }
+
+    /// <summary>
+    /// 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.
+    /// </summary>
+    [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();
+}

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 = _);
 
+    /// <summary>
+    /// The two start flags the tool itself declares, which is what <c>DiffRunner.LaunchProcess</c>
+    /// 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.
+    /// <para>
+    /// 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.
+    /// </para>
+    /// <para>
+    /// 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.
+    /// </para>
+    /// <para>
+    /// 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
+    /// <see href="https://github.com/VerifyTests/Verify/issues/1229" /> is about.
+    /// </para>
+    /// </summary>
+    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<Process?> 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