From 69d9bb8a59ec16287b697ebf552cb3870990cbec Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sun, 4 Oct 2026 13:34:50 +1100 Subject: [PATCH 1/7] Bind the Options form tests' hot keys to a stand-in desktop OptionsFormLauncherTests registered Ctrl+Alt+Shift+F13 to F17 with Windows for the length of each test, and asked what was held by trying to take it. A hot key is held for every process on the machine, so the tests failed wherever something else had one of those keys and took them from anything that wanted one while they ran. They are about which keys a save leaves bound, and all the desktop adds to that is refusing a combination that is already held. So the register is given a stand-in through KeyRegister's internal constructor, as KeyRegisterTests was: it keeps which id holds which combination and refuses a combination or an id that is taken, the two refusals RegisterHotKey gives. What is asserted is what the stand-in was left holding. Checked by running the class in Release: both tests pass, and nothing in the file calls the constructor that registers with Windows. --- .../OptionsFormLauncherTests.cs | 70 ++++++++++++------- 1 file changed, 46 insertions(+), 24 deletions(-) diff --git a/src/DiffEngineTray.Tests/OptionsFormLauncherTests.cs b/src/DiffEngineTray.Tests/OptionsFormLauncherTests.cs index 11639abce..d8ffd2f76 100644 --- a/src/DiffEngineTray.Tests/OptionsFormLauncherTests.cs +++ b/src/DiffEngineTray.Tests/OptionsFormLauncherTests.cs @@ -1,12 +1,20 @@ using Keys = System.Windows.Forms.Keys; /// -/// Saving the Options form re-registers all three hot keys. The combinations used here are held -/// for the length of a test and are ones nothing else is likely to want. +/// Saving the Options form re-registers all three hot keys. +/// +/// Nothing is registered with the desktop. These used to take Ctrl+Alt+Shift+F13 to F17 for the +/// length of a test, keys held for every process on the machine: they failed wherever something +/// else had one. What they are about is which keys a save leaves bound, and the one thing the +/// desktop adds to that is refusing a combination that is already held, which the stand-in below +/// does as Windows does. +/// /// [TUnit.Core.Executors.STAThreadExecutor] public class OptionsFormLauncherTests { + const KeyModifiers modifiers = KeyModifiers.Control | KeyModifiers.Shift | KeyModifiers.Alt; + /// /// Each hot key used to be bound as it was reached, so a collision on the second returned /// with the first already live on its new combination - while settings.json, which is written @@ -20,23 +28,24 @@ public async Task A_collision_leaves_every_hot_key_as_it_was() { AcceptAllHotKey = HotKey(Keys.F13) }; - // Both on one combination, so the second registration is refused by the OS rather than by - // whatever else happens to be running + // Both on one combination, so the second registration is refused for a reason this test + // made itself var settings = new Settings { AcceptAllHotKey = HotKey(Keys.F14), DiscardAllHotKey = HotKey(Keys.F14) }; + var desktop = new Desktop(); await using var tracker = new RecordingTracker(); - using var register = new KeyRegister(0); + using var register = desktop.Register(); Bind(register, previous, tracker); var errors = OptionsFormLauncher.ReBind(register, tracker, previous, settings); await Assert.That(errors).IsNotEmpty(); - // What the OS thinks, which is the only account of it that matters - await Assert.That(IsHeld(Keys.F14)).IsFalse(); - await Assert.That(IsHeld(Keys.F13)).IsTrue(); + // What the desktop was left holding, which is the only account of it that matters + await Assert.That(desktop.IsHeld(Keys.F14)).IsFalse(); + await Assert.That(desktop.IsHeld(Keys.F13)).IsTrue(); } [Test] @@ -51,38 +60,51 @@ public async Task A_save_that_binds_takes_every_hot_key() AcceptAllHotKey = HotKey(Keys.F16), DiscardAllHotKey = HotKey(Keys.F17) }; + var desktop = new Desktop(); await using var tracker = new RecordingTracker(); - using var register = new KeyRegister(0); + using var register = desktop.Register(); Bind(register, previous, tracker); var errors = OptionsFormLauncher.ReBind(register, tracker, previous, settings); await Assert.That(errors).IsEmpty(); - await Assert.That(IsHeld(Keys.F16)).IsTrue(); - await Assert.That(IsHeld(Keys.F17)).IsTrue(); + await Assert.That(desktop.IsHeld(Keys.F16)).IsTrue(); + await Assert.That(desktop.IsHeld(Keys.F17)).IsTrue(); // The one it replaced is given up, rather than left registered for a key the settings no // longer mention - await Assert.That(IsHeld(Keys.F15)).IsFalse(); + await Assert.That(desktop.IsHeld(Keys.F15)).IsFalse(); } static void Bind(KeyRegister register, Settings settings, RecordingTracker tracker) => Program.ReBindKeys(settings, register, tracker); /// - /// Whether anything holds the combination, asked by trying to take it. A register of one that - /// is already taken is what the OS refuses, so a success means it was free - and gives it - /// straight back. + /// What a register asks of the desktop, answered here: which id holds which combination. A + /// combination something holds is refused to the next id that asks for it, and so is an id + /// that already holds one, which are the two refusals RegisterHotKey gives. /// - static bool IsHeld(Keys key) + class Desktop { - using var probe = new KeyRegister(0); - return !probe.TryAddBinding( - id: 9, - KeyModifiers.Control | KeyModifiers.Shift | KeyModifiers.Alt, - key, - () => - { - }); + Dictionary held = []; + + public bool IsHeld(Keys key) => + held.ContainsValue((modifiers, key)); + + public KeyRegister Register() => + new( + IntPtr.Zero, + (_, id, modifiers, key) => + { + if (held.ContainsKey(id) || + held.ContainsValue((modifiers, key))) + { + return false; + } + + held.Add(id, (modifiers, key)); + return true; + }, + (_, id) => held.Remove(id)); } static HotKey HotKey(Keys key) => From 271a8de17b5f7265c9710ab8d0f9e637d62228ea Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sun, 4 Oct 2026 13:34:50 +1100 Subject: [PATCH 2/7] Give the tray version marker tests a marker of their own TrayVersionFileTest wrote and deleted %TEMP%\DiffEngineTray\version.txt, which is one file for the whole machine: a run of the suite removed the marker of the tray running there, and two of the tests running together read what the other had written. Write, Delete and TryRead each gain an internal overload that takes the file, and the public ones call them with FilePath, so the tray does what it did. Each test now uses a file in a folder named for that test instance, removed when it is done. Delete, which the tests only ever used to tidy up, has two tests of its own, and one more holds FilePath to where released libraries look for it without touching the file. Nothing else in the test projects reads or writes the marker: DiffEngine.Tests does not name TrayVersionFile, and the tray's session end test is handed its own action in place of Delete. Checked by running the class and then the whole of DiffEngineTray.Tests in Release: the real marker was absent before and after, and no folder of the tests' was left in the temp directory. --- src/DiffEngine/Tray/TrayVersionFile.cs | 28 +++-- .../TrayVersionFileTest.cs | 103 ++++++++++++------ 2 files changed, 88 insertions(+), 43 deletions(-) diff --git a/src/DiffEngine/Tray/TrayVersionFile.cs b/src/DiffEngine/Tray/TrayVersionFile.cs index 4eb9c25e2..d8e21742b 100644 --- a/src/DiffEngine/Tray/TrayVersionFile.cs +++ b/src/DiffEngine/Tray/TrayVersionFile.cs @@ -7,18 +7,30 @@ static class TrayVersionFile public static string FilePath { get; } = Path.Combine(Path.GetTempPath(), "DiffEngineTray", "version.txt"); - public static void Write(string informationalVersion) + public static void Write(string informationalVersion) => + Write(FilePath, informationalVersion); + + public static void Delete() => + Delete(FilePath); + + public static bool TryRead([NotNullWhen(true)] out Version? version) => + TryRead(FilePath, out version); + + // The three below take the file, for the tests: the marker is one file for the whole machine, + // and a test that wrote and deleted it removed the marker of the tray running there. + + internal static void Write(string path, string informationalVersion) { - var directory = Path.GetDirectoryName(FilePath)!; + var directory = Path.GetDirectoryName(path)!; Directory.CreateDirectory(directory); - File.WriteAllText(FilePath, StripSuffix(informationalVersion)); + File.WriteAllText(path, StripSuffix(informationalVersion)); } - public static void Delete() + internal static void Delete(string path) { try { - File.Delete(FilePath); + File.Delete(path); } catch { @@ -26,17 +38,17 @@ public static void Delete() } } - public static bool TryRead([NotNullWhen(true)] out Version? version) + internal static bool TryRead(string path, [NotNullWhen(true)] out Version? version) { version = null; try { - if (!File.Exists(FilePath)) + if (!File.Exists(path)) { return false; } - var text = StripSuffix(File.ReadAllText(FilePath).Trim()); + var text = StripSuffix(File.ReadAllText(path).Trim()); return Version.TryParse(text, out version); } catch diff --git a/src/DiffEngineTray.Tests/TrayVersionFileTest.cs b/src/DiffEngineTray.Tests/TrayVersionFileTest.cs index 817ba0317..50c5bf2a1 100644 --- a/src/DiffEngineTray.Tests/TrayVersionFileTest.cs +++ b/src/DiffEngineTray.Tests/TrayVersionFileTest.cs @@ -1,57 +1,90 @@ -public class TrayVersionFileTest +/// +/// Against a marker of each test's own, in a folder nothing else knows about. These used to write +/// and delete the real one, which is one file for the whole machine: a run of them removed the +/// marker of the tray running there, and two of them at once read what the other had written. +/// +public class TrayVersionFileTest : + IDisposable { - [Test] - public async Task RoundTrip() + string directory = Path.Combine(Path.GetTempPath(), $"TrayVersionFileTest_{Guid.NewGuid():N}"); + string path; + + // The folder is left for Write to make, as the tray's first start on a machine leaves it + public TrayVersionFileTest() => + path = Path.Combine(directory, "version.txt"); + + public void Dispose() { - TrayVersionFile.Write("20.1.3+abc123"); - try + if (Directory.Exists(directory)) { - var read = TrayVersionFile.TryRead(out var version); - await Assert.That(read).IsTrue(); - await Assert.That(version).IsEqualTo(new(20, 1, 3)); - } - finally - { - TrayVersionFile.Delete(); + Directory.Delete(directory, true); } } + [Test] + public async Task RoundTrip() + { + TrayVersionFile.Write(path, "20.1.3+abc123"); + + var read = TrayVersionFile.TryRead(path, out var version); + await Assert.That(read).IsTrue(); + await Assert.That(version).IsEqualTo(new(20, 1, 3)); + } + [Test] public async Task PrereleaseSuffixStripped() { - TrayVersionFile.Write("21.0.0-beta.1"); - try - { - var read = TrayVersionFile.TryRead(out var version); - await Assert.That(read).IsTrue(); - await Assert.That(version).IsEqualTo(new(21, 0, 0)); - } - finally - { - TrayVersionFile.Delete(); - } + TrayVersionFile.Write(path, "21.0.0-beta.1"); + + var read = TrayVersionFile.TryRead(path, out var version); + await Assert.That(read).IsTrue(); + await Assert.That(version).IsEqualTo(new(21, 0, 0)); } [Test] public async Task MissingFileFails() { - TrayVersionFile.Delete(); - var read = TrayVersionFile.TryRead(out _); + var read = TrayVersionFile.TryRead(path, out _); await Assert.That(read).IsFalse(); } + [Test] + public async Task DeleteRemovesTheMarker() + { + TrayVersionFile.Write(path, "20.1.3"); + + TrayVersionFile.Delete(path); + + await Assert.That(File.Exists(path)).IsFalse(); + await Assert.That(TrayVersionFile.TryRead(path, out _)).IsFalse(); + } + + /// + /// A session that ends removes the marker whether or not the tray got as far as writing it. + /// + [Test] + public async Task DeleteOfAMarkerThatIsNotThereIsQuiet() + { + TrayVersionFile.Delete(path); + + await Assert.That(Directory.Exists(directory)).IsFalse(); + } + [Test] public async Task GarbageFails() { - TrayVersionFile.Write("garbage"); - try - { - var read = TrayVersionFile.TryRead(out _); - await Assert.That(read).IsFalse(); - } - finally - { - TrayVersionFile.Delete(); - } + TrayVersionFile.Write(path, "garbage"); + + var read = TrayVersionFile.TryRead(path, out _); + await Assert.That(read).IsFalse(); } + + /// + /// The one thing here that names the real marker, and only names it: where it is has to stay + /// where every DiffEngine already released looks for it. + /// + [Test] + public async Task TheMarkerIsWhereItHasAlwaysBeen() => + await Assert.That(TrayVersionFile.FilePath) + .IsEqualTo(Path.Combine(Path.GetTempPath(), "DiffEngineTray", "version.txt")); } From 905cfbd6a4d51a14f6a0ad7628fb83f1a617decb Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sun, 4 Oct 2026 13:34:50 +1100 Subject: [PATCH 3/7] Open the menu in the menu tests without putting it on the desktop Five places in MenuBuilderTest called menu.Show(0, 0) to raise Opening, which is where the tracked items are built. A context menu is a top level window, so each one came up at the top left of the screen of whoever was at the machine, and one of the tests opened an item's drop down beside it. The menu is now made not top level before it is shown, and so is the drop down that is drawn. Neither then has a parent that is on a screen, and both still open, lay out, paint and close: Opening and Closed are raised as before, which is what four of the tests are about. Verify.WinForms draws a menu the same way, which is why the tests that only hand it a menu never showed one. A new test asks Windows, with IsWindowVisible, that a menu and a drop down opened this way are open and not visible. It was not run against the old call, since that would have put the menu on the desktop. Checked by running the class in Release: fifteen tests pass and no baseline moved, SnapshotThatWasNotWrittenDropDown's picture included. --- src/DiffEngineTray.Tests/MenuBuilderTest.cs | 64 +++++++++++++++++++-- 1 file changed, 59 insertions(+), 5 deletions(-) diff --git a/src/DiffEngineTray.Tests/MenuBuilderTest.cs b/src/DiffEngineTray.Tests/MenuBuilderTest.cs index b78d84690..16fd10747 100644 --- a/src/DiffEngineTray.Tests/MenuBuilderTest.cs +++ b/src/DiffEngineTray.Tests/MenuBuilderTest.cs @@ -78,7 +78,7 @@ public async Task SnapshotThatWasNotWrittenCarriesItsReason() tracker); // The tracked items are built on Opening and exist only while the menu is up, so a test // that reads Items without this one sees the six fixed entries and nothing else - menu.Show(0, 0); + Open(menu); var item = menu.Items .OfType() @@ -187,17 +187,71 @@ public async Task SnapshotThatWasNotWrittenDropDown() emptyAction, emptyAction, tracker); - menu.Show(0, 0); + Open(menu); var item = menu.Items .OfType() .Single(_ => _.Text!.StartsWith("Sample.cs:12")); + // Before it is shown, as the menu was: a drop down is a window of its own + item.DropDown.TopLevel = false; item.ShowDropDown(); await Verify(Draw(item.DropDown), "png", settings); menu.Close(); } + /// + /// Opened, for the items an open builds, with nothing put on the desktop. A menu is a top + /// level window, and shown as one it came up at the top left of the screen of whoever was at + /// the machine, over what they were doing, for as long as the test took. Not top level it is + /// a child of no window that is on a screen, and still opens, lays out, paints and closes as + /// it does under the tray's icon. Verify.WinForms draws a menu the same way, which is why the + /// tests here that only hand it one never showed anything. + /// + static void Open(System.Windows.Forms.ContextMenuStrip menu) + { + menu.TopLevel = false; + menu.Show(); + } + + /// + /// What is for, asked of Windows rather than of WinForms: a menu that is + /// open as far as its own events and items go, in a window nobody can see. The drop down is + /// here because it is a second window, and has to be told separately. + /// + [Test] + public async Task AMenuOpenedByATestIsNotOnTheDesktop() + { + await using var tracker = new RecordingTracker( + inline: new StubInlineHost(new PendingSnapshot("c:\\repo\\sample.cs|12", "Sample.cs:12", null))); + var menu = MenuBuilder.Build( + emptyAction, + emptyAction, + tracker); + Open(menu); + var item = menu.Items + .OfType() + .Single(); + item.DropDown.TopLevel = false; + item.ShowDropDown(); + + var menuOpen = menu.Visible; + var menuSeen = IsWindowVisible(menu.Handle); + var dropDownOpen = item.DropDown.Visible; + var dropDownSeen = IsWindowVisible(item.DropDown.Handle); + menu.Close(); + + await Assert.That(menuOpen).IsTrue(); + await Assert.That(menuSeen).IsFalse(); + await Assert.That(dropDownOpen).IsTrue(); + await Assert.That(dropDownSeen).IsFalse(); + } + + // False for a window any of whose parents is hidden, which is what says a window that is + // shown is still not on a screen + [System.Runtime.InteropServices.DllImport("user32.dll")] + static extern bool IsWindowVisible(IntPtr handle); + /// /// Drawn rather than handed to Verify.WinForms, which renders a control by parenting it to a /// form of its own - something a drop down, being a top level window already, refuses. @@ -305,7 +359,7 @@ public async Task A_group_named_after_a_fixed_item_is_removed_with_the_rest() tracker); var fixedCount = menu.Items.Count; - menu.Show(0, 0); + Open(menu); var opened = menu.Items .Cast() .Skip(fixedCount) @@ -339,7 +393,7 @@ public async Task Tracked_items_are_disposed_by_the_next_open() tracker); var fixedCount = menu.Items.Count; - menu.Show(0, 0); + Open(menu); var tracked = menu.Items .Cast() .Skip(fixedCount) @@ -353,7 +407,7 @@ public async Task Tracked_items_are_disposed_by_the_next_open() } menu.Close(); - menu.Show(0, 0); + Open(menu); menu.Close(); await Assert.That(tracked).IsNotEmpty(); From af3085ee6d00bfe4803cc3b071f1678e270ef636 Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sun, 4 Oct 2026 13:41:41 +1100 Subject: [PATCH 4/7] Move the tests of RowText.Shown to the core's test project RowText is the core's, and its tests were in DiffEngineViewer.Windows.Tests because the WinForms head is what asks for the start of a row. Nothing in them is about WinForms, and there they ran on Windows alone. Beside CellGridTests they run on all three platforms, as the grid's own tests do. The file is moved as it was, apart from the paragraph of its summary that explained why it was not there. No baselines go with it: these tests have none. Checked by a Release build of the solution and a Release run of DiffEngineViewer.Tests, which has the three tests in it and passes. --- .../RowTextTests.cs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) rename src/{DiffEngineViewer.Windows.Tests => DiffEngineViewer.Tests}/RowTextTests.cs (93%) diff --git a/src/DiffEngineViewer.Windows.Tests/RowTextTests.cs b/src/DiffEngineViewer.Tests/RowTextTests.cs similarity index 93% rename from src/DiffEngineViewer.Windows.Tests/RowTextTests.cs rename to src/DiffEngineViewer.Tests/RowTextTests.cs index a5848c09f..f0cffce9b 100644 --- a/src/DiffEngineViewer.Windows.Tests/RowTextTests.cs +++ b/src/DiffEngineViewer.Tests/RowTextTests.cs @@ -1,10 +1,6 @@ /// -/// The start of a row, which is all of it this head hands to GDI+: no more than its pane has cells -/// for, cut where the grid would cut it, and found without reading the rest of the row. -/// -/// Here rather than beside CellGridTests because this head is what asks for it. Nothing in -/// it is about WinForms. -/// +/// The start of a row, which is all of it the WinForms head hands to GDI+: no more than its pane +/// has cells for, cut where the grid would cut it, and found without reading the rest of the row. /// public class RowTextTests { From 228f83fffd905da12e2aefc33795ba9000b28045 Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sun, 4 Oct 2026 13:41:41 +1100 Subject: [PATCH 5/7] Show the windows of the raise, placement and frame wait tests on a desktop nobody is at Seven tests in WindowPlacementTests, ViewerFormRaiseTests and FrameWaitTests showed a ViewerForm the ordinary way, so it was the foreground window while they ran and took the keyboard of whoever was at the machine. Five of them also put a window nobody had made transparent on the screen, three of those maximised. Parking, which is what the hosts that post input got, does not reach these. A parked form is shown without activation, but what these tests do next is maximise, minimise, restore and raise, and WinForms asks each of Windows by a call that activates the window whatever it was shown with. A maximised window is on a display wherever it was put. And Raise ends in Activate, which is its point: a seam that left that out would be a test of something other than a raise. So the windows are made on a desktop of the test process's own (UnseenDesktop). Only the desktop that has the display is drawn or takes input, and on another one a window is shown, activated and sized by the same calls against the same displays. Nothing about the product changes and no assertion is weaker: each test asks what it asked before, of a window that really was raised or maximised. A thread with a window cannot be moved to another desktop, and the thread a test starts on has one already, COM's for its apartment, so each such test runs its window on a thread of its own and asserts on what comes back. Where a desktop cannot be made the tests run where they always did. The three placement tests that make a handle and never show the window are left as they were: there is nothing of them on the desktop. Checked by UnseenDesktopTests, which shows, maximises and activates a plain form there and finds it is never the foreground window, and by breaking Raise in a scratch build, which failed all four raise tests. The three classes ran five times each, and five more with DOTNET_PROCESSOR_COUNT=2. --- .../FrameWaitTests.cs | 30 +++-- .../UnseenDesktop.cs | 122 ++++++++++++++++++ .../UnseenDesktopTests.cs | 48 +++++++ .../ViewerFormRaiseTests.cs | 75 +++++++---- .../WindowPlacementTests.cs | 59 ++++++--- 5 files changed, 276 insertions(+), 58 deletions(-) create mode 100644 src/DiffEngineViewer.Windows.Tests/UnseenDesktop.cs create mode 100644 src/DiffEngineViewer.Windows.Tests/UnseenDesktopTests.cs diff --git a/src/DiffEngineViewer.Windows.Tests/FrameWaitTests.cs b/src/DiffEngineViewer.Windows.Tests/FrameWaitTests.cs index ed5778b8d..734e03a86 100644 --- a/src/DiffEngineViewer.Windows.Tests/FrameWaitTests.cs +++ b/src/DiffEngineViewer.Windows.Tests/FrameWaitTests.cs @@ -22,19 +22,25 @@ public async Task Waits(bool visible, FormWindowState state, int milliseconds) = [Test] public async Task AMinimisedWindowIsStillVisible() { - // Shown, since a window never shown is not Visible whatever its state, and transparent - // and parked, as ViewerFormRaiseTests shows one, so nothing appears on the desktop - using var form = new ViewerForm("title", 800, 600) + // Shown, since a window never shown is not Visible whatever its state. Minimising is asked + // of Windows by a call that hands the keyboard on, so on a desktop nobody is at, and + // parked and transparent besides for a machine that has only the one + var (visible, state) = UnseenDesktop.Run(() => { - Opacity = 0, - ShowInTaskbar = false, - StartPosition = FormStartPosition.Manual, - Location = new(-4000, -2000) - }; - form.Show(); - form.WindowState = FormWindowState.Minimized; + using var form = new ViewerForm("title", 800, 600) + { + Opacity = 0, + ShowInTaskbar = false, + StartPosition = FormStartPosition.Manual, + Location = new(-4000, -2000), + Parked = true + }; + form.Show(); + form.WindowState = FormWindowState.Minimized; + return (form.Visible, form.WindowState); + }); - await Assert.That(form.Visible).IsTrue(); - await Assert.That(FormsViewerWindow.FrameWait(form.Visible, form.WindowState)).IsEqualTo(100); + await Assert.That(visible).IsTrue(); + await Assert.That(FormsViewerWindow.FrameWait(visible, state)).IsEqualTo(100); } } diff --git a/src/DiffEngineViewer.Windows.Tests/UnseenDesktop.cs b/src/DiffEngineViewer.Windows.Tests/UnseenDesktop.cs new file mode 100644 index 000000000..2abb7b068 --- /dev/null +++ b/src/DiffEngineViewer.Windows.Tests/UnseenDesktop.cs @@ -0,0 +1,122 @@ +using System.Runtime.ExceptionServices; + +/// +/// A desktop of this process's own, for a test whose subject is what +/// takes away: a window that is activated, maximised, minimised and brought back. Each of those +/// is asked of Windows by a call that also makes the window the one the keyboard goes to, shown +/// without activation or not, and a maximised window is on a display wherever it was put. So on +/// the desktop somebody is working at, such a test took their keyboard and covered their screen. +/// +/// A window belongs to the desktop of the thread that made it, and only the desktop that has the +/// display takes input or is drawn. On another one a window is still a window: it is shown, +/// activated and sized by the same calls, against the same displays, and says the same of itself +/// afterwards. Nobody sees it, and nothing typed reaches it. +/// +/// +static class UnseenDesktop +{ + /// + /// Made the first time it is asked for and kept for the life of the process, which is what + /// closes it. Zero where one could not be made. + /// + static readonly Lazy desktop = new(Create); + + /// + /// Runs on a thread of its own that is on the desktop, and waits for + /// it. Every window the test is about has to be made, used and disposed inside it. + /// + /// A thread of its own because one that has a window cannot be moved, and the thread a test + /// starts on has one before the test's first line: it is a single threaded apartment, which + /// COM gives a window. This one is in no apartment of its own, which nothing these windows do + /// asks for: no clipboard and no drag and drop. + /// + /// + /// Where no desktop could be made, which is a window station that does not allow one, the + /// test runs on the desktop it always did. A machine like that is not one with somebody + /// typing at it. + /// + /// + public static void Run(Action test) + { + ExceptionDispatchInfo? thrown = null; + var thread = new Thread(() => + { + try + { + Enter(); + test(); + } + catch (Exception exception) + { + thrown = ExceptionDispatchInfo.Capture(exception); + } + }); + thread.Start(); + thread.Join(); + thrown?.Throw(); + } + + /// + /// As , for a test that asserts on what the window came to. + /// + public static T Run(Func test) + { + T result = default!; + Run(() => + { + result = test(); + }); + return result; + } + + /// + /// Whether the calling thread is on the desktop, which is every thread + /// starts on a machine that could make one. + /// + public static bool Entered => + desktop.Value != IntPtr.Zero && + GetThreadDesktop(GetCurrentThreadId()) == desktop.Value; + + /// + /// Whether is the window the keyboard goes to at the machine. + /// + public static bool HasTheKeyboard(Form form) => + GetForegroundWindow() == form.Handle; + + static void Enter() + { + if (desktop.Value == IntPtr.Zero) + { + return; + } + + // Not something to carry on from: the desktop is there and this thread is not on it, so + // what the test shows next is shown to whoever is at the machine + if (!SetThreadDesktop(desktop.Value)) + { + throw new System.ComponentModel.Win32Exception(Marshal.GetLastWin32Error(), "The thread could not be moved to the tests' own desktop."); + } + } + + // Named for the process, since a desktop is found by its name and two runs at once would + // otherwise share one + static IntPtr Create() => + CreateDesktop($"DiffEngineViewer.Windows.Tests.{Environment.ProcessId}", null, IntPtr.Zero, 0, genericAll, IntPtr.Zero); + + const uint genericAll = 0x10000000; + + [DllImport("user32.dll", EntryPoint = "CreateDesktopW", CharSet = CharSet.Unicode, SetLastError = true)] + static extern IntPtr CreateDesktop(string name, string? device, IntPtr deviceMode, uint flags, uint access, IntPtr security); + + [DllImport("user32.dll", SetLastError = true)] + static extern bool SetThreadDesktop(IntPtr desktop); + + [DllImport("user32.dll")] + static extern IntPtr GetThreadDesktop(uint thread); + + [DllImport("kernel32.dll")] + static extern uint GetCurrentThreadId(); + + [DllImport("user32.dll")] + static extern IntPtr GetForegroundWindow(); +} diff --git a/src/DiffEngineViewer.Windows.Tests/UnseenDesktopTests.cs b/src/DiffEngineViewer.Windows.Tests/UnseenDesktopTests.cs new file mode 100644 index 000000000..711b479ff --- /dev/null +++ b/src/DiffEngineViewer.Windows.Tests/UnseenDesktopTests.cs @@ -0,0 +1,48 @@ +/// +/// What the tests that show a window the ordinary way rest on: the desktop they show it on is +/// not the one somebody is working at. +/// +[NotInParallel] +public class UnseenDesktopTests +{ + /// + /// A window shown as any window is, activated, maximised and asked for the foreground, which + /// is everything that takes a keyboard. Skipped where no desktop could be made, since there it + /// would be showing exactly that window to whoever is at the machine. + /// + [Test] + public async Task AWindowShownThereNeverHasTheKeyboard() + { + var (entered, shown, hadTheKeyboard) = UnseenDesktop.Run(() => + { + if (!UnseenDesktop.Entered) + { + return (false, false, false); + } + + using var form = new Form + { + ShowInTaskbar = false + }; + form.Show(); + var afterShow = UnseenDesktop.HasTheKeyboard(form); + form.WindowState = FormWindowState.Maximized; + var afterMaximise = UnseenDesktop.HasTheKeyboard(form); + form.Activate(); + return (true, form.Visible, afterShow || afterMaximise || UnseenDesktop.HasTheKeyboard(form)); + }); + Skip.Unless(entered, "This window station does not allow a desktop to be made"); + + await Assert.That(shown).IsTrue(); + await Assert.That(hadTheKeyboard).IsFalse(); + } + + /// + /// A test that fails on the desktop's thread fails where it was written, with what it threw. + /// + [Test] + public async Task WhatATestThrowsThereComesOutHere() => + await Assert.That(() => UnseenDesktop.Run(() => throw new InvalidOperationException("thrown there"))) + .Throws() + .WithMessage("thrown there"); +} diff --git a/src/DiffEngineViewer.Windows.Tests/ViewerFormRaiseTests.cs b/src/DiffEngineViewer.Windows.Tests/ViewerFormRaiseTests.cs index bfde1efa6..3cb5bbd0b 100644 --- a/src/DiffEngineViewer.Windows.Tests/ViewerFormRaiseTests.cs +++ b/src/DiffEngineViewer.Windows.Tests/ViewerFormRaiseTests.cs @@ -1,9 +1,13 @@ /// /// Bringing the window up for a snapshot that has just arrived. The queue owner asks for this over /// the socket, and it is the only thing that puts a new snapshot in front of anyone. +/// +/// Which is to say in front of whoever is at the machine, with their keyboard: a raise that did +/// not do that would not be one. So every window here is raised on , +/// where it is raised for real and nobody is there. +/// /// [NotInParallel] -[TUnit.Core.Executors.STAThreadExecutor] public class ViewerFormRaiseTests { /// @@ -14,12 +18,17 @@ public class ViewerFormRaiseTests [Test] public async Task Restores_a_minimised_window() { - using var form = new ViewerForm("title", 800, 600); - form.WindowState = FormWindowState.Minimized; + var state = UnseenDesktop.Run(() => + { + using var form = new ViewerForm("title", 800, 600); + form.WindowState = FormWindowState.Minimized; + + form.Raise(); - form.Raise(); + return form.WindowState; + }); - await Assert.That(form.WindowState).IsEqualTo(FormWindowState.Normal); + await Assert.That(state).IsEqualTo(FormWindowState.Normal); } /// @@ -29,12 +38,17 @@ public async Task Restores_a_minimised_window() [Test] public async Task Leaves_a_maximised_window_maximised() { - using var form = new ViewerForm("title", 800, 600); - form.WindowState = FormWindowState.Maximized; + var state = UnseenDesktop.Run(() => + { + using var form = new ViewerForm("title", 800, 600); + form.WindowState = FormWindowState.Maximized; + + form.Raise(); - form.Raise(); + return form.WindowState; + }); - await Assert.That(form.WindowState).IsEqualTo(FormWindowState.Maximized); + await Assert.That(state).IsEqualTo(FormWindowState.Maximized); } /// @@ -45,31 +59,42 @@ public async Task Leaves_a_maximised_window_maximised() [Test] public async Task Restores_a_window_minimised_from_maximised_as_maximised() { - // Shown, since what it was minimised from is learnt from the window as it resizes, and - // transparent, so a maximised window does not flash over whoever is running the tests - using var form = new ViewerForm("title", 800, 600) + var (state, placement) = UnseenDesktop.Run(() => { - Opacity = 0, - ShowInTaskbar = false - }; - form.Show(); - form.WindowState = FormWindowState.Maximized; - form.WindowState = FormWindowState.Minimized; + // Shown, since what it was minimised from is learnt from the window as it resizes, + // and transparent, so that on a machine with only the one desktop a maximised window + // does not flash over whoever is running the tests + using var form = new ViewerForm("title", 800, 600) + { + Opacity = 0, + ShowInTaskbar = false + }; + form.Show(); + form.WindowState = FormWindowState.Maximized; + form.WindowState = FormWindowState.Minimized; - form.Raise(); + form.Raise(); - await Assert.That(form.WindowState).IsEqualTo(FormWindowState.Maximized); - await Assert.That(form.Placement!.Value.Maximized).IsTrue(); + return (form.WindowState, form.Placement); + }); + + await Assert.That(state).IsEqualTo(FormWindowState.Maximized); + await Assert.That(placement!.Value.Maximized).IsTrue(); } [Test] public async Task Shows_a_hidden_window() { - using var form = new ViewerForm("title", 800, 600); - form.Visible = false; + var visible = UnseenDesktop.Run(() => + { + using var form = new ViewerForm("title", 800, 600); + form.Visible = false; + + form.Raise(); - form.Raise(); + return form.Visible; + }); - await Assert.That(form.Visible).IsTrue(); + await Assert.That(visible).IsTrue(); } } diff --git a/src/DiffEngineViewer.Windows.Tests/WindowPlacementTests.cs b/src/DiffEngineViewer.Windows.Tests/WindowPlacementTests.cs index 522ab00c4..d26359bf5 100644 --- a/src/DiffEngineViewer.Windows.Tests/WindowPlacementTests.cs +++ b/src/DiffEngineViewer.Windows.Tests/WindowPlacementTests.cs @@ -144,26 +144,33 @@ public async Task AFormOpensAtTheBoundsItIsGiven() [Test] public async Task AFormLeftMaximisedOpensMaximisedAndRestoresToWhereItWas() { - var bounds = OnThisDisplay(); - WindowPlacement? left; - using (var first = new ViewerForm("title", 800, 600, new(bounds.X, bounds.Y, bounds.Width, bounds.Height, false))) + // Maximised, which no parked window can be: it covers a display and is the window the + // keyboard goes to. So on a desktop nobody is at + var (bounds, left, reopened, reopenedAs, restoredTo) = UnseenDesktop.Run(() => { - first.Show(); - first.WindowState = FormWindowState.Maximized; - left = first.Placement; - } - - await Assert.That(left).IsEqualTo(new WindowPlacement(bounds.X, bounds.Y, bounds.Width, bounds.Height, true)); + var bounds = OnThisDisplay(); + WindowPlacement? left; + using (var first = new ViewerForm("title", 800, 600, new(bounds.X, bounds.Y, bounds.Width, bounds.Height, false))) + { + first.Show(); + first.WindowState = FormWindowState.Maximized; + left = first.Placement; + } - using var second = new ViewerForm("title", 800, 600, left); - second.Show(); + using var second = new ViewerForm("title", 800, 600, left); + second.Show(); + var reopened = second.WindowState; + var reopenedAs = second.Placement; - await Assert.That(second.WindowState).IsEqualTo(FormWindowState.Maximized); - await Assert.That(second.Placement).IsEqualTo(left); + second.WindowState = FormWindowState.Normal; - second.WindowState = FormWindowState.Normal; + return (bounds, left, reopened, reopenedAs, second.Bounds); + }); - await Assert.That(second.Bounds).IsEqualTo(bounds); + await Assert.That(left).IsEqualTo(new WindowPlacement(bounds.X, bounds.Y, bounds.Width, bounds.Height, true)); + await Assert.That(reopened).IsEqualTo(FormWindowState.Maximized); + await Assert.That(reopenedAs).IsEqualTo(left); + await Assert.That(restoredTo).IsEqualTo(bounds); } /// @@ -173,14 +180,19 @@ public async Task AFormLeftMaximisedOpensMaximisedAndRestoresToWhereItWas() [Test] public async Task AFormMinimisedFromMaximisedIsRememberedAsMaximised() { - var bounds = OnThisDisplay(); - using var form = new ViewerForm("title", 800, 600, new(bounds.X, bounds.Y, bounds.Width, bounds.Height, false)); - form.Show(); - form.WindowState = FormWindowState.Maximized; + var (bounds, placement) = UnseenDesktop.Run(() => + { + var bounds = OnThisDisplay(); + using var form = new ViewerForm("title", 800, 600, new(bounds.X, bounds.Y, bounds.Width, bounds.Height, false)); + form.Show(); + form.WindowState = FormWindowState.Maximized; + + form.WindowState = FormWindowState.Minimized; - form.WindowState = FormWindowState.Minimized; + return (bounds, form.Placement); + }); - await Assert.That(form.Placement).IsEqualTo(new WindowPlacement(bounds.X, bounds.Y, bounds.Width, bounds.Height, true)); + await Assert.That(placement).IsEqualTo(new WindowPlacement(bounds.X, bounds.Y, bounds.Width, bounds.Height, true)); } /// @@ -212,6 +224,11 @@ public async Task AFormWithNothingRememberedOpensAsItAlwaysDid() /// /// Bounds that are on whatever display the tests are running on, since a form really opens. + /// + /// Opens, in three of these tests, is a handle and no more: the window is made, asked where it + /// is and never shown, so there is nothing of it on the desktop to park. The two that show + /// one do it on , which has the same displays. + /// /// static Rectangle OnThisDisplay() { From 373a349d6341def0f90fe03f1f5daf2ef91186d6 Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sun, 4 Oct 2026 13:48:03 +1100 Subject: [PATCH 6/7] Take what is pending for a file to the lines an accept moved its call sites to A queued snapshot is named by its line, and accepting a snapshot moves every call site under it in the file. The entries left behind were then named by lines that had become other calls', and stayed that way until a run reported them again. Everything that finds an entry by its key had to guess: a re-run from a framework with no content in the entry was queued a second time beside it, a settle from a sibling call that had come to stand on the old line took an entry that was still failing, and two call sites only the line told apart were folded into one. In a batch the same thing happened inside the applier: every patch of a file was recorded against the file as it was read, so the second snapshot was looked for by a line the first had just moved, and an Append, which has nothing but its line, was refused wherever its member had two calls to choose from. A file with several such members took a run for each. The applier now says which lines an applied patch moved (InlineApplyResult.MovedFrom and MovedBy), read off the source before and after as SourceScan.Edited reads where to lex again. A batch asks each patch about the line the edits before it moved its call site to. And whoever owns the queue takes the file's remaining entries along once the outcomes are in (InlineQueue.Rebased): the tray after one accept and after each file of a bulk accept, the viewer the same, on its list where it stands, since an entry at another line is the same texts and the same diff under another key. A batch in the viewer names what it has still to do and a group's members by key, so those follow too, as does the entry being read. A tray driving a viewer's queue decides whether an accept failed by the entry still being under its key. After a snapshot that got shorter the one under it can come up onto that very line, so it now asks for an entry there that says why it failed. What this does not reach is an edit nothing reports: one made by hand, or an accept applied by another process, the IDE plugin among them. For those the questions about whose entry a line names are still asked, as before. Checked by AnAppliedPatchSaysWhichLinesItMoved, ALiteralThatShrinksMovesTheLinesUnderItUp, ABatchsResultsRebaseALineInOrder and AnAppendUnderAnEarlierEditIsAskedAboutTheLineItsCallIsOnNow against real files, four tests of InlineQueue.Rebased, RebasedEntriesTests in the viewer, WhatIsLeftOfAFileGoesToWhereItsCallSitesAre in the tray, and AnEntryThatMovesOntoTheAcceptedLineIsNotAFailedAccept over a real socket. TogetherIsWhatInTurnLeaves and ACarriedScanTellsEachPatchWhatLexingAgainWould now hold a batch to patches applied in turn at their moved lines, the second with the moves worked out a second way. --- .../InlineApplierBatchTests.cs | 166 +++++++++++++++++- src/DiffEngine.Tests/InlineQueueTests.cs | 98 +++++++++++ src/DiffEngine/Inline/InlineApplier.cs | 42 ++++- src/DiffEngine/Inline/InlineApplyResult.cs | 39 ++++ src/DiffEngine/Inline/InlineQueue.cs | 83 +++++++++ src/DiffEngine/Inline/SourceScan.cs | 34 ++++ .../OwnedInlineHostTest.cs | 37 ++++ .../TrayViewerSyncTest.cs | 23 +++ src/DiffEngineTray/OwnedInlineHost.cs | 7 + src/DiffEngineTray/RemoteInlineHost.cs | 6 +- .../RebasedEntriesTests.cs | 141 +++++++++++++++ src/DiffEngineViewer/ViewerSession.cs | 119 ++++++++++++- 12 files changed, 779 insertions(+), 16 deletions(-) create mode 100644 src/DiffEngineViewer.Tests/RebasedEntriesTests.cs diff --git a/src/DiffEngine.Tests/InlineApplierBatchTests.cs b/src/DiffEngine.Tests/InlineApplierBatchTests.cs index 7bc31b186..853b8218a 100644 --- a/src/DiffEngine.Tests/InlineApplierBatchTests.cs +++ b/src/DiffEngine.Tests/InlineApplierBatchTests.cs @@ -33,9 +33,15 @@ InlinePatch[] Patches(string path) => ]; var batch = InlineApplier.ApplyAll(Patches(together.FullName)); - var single = Patches(inTurn.FullName) - .Select(InlineApplier.Apply) - .ToList(); + // In turn, each asked about the line its call site is on by then: a batch brings a + // patch's line along with the edits made above it, where every patch here was recorded + // against the file as it started + var single = new List(); + foreach (var patch in Patches(inTurn.FullName)) + { + var line = single.Aggregate(patch.LineHint, (current, earlier) => earlier.Rebase(current)); + single.Add(InlineApplier.Apply(patch.At(line))); + } await Assert.That(Statuses(batch)).IsEqualTo(Statuses(single)); await Assert.That(Statuses(batch)).IsEqualTo("Applied, Applied, AlreadyApplied, NotFound, Applied, NotFound, Applied, Applied"); @@ -236,6 +242,124 @@ public async Task EncodingAndLineEndingsSurviveABatch() await Assert.That(text.Replace("\r\n", "")).DoesNotContain("\n"); } + /// + /// An applied patch says which lines it moved, so whoever holds other patches for the file + /// can bring their lines along: the lines under the call site, by what the literal grew, and + /// none at or above it. + /// + [Test] + public async Task AnAppliedPatchSaysWhichLinesItMoved() + { + using var file = new TempSource(Members(4)); + + var result = InlineApplier.Apply(Set(file.FullName, 1)); + + await Assert.That(result.Status).IsEqualTo(InlineApplyStatus.Applied); + await Assert.That(result.MovedBy).IsGreaterThan(0); + var lines = file.Text.Split('\n'); + // Each member was on line member + 3 + await Assert.That(lines[result.Rebase(3) - 1]).Contains("void M0()"); + await Assert.That(lines[result.Rebase(4) - 1]).Contains("void M1()"); + await Assert.That(lines[result.Rebase(5) - 1]).Contains("void M2()"); + await Assert.That(lines[result.Rebase(6) - 1]).Contains("void M3()"); + await Assert.That(result.Rebase(4)).IsEqualTo(4); + await Assert.That(result.Rebase(5)).IsEqualTo(5 + result.MovedBy); + } + + /// + /// A literal that gets shorter moves the lines under it up, and one that stays as long moves + /// none. + /// + [Test] + public async Task ALiteralThatShrinksMovesTheLinesUnderItUp() + { + using var file = new TempSource(Members(3)); + var grown = InlineApplier.Apply(Set(file.FullName, 0)); + + var shrunk = InlineApplier.Apply( + new(file.FullName, 3, null, "one line") + { + TestName = null, + MemberName = "M0", + OriginalValue = "new 0\nsecond line" + }); + var same = InlineApplier.Apply( + new(file.FullName, 3, null, "another") + { + TestName = null, + MemberName = "M0", + OriginalValue = "one line" + }); + + await Assert.That(shrunk.Status).IsEqualTo(InlineApplyStatus.Applied); + await Assert.That(shrunk.MovedBy).IsLessThan(0); + await Assert.That(shrunk.MovedBy).IsGreaterThan(-grown.MovedBy - 1); + var lines = file.Text.Split('\n'); + await Assert.That(lines[shrunk.Rebase(grown.Rebase(5)) - 1]).Contains("void M2()"); + await Assert.That(same.Status).IsEqualTo(InlineApplyStatus.Applied); + await Assert.That(same.MovedBy).IsEqualTo(0); + await Assert.That(same).IsSameReferenceAs(InlineApplyResult.Applied); + } + + /// + /// Each result of a batch counts lines as the patches before it left them, so rebasing a + /// line through the results in order gives where it is in the file that was written. + /// + [Test] + public async Task ABatchsResultsRebaseALineInOrder() + { + using var file = new TempSource(Members(5)); + + var results = InlineApplier.ApplyAll([Set(file.FullName, 2), Set(file.FullName, 0), Set(file.FullName, 3)]); + + await Assert.That(Statuses(results)).IsEqualTo("Applied, Applied, Applied"); + var lines = file.Text.Split('\n'); + for (var member = 0; member < 5; member++) + { + var line = results.Aggregate(member + 3, (current, result) => result.Rebase(current)); + await Assert.That(lines[line - 1]).Contains($"void M{member}()"); + } + } + + /// + /// An Append is the patch that cannot do without its line: with no anchor, a line that names + /// no call leaves it to choose among the member's calls, and it is refused where there are + /// two. In a batch the snapshot accepted above it had moved that line, so the second + /// snapshot of a file was refused for the accept of the first. + /// + [Test] + public async Task AnAppendUnderAnEarlierEditIsAskedAboutTheLineItsCallIsOnNow() + { + using var file = new TempSource( + """ + class C + { + void M0() => Verify(value0).Snapshot("old 0"); + Task M1() + { + Verify(first); + return Verify(second); + } + } + """); + InlinePatch[] patches = + [ + Set(file.FullName, 0), + new(file.FullName, 7, null, "appended", InlinePatchMode.Append) + { + TestName = null, + MemberName = "M1" + } + ]; + + var results = InlineApplier.ApplyAll(patches); + + await Assert.That(Statuses(results)).IsEqualTo("Applied, Applied"); + await Assert.That(file.Text).Contains("Verify(second)"); + await Assert.That(file.Text).Contains("Verify(first);"); + await Assert.That(file.Text).DoesNotContain("Verify(second);"); + } + [Test] public async Task AnEmptyBatchIsNoResults() => await Assert.That(InlineApplier.ApplyAll([])).IsEmpty(); @@ -266,13 +390,25 @@ public async Task ACarriedScanTellsEachPatchWhatLexingAgainWould(string extensio InlineApplier.PatchInTurn(language, ref carried, patches, path, results); var lexedAgain = source; + // Where each edit so far moved the lines, worked out here from the two texts, since + // a patch is asked about the line its call site has been moved to + var moves = new List<(int From, int By)>(); for (var index = 0; index < patches.Count; index++) { var patch = patches[index]; + var line = patch.LineHint; + foreach (var (from, by) in moves) + { + if (line >= from) + { + line += by; + } + } + var status = InlinePatcher.TryApply( language, lexedAgain, - patch.LineHint, + line, patch.Mode, patch.OriginalExpression, patch.OriginalValue, @@ -284,6 +420,7 @@ public async Task ACarriedScanTellsEachPatchWhatLexingAgainWould(string extensio out var reason); if (status == PatchStatus.Applied) { + moves.Add(Moved(lexedAgain, patched)); lexedAgain = patched; } @@ -314,6 +451,27 @@ public async Task ACarriedScanTellsEachPatchWhatLexingAgainWould(string extensio await Assert.That(seen).IsEquivalentTo([InlineApplyStatus.Applied, InlineApplyStatus.AlreadyApplied, InlineApplyStatus.NotFound]); } + /// + /// Which lines an edit moved, the slow way: the first line that is whole in what the two + /// texts end with alike, and how many more lines there are. + /// + static (int From, int By) Moved(string before, string after) + { + var beforeLines = before.Split('\n'); + var afterLines = after.Split('\n'); + var limit = Math.Min(beforeLines.Length, afterLines.Length); + var same = 0; + while (same < limit && + beforeLines[beforeLines.Length - 1 - same] == afterLines[afterLines.Length - 1 - same]) + { + same++; + } + + // Counted from the end, so the lines of a literal that were in both texts are not the + // edit's. The first of them is 1 based + return (beforeLines.Length - same + 1, afterLines.Length - beforeLines.Length); + } + /// /// A file of members, each with a call that has a snapshot and one that has none, and a /// shuffled batch of patches for them. diff --git a/src/DiffEngine.Tests/InlineQueueTests.cs b/src/DiffEngine.Tests/InlineQueueTests.cs index 140591a5f..26bb80f17 100644 --- a/src/DiffEngine.Tests/InlineQueueTests.cs +++ b/src/DiffEngine.Tests/InlineQueueTests.cs @@ -784,6 +784,104 @@ public async Task ABatchStepSkipsAnEntryReplacedWhileItApplied() await Assert.That(tally).IsEqualTo(new()); } + /// + /// An accept moves the call sites under it, and the entries pending for that file go with + /// them: the ones under the edit, by what it added, with their variants and status, and no + /// entry above it or in another file. + /// + [Test] + public async Task EntriesUnderAnAcceptedSnapshotGoToTheLinesTheirCallSitesAreOn() + { + var queue = InlineQueue.Empty + .Enqueue(Patch(line: 10, member: "First")) + .Enqueue(Patch(line: 20, member: "Second")) + .Enqueue(Patch(line: 30, member: "Third", framework: "net9.0", content: "nine")) + .Enqueue(Patch(line: 30, member: "Third", framework: "net10.0", content: "ten")) + .Enqueue(Patch("Other.cs", 30)); + var tally = new AcceptAllTally(); + queue = queue.AcceptInBatch(queue.Items[2], InlineApplyResult.Failed("locked"), ref tally); + var held = queue.Items; + + // The snapshot at line 20 is accepted and its literal is four lines longer + var accepted = InlineApplyResult.AppliedMoving(21, 4); + queue = queue + .Accept(held[1], accepted, out _) + .Rebased("Sample.cs", [accepted]); + + await Assert.That(queue.Items.Select(_ => _.Name)).IsEquivalentTo(["Sample.cs:10", "Sample.cs:34", "Other.cs:30"]); + await Assert.That(queue.Items[0]).IsSameReferenceAs(held[0]); + await Assert.That(queue.Items[2]).IsSameReferenceAs(held[3]); + var moved = queue.Items[1]; + await Assert.That(moved.Key).IsEqualTo(InlineKey.For("Sample.cs", 34)); + await Assert.That(moved.Status).IsEqualTo("locked"); + await Assert.That(moved.Variants.Select(_ => _.Patch.LineHint)).IsEquivalentTo([34, 34]); + await Assert.That(moved.Variants.Select(_ => _.Patch.NewContent)).IsEquivalentTo(["nine", "ten"]); + // A copy, since what was queued is shared with whoever is showing or applying it + await Assert.That(held[2].Patch.LineHint).IsEqualTo(30); + } + + /// + /// With the lines kept right, the re-run that follows an accept finds its entry by its key, + /// which is what every other way of finding it was a guess at: here a framework that has no + /// content in the entry yet, which was queued a second time beside it, and a settle from a + /// sibling call in the same member that now stands on the old line, which took the entry. + /// + [Test] + public async Task AfterAnAcceptARerunAndASettleFindAnEntryByItsKey() + { + var accepted = InlineApplyResult.AppliedMoving(11, 5); + var queue = InlineQueue.Empty + .Enqueue(Patch(line: 10, member: "First")) + .Enqueue(Patch(line: 20, member: "Second", framework: "net9.0")); + queue = queue + .Accept(queue.Items[0], accepted, out _) + .Rebased("Sample.cs", [accepted]); + + // Another framework reports the call site where it is now + var rerun = queue.Enqueue(Patch(line: 25, member: "Second", framework: "net10.0")); + await Assert.That(rerun.Items.Single().OriginsLabel).IsEqualTo("net9.0 / net10.0"); + + // A passing call of the same member, on the line the entry used to have and holding + // something else, settles nothing + var settled = queue.Settle(InlineKey.For("Sample.cs", 20), "net9.0", "Second", "unrelated"); + await Assert.That(settled.Items.Single().Key).IsEqualTo(InlineKey.For("Sample.cs", 25)); + } + + /// + /// A snapshot that got shorter brings the lines under it up, and an entry whose hint was + /// already wrong can be standing where another is taken to. A queue holds one entry to a + /// key, so that one stays where it was. + /// + [Test] + public async Task AnEntryIsNotTakenToALineAnotherHolds() + { + var queue = InlineQueue.Empty + .Enqueue(Patch(line: 18, member: "Stale")) + .Enqueue(Patch(line: 20, member: "Second")) + .Enqueue(Patch(line: 40, member: "Third")); + + var after = queue.Rebased("Sample.cs", [InlineApplyResult.AppliedMoving(19, -2)]); + + await Assert.That(after.Items.Select(_ => _.Name)).IsEquivalentTo(["Sample.cs:18", "Sample.cs:20", "Sample.cs:38"]); + await Assert.That(after.Items.Select(_ => _.Key).Distinct().Count()).IsEqualTo(3); + } + + /// + /// Nothing moved is the same queue, so a host can tell, and so can whatever reads a queue's + /// identity as its generation. + /// + [Test] + public async Task AnEditThatMovedNothingLeavesTheQueueAsItIs() + { + var queue = InlineQueue.Empty + .Enqueue(Patch(line: 10)) + .Enqueue(Patch(line: 20)); + + await Assert.That(queue.Rebased("Sample.cs", [InlineApplyResult.Applied, InlineApplyResult.NotFound("gone")])).IsSameReferenceAs(queue); + await Assert.That(queue.Rebased("Sample.cs", [InlineApplyResult.AppliedMoving(21, 3)])).IsSameReferenceAs(queue); + await Assert.That(queue.Rebased("Other.cs", [InlineApplyResult.AppliedMoving(1, 3)])).IsSameReferenceAs(queue); + } + [Test] public async Task DiscardRemovesWithoutApplying() { diff --git a/src/DiffEngine/Inline/InlineApplier.cs b/src/DiffEngine/Inline/InlineApplier.cs index 140bacb15..e9cced12a 100644 --- a/src/DiffEngine/Inline/InlineApplier.cs +++ b/src/DiffEngine/Inline/InlineApplier.cs @@ -22,9 +22,16 @@ public static InlineApplyResult Apply(InlinePatch patch) => /// name. /// /// Each outcome is the one would have reported had it been called on them - /// in turn. A patch is applied to what the ones before it left of its file, so the second of - /// two for one call site finds the first one's literal there, and a call site that an earlier - /// one moved is found where it now is. + /// in turn, each asked about the line the edits before it had moved its call site to. A patch + /// is applied to what the ones before it left of its file, so the second of two for one call + /// site finds the first one's literal there, and a call site that an earlier one moved is + /// looked for where it now is: every patch of a batch was recorded against the file as it + /// was read, and the line is all an Append has to go by. + /// + /// + /// An applied patch says which lines it moved (), + /// counted in the file as the patches before it left it, so the results of a file taken in + /// order bring any other line of that file along. /// /// /// What differs is when the file is written: once, after the last of its patches, through the @@ -445,12 +452,26 @@ internal static int PatchInTurn( continue; } - results[index] = Judge(scan, patches[index], false, fullPath, out var newSource); + // Every patch here was recorded against the file as it was read, and each edit + // above a call site moves it. So a patch is asked about the line its own is on + // now. Asked about the recorded one, the second snapshot of a file was looked + // for by a line that had just stopped being its, and an Append whose line names + // no call is refused where its member has more than one to choose from + var line = patches[index].LineHint; + for (var earlier = firstToWrite; earlier >= 0 && earlier < index; earlier++) + { + line = results[earlier].Rebase(line); + } + + results[index] = Judge(scan, patches[index], false, fullPath, out var newSource, line); if (results[index].Status != InlineApplyStatus.Applied) { continue; } + var (from, by) = scan.LinesMoved(newSource); + results[index] = InlineApplyResult.AppliedMoving(from, by); + // The next patch is applied to this one's result, as it would have been to the // file this one had written source = newSource; @@ -492,7 +513,16 @@ internal static int PatchInTurn( /// caller is given. is the edited source where the outcome is /// Applied, which is all a dry run wanted to know and what a write has still to carry out. /// - static InlineApplyResult Judge(SourceScan scan, InlinePatch patch, bool anchorOnly, string fullPath, out string newSource) + /// The source, lexed. + /// The patch asked about. + /// Whether only a call site to hang a Snapshot call off is asked for. + /// The file, for a failure to name. + /// The edited source, where the outcome is Applied. + /// + /// The line the call site is taken to be on in this source, where edits made since the patch + /// was recorded have moved it. The patch's own when not given. + /// + static InlineApplyResult Judge(SourceScan scan, InlinePatch patch, bool anchorOnly, string fullPath, out string newSource, int? line = null) { PatchStatus status; string failReason; @@ -500,7 +530,7 @@ static InlineApplyResult Judge(SourceScan scan, InlinePatch patch, bool anchorOn { status = InlinePatcher.TryApply( scan, - patch.LineHint, + line ?? patch.LineHint, patch.Mode, patch.OriginalExpression, patch.OriginalValue, diff --git a/src/DiffEngine/Inline/InlineApplyResult.cs b/src/DiffEngine/Inline/InlineApplyResult.cs index 8a3189ed6..1c8f217cb 100644 --- a/src/DiffEngine/Inline/InlineApplyResult.cs +++ b/src/DiffEngine/Inline/InlineApplyResult.cs @@ -36,6 +36,45 @@ public sealed class InlineApplyResult public string? Message { get; } public Exception? Exception { get; } + /// + /// The first line the edit moved, 1 based, in the source as it was before this patch, and + /// is how many lines it and every line after it moved down by, or up + /// by when negative. Zero and zero for an edit that left every line where it was, and for + /// anything that is not . + /// + /// A snapshot is several lines of source, so accepting one moves every call site under it in + /// the file, and every other patch for that file was recorded against the lines as they + /// were. Whoever holds such patches has this to bring them along (), + /// where they otherwise stay wrong until a run reports them again. + /// + /// + public int MovedFrom { get; } + + /// + public int MovedBy { get; } + + InlineApplyResult(int movedFrom, int movedBy) : + this(InlineApplyStatus.Applied, null, null) + { + MovedFrom = movedFrom; + MovedBy = movedBy; + } + + /// + /// Where a line of the source as it was before this patch is once the patch has been applied. + /// + public int Rebase(int line) => + MovedBy != 0 && line >= MovedFrom + ? Math.Max(line + MovedBy, 1) + : line; + + /// + /// Applied, by an edit that moved the lines from on by + /// . + /// + internal static InlineApplyResult AppliedMoving(int from, int by) => + by == 0 ? Applied : new(from, by); + public static readonly InlineApplyResult Applied = new(InlineApplyStatus.Applied, null, null); public static readonly InlineApplyResult AlreadyApplied = new(InlineApplyStatus.AlreadyApplied, null, null); diff --git a/src/DiffEngine/Inline/InlineQueue.cs b/src/DiffEngine/Inline/InlineQueue.cs index 33a8b2d35..2c4051ff8 100644 --- a/src/DiffEngine/Inline/InlineQueue.cs +++ b/src/DiffEngine/Inline/InlineQueue.cs @@ -702,6 +702,89 @@ internal InlineQueue AcceptInBatch(PendingInline entry, InlineApplyResult result return new(items); } + /// + /// The queue with every entry of taken to the line its call + /// site is on once the edits report have been made, in the order + /// they were made. This same queue when nothing moved. + /// + /// A key is a line, and accepting a snapshot moves every call site under it. The entries + /// left behind were then named by lines that had become other calls', and everything that + /// finds an entry by its key had to ask whose it was before believing it: a re-run folded + /// into another call site's entry or queued a second one beside its own, a settle took an + /// entry that was still failing, and the patcher was handed a line that named the wrong + /// call. Those questions are still asked, for an edit made by hand or by another process, + /// which nothing reports. For an accept made here the lines are simply kept right. + /// + /// + /// A step of its own, after the outcomes are recorded and never part of recording one: a + /// batch finds what it claimed by its variants (), and an entry + /// that has moved is another list of them. So whoever accepts calls this once the patches it + /// applied to a file have all been answered for. + /// + /// + /// An entry whose new line is another entry's stays where it is: a queue holds one entry to + /// a key, and the hint was wrong before the edit for that to happen. + /// + /// + internal InlineQueue Rebased(string sourceFile, IReadOnlyList results) + { + if (!results.Any(_ => _.MovedBy != 0)) + { + return this; + } + + // The lines of this file that are spoken for: an entry that stays, and each that moves + // once it has + HashSet? taken = null; + List<(int Index, int Line)>? moving = null; + for (var index = 0; index < Items.Count; index++) + { + var patch = Items[index].Patch; + if (!InlineKey.SamePath(patch.SourceFile, sourceFile)) + { + continue; + } + + var line = patch.LineHint; + foreach (var result in results) + { + line = result.Rebase(line); + } + + taken ??= []; + if (line == patch.LineHint) + { + taken.Add(line); + } + else + { + moving ??= []; + moving.Add((index, line)); + } + } + + if (moving is null) + { + return this; + } + + var items = Items.ToList(); + var changed = false; + foreach (var (index, line) in moving) + { + if (!taken!.Add(line)) + { + taken.Add(items[index].Patch.LineHint); + continue; + } + + items[index] = At(items[index], line); + changed = true; + } + + return changed ? new(items) : this; + } + /// /// What a bulk accept that has finished counts as still needing review: it never applies an /// entry with more than one variant, so whatever the queue holds of those is left for a diff --git a/src/DiffEngine/Inline/SourceScan.cs b/src/DiffEngine/Inline/SourceScan.cs index 4b57405b3..1fe07cf77 100644 --- a/src/DiffEngine/Inline/SourceScan.cs +++ b/src/DiffEngine/Inline/SourceScan.cs @@ -161,6 +161,40 @@ public SourceScan Edited(string edited) return next; } + /// + /// Which lines an edit of this source moved: the first line, 1 based and in this source, that + /// starts at or past the end of what the edit replaced, and how many lines further down it + /// and those after it are in . + /// + /// Read off the two texts, as reads where to lex again, rather than + /// reported by whatever made the edit: every shape the patcher writes is then counted the + /// same way, including one it learns later. + /// + /// + public (int From, int By) LinesMoved(string edited) + { + var limit = Math.Min(Source.Length, edited.Length); + var prefix = CommonPrefix(Source, edited, limit); + var suffix = CommonSuffix(Source, edited, limit - prefix); + var end = Source.Length - suffix; + var by = LineBreaks(edited, prefix, edited.Length - suffix) - LineBreaks(Source, prefix, end); + return (LowerBound(LineStarts, end) + 1, by); + } + + static int LineBreaks(string text, int start, int end) + { + var count = 0; + foreach (var ch in text.AsSpan(start, end - start)) + { + if (ch == '\n') + { + count++; + } + } + + return count; + } + /// /// Where lexing starts again for an edit that begins at : the start /// of its line, or of an earlier one where that start is inside a comment or a literal, or diff --git a/src/DiffEngineTray.Tests/OwnedInlineHostTest.cs b/src/DiffEngineTray.Tests/OwnedInlineHostTest.cs index 57a7ca1ba..5657b851f 100644 --- a/src/DiffEngineTray.Tests/OwnedInlineHostTest.cs +++ b/src/DiffEngineTray.Tests/OwnedInlineHostTest.cs @@ -871,6 +871,43 @@ public async Task EachSnapshotOfAFileHasItsOwnOutcome() await Assert.That(left.Key).IsEqualTo(InlineKey.For(@"c:\repo\SampleTests.cs", 9)); } + /// + /// An accept moves the call sites under it, and what is still pending for the file is taken + /// to the lines they are on now: by one accept, and by a batch once the outcomes of the file + /// are in. Another file's entry stays where it is. + /// + [Test] + public async Task WhatIsLeftOfAFileGoesToWhereItsCallSitesAre() + { + // The literal accepted at line 7 is four lines longer + using var owner = new Owner( + _ => _.LineHint switch + { + 7 => InlineApplyResult.AppliedMoving(8, 4), + 9 => InlineApplyResult.Failed("the file is held"), + _ => InlineApplyResult.Applied + }); + owner.Queue(@"c:\repo\SampleTests.cs", 7); + owner.Queue(@"c:\repo\SampleTests.cs", 9); + owner.Queue(@"c:\repo\OtherTests.cs", 11); + + var accepted = owner.Host.Accept(owner.Host.List()[0], out _); + + await Assert.That(accepted).IsEqualTo(AcceptOutcome.Applied); + await Assert.That(owner.Send(new(ViewerVerb.ListFull)).Items.Select(_ => _.Key)) + .IsEquivalentTo([InlineKey.For(@"c:\repo\SampleTests.cs", 13), InlineKey.For(@"c:\repo\OtherTests.cs", 11)]); + + owner.Queue(@"c:\repo\SampleTests.cs", 7); + owner.Queue(@"c:\repo\SampleTests.cs", 9); + var response = owner.Send(new(ViewerVerb.AcceptAll), TimeSpan.FromSeconds(30)); + + // The one at 13 is applied, and the one at 9 is not and is four lines down + await Assert.That(response.Message).IsEqualTo("Accepted 3, 1 failed. the file is held"); + var left = owner.Send(new(ViewerVerb.ListFull)).Items.Single(); + await Assert.That(left.Key).IsEqualTo(InlineKey.For(@"c:\repo\SampleTests.cs", 13)); + await Assert.That(left.Status).IsEqualTo("the file is held"); + } + /// /// With the applier a tray really has, several snapshots land in one real file. /// diff --git a/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs b/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs index 3a72e6de5..3c74511a7 100644 --- a/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs +++ b/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs @@ -547,6 +547,29 @@ public async Task AcceptingASnapshotThatIsAlreadyGoneSaysNothing() await Assert.That(pair.Listing.Select(_ => _.Key)).IsEquivalentTo([Key(other, 7)]); } + /// + /// An owner takes what is pending for a file to the lines an accept moved its call sites to, + /// and both surfaces show them there. After a snapshot that got shorter, the one under it + /// can come up onto the very line the accepted one was on: the tray, which reads whether an + /// accept failed off the entry still being under its key, must not take that one for it. + /// + [Test] + public async Task AnEntryThatMovesOntoTheAcceptedLineIsNotAFailedAccept() + { + await using var pair = new ViewerOwned(_ => ViewerSideApplyResult.AppliedMoving(6, -2)); + var snapshot = pair.Snapshot(sample, 5); + pair.Queue(sample, 7, "under it"); + pair.Queue(other, 7); + + await pair.Tracker.Accept(snapshot); + + await Assert.That(pair.Failures).IsEmpty(); + await Assert.That(pair.Applied.Single().LineHint).IsEqualTo(5); + await Assert.That(pair.Viewer.Keys()).IsEquivalentTo([Key(sample, 5), Key(other, 7)]); + await Assert.That(pair.Viewer.Queue.Single(_ => _.Key == Key(sample, 5)).LeftText).IsEqualTo("under it"); + await Assert.That(pair.Listing.Select(_ => _.Key)).IsEquivalentTo([Key(sample, 5), Key(other, 7)]); + } + /// /// A failed apply keeps its entry so it can be retried, and both surfaces have to say the same /// thing about it — the tray in a balloon, the viewer in the entry's status. diff --git a/src/DiffEngineTray/OwnedInlineHost.cs b/src/DiffEngineTray/OwnedInlineHost.cs index 68726e27c..16c15c607 100644 --- a/src/DiffEngineTray/OwnedInlineHost.cs +++ b/src/DiffEngineTray/OwnedInlineHost.cs @@ -574,6 +574,9 @@ void IQueueOwner.Window(WindowCommand command, string? key) lock (gate) { queue = queue.Accept(entry, result, out message); + // Whether or not the completion was taken: the edit is in the file either way, and + // what is pending for that file was recorded against the lines as they were + queue = queue.Rebased(patch!.SourceFile, [result]); } if (message is null) @@ -699,6 +702,10 @@ string AcceptEvery(int files, out bool refused) queue = queue.AcceptInBatch(claimed[index], results[index], ref tally); progress = progress?.Advance(); } + + // After the last of them, since each is found by its variants and an entry + // taken to another line has others + queue = queue.Rebased(claimed[0].Patch.SourceFile, results); } Changed?.Invoke(); diff --git a/src/DiffEngineTray/RemoteInlineHost.cs b/src/DiffEngineTray/RemoteInlineHost.cs index 70ab24593..73dc4b2b3 100644 --- a/src/DiffEngineTray/RemoteInlineHost.cs +++ b/src/DiffEngineTray/RemoteInlineHost.cs @@ -125,7 +125,11 @@ public AcceptOutcome Accept(PendingSnapshot snapshot, out string? message) return AcceptOutcome.Unknown; } - return pending.Any(_ => _.Key == snapshot.Key) + // Still there and saying why. An entry under the key with nothing to say is another one: + // an accept moves the lines under it, the owner takes what is pending for the file along + // (InlineQueue.Rebased), and after a snapshot that got shorter one of those can come up + // onto the line this one was on + return pending.Any(_ => _.Key == snapshot.Key && _.Status is not null) ? AcceptOutcome.Failed : AcceptOutcome.Applied; } diff --git a/src/DiffEngineViewer.Tests/RebasedEntriesTests.cs b/src/DiffEngineViewer.Tests/RebasedEntriesTests.cs new file mode 100644 index 000000000..a61611940 --- /dev/null +++ b/src/DiffEngineViewer.Tests/RebasedEntriesTests.cs @@ -0,0 +1,141 @@ +/// +/// Accepting a snapshot moves every call site under it in its file, and the entries still pending +/// for that file go with them (). The list is edited where it +/// stands: an entry at another line is the same texts under another key. +/// +public class RebasedEntriesTests +{ + /// + /// The literal accepted at line 42 is five lines longer, so everything under it is five + /// lines down. + /// + static InlineApplyResult Grew => + InlineApplyResult.AppliedMoving(43, 5); + + static SessionState ThreeInOneFile() => + Fixtures.Inline( + Fixtures.Patch("SampleTests.cs", 42), + Fixtures.Patch("SampleTests.cs", 88, "\"one\"", "two"), + Fixtures.Patch("OtherTests.cs", 88, "\"one\"", "two"), + Fixtures.Patch("SampleTests.cs", 90, "\"three\"", "four")); + + [Test] + public async Task AnAcceptTakesTheFilesOtherEntriesToWhereTheirCallSitesAre() + { + var state = ThreeInOneFile(); + var before = state.Queue; + + var after = ViewerSession.Apply(state, CommandKind.Accept, Fixtures.Applying(Grew)); + + await Assert.That(after.Queue.Select(_ => _.Name)).IsEquivalentTo(["SampleTests.cs:93", "OtherTests.cs:88", "SampleTests.cs:95"]); + var moved = after.Queue.Single(_ => _.Name == "SampleTests.cs:93"); + await Assert.That(moved.Key).IsEqualTo(InlineKey.For("SampleTests.cs", 93)); + await Assert.That(moved.Patch!.LineHint).IsEqualTo(93); + await Assert.That(moved.Variants.Single().Patch).IsSameReferenceAs(moved.Patch); + // The same diff, not one made again + await Assert.That(moved.LeftRows).IsSameReferenceAs(before.Single(_ => _.Name == "SampleTests.cs:88").LeftRows); + // And another file's entry is the entry it was + await Assert.That(after.Queue.Single(_ => _.Name == "OtherTests.cs:88")) + .IsSameReferenceAs(before.Single(_ => _.Name == "OtherTests.cs:88")); + } + + /// + /// The owner is asked to accept by key, from the tray or an attached window, while the + /// reader is on another entry of the same file. That entry is still the one being read. + /// + [Test] + public async Task TheEntryBeingReadIsStillSelectedAfterItMoves() + { + var actions = Fixtures.Applied with + { + ApplyInlineTogether = _ => _.Select(patch => patch.LineHint == 42 ? Grew : InlineApplyResult.Failed("held")).ToList() + }; + var state = ViewerSession.SelectKey(ThreeInOneFile(), InlineKey.For("SampleTests.cs", 90)); + + var after = ViewerSession.Apply(state, CommandKind.AcceptAll, actions); + + await Assert.That(after.Current!.Name).IsEqualTo("SampleTests.cs:95"); + await Assert.That(after.Current.Status).IsEqualTo("held"); + } + + /// + /// A batch finds what it claimed by its variants, so the entries are taken along once every + /// outcome of the file is in. What is left of the file is a snapshot that was not written and + /// a conflict the batch passed over, and both are where their call sites now are. + /// + [Test] + public async Task ABatchTakesWhatItLeftOfAFileAlong() + { + var state = ViewerSession.EnqueueInline( + Fixtures.Inline( + Fixtures.Patch("SampleTests.cs", 42, framework: "net8.0"), + Fixtures.Patch("SampleTests.cs", 88, "\"one\"", "two", framework: "net8.0"), + Fixtures.Patch("SampleTests.cs", 90, "\"three\"", "four", framework: "net8.0"), + Fixtures.Patch("SampleTests.cs", 99, "\"five\"", "six", framework: "net8.0")), + Fixtures.Patch("SampleTests.cs", 90, "\"three\"", "nine", framework: "net9.0")); + var actions = Fixtures.Applied with + { + ApplyInlineTogether = _ => _ + .Select(patch => patch.LineHint switch + { + 42 => Grew, + 88 => InlineApplyResult.NotFound("no Verify call"), + _ => InlineApplyResult.Applied + }) + .ToList() + }; + + var after = ViewerSession.Apply(state, CommandKind.AcceptAll, actions); + + await Assert.That(after.Queue.Select(_ => _.Name)).IsEquivalentTo(["SampleTests.cs:93", "SampleTests.cs:95"]); + await Assert.That(after.Queue.Single(_ => _.Name == "SampleTests.cs:93").Status!).Contains("no Verify call"); + var conflict = after.Queue.Single(_ => _.Name == "SampleTests.cs:95"); + await Assert.That(conflict.Conflicted).IsTrue(); + await Assert.That(conflict.Variants.Select(_ => _.Patch.LineHint)).IsEquivalentTo([95, 95]); + await Assert.That(after.Message!).StartsWith("Accepted 2, 1 not written, 1 conflict needs review"); + } + + /// + /// A group's batch counts the conflicts among its own members when it ends, and knows its + /// members by key. One that moved is still a member. + /// + [Test] + public async Task AGroupStillCountsAConflictThatMoved() + { + var state = ViewerSession.EnqueueInline( + Fixtures.Inline( + Fixtures.Patch("SampleTests.cs", 42, testName: "First", framework: "net8.0"), + Fixtures.Patch("SampleTests.cs", 90, "\"three\"", "four", testName: "First", framework: "net8.0"), + Fixtures.Patch("OtherTests.cs", 12, "\"five\"", "six", testName: "Second", framework: "net8.0")), + Fixtures.Patch("SampleTests.cs", 90, "\"three\"", "nine", testName: "First", framework: "net9.0")); + var rows = QueueProjection.Visible(state, ScreenBuilder.BodyRows(state), out _).ToList(); + var open = ViewerSession.OpenMenu(state, rows.FindIndex(_ => _.GroupName == "First")); + + var after = ViewerSession.BeginAcceptGroup(open); + while ((after = ViewerSession.ClaimNext(after)).Batch?.Current is not null) + { + after = ViewerSession.ApplyClaimed(after, Fixtures.Applying(Grew))(after); + } + + await Assert.That(after.Queue.Select(_ => _.Name)).IsEquivalentTo(["SampleTests.cs:95", "OtherTests.cs:12"]); + await Assert.That(after.Message).IsEqualTo("Accepted 1, 1 conflict needs review"); + } + + /// + /// An edit that moved no line changes nothing about the entries beside it, which stay the + /// instances they were. + /// + [Test] + public async Task AnAcceptThatMovedNothingLeavesTheOtherEntriesAlone() + { + var state = ThreeInOneFile(); + + var after = ViewerSession.Apply(state, CommandKind.Accept, Fixtures.Applied); + + await Assert.That(after.Queue.Count).IsEqualTo(3); + foreach (var entry in after.Queue) + { + await Assert.That(state.Queue.Any(_ => ReferenceEquals(_, entry))).IsTrue(); + } + } +} diff --git a/src/DiffEngineViewer/ViewerSession.cs b/src/DiffEngineViewer/ViewerSession.cs index cb349e2e5..d0933df2b 100644 --- a/src/DiffEngineViewer/ViewerSession.cs +++ b/src/DiffEngineViewer/ViewerSession.cs @@ -763,22 +763,32 @@ static SessionState AcceptInline(SessionState state, ViewerActions actions) var pending = Pending(state); InlineQueue accepted; string? message; + // What the applier answered, for the lines it moved + InlineApplyResult? applied = null; + InlineApplyResult Apply(InlinePatch patch) => + applied = actions.ApplyInline(patch); + if (current.Conflicted && current.Variants[current.SelectedVariant].Origins is { Count: > 0 } origins && origins[0] is { } origin) { // The reviewer picked a side by cycling to it; accepting applies exactly what is on // screen and resolves the whole call site. - accepted = pending.Accept(current.Key, origin, actions.ApplyInline, out message); + accepted = pending.Accept(current.Key, origin, Apply, out message); } else { - accepted = pending.Accept(current.Key, actions.ApplyInline, out message); + accepted = pending.Accept(current.Key, Apply, out message); } var queue = Rebuild(state, pending, accepted); if (accepted.Count < pending.Count) { + if (applied is not null) + { + queue = Rebased(queue, current.Patch!.SourceFile, [applied]).Queue; + } + return Remove(state, queue, message); } @@ -1414,6 +1424,29 @@ static SessionState RecordInline(SessionState state, IReadOnlyList e } } + // Once every outcome is in, since a claimed entry is found by its variants and one that + // has moved has others. What is left of the file - a conflict, a snapshot that was not + // written, one that arrived since - goes to where its call site now is + var (rebased, moved) = Rebased(queue, entries[0].Patch!.SourceFile, results); + if (moved is not null) + { + // The batch names its entries by key: what it has still to do, where a conflict is + // passed over when its turn comes and something else of this file may now stand + // under the key it had, and a group's members, which is how its conflicts are + // counted at the end + batch = batch with + { + Remaining = batch + .Remaining + .Select(_ => moved.GetValueOrDefault(_, _)) + .ToList(), + Only = batch + .Only? + .Select(_ => moved.GetValueOrDefault(_, _)) + .ToHashSet() + }; + } + return Remove( state with { @@ -1424,8 +1457,77 @@ state with Together = [] } }, - queue, - state.Message); + rebased, + state.Message, + moved); + } + + /// + /// The list with the snapshots of taken to the lines their + /// call sites are on after the edits report, and the keys that + /// changed, old to new, or null when none did. See , which + /// decides it. + /// + /// Done to the list where it stands and not by rebuilding it from the queue: an entry at + /// another line is the same texts and the same diff under another key, and a list ordered + /// by solution and test is still in order. Rebuilt, each of them was diffed again, a file's + /// worth for every accept in it. + /// + /// + static (IReadOnlyList Queue, Dictionary? Moved) Rebased( + IReadOnlyList queue, + string sourceFile, + IReadOnlyList results) + { + if (!results.Any(_ => _.MovedBy != 0)) + { + return (queue, null); + } + + var indexes = new List(); + for (var index = 0; index < queue.Count; index++) + { + if (queue[index] is { Kind: QueueEntryKind.Inline, Patch: { } patch } && + InlineKey.SamePath(patch.SourceFile, sourceFile)) + { + indexes.Add(index); + } + } + + if (indexes.Count == 0) + { + return (queue, null); + } + + var before = InlineQueue.From(indexes.Select(_ => new PendingInline(queue[_].Variants, queue[_].Status))); + var after = before.Rebased(sourceFile, results); + if (ReferenceEquals(after, before)) + { + return (queue, null); + } + + var list = queue.ToList(); + var moved = new Dictionary(); + for (var position = 0; position < indexes.Count; position++) + { + var pending = after.Items[position]; + if (ReferenceEquals(pending, before.Items[position])) + { + continue; + } + + var entry = list[indexes[position]]; + moved[entry.Key] = pending.Key; + list[indexes[position]] = entry with + { + Key = pending.Key, + Name = pending.Name, + Patch = pending.Variants[entry.SelectedVariant].Patch, + Variants = pending.Variants + }; + } + + return (list, moved); } /// @@ -2097,9 +2199,16 @@ static int IndexOfMoved(IReadOnlyList queue, QueueEntry? current) /// an entry that is itself gone falls back to advancing by index. /// /// - static SessionState Remove(SessionState state, IReadOnlyList queue, string? message) + static SessionState Remove(SessionState state, IReadOnlyList queue, string? message, Dictionary? moved = null) { var key = state.Current?.Key; + // An entry taken to another line (Rebased) is still the entry being read + if (key is not null && + moved is not null) + { + key = moved.GetValueOrDefault(key, key); + } + var selected = key is null ? -1 : IndexOf(queue, key); var next = state with { From abe1b8780d0c101b3fbf30828eb3e24b969cc7a6 Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sun, 4 Oct 2026 13:51:46 +1100 Subject: [PATCH 7/7] Close what this round fixed in todo.md, and describe what changed The entries found by line and the Append that needed a run a member are replaced by what rebasing leaves: edits nothing reports, a run under way during an accept, and a key changing under whoever shows the queue. The test hygiene items go, with what each fix left. claude.md gains the rebasing and the three ways a test now keeps off the desktop. --- claude.md | 17 ++++++++++++++++- todo.md | 16 +++++++++------- 2 files changed, 25 insertions(+), 8 deletions(-) diff --git a/claude.md b/claude.md index 57969e43f..9c0b12338 100644 --- a/claude.md +++ b/claude.md @@ -122,6 +122,21 @@ is at now, rather than queueing a second one beside it. `InlineStaging` asks the trio, and scopes a settle to the running framework (`InlineStaging.Settle`), which `InlinePatchFile.Write` labels a staged patch with. +Those questions are for an edit nothing reports. An accept the queue's owner makes is reported: an +applied patch says which lines it moved (`InlineApplyResult.MovedFrom` and `MovedBy`, read off the +source before and after by `SourceScan.LinesMoved`), and the owner takes what is still pending for +that file to the lines its call sites are on now (`InlineQueue.Rebased`), so the key stays right +and the run after an accept finds its entries by it. It is a step of its own, after the outcomes +of a file are recorded and never part of recording one, because a batch finds what it claimed by +its variants and an entry at another line has others. The viewer does it to its list where it +stands, an entry at another line being the same texts and diff under another key, and brings +along the keys its batch holds (`AcceptBatch.Remaining` and `Only`) and the entry being read. +Inside a batch the applier does the same for itself: each patch is asked about the line the edits +before it moved its call site to, since an Append has nothing but its line. A key therefore +changes under whoever is showing the queue, which is why `RemoteInlineHost` takes an accept to +have failed only where the entry under its key says why. An accept applied by another process, +the IDE plugin among them, moves the lines and tells nobody which. + What the patcher writes has to compile where it lands, and three shapes did not. F# measures a continuation from the column the call's expression starts at, not from the line's indentation, whenever something precedes the call on its line (`do!`, `let! x =`): the offside rule @@ -729,7 +744,7 @@ the comment there about not caching "nothing staged" asks for. - `ViewerClient` remembers a port found unowned for ten minutes (`RecheckUnownedAfter`), and the library's telling sends - settle, retire, move, delete, the first inline or diff send - skip the connect while that stands. A refused loopback connection costs two seconds on Windows (firewall stealth mode drops the reset), and a green run settles once per inline verification, which was six minutes for a class of 188 inline tests. Probes (`IsOwned`), the hosts and `InlineQueueClient` always ask and correct the memory; so does `SettleAppliedInline`, being one send per accept. Asking, on Windows, is the operating system's listener table first (`ListenerTable`, shared with `PiperClient`): no listener on the port means nobody to connect to, said without the two seconds, and a listener or a table that cannot be read leaves the connect to answer. So the first telling send of a test process, the launch gate's probe and each of its polls no longer wait to be refused. By port alone, whichever address, since the table is only believed when it says nobody is there. Not for a port that accepted a connection in the last second (`TrustOwnerFor`), because reading the table is reading every connection the machine has, and a run of settles to a live owner would pay more for each than the connect costs. The table is Windows' listeners alone, by `GetExtendedTcpTable` (`ListenerTable.Listeners`), falling back to .NET's list of every connection where that call is not there: 0.32 ms beside 3,000 connections where it was 13. What the table found empty is asked about again after a second (`RecheckUnlistedAfter`), so a tray started after a test process is found; the ten minutes is only for what a connect found. And only a refusal or an unanswered connect is remembered as nobody being there (`NobodyThere`). A viewer launch that failed gives its `MaxInstance` slot back - `TrayDisabledChecker` respects `DiffEngine_TrayDisabled` env var, behind `DiffRunner.TrayDisabled`. Separate from `Disabled` because tracking a pending move is separate from launching a tool: every exit of `InnerLaunch`, `Disabled` included, still calls `AddMove`. `PendingFiles.TrayAvailable` is the single gate - Tests use TUnit and Verify for snapshot testing -- The three WinForms test and benchmark hosts set `UnhandledExceptionMode.ThrowException` from a module initializer with `threadScope: false`. The one argument overload covers only the thread that calls it, which leaves every test thread showing WinForms' dialog on the desktop of whoever is running the tests, and waiting for a click. Never run a test that throws inside a window message in a build without it. `DiffEngineTray.Tests` also declines the tray's "open an issue" box (`IssueLauncher.Declined`) and records what was asked in `ModuleInitializer.IssuesAsked`. A form shown the ordinary way is the foreground window even parked off screen, and takes the keyboard of whoever is at the machine, so a test that posts input shows its form without activating it (`ViewerForm.Parked`, `ParkedForm`) and says what modifiers its thread holds (`ThreadKeys`) +- The three WinForms test and benchmark hosts set `UnhandledExceptionMode.ThrowException` from a module initializer with `threadScope: false`. The one argument overload covers only the thread that calls it, which leaves every test thread showing WinForms' dialog on the desktop of whoever is running the tests, and waiting for a click. Never run a test that throws inside a window message in a build without it. `DiffEngineTray.Tests` also declines the tray's "open an issue" box (`IssueLauncher.Declined`) and records what was asked in `ModuleInitializer.IssuesAsked`. A form shown the ordinary way is the foreground window even parked off screen, and takes the keyboard of whoever is at the machine, so a test that posts input shows its form without activating it (`ViewerForm.Parked`, `ParkedForm`) and says what modifiers its thread holds (`ThreadKeys`). A test whose subject is activation, or a maximised or minimised window, cannot be parked: WinForms changes window state by calls that activate, and `Raise` ends in `Activate`. Those run their windows inside `UnseenDesktop.Run`, on a desktop of the process's own that is neither drawn nor given input, and on a thread of its own, since a test's STA thread could not be moved to it. A tray test that needs a menu's `Opening` shows it with `TopLevel` false (`MenuBuilderTest.Open`), as Verify.WinForms does, and a drop down is a second window that needs the same. No test registers a hot key with Windows or touches the machine's tray version marker: `KeyRegister` is handed a stand-in desktop and `TrayVersionFile` has internal overloads that take the path - CI's runners have two to four cores. A test must not block pool threads while something in its own process needs the pool to answer it, nor assert that something happened inside a fixed short wait: `DOTNET_PROCESSOR_COUNT=2` shows both locally - `MachineSettings.Ignore` clears `DiffEngine_Disabled`, so a snapshot test that is expected to fail is run with a build server variable such as `TEAMCITY_VERSION` set, or Verify tells the real tray about it - `ps` is asked with `-ww`: procps lets an exported `COLUMNS` cut every command line `ProcessCleanup` reads diff --git a/todo.md b/todo.md index 1c95b9ff1..dfb54beee 100644 --- a/todo.md +++ b/todo.md @@ -28,8 +28,11 @@ An item with no tag was said by whoever made the fix it follows from. A tag says - [ ] Verify has to change for the staged trios of a multi-targeted project to be cleared per framework: in `InlineEngine.Settle()`, call `InlineStaging.Settle(MappedSourceFile, inline.Line, inline.MemberName, VerifierSettings.IntermediateDir, SnapshotInSource)` in place of `ClearStaged(...)`. DiffEngine's half is in: the trios are labelled, and `Settle` clears only the running framework's. - [ ] A `Remove` with a stale hint can answer AlreadyApplied while its anchored call is still there. The source cannot tell that from the same Remove applied twice over a sibling with the same literal, so it needs Verify's half, skipping the Remove when the source file is newer than the test assembly, or a Remove that never changes line numbers. An empty line over a Snapshot call is read as removed, which is one more shape of it. (reproduced) -- [ ] A queued entry is still found by its line, with the member asked second. Three cases remain: a test that carries on past a failed verification and has two call sites only the line tells apart folds them into one; an entry the reporting framework has no content in is not moved, so a multi-target run can keep a stale duplicate; and a settle from the same member is believed on a key hit. Rebasing the hints of a file's remaining entries when one is accepted would close all three. It needs the applier to report where each edit began and how many lines it added, `InlineQueue.Accept` and `AcceptInBatch` to move the file's remaining entries, the viewer's batch and the tray's `AcceptEvery` to find a claimed entry by something that survives a move rather than by its `Variants` reference, and an attached viewer and `RemoteInlineHost` to cope with a key changing under a selection. -- [ ] An `Append` with a stale hint and more than one call in its member with no Snapshot call is refused and has to be run again. A file with several such members takes a run each under accept-all, because each accept above makes the next hint stale. Rebasing hints would end it. +- [ ] An owner now takes a file's pending entries to the lines an accept moved their call sites to (`InlineQueue.Rebased`), and a batch asks each patch about its moved line. An edit nothing reports still leaves the lines wrong until a run reports them again: one made by hand, and an accept applied by another process, the IDE plugin among them, whose `SettleAppliedInline` carries no lines. For those a queued entry is still found by its line with the member asked second, and the three cases stand: two call sites only the line tells apart are folded into one, an entry the reporting framework has no content in is not moved, and a settle from the same member is believed on a key hit. The wire would have to carry which lines moved. +- [ ] A run that was under way when a snapshot was accepted reports its later call sites by the lines they had. Its patch is then looked for under a key the owner has moved on from, and where another entry of the same member has been taken to that very line, it is folded into that one. Before, the stale lines were the queue's and it was the run after the accept that missed. (reasoned, not run) +- [ ] Which lines an edit moved is read off the source before and after, by what the two begin and end with alike. An edit that inserts text beginning as the text after it does, across a line break, is counted from a line too far down. Nothing the patcher writes was found to do that. +- [ ] A menu or a window showing an owner's queue names an entry by a key that the owner changes when it accepts another snapshot of the file. Until the next listing, a click there is answered as an entry that has gone, or, after a snapshot that got shorter, can name the entry that came up onto that line. An attached viewer follows its selection across the move by call site (`IndexOfMoved`), which two call sites alike in everything but the line defeat. +- [ ] An `Append` whose line was already wrong when the batch began, with more than one call in its member, is still refused: only the moves the batch itself made are brought along. - [ ] A `Remove` of a statement whose receiver is on a line above the call still brings the lines under it up. - [ ] A batch still copies the whole source for each patch that edits: 1.2 MB a patch in a 600 KB file, 0.29 s for 500. - [ ] A dry run is answered from a kept scan while the file's length and write time stand. A rewrite of the same length that puts the old write time back is not seen, and up to four files' source and maps are held for the life of the process. `ASecondProbeOfAFileDoesNotReadItAgain` relies on a second open being refused, which was not run off Windows. @@ -48,9 +51,8 @@ An item with no tag was said by whoever made the fix it follows from. A tag says - [ ] A tool started through a script is never tracked as a process: VS Code always (`code.cmd`), and Rider when it is found on the PATH as `rider.cmd`. Nor is one whose launcher hands over and exits: Araxis, Sublime Merge, Cursor, Meld and an already running Rider, from knowledge of the tools and not from running them. None can be tracked safely from the tray, since the window lives in a process whose command line does not name the pair. - [ ] "Discard" on a single move still ends its tool on the UI thread, up to 500 ms. A move whose received file could not be deleted in a bulk discard is untracked with only a log line. - [ ] A scan that keeps failing still writes a log line every two seconds. -- [ ] `OptionsFormLauncherTests` registers real global hot keys, Ctrl+Alt+Shift+F13 to F17, and fails where something else holds one. `KeyRegister`'s internal constructor is there to give it a stand-in. -- [ ] `TrayVersionFileTest` writes and deletes the real `%TEMP%\DiffEngineTray\version.txt`, so running the suite removes the marker of a tray running on the machine. -- [ ] The `MenuBuilderTest` tests that call `menu.Show(0, 0)` briefly put a context menu at the top left of the desktop. +- [ ] `KeyNameTests` still builds two registers with the constructor that registers with Windows. Both bind only a key name that does not parse, so nothing reaches `RegisterHotKey` today. +- [ ] `MenuBuilderTest` opens its menus with `TopLevel` false, which Windows reports as not visible. That nothing shows was asserted through `IsWindowVisible`, not seen by eye, and the new test was not run against the old `Show(0, 0)`, since failing it puts a menu on the desktop. ## Viewer model @@ -74,10 +76,10 @@ An item with no tag was said by whoever made the fix it follows from. A tag says - [ ] In a capture a footer taller than one row cuts the last rows of a screen built for the whole window, since a capture's screen is built before the footer is laid out. The window is not affected. - [ ] Between half its own size and its own size an enlarged picture is still scaled on every paint: 9 ms for a 4000 by 3000 pair at 400%. A copy there would cost up to the decoded picture again, 96 MB for that pair. - [ ] The picture baselines were not taken again for the premultiplied decode, apart from the two the footer moved. They differ from what is drawn by one level in translucent pixels and pass at the suite's 0.9999. -- [ ] `WindowPlacementTests`, `ViewerFormRaiseTests` and `FrameWaitTests` still show a `ViewerForm` the ordinary way, so it is the foreground window while they run and takes the keyboard of whoever is at the machine. Only the hosts that post input, and captures, are parked. (noticed, not looked into) +- [ ] The tests that raise, maximise or minimise a window run it on a desktop of the test process's own (`UnseenDesktop`). Where one cannot be made they run on the ordinary desktop, as before, and `AWindowShownThereNeverHasTheKeyboard` is skipped: not seen to happen, and CI's runners were not tried before this. Their windows are on an MTA thread, so a test that needs an apartment (the clipboard, drag and drop) cannot use it as it stands. +- [ ] `ImageCacheTests` and `FormsHeadTests.EveryPictureEverDrawnStaysDecoded` write to fixed folders under the temp folder (`deview-image-cache`, `deview-review-cache`), so two runs of the suite at once on one machine fail each other. (seen) - [ ] In a window about 560 wide the left pane's header runs into the right pane's with no gap. It shows in `FooterThatWraps`. (noticed, not looked into) - [ ] The test that an exception comes out of a message pump has only been seen to pass, since failing it is what shows the dialog. -- [ ] `RowText.Shown`'s tests are in `DiffEngineViewer.Windows.Tests`, though it is the core's. They belong beside `CellGridTests`. - [ ] A minimised window slows only its own frame wait. `OwnerLink`, `TrackedWatch` and the document reader go by `Hidden`, which only a Hide command sets. The head would have to tell the loop.