Fix the verified bugs from the review

native/src/deview.cpp

@@ -539,24 +539,36 @@ int ReadKey()
         return DEVIEW_KEY_NONE;
     }
 
+    /* Letters by the character typed rather than by key position. raylib's key codes are
+     * positions on a US layout, so on AZERTY the key labelled Q reported KEY_A and accepted - a
+     * snapshot written into source by a key meant to quit - while the one labelled A quit.
+     * Characters follow the layout, the way the macOS and Windows heads already do. */
+    for (int character = GetCharPressed(); character != 0; character = GetCharPressed())
+    {
+        switch (character)
+        {
+            case 'a': return DEVIEW_KEY_ACCEPT;
+            case 'A': return DEVIEW_KEY_ACCEPT_ALL;
+            case 'd': return DEVIEW_KEY_DISCARD;
+            case 'v': return DEVIEW_KEY_NEXT_VARIANT;
+            case 'q': return DEVIEW_KEY_QUIT;
+            case 'n': return DEVIEW_KEY_NEXT_CHANGE;
+            case 'p': return DEVIEW_KEY_PREVIOUS_CHANGE;
+            case 'm': return DEVIEW_KEY_TOGGLE_MINIMAL;
+            default: break;
+        }
+    }
+
     if (IsKeyPressed(KEY_UP)) return DEVIEW_KEY_SCROLL_UP;
     if (IsKeyPressed(KEY_DOWN)) return DEVIEW_KEY_SCROLL_DOWN;
     if (IsKeyPressed(KEY_PAGE_UP)) return DEVIEW_KEY_PAGE_UP;
     if (IsKeyPressed(KEY_PAGE_DOWN)) return DEVIEW_KEY_PAGE_DOWN;
     if (IsKeyPressed(KEY_HOME)) return DEVIEW_KEY_HOME;
     if (IsKeyPressed(KEY_END)) return DEVIEW_KEY_END;
-    if (IsKeyPressed(KEY_N)) return DEVIEW_KEY_NEXT_CHANGE;
-    if (IsKeyPressed(KEY_P)) return DEVIEW_KEY_PREVIOUS_CHANGE;
-    if (IsKeyPressed(KEY_M)) return DEVIEW_KEY_TOGGLE_MINIMAL;
     if (IsKeyPressed(KEY_TAB)) return IsKeyDown(KEY_LEFT_SHIFT) || IsKeyDown(KEY_RIGHT_SHIFT)
         ? DEVIEW_KEY_PREVIOUS_ITEM
         : DEVIEW_KEY_NEXT_ITEM;
-    if (IsKeyPressed(KEY_A)) return IsKeyDown(KEY_LEFT_SHIFT) || IsKeyDown(KEY_RIGHT_SHIFT)
-        ? DEVIEW_KEY_ACCEPT_ALL
-        : DEVIEW_KEY_ACCEPT;
-    if (IsKeyPressed(KEY_D)) return DEVIEW_KEY_DISCARD;
-    if (IsKeyPressed(KEY_V)) return DEVIEW_KEY_NEXT_VARIANT;
-    if (IsKeyPressed(KEY_Q) || IsKeyPressed(KEY_ESCAPE)) return DEVIEW_KEY_QUIT;
+    if (IsKeyPressed(KEY_ESCAPE)) return DEVIEW_KEY_QUIT;
     return DEVIEW_KEY_NONE;
 }
 

src/DiffEngine.Tests/InlinePatcherFsTests.cs

@@ -285,6 +285,20 @@ let TestB () =
               """);
 
     // A call above TestB's declaration is not inside TestB, whatever the hint says, so the
+    // The usual way an F# test is named. Judged from the name, the backtick in front of it is no
+    // keyword, so the member was never found and the search ran across the whole file
+    [Test]
+    public async Task MemberNameBoundsTheSearchForABacktickedName()
+    {
+        var source = Source("module Tests\n\nlet ``test a`` () =\n    Verifier.Verify(a).Snapshot(\"dup\").ToTask()\n\nlet ``test b`` () =\n    Verifier.Verify(b).Snapshot(\"dup\").ToTask()\n");
+
+        var status = TryApply(source, 4, InlinePatchMode.Set, null, "new", out var newSource, out _, originalValue: "dup", memberName: "test b");
+
+        await Assert.That(status).IsEqualTo(PatchStatus.Applied);
+        await Assert.That(newSource).Contains("Verify(a).Snapshot(\"dup\")");
+        await Assert.That(newSource).Contains("Verify(b).Snapshot(\"new\")");
+    }
+
     // identical snapshot in the test above is not even a candidate
     [Test]
     public async Task MemberNameBoundsTheSearch()

src/DiffEngine.Tests/InlinePatcherTests.cs

@@ -1173,6 +1173,21 @@ public async Task RemoveWithNoSnapshotCallIsAlreadyDone()
     /// <summary>
     /// Nothing at the recorded line at all, and nothing anywhere else either, is still reported.
     /// </summary>
+    /// <summary>
+    /// An empty name matches everywhere and advances nothing, so the search for it never ended.
+    /// A payload of "VerifyDocx," declares one.
+    /// </summary>
+    [Test]
+    public async Task AnEmptyEntryPointIsIgnored()
+    {
+        var source = "class Tests\n{\n    Task Test() =>\n        Verify(a);\n}\n";
+
+        var apply = Task.Run(() => InlinePatcher.TryApply(SourceLanguage.CSharp, source, 4, InlinePatchMode.Append, null, null, null, ["VerifyDocx", ""], true, "x", out _, out _));
+
+        await Assert.That(await Task.WhenAny(apply, Task.Delay(TimeSpan.FromSeconds(10)))).IsSameReferenceAs(apply);
+        await Assert.That(await apply).IsEqualTo(PatchStatus.Applied);
+    }
+
     [Test]
     public async Task RemoveWithNoCallAtAll()
     {

src/DiffEngine.Tests/OsSettingsResolverTest.cs

@@ -3,6 +3,19 @@
 [NotInParallel]
 public class OsSettingsResolverTest
 {
+    /// <summary>
+    /// Windows allows a PATH entry in quotes, and .NET Framework's Path.Combine throws on the
+    /// quote, out of the static constructor that reads PATH, so every tool lookup in the process
+    /// failed for good.
+    /// </summary>
+    [Test]
+    public async Task PathEntriesAreUnquotedAndEmptiesDropped()
+    {
+        var paths = OsSettingsResolver.ParsePath(@"C:\one;""C:\Program Files\two"" ; ;C:\thr|ee;", ';');
+
+        await Assert.That(paths).IsEquivalentTo([@"C:\one", @"C:\Program Files\two"]);
+    }
+
     [Test]
     public async Task Simple()
     {

src/DiffEngine/DiffRunner.cs

@@ -272,6 +272,7 @@ static LaunchResult InnerLaunch(TryResolveTool tryResolveTool, string tempFile,
         }
 
         var processId = LaunchProcess(tool, arguments);
+        ProcessCleanup.Track(command, processId);
 
         DiffEngineTray.AddMove(tempFile, targetFile, tool.ExePath, arguments, canKill, processId);
 
@@ -316,6 +317,7 @@ static async Task<LaunchResult> InnerLaunchAsync(TryResolveTool tryResolveTool,
         }
 
         var processId = LaunchProcess(tool, arguments);
+        ProcessCleanup.Track(command, processId);
 
         await DiffEngineTray.AddMoveAsync(tempFile, targetFile, tool.ExePath, arguments, canKill, processId);
 

src/DiffEngine/Inline/InlineApplier.cs

@@ -81,7 +81,7 @@ static InlineApplyResult Run(InlinePatch patch, bool write, bool anchorOnly = fa
         var normalizedPath = fullPath.ToLowerInvariant();
         lock (gates.GetOrAdd(normalizedPath, static _ => new()))
         {
-            using var mutex = new Mutex(false, MutexName(normalizedPath));
+            using var mutex = OpenMutex(MutexName(normalizedPath));
             var owned = false;
             try
             {
@@ -397,6 +397,32 @@ static void MoveIntoPlace(string temporary, string fullPath, Exception replaceFa
         return (new UTF8Encoding(false, true), 0);
     }
 
+    /// <summary>
+    /// Machine wide off Windows. A name with no prefix is session scoped, and on Linux and macOS a
+    /// session is a POSIX session - every terminal has its own - so an IDE applying a staged patch
+    /// and a viewer started from a terminal's test run each held a mutex of their own, both
+    /// rewrote the file, and one literal was lost. On Windows the session is the logon session,
+    /// which every process involved already shares. Falls back to the session scoped name where
+    /// the global namespace cannot be used.
+    /// </summary>
+    static Mutex OpenMutex(string name)
+    {
+        if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
+        {
+            return new(false, name);
+        }
+
+        try
+        {
+            return new(false, $@"Global\{name}");
+        }
+        catch (Exception exception)
+            when (exception is UnauthorizedAccessException or IOException)
+        {
+            return new(false, name);
+        }
+    }
+
     static string MutexName(string normalizedPath)
     {
         var hash = SHA256.HashData(Encoding.UTF8.GetBytes(normalizedPath));

src/DiffEngine/Inline/InlinePatcher.cs

@@ -86,6 +86,13 @@ static string[] EntryPoints(string[]? declared)
         var names = new List<string>(builtInEntryPoints);
         foreach (var name in declared)
         {
+            // An empty name matches everywhere and advances nothing, so the search for it never
+            // ended - while holding the file's mutex. "VerifyDocx," arrives as one from a payload
+            if (string.IsNullOrWhiteSpace(name))
+            {
+                continue;
+            }
+
             if (!names.Contains(name, StringComparer.Ordinal))
             {
                 names.Add(name);
@@ -969,7 +976,7 @@ static int NextMemberLine(string source, SourceScan scan, List<int> lineStarts,
                 continue;
             }
 
-            if (scan.IsDeclaration(index) &&
+            if (scan.IsDeclaration(DeclarationStart(source, index)) &&
                 LeadingWhitespace(source, lineStarts, index).Length <= memberIndent)
             {
                 return LineOf(lineStarts, index);
@@ -1096,7 +1103,7 @@ static int Clamp(int line, int lineCount) =>
             if (scan.IsCode(index) &&
                 StartsToken(source, scan, index) &&
                 (end >= source.Length || !scan.IsIdentifierChar(source[end])) &&
-                scan.IsDeclaration(index))
+                scan.IsDeclaration(DeclarationStart(source, index)))
             {
                 var line = LineOf(lineStarts, index);
                 if (best < 0 ||
@@ -1112,6 +1119,24 @@ static int Clamp(int line, int lineCount) =>
         return best < 0 ? null : best;
     }
 
+    /// <summary>
+    /// Where a declared name starts for the purpose of asking what declares it: before the opening
+    /// backticks of an F# <c>``test name``</c>, the usual way an F# test is named. Judged from the
+    /// name itself, the backtick in front of it is not a keyword, so no such member was ever found
+    /// and the search it should have bounded ran across the whole file.
+    /// </summary>
+    static int DeclarationStart(string source, int nameStart)
+    {
+        if (nameStart >= 2 &&
+            source[nameStart - 1] == '`' &&
+            source[nameStart - 2] == '`')
+        {
+            return nameStart - 2;
+        }
+
+        return nameStart;
+    }
+
     static bool StartsToken(string source, SourceScan scan, int index) =>
         index == 0 ||
         !scan.IsIdentifierChar(source[index - 1]);

src/DiffEngine/OsSettingsResolver.cs

@@ -11,19 +11,45 @@ static OsSettingsResolver()
 
         if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
         {
-            envPaths = pathVariable.Split(';');
+            envPaths = ParsePath(pathVariable, ';');
         }
         else if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux) ||
                  RuntimeInformation.IsOSPlatform(OSPlatform.OSX))
         {
-            envPaths = pathVariable.Split(':');
+            envPaths = ParsePath(pathVariable, ':');
         }
         else
         {
             envPaths = [];
         }
     }
 
+    /// <summary>
+    /// PATH as directories that can be combined with a file name. Windows allows an entry in
+    /// quotes, and some installers write them that way, and .NET Framework's Path.Combine throws
+    /// on the quote - out of this type's static constructor, so every tool lookup in the process
+    /// failed for good. Quotes and surrounding space are taken off, and whatever still holds a
+    /// character no path can is dropped, along with empty entries.
+    /// </summary>
+    internal static string[] ParsePath(string value, char separator)
+    {
+        var invalid = Path.GetInvalidPathChars();
+        var paths = new List<string>();
+        foreach (var entry in value.Split(separator))
+        {
+            var path = entry.Trim().Trim('"').Trim();
+            if (path.Length == 0 ||
+                path.IndexOfAny(invalid) >= 0)
+            {
+                continue;
+            }
+
+            paths.Add(path);
+        }
+
+        return paths.ToArray();
+    }
+
     public static bool Resolve(
         string tool,
         OsSupport osSupport,

src/DiffEngine/Process/ProcessCleanup.cs

@@ -3,6 +3,7 @@
 public static class ProcessCleanup
 {
     static List<ProcessCommand> commands;
+    static readonly object gate = new();
     static Func<HashSet<string>?, List<ProcessCommand>> findAll;
     static Func<int, bool> tryTerminateProcess;
 
@@ -61,7 +62,9 @@ public static void Kill(string command)
         }
 
         var matchingCommands = Commands
-            .Where(_ => _.Command == command).ToList();
+            .Where(_ => _.Command == command)
+            .Where(StillRunning)
+            .ToList();
         Logging.Write($"Kill: {command}. Matching count: {matchingCommands.Count}");
         if (matchingCommands.Count == 0)
         {
@@ -92,7 +95,55 @@ public static bool TryGetProcessInfo(string command, out ProcessCommand process)
         }
 
         process = commands.FirstOrDefault(_ => _.Command == command);
-        return !process.Equals(default(ProcessCommand));
+        if (process.Equals(default(ProcessCommand)))
+        {
+            return false;
+        }
+
+        if (StillRunning(process))
+        {
+            return true;
+        }
+
+        Forget(process);
+        process = default;
+        return false;
+    }
+
+    /// <summary>
+    /// A tool this process started, so a relaunch or a kill later in the same run finds it. The
+    /// list is otherwise taken once, when the type initialises.
+    /// </summary>
+    internal static void Track(string command, int processId)
+    {
+        if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
+        {
+            command = TrimCommand(command);
+        }
+
+        lock (gate)
+        {
+            commands = [new(command, processId), ..commands];
+        }
+    }
+
+    /// <summary>
+    /// Whether the process a snapshot of the list named is still the one it named. The list is
+    /// taken once per test process, so a tool closed since then has left a PID that Windows can
+    /// hand to anything else - and killing by that PID terminated whatever got it. Asked only on a
+    /// hit, which is rare, and answered by reading that process's command line again.
+    /// </summary>
+    static bool StillRunning(ProcessCommand process) =>
+        findAll(CandidateExeNames())
+            .Any(_ => _.Process == process.Process &&
+                      _.Command == process.Command);
+
+    static void Forget(ProcessCommand process)
+    {
+        lock (gate)
+        {
+            commands = commands.Where(_ => !_.Equals(process)).ToList();
+        }
     }
 
     static void TerminateProcessIfExists(in int processId)

src/DiffEngineViewer.Tests/ViewerSessionTests.cs

@@ -780,6 +780,40 @@ public async Task AQueueChangeClosesTheMenu()
         await Assert.That(synced.Menu).IsNull();
     }
 
+    /// <summary>
+    /// An attached viewer syncs five times a second, almost always to the same queue. Each one
+    /// cleared the open menu, so a right-click menu closed within 200ms.
+    /// </summary>
+    [Test]
+    public async Task AnUnchangedListingKeepsTheMenuOpen()
+    {
+        var open = ViewerSession.OpenMenu(Fixtures.Attached(Fixtures.Pending(Fixtures.Patch())), 0);
+        await Assert.That(open.Menu).IsNotNull();
+
+        var synced = ViewerSession.Sync(open, Fixtures.Pending(Fixtures.Patch()), [], null);
+
+        await Assert.That(synced.Menu).IsNotNull();
+    }
+
+    /// <summary>
+    /// Copying a whole side hands over the file's lines. Flattened for the screen, every tab
+    /// became four spaces, and pasting that into a verified file changed it.
+    /// </summary>
+    [Test]
+    public async Task CopyingAWholeSideKeepsTabs() =>
+        await Assert.That(SelectionText.All(Fixtures.Move(left: "a\tb", right: "a\tb"), PaneSide.Left)).IsEqualTo("a\tb");
+
+    [Test]
+    public async Task AFrameWithNothingInItTakesNoLock()
+    {
+        var state = Fixtures.Inline(Fixtures.Patch());
+        var idle = new ViewerInput(CommandKind.None, -1, -1, 0, false, state.Columns, state.Rows);
+
+        await Assert.That(ViewerProgram.IsIdle(idle, state)).IsTrue();
+        await Assert.That(ViewerProgram.IsIdle(idle with { ScrollDelta = 1 }, state)).IsFalse();
+        await Assert.That(ViewerProgram.IsIdle(idle with { Columns = state.Columns + 1 }, state)).IsFalse();
+    }
+
     /// <summary>
     /// A settle that empties the queue sets Exit, and the loop acts on it a frame later. An arrival
     /// in between is a reason to stay: carried across, Exit took the new entry out with the window.