diff --git a/src/DiffEngineViewer.Tests/AttachedViewerTests.cs b/src/DiffEngineViewer.Tests/AttachedViewerTests.cs
index 21006a6b..25e995c6 100644
--- a/src/DiffEngineViewer.Tests/AttachedViewerTests.cs
+++ b/src/DiffEngineViewer.Tests/AttachedViewerTests.cs
@@ -571,4 +571,53 @@ public void Dispose()
{
}
}
+
+ ///
+ /// A tracked file the owner lists that this side cannot open. The listing is the same from one
+ /// pump to the next, so the entry should be too: rebuilt anyway, it replaced the one on screen
+ /// every 200ms and closed the reader's open menu with it.
+ ///
+ [Test]
+ public async Task AnUnreadableTrackedFileLeavesTheMenuOpen()
+ {
+ var file = Path.Combine(Path.GetTempPath(), $"AttachedViewerTests_{Guid.NewGuid():N}.verified.txt");
+ await File.WriteAllTextAsync(file, "locked away");
+ try
+ {
+ using var holder = new FileStream(file, FileMode.Open, FileAccess.Read, FileShare.None);
+ if (!ViewerServer.TryBind(0, out var server))
+ {
+ throw new("Could not bind an ephemeral port.");
+ }
+
+ using (server)
+ using (var cancel = new CancelSource())
+ {
+ _ = server.Listen(
+ _ => ViewerResponse.Listing(
+ [],
+ deletes: [new(TrackedKeys.ForDelete(file), "Extra.verified.txt", null, file)]),
+ cancel.Token);
+ var host = new SessionHost(SessionState.Start(ViewerMode.Inline, Fixtures.Columns, Fixtures.Rows));
+ var link = new OwnerLink(host, server.Port);
+
+ await Assert.That(link.Pump()).IsTrue();
+ var first = host.State.Queue.Single();
+ await Assert.That(first.Warning).Contains("Could not read");
+ var row = QueueProjection.Rows(host.State).ToList().FindIndex(_ => _.Kind == QueueRowKind.Entry);
+ host.Mutate(_ => ViewerSession.OpenMenu(_, row));
+ await Assert.That(host.State.Menu).IsNotNull();
+
+ link.Pump();
+
+ await Assert.That(host.State.Menu).IsNotNull();
+ await Assert.That(host.State.Queue.Single()).IsSameReferenceAs(first);
+ await cancel.CancelAsync();
+ }
+ }
+ finally
+ {
+ File.Delete(file);
+ }
+ }
}
diff --git a/src/DiffEngineViewer.Tests/CollapseTests.cs b/src/DiffEngineViewer.Tests/CollapseTests.cs
index 34793cb3..ecd628b1 100644
--- a/src/DiffEngineViewer.Tests/CollapseTests.cs
+++ b/src/DiffEngineViewer.Tests/CollapseTests.cs
@@ -178,4 +178,46 @@ static List Labels(SessionState state) =>
QueueProjection.Rows(state)
.Select(_ => _.Label)
.ToList();
+
+ ///
+ /// The entry being read is accepted, and the queue closes up: the index it leaves behind names
+ /// the first entry of the folded group after it. Nothing in the list would be highlighted while
+ /// the panes and Accept went on acting on an entry nobody could see.
+ ///
+ [Test]
+ public async Task Accepting_the_entry_being_read_does_not_select_one_under_a_fold()
+ {
+ var state = ViewerSession.ToggleGroup(Fixtures.Inline(solutionA1, solutionA2, solutionB1, solutionB2), solutionB);
+ state = ViewerSession.SelectKey(state, KeyOf(solutionA2));
+ await Assert.That(QueueProjection.VisibleEntries(state)).Contains(state.Selected);
+
+ var accepted = ViewerSession.Apply(state, CommandKind.Accept, Fixtures.Applied);
+
+ await Assert.That(QueueProjection.VisibleEntries(accepted)).Contains(accepted.Selected);
+ }
+
+ ///
+ /// The same for a window attached to someone else's queue, whose listing no longer has the
+ /// entry being read - accepted from the tray menu, say.
+ ///
+ [Test]
+ public async Task A_listing_without_the_entry_being_read_does_not_select_one_under_a_fold()
+ {
+ var state = ViewerSession.ToggleGroup(Fixtures.Attached(Fixtures.Pending(solutionA1, solutionA2, solutionB1, solutionB2)), solutionB);
+ state = ViewerSession.SelectKey(state, KeyOf(solutionA2));
+ await Assert.That(QueueProjection.VisibleEntries(state)).Contains(state.Selected);
+
+ var synced = ViewerSession.Sync(state, Fixtures.Pending(solutionA1, solutionB1, solutionB2), [], null);
+
+ await Assert.That(QueueProjection.VisibleEntries(synced)).Contains(synced.Selected);
+ }
+
+ const string solutionB = "solution|SolutionB";
+ static readonly InlinePatch solutionA1 = Fixtures.Patch(Fixtures.SolutionFile("SolutionA", "Tests", "ATests.cs"), 10);
+ static readonly InlinePatch solutionA2 = Fixtures.Patch(Fixtures.SolutionFile("SolutionA", "Tests", "ATests.cs"), 30);
+ static readonly InlinePatch solutionB1 = Fixtures.Patch(Fixtures.SolutionFile("SolutionB", "Tests", "BTests.cs"), 10);
+ static readonly InlinePatch solutionB2 = Fixtures.Patch(Fixtures.SolutionFile("SolutionB", "Tests", "BTests.cs"), 30);
+
+ static string KeyOf(InlinePatch patch) =>
+ QueueEntry.KeyForInline(patch.SourceFile, patch.LineHint);
}
diff --git a/src/DiffEngineViewer.Tests/EntryMenuTests.cs b/src/DiffEngineViewer.Tests/EntryMenuTests.cs
new file mode 100644
index 00000000..58d3a077
--- /dev/null
+++ b/src/DiffEngineViewer.Tests/EntryMenuTests.cs
@@ -0,0 +1,175 @@
+///
+/// An entry's context menu acts on the entry selected when one of its items is clicked, so the
+/// selection moving while the menu is open retargets it: Discard, clicked in a menu opened on one
+/// entry, discarded another. Nothing the reader did has to happen for that - a focus from the tray
+/// or an IDE, or a test re-sending a snapshot, moves the selection.
+///
+public class EntryMenuTests
+{
+ ///
+ /// A menu opened on A, then a wire Focus naming B - the tray menu or an IDE asking for it -
+ /// through the real handler, then Discard.
+ ///
+ [Test]
+ public async Task A_wire_focus_does_not_retarget_an_open_entry_menu()
+ {
+ var a = Fixtures.Patch("A.cs", 1);
+ var b = Fixtures.Patch("B.cs", 2);
+ var host = new SessionHost(Fixtures.Inline(a, b));
+ var handler = new MessageHandler(host, Fixtures.Applied, _ =>
+ {
+ });
+ host.Mutate(_ => ViewerSession.OpenMenu(_, VisibleRowOf(_, _ => _.EntryIndex == 0)));
+ await Assert.That(host.State.Current!.Key).IsEqualTo(Key(a));
+ var discard = MenuIndex(host.State, "Discard");
+
+ var focus = handler.Handle(new(ViewerVerb.Focus, Key(b)));
+ await Assert.That(focus.Ok).IsTrue();
+
+ host.Mutate(_ => ViewerProgram.Apply(_, Click(discard), link: null, new NoWindow()));
+
+ await Assert.That(host.State.Queue.Select(_ => _.Key)).Contains(Key(b));
+ }
+
+ ///
+ /// Attached, with nothing but a test process involved. B's test fails again with the same
+ /// content: a tray owner folds it into the same entry and stashes a focus on it, which its next
+ /// full listing carries. The window syncs - the same entries, so the open menu is kept - and
+ /// then selects B, under the menu opened on A.
+ ///
+ [Test]
+ public async Task A_focus_riding_the_owners_listing_does_not_retarget_an_open_entry_menu()
+ {
+ var a = Fixtures.Patch("A.cs", 1);
+ var b = Fixtures.Patch("B.cs", 2);
+ using var owner = new TrayLikeOwner(Fixtures.Pending(a, b));
+ var host = new SessionHost(SessionState.Start(ViewerMode.Inline, Fixtures.Columns, Fixtures.Rows));
+ var link = new OwnerLink(host, owner.Port);
+ await Assert.That(link.Pump()).IsTrue();
+ host.Mutate(_ => ViewerSession.OpenMenu(_, VisibleRowOf(_, _ => _.EntryIndex == 0)));
+ await Assert.That(host.State.Current!.Key).IsEqualTo(Key(a));
+ var discard = MenuIndex(host.State, "Discard");
+
+ owner.StashFocus(Key(b));
+ await Assert.That(link.Pump()).IsTrue();
+
+ host.Mutate(_ => ViewerProgram.Apply(_, Click(discard), link, new NoWindow()));
+ link.Pump();
+
+ await Assert.That(owner.Discarded).DoesNotContain(Key(b));
+ }
+
+ static string Key(InlinePatch patch) =>
+ QueueEntry.KeyForInline(patch.SourceFile, patch.LineHint);
+
+ static int VisibleRowOf(SessionState state, Func match) =>
+ QueueProjection.Visible(state, ScreenBuilder.BodyRows(state), out _).ToList().FindIndex(_ => match(_));
+
+ static int MenuIndex(SessionState state, string label) =>
+ state.Menu!.Items.ToList().FindIndex(_ => _.Label == label);
+
+ static ViewerInput Click(int menuItem) =>
+ new(CommandKind.None, -1, -1, 0, false, Fixtures.Columns, Fixtures.Rows)
+ {
+ ClickedMenuItem = menuItem
+ };
+
+ sealed class NoWindow : IViewerWindow
+ {
+ public bool Present(Screen screen) =>
+ true;
+
+ public ViewerInput Poll() =>
+ default;
+
+ public void SetHidden(bool hidden)
+ {
+ }
+
+ public void Focus()
+ {
+ }
+
+ public void SetClipboard(string text)
+ {
+ }
+
+ public bool Capture(Screen screen, int width, int height, string pngPath) =>
+ false;
+
+ public void Dispose()
+ {
+ }
+ }
+
+ ///
+ /// A queue owner answering the way OwnedInlineHost does where it matters here: a full listing
+ /// of a fixed queue, carrying a stashed window command once. Discards are recorded rather than
+ /// carried out.
+ ///
+ sealed class TrayLikeOwner : IDisposable
+ {
+ readonly CancelSource cancel = new();
+ readonly ViewerServer server;
+ readonly Task listening;
+ readonly List items;
+ string? focus;
+
+ public TrayLikeOwner(InlineQueue queue)
+ {
+ items = ViewerListing.Items(queue.Items, withPatches: true);
+ if (!ViewerServer.TryBind(0, out var bound))
+ {
+ throw new("Could not bind an ephemeral port.");
+ }
+
+ server = bound;
+ listening = server.Listen(Handle, cancel.Token);
+ }
+
+ public int Port => server.Port;
+
+ public List Discarded { get; } = [];
+
+ public void StashFocus(string key) =>
+ Interlocked.Exchange(ref focus, key);
+
+ ViewerResponse Handle(ViewerMessage message)
+ {
+ if (message.Verb == ViewerVerb.ListFull)
+ {
+ if (Interlocked.Exchange(ref focus, null) is { } key)
+ {
+ return ViewerResponse.Listing(items, WindowCommand.Focus, key);
+ }
+
+ return ViewerResponse.Listing(items);
+ }
+
+ if (message.Verb == ViewerVerb.Discard)
+ {
+ lock (Discarded)
+ {
+ Discarded.Add(message.Key);
+ }
+ }
+
+ return ViewerResponse.Success();
+ }
+
+ public void Dispose()
+ {
+ cancel.Cancel();
+ server.Dispose();
+ try
+ {
+ listening.Wait(TimeSpan.FromSeconds(5));
+ }
+ catch (AggregateException)
+ {
+ }
+
+ cancel.Dispose();
+ }
+ }
+}
diff --git a/src/DiffEngineViewer.Tests/FileSideTests.cs b/src/DiffEngineViewer.Tests/FileSideTests.cs
index 39bbee87..0e8a8354 100644
--- a/src/DiffEngineViewer.Tests/FileSideTests.cs
+++ b/src/DiffEngineViewer.Tests/FileSideTests.cs
@@ -109,4 +109,27 @@ static byte[] Png()
bytes[23] = 600 & 0xFF;
return bytes;
}
+
+ ///
+ /// What a reader met for a bitmap with the one height Math.Abs throws on: FileSide.Read's
+ /// catch-all made the side unreadable, with the exception's message as its warning and no
+ /// stamp, so it was read again on every pass.
+ ///
+ [Test]
+ public async Task ABmpWithTheMinimumHeightIsReadAsABmp()
+ {
+ var bytes = new byte[54];
+ "BM"u8.CopyTo(bytes);
+ BinaryPrimitives.WriteInt32LittleEndian(bytes.AsSpan(2), bytes.Length);
+ BinaryPrimitives.WriteInt32LittleEndian(bytes.AsSpan(14), 40);
+ BinaryPrimitives.WriteInt32LittleEndian(bytes.AsSpan(18), 64);
+ BinaryPrimitives.WriteInt32LittleEndian(bytes.AsSpan(22), int.MinValue);
+ var path = Write("MinimumHeight.received.bmp", bytes);
+
+ var side = FileSide.Read(path);
+
+ await Assert.That(side.Warning).IsNull();
+ await Assert.That(side.Stamp).IsNotNull();
+ await Assert.That(side.Image!.Value.Header!.Value.Format).IsEqualTo(ImageFormat.Bmp);
+ }
}
diff --git a/src/DiffEngineViewer.Tests/HeldDeleteTests.cs b/src/DiffEngineViewer.Tests/HeldDeleteTests.cs
index 01e467e6..ae78a267 100644
--- a/src/DiffEngineViewer.Tests/HeldDeleteTests.cs
+++ b/src/DiffEngineViewer.Tests/HeldDeleteTests.cs
@@ -66,4 +66,62 @@ static SessionState Conflicted() =>
Fixtures.Patch(content: "eight", framework: "net8.0"),
Fixtures.Patch(content: "nine", framework: "net9.0")),
Fixtures.Delete());
+
+ ///
+ /// B was accepted on its own earlier and refused, so it carries a status. "Accept all in
+ /// SolutionA" then writes A's only snapshot, and carries out A's delete: whether a group holds
+ /// its deletes is about what that group's own accepts did, not about statuses other accepts
+ /// left elsewhere in the queue.
+ ///
+ [Test]
+ public async Task An_earlier_failure_in_another_solution_does_not_hold_this_solutions_deletes()
+ {
+ var a = Fixtures.Patch(Fixtures.SolutionFile("SolutionA", "Tests", "ATests.cs"), 10);
+ var b = Fixtures.Patch(Fixtures.SolutionFile("SolutionB", "Tests", "BTests.cs"), 10);
+ var state = ViewerSession.EnqueueTracked(Fixtures.Inline(a, b), Fixtures.Delete(solution: "SolutionA"));
+ state = ViewerSession.SelectKey(state, QueueEntry.KeyForInline(b.SourceFile, b.LineHint));
+ state = ViewerSession.Apply(
+ state,
+ CommandKind.Accept,
+ Fixtures.Applying(InlineApplyResult.Failed("Failed to write: BTests.cs")));
+ await Assert.That(state.Queue.Single(_ => _.Key == QueueEntry.KeyForInline(b.SourceFile, b.LineHint)).Status).IsNotNull();
+
+ var deleted = new List();
+ var actions = Fixtures.Applied with
+ {
+ DeleteFile = _ => deleted.Add(_)
+ };
+ var visible = QueueProjection.Visible(state, ScreenBuilder.BodyRows(state), out _).ToList();
+ state = ViewerSession.OpenMenu(state, visible.FindIndex(_ => _.GroupName == "SolutionA"));
+ await Assert.That(state.Menu!.Items[1].Label).IsEqualTo("Accept all in SolutionA");
+
+ var swept = ViewerSession.Apply(state, CommandKind.AcceptGroup, actions);
+
+ await Assert.That(deleted).IsEquivalentTo(["code/extra.verified.txt"]);
+ await Assert.That(swept.Queue.Where(_ => _.Kind == QueueEntryKind.Delete)).IsEmpty();
+ }
+
+ ///
+ /// And one of the group's own that failed, rather than going stale, holds them: it is still in
+ /// the queue, unwritten.
+ ///
+ [Test]
+ public async Task A_failure_in_this_solution_holds_its_deletes()
+ {
+ var a = Fixtures.Patch(Fixtures.SolutionFile("SolutionA", "Tests", "ATests.cs"), 10);
+ var b = Fixtures.Patch(Fixtures.SolutionFile("SolutionB", "Tests", "BTests.cs"), 10);
+ var state = ViewerSession.EnqueueTracked(Fixtures.Inline(a, b), Fixtures.Delete(solution: "SolutionA"));
+ var deleted = new List();
+ var actions = Fixtures.Applying(InlineApplyResult.Failed("Failed to write: ATests.cs")) with
+ {
+ DeleteFile = _ => deleted.Add(_)
+ };
+ var visible = QueueProjection.Visible(state, ScreenBuilder.BodyRows(state), out _).ToList();
+ state = ViewerSession.OpenMenu(state, visible.FindIndex(_ => _.GroupName == "SolutionA"));
+
+ var swept = ViewerSession.Apply(state, CommandKind.AcceptGroup, actions);
+
+ await Assert.That(deleted).IsEmpty();
+ await Assert.That(swept.Queue.Single(_ => _.Kind == QueueEntryKind.Delete).Status).IsNotNull();
+ }
}
diff --git a/src/DiffEngineViewer.Tests/ImageHeaderTests.cs b/src/DiffEngineViewer.Tests/ImageHeaderTests.cs
index dc4ae5eb..7a5e6915 100644
--- a/src/DiffEngineViewer.Tests/ImageHeaderTests.cs
+++ b/src/DiffEngineViewer.Tests/ImageHeaderTests.cs
@@ -152,4 +152,23 @@ static byte[] Ico(int width, int height)
bytes[7] = (byte) (height == 256 ? 0 : height);
return bytes;
}
+
+ ///
+ /// The one height with no positive counterpart. Math.Abs throws on it, and the side it was read
+ /// for came back unreadable with that exception's message as its warning.
+ ///
+ [Test]
+ public async Task ABmpWithTheMinimumHeightIsStillABmp()
+ {
+ var bytes = new byte[54];
+ "BM"u8.CopyTo(bytes);
+ BinaryPrimitives.WriteInt32LittleEndian(bytes.AsSpan(14), 40);
+ BinaryPrimitives.WriteInt32LittleEndian(bytes.AsSpan(18), 64);
+ BinaryPrimitives.WriteInt32LittleEndian(bytes.AsSpan(22), int.MinValue);
+
+ await Assert.That(ImageHeader.TryRead(bytes, out var header)).IsTrue();
+ await Assert.That(header.Format).IsEqualTo(ImageFormat.Bmp);
+ await Assert.That(header.Width).IsEqualTo(64);
+ await Assert.That(header.Height).IsEqualTo(0);
+ }
}
diff --git a/src/DiffEngineViewer.Tests/ReEnqueueTests.cs b/src/DiffEngineViewer.Tests/ReEnqueueTests.cs
index 4f09894c..345e5b7e 100644
--- a/src/DiffEngineViewer.Tests/ReEnqueueTests.cs
+++ b/src/DiffEngineViewer.Tests/ReEnqueueTests.cs
@@ -47,4 +47,40 @@ static SessionState Scrolled()
static InlinePatch Patch(string content) =>
Fixtures.Patch("A.cs", 1, Fixtures.Literal(Fixtures.Deep(false)), content);
+
+ static SessionState ThreeVariants() =>
+ Fixtures.Inline(
+ Fixtures.Patch(content: "eight", framework: "net8.0"),
+ Fixtures.Patch(content: "nine", framework: "net9.0"),
+ Fixtures.Patch(content: "ten", framework: "net10.0"));
+
+ ///
+ /// net8.0 starts passing, so its variant goes. The reader had cycled to net9.0's, which is
+ /// still there: kept by index, the screen switched to net10.0's with nothing to say so, and
+ /// Accept would have applied that.
+ ///
+ [Test]
+ public async Task A_settle_of_an_earlier_variant_keeps_the_variant_on_screen()
+ {
+ var onNine = ViewerSession.Apply(ThreeVariants(), CommandKind.NextVariant);
+ await Assert.That(onNine.Current!.LeftHeader).IsEqualTo("received (net9.0)");
+
+ var settled = ViewerSession.Settle(onNine, onNine.Current.Key, "net8.0");
+
+ await Assert.That(settled.Current!.LeftHeader).IsEqualTo("received (net9.0)");
+ }
+
+ ///
+ /// net8.0 re-runs and now agrees with net10.0: its variant merges into that one. Nothing
+ /// happened to the net9.0 variant the reader is on.
+ ///
+ [Test]
+ public async Task A_rerun_that_merges_an_earlier_variant_keeps_the_variant_on_screen()
+ {
+ var onNine = ViewerSession.Apply(ThreeVariants(), CommandKind.NextVariant);
+
+ var merged = ViewerSession.EnqueueInline(onNine, Fixtures.Patch(content: "ten", framework: "net8.0"));
+
+ await Assert.That(merged.Current!.LeftHeader).IsEqualTo("received (net9.0)");
+ }
}
diff --git a/src/DiffEngineViewer.Tests/SelectionTests.cs b/src/DiffEngineViewer.Tests/SelectionTests.cs
index 1a9cf3de..ee16566b 100644
--- a/src/DiffEngineViewer.Tests/SelectionTests.cs
+++ b/src/DiffEngineViewer.Tests/SelectionTests.cs
@@ -344,4 +344,83 @@ public void Dispose()
{
}
}
+
+ ///
+ /// A re-run lands on the same key with other text: two lines more at the top, so what the
+ /// reader selected is now two rows down. The selection was made on text that is gone, so it
+ /// goes with it, rather than highlighting and copying rows the reader never selected.
+ ///
+ [Test]
+ public async Task A_rerun_that_replaces_the_text_under_a_selection_ends_the_selection()
+ {
+ var state = Drag(Fixtures.Inline(Fixtures.Patch()), PaneSide.Left, 1, 0, 1, 9);
+ await Assert.That(Copy(state)).IsEqualTo("brown dog");
+
+ var rerun = ViewerSession.EnqueueInline(
+ state,
+ Fixtures.Patch(content: $"added one\nadded two\n{Fixtures.Received}"));
+ await Assert.That(rerun.Current!.Key).IsEqualTo(state.Current!.Key);
+
+ await Assert.That(Copy(rerun)).IsNull();
+ }
+
+ ///
+ /// A status change is not new text, so a selection survives it.
+ ///
+ [Test]
+ public async Task A_status_change_keeps_the_selection()
+ {
+ var state = Drag(Fixtures.Inline(Fixtures.Patch()), PaneSide.Left, 1, 0, 1, 9);
+
+ var failed = ViewerSession.Apply(state, CommandKind.Accept, Fixtures.Applying(InlineApplyResult.Failed("locked")));
+
+ await Assert.That(failed.Current!.Status).IsNotNull();
+ await Assert.That(Copy(failed)).IsEqualTo("brown dog");
+ }
+
+ ///
+ /// A head reports cells, and draws a character outside the basic plane in one: a drag across
+ /// the emoji alone ends at column 1, and copies all of it rather than half.
+ ///
+ [Test]
+ public async Task A_drag_across_one_non_bmp_character_copies_all_of_it()
+ {
+ var state = Drag(Fixtures.File("\U0001F600x", "x"), PaneSide.Left, 0, 0, 0, 1);
+
+ await Assert.That(Copy(state)).IsEqualTo("\U0001F600");
+ }
+
+ ///
+ /// "ab" drawn in cells 1 and 2, after an emoji in cell 0: the copy is what was highlighted.
+ ///
+ [Test]
+ public async Task A_drag_after_a_non_bmp_character_copies_what_was_highlighted()
+ {
+ var state = Drag(Fixtures.File("\U0001F600ab", "x"), PaneSide.Left, 0, 1, 0, 3);
+
+ await Assert.That(Copy(state)).IsEqualTo("ab");
+ await Assert.That(ScreenBuilder.Build(state).Left.Rows[0].Selection).IsEqualTo(new SelectionSpan(1, 2));
+ }
+
+ ///
+ /// Select all ends at the last cell of the last row, not a cell further per wide character.
+ ///
+ [Test]
+ public async Task Select_all_ends_on_the_last_cell()
+ {
+ var state = Key(Files("\U0001F600ab", "x"), CommandKind.SelectAll);
+
+ await Assert.That(state.Selection!.FocusColumn).IsEqualTo(3);
+ await Assert.That(Copy(state)).IsEqualTo("\U0001F600ab");
+ }
+
+ ///
+ /// What ctrl+c puts on the clipboard, or null when it puts nothing there.
+ ///
+ static string? Copy(SessionState state)
+ {
+ var window = new Recorder();
+ ViewerProgram.Apply(state, Input(CommandKind.Copy), link: null, window);
+ return window.Copied;
+ }
}
diff --git a/src/DiffEngineViewer.Tests/TrackedFileTests.cs b/src/DiffEngineViewer.Tests/TrackedFileTests.cs
index fceec7da..e061fc91 100644
--- a/src/DiffEngineViewer.Tests/TrackedFileTests.cs
+++ b/src/DiffEngineViewer.Tests/TrackedFileTests.cs
@@ -140,8 +140,11 @@ public async Task SettlingAPairThatIsNotQueuedChangesNothing()
await Assert.That(settled).IsSameReferenceAs(state);
}
+ static QueueEntry Seen(SessionState state, QueueEntryKind kind) =>
+ state.Queue.Single(_ => _.Kind == kind);
+
///
- /// The keys a watch pass reports gone leave, and the rest of the queue is untouched.
+ /// The entries a watch pass reports gone leave, and the rest of the queue is untouched.
///
[Test]
public async Task RefreshDropsWhatWentAndKeepsWhatDidNot()
@@ -149,19 +152,19 @@ public async Task RefreshDropsWhatWentAndKeepsWhatDidNot()
var state = Owned(Fixtures.Move(), Fixtures.Delete());
state = ViewerSession.EnqueueInline(state, Fixtures.Patch());
- var refreshed = ViewerSession.Refresh(state, [Fixtures.Move().Key], []);
+ var refreshed = ViewerSession.Refresh(state, [Seen(state, QueueEntryKind.Move)], []);
await Assert.That(refreshed.Queue.Select(_ => _.Kind))
.IsEquivalentTo([QueueEntryKind.Inline, QueueEntryKind.Delete]);
}
[Test]
- public async Task RefreshReplacesAnEntryByKey()
+ public async Task RefreshReplacesTheEntryThePassSaw()
{
var state = Owned(Fixtures.Move());
var fresh = Fixtures.Move(left: "rewritten");
- var refreshed = ViewerSession.Refresh(state, [], [fresh]);
+ var refreshed = ViewerSession.Refresh(state, [], [(state.Queue.Single(), fresh)]);
await Assert.That(refreshed.Queue.Single().LeftText).IsEqualTo("rewritten");
}
@@ -176,7 +179,8 @@ public async Task RefreshFindingNothingChangesNothing()
var state = Owned(Fixtures.Move());
await Assert.That(ViewerSession.Refresh(state, [], [])).IsSameReferenceAs(state);
- await Assert.That(ViewerSession.Refresh(state, ["not queued"], [])).IsSameReferenceAs(state);
+ // An entry the pass saw that is no longer queued, which is all a key naming nothing ever was
+ await Assert.That(ViewerSession.Refresh(state, [Fixtures.Delete()], [])).IsSameReferenceAs(state);
}
///
@@ -188,7 +192,7 @@ public async Task RefreshKeepsTheMessage()
{
var state = Owned(Fixtures.Move(), Fixtures.Delete()) with { Message = "Accepted something" };
- var refreshed = ViewerSession.Refresh(state, [Fixtures.Move().Key], []);
+ var refreshed = ViewerSession.Refresh(state, [Seen(state, QueueEntryKind.Move)], []);
await Assert.That(refreshed.Message).IsEqualTo("Accepted something");
}
diff --git a/src/DiffEngineViewer.Tests/TrackedWatchTests.cs b/src/DiffEngineViewer.Tests/TrackedWatchTests.cs
index df814ad6..526d8e6b 100644
--- a/src/DiffEngineViewer.Tests/TrackedWatchTests.cs
+++ b/src/DiffEngineViewer.Tests/TrackedWatchTests.cs
@@ -140,4 +140,93 @@ public TrackedWatchTests() =>
public void Dispose() =>
Directory.Delete(directory, true);
+
+ ///
+ /// The real pass, parked between its stat and its apply by holding the host's lock: the stat
+ /// runs outside the lock and the apply inside it. A re-run clears its old received file, the
+ /// pass finds it gone, and before that is applied the re-run writes the new one and the pair
+ /// arrives again under the same key. Dropped by key, the new pair went with the old.
+ ///
+ [Test]
+ public async Task APassDoesNotDropAPairReStagedAfterItsStat()
+ {
+ var (temp, target) = Pair("Sample.Test");
+ var host = Owned(TrackedEntry.ForMove(temp, target));
+ new TrackedWatch(host).Pump();
+ File.Delete(temp);
+
+ Thread? pass = null;
+ var parked = false;
+ host.Mutate(state =>
+ {
+ pass = new(() => new TrackedWatch(host).Pump());
+ pass.Start();
+ parked = WaitUntilBlocked(pass);
+
+ File.WriteAllText(temp, "second run");
+ return ViewerSession.EnqueueTracked(state, TrackedEntry.ForMove(temp, target));
+ });
+ pass!.Join();
+
+ await Assert.That(parked).IsTrue();
+ await Assert.That(host.State.Queue.Select(_ => _.LeftText)).IsEquivalentTo(["second run"]);
+ }
+
+ ///
+ /// The same interleaving from its two halves: what the pass found about the entry it saw, and
+ /// the arrival that replaced that entry before the finding was applied.
+ ///
+ [Test]
+ public async Task ARefreshLeavesAnEntryThatArrivedAfterThePass()
+ {
+ var (temp, target) = Pair("Other.Test");
+ var seen = TrackedEntry.ForMove(temp, target);
+ var state = Owned(seen).State;
+ File.WriteAllText(temp, "second run");
+ var restaged = TrackedEntry.ForMove(temp, target);
+ state = ViewerSession.EnqueueTracked(state, restaged);
+
+ var refreshed = ViewerSession.Refresh(state, [seen], [(seen, Fixtures.Move())]);
+
+ await Assert.That(refreshed).IsSameReferenceAs(state);
+ }
+
+ ///
+ /// A file that stats but cannot be read is not re-read and re-diffed on every pass while it
+ /// stays that way, and the entry for it is left alone.
+ ///
+ [Test]
+ public async Task AnUnreadableFileIsNotReReadEveryPass()
+ {
+ var (temp, target) = Pair("Locked.Test");
+ var host = Owned(TrackedEntry.ForMove(temp, target));
+ var watch = new TrackedWatch(host);
+ File.WriteAllText(temp, "rewritten and then held");
+ using (new FileStream(temp, FileMode.Open, FileAccess.Read, FileShare.None))
+ {
+ watch.Pump();
+ var held = host.State;
+
+ watch.Pump();
+ watch.Pump();
+
+ await Assert.That(host.State).IsSameReferenceAs(held);
+ }
+ }
+
+ static bool WaitUntilBlocked(Thread thread)
+ {
+ var watch = Stopwatch.StartNew();
+ while (watch.Elapsed < TimeSpan.FromSeconds(5))
+ {
+ if ((thread.ThreadState & System.Threading.ThreadState.WaitSleepJoin) != 0)
+ {
+ return true;
+ }
+
+ Thread.Sleep(1);
+ }
+
+ return false;
+ }
}
diff --git a/src/DiffEngineViewer/Images/ImageHeader.cs b/src/DiffEngineViewer/Images/ImageHeader.cs
index c50d5f80..5111472e 100644
--- a/src/DiffEngineViewer/Images/ImageHeader.cs
+++ b/src/DiffEngineViewer/Images/ImageHeader.cs
@@ -174,11 +174,13 @@ static bool TryBmp(ReadOnlySpan bytes, out ImageHeader header)
}
// A negative height means the rows are stored top down, which is not something the size
- // should report.
+ // should report. The one negative with no positive counterpart is no real bitmap, and
+ // Math.Abs throws on it, so it reads as unknown the way a truncated header does.
+ var height = BinaryPrimitives.ReadInt32LittleEndian(bytes[22..]);
header = new(
ImageFormat.Bmp,
BinaryPrimitives.ReadInt32LittleEndian(bytes[18..]),
- Math.Abs(BinaryPrimitives.ReadInt32LittleEndian(bytes[22..])));
+ height == int.MinValue ? 0 : Math.Abs(height));
return true;
}
diff --git a/src/DiffEngineViewer/Ipc/OwnerLink.cs b/src/DiffEngineViewer/Ipc/OwnerLink.cs
index 15d95622..dcf8daf6 100644
--- a/src/DiffEngineViewer/Ipc/OwnerLink.cs
+++ b/src/DiffEngineViewer/Ipc/OwnerLink.cs
@@ -349,44 +349,85 @@ List ReadChanges(ViewerResponse response)
var changes = new List(response.Moves.Count + response.Deletes.Count);
foreach (var move in response.Moves)
{
- if (existing.TryGetValue(move.Key, out var entry) &&
- entry.LeftFile == move.Temp &&
- entry.TargetFile == move.Target &&
- entry.LeftStamp == FileSide.StampOf(move.Temp) &&
- entry.RightStamp == FileSide.StampOf(move.Target))
+ existing.TryGetValue(move.Key, out var held);
+ if (held is not null &&
+ (held.LeftFile != move.Temp ||
+ held.TargetFile != move.Target))
{
- changes.Add(entry);
- continue;
+ held = null;
}
- changes.Add(QueueEntry.ForMove(
- move.Key,
- move.Name,
- move.Group,
- move.Temp,
- move.Target,
- FileSide.Read(move.Temp),
- FileSide.Read(move.Target)));
+ changes.Add(Read(
+ held,
+ () => QueueEntry.ForMove(
+ move.Key,
+ move.Name,
+ move.Group,
+ move.Temp,
+ move.Target,
+ FileSide.Read(move.Temp),
+ FileSide.Read(move.Target))));
}
foreach (var delete in response.Deletes)
{
- if (existing.TryGetValue(delete.Key, out var entry) &&
- entry.LeftFile == delete.File &&
- entry.LeftStamp == FileSide.StampOf(delete.File))
+ existing.TryGetValue(delete.Key, out var held);
+ if (held is not null &&
+ held.LeftFile != delete.File)
{
- changes.Add(entry);
- continue;
+ held = null;
}
- changes.Add(QueueEntry.ForDelete(
- delete.Key,
- delete.Name,
- delete.Group,
- delete.File,
- FileSide.Read(delete.File)));
+ changes.Add(Read(
+ held,
+ () => QueueEntry.ForDelete(
+ delete.Key,
+ delete.Name,
+ delete.Group,
+ delete.File,
+ FileSide.Read(delete.File))));
}
return changes;
}
+
+ readonly ReadRetry retry = new();
+
+ ///
+ /// The entry already held for the same files when nothing about them has changed, and a fresh
+ /// read otherwise.
+ ///
+ /// Held is also kept when a read of it failed a moment ago (), and when
+ /// a read finds no more than it has - a file that stats but cannot be opened comes back with no
+ /// stamp every time. Replacing it anyway handed a new entry
+ /// on every pump, which closed any open menu within 200ms, for as long as the file was locked.
+ ///
+ ///
+ QueueEntry Read(QueueEntry? held, Func read)
+ {
+ if (held is not null)
+ {
+ if (held.LeftStamp == FileSide.StampOf(held.LeftFile!) &&
+ (held.TargetFile is null || held.RightStamp == FileSide.StampOf(held.TargetFile)))
+ {
+ return held;
+ }
+
+ if (retry.Waiting(held.Key))
+ {
+ return held;
+ }
+ }
+
+ var fresh = read();
+ retry.Read(fresh);
+ if (held is not null &&
+ fresh.LeftStamp == held.LeftStamp &&
+ fresh.RightStamp == held.RightStamp)
+ {
+ return held;
+ }
+
+ return fresh;
+ }
}
diff --git a/src/DiffEngineViewer/ReadRetry.cs b/src/DiffEngineViewer/ReadRetry.cs
new file mode 100644
index 00000000..2b7ed399
--- /dev/null
+++ b/src/DiffEngineViewer/ReadRetry.cs
@@ -0,0 +1,48 @@
+///
+/// When to read a tracked file again that stats but cannot be read: one another process holds
+/// open, or one this process may not open. Shared by the two passes that keep tracked entries in
+/// step with the disk, for a queue this process owns and
+/// for one it displays.
+///
+/// A failed read leaves an entry with no stamp, which differs from the stat on every pass, so each
+/// pass read the file again and re-diffed it, five times a second for as long as whatever held it
+/// held it. It is retried at instead: rare enough to cost nothing, and soon
+/// enough that a file that was only briefly locked is shown shortly after it frees up.
+///
+///
+sealed class ReadRetry
+{
+ public static TimeSpan Interval { get; set; } = TimeSpan.FromSeconds(2);
+
+ readonly Dictionary waiting = [];
+
+ ///
+ /// Whether the entry for failed a read too recently to try again.
+ ///
+ public bool Waiting(string key) =>
+ waiting.TryGetValue(key, out var until) &&
+ DateTime.UtcNow < until;
+
+ ///
+ /// Notes how a read of an entry's files went, from the entry it produced.
+ ///
+ public void Read(QueueEntry entry)
+ {
+ if (Unreadable(entry.LeftFile, entry.LeftStamp) ||
+ Unreadable(entry.TargetFile, entry.RightStamp))
+ {
+ waiting[entry.Key] = DateTime.UtcNow + Interval;
+ return;
+ }
+
+ waiting.Remove(entry.Key);
+ }
+
+ ///
+ /// There, and not read. A target that is not there at all is a new snapshot, not a failure.
+ ///
+ static bool Unreadable(string? path, FileStamp? read) =>
+ path is not null &&
+ read is null &&
+ FileSide.StampOf(path) is not null;
+}
diff --git a/src/DiffEngineViewer/SelectionText.cs b/src/DiffEngineViewer/SelectionText.cs
index 7a52dcb1..7dd0225a 100644
--- a/src/DiffEngineViewer/SelectionText.cs
+++ b/src/DiffEngineViewer/SelectionText.cs
@@ -53,7 +53,7 @@ public static SelectionSpan Span(TextSelection? selection, PaneSide side, DiffVi
return default;
}
- return new(0, RowText.Flatten(shown.Text).Length);
+ return new(0, Cells(RowText.Flatten(shown.Text)));
}
///
@@ -76,7 +76,7 @@ public static SelectionSpan Span(TextSelection? selection, PaneSide side, int ro
return default;
}
- var length = RowText.Flatten(text).Length;
+ var length = Cells(RowText.Flatten(text));
var from = row == startRow ? Math.Min(startColumn, length) : 0;
var to = row == endRow ? Math.Min(endColumn, length) : length;
return to <= from ? default : new(from, to - from);
@@ -106,7 +106,8 @@ public static string Of(TextSelection selection, QueueEntry entry)
var text = RowText.Flatten(row.Text);
var span = Span(selection, selection.Side, index, row.Text);
- lines.Add(span.Length == 0 ? "" : text.Substring(span.Start, span.Length));
+ var from = Index(text, span.Start);
+ lines.Add(text[from..Index(text, span.Start + span.Length)]);
}
return string.Join("\n", lines);
@@ -179,6 +180,50 @@ public static string Summary(TextSelection selection, QueueEntry entry)
return $"selected {lines} lines, {characters}";
}
+ ///
+ /// How many cells a row's flattened text takes, which is what a selection's columns count.
+ ///
+ /// A head reports a drag in cells, and every head draws one code point to a cell: GDI+ draws a
+ /// character outside the basic plane one cell wide, as ImGui lays out one glyph per code point.
+ /// Counted in UTF-16 units instead, each such character shifted the copy one place from the
+ /// highlight, and a selection could end between the two halves of it and copy half a
+ /// character. Wide CJK and combining marks still do not fit this; that takes each head
+ /// reporting string positions from its own layout.
+ ///
+ ///
+ public static int Cells(string flattened)
+ {
+ var cells = 0;
+ foreach (var character in flattened)
+ {
+ if (!char.IsLowSurrogate(character))
+ {
+ cells++;
+ }
+ }
+
+ return cells;
+ }
+
+ ///
+ /// Where in the flattened text a cell starts: never inside a surrogate pair.
+ ///
+ static int Index(string flattened, int cell)
+ {
+ var index = 0;
+ for (var count = 0; count < cell && index < flattened.Length; count++)
+ {
+ index++;
+ if (index < flattened.Length &&
+ char.IsLowSurrogate(flattened[index]))
+ {
+ index++;
+ }
+ }
+
+ return index;
+ }
+
static int ClampRow(int row, IReadOnlyList rows) =>
Math.Clamp(row, 0, Math.Max(0, rows.Count - 1));
@@ -190,6 +235,6 @@ static int ClampColumn(int row, int column, IReadOnlyList rows)
}
var text = RowText.Flatten(rows[ClampRow(row, rows)].Text);
- return Math.Clamp(column, 0, text.Length);
+ return Math.Clamp(column, 0, Cells(text));
}
}
diff --git a/src/DiffEngineViewer/TextSelection.cs b/src/DiffEngineViewer/TextSelection.cs
index 609776f0..6686dde7 100644
--- a/src/DiffEngineViewer/TextSelection.cs
+++ b/src/DiffEngineViewer/TextSelection.cs
@@ -15,8 +15,8 @@ enum PaneSide
/// Rows are indexes into the whole side, not into the visible slice: a drag that continues while
/// the wheel scrolls has to mean the same thing before and after. The entry's side, too, rather
/// than the minimal view's, so a selection means the same text whichever view is on screen.
-/// Columns are characters of the row's flattened text, which is what is on screen and therefore
-/// what was pointed at.
+/// Columns are cells of the row's flattened text - code points, one to a cell - which is what is
+/// on screen and therefore what was pointed at ().
///
///
/// and are the entry this describes. Anything
@@ -60,5 +60,14 @@ record TextSelection(
public bool Describes(QueueEntry? entry) =>
entry is not null &&
entry.Key == Key &&
- entry.SelectedVariant == Variant;
+ entry.SelectedVariant == Variant &&
+ ReferenceEquals(entry.View(false), Text);
+
+ ///
+ /// The text this was dragged across, as the entry's built view. A re-run lands on the same key
+ /// with other text, and a selection that outlived that highlighted and copied rows the reader
+ /// never selected. The view is built with the entry and carried by every with on it, so
+ /// a status change keeps the selection and a rebuild of the text ends it.
+ ///
+ public required DiffView Text { get; init; }
}
diff --git a/src/DiffEngineViewer/TrackedWatch.cs b/src/DiffEngineViewer/TrackedWatch.cs
index d3eb6aae..c0dd6983 100644
--- a/src/DiffEngineViewer/TrackedWatch.cs
+++ b/src/DiffEngineViewer/TrackedWatch.cs
@@ -57,11 +57,17 @@ public void Run(Cancel cancel)
///
/// One pass. Public for the tests, which drive it directly rather than waiting on a thread.
+ ///
+ /// What it found is handed on with the entries it found it about, and applied only to those
+ /// same entries. The stat runs outside the host's lock, and a re-run can stage its pair again
+ /// under the same key in between: dropped by key, the new pair went with the old one's missing
+ /// file, and stayed gone until the test failed again.
+ ///
///
public void Pump()
{
- var gone = new List();
- var changed = new List();
+ var gone = new List();
+ var changed = new List<(QueueEntry Seen, QueueEntry Fresh)>();
foreach (var entry in host.State.Queue)
{
if (entry.Kind == QueueEntryKind.Move)
@@ -85,7 +91,9 @@ public void Pump()
host.Mutate(_ => ViewerSession.Refresh(_, gone, changed));
}
- static void Move(QueueEntry entry, List gone, List changed)
+ readonly ReadRetry retry = new();
+
+ void Move(QueueEntry entry, List gone, List<(QueueEntry Seen, QueueEntry Fresh)> changed)
{
var temp = entry.LeftFile!;
var target = entry.TargetFile!;
@@ -94,7 +102,7 @@ static void Move(QueueEntry entry, List gone, List changed)
// The received file is what the pair exists for, so its absence ends the entry. A
// target that is not there is not the same thing at all: a brand new snapshot never
// has one, and an entry offering to create it is the whole point.
- gone.Add(entry.Key);
+ gone.Add(entry);
return;
}
@@ -106,7 +114,7 @@ static void Move(QueueEntry entry, List gone, List changed)
Changed(
entry,
- QueueEntry.ForMove(
+ () => QueueEntry.ForMove(
entry.Key,
entry.Name,
entry.Solution,
@@ -117,13 +125,13 @@ static void Move(QueueEntry entry, List gone, List changed)
changed);
}
- static void Delete(QueueEntry entry, List gone, List changed)
+ void Delete(QueueEntry entry, List gone, List<(QueueEntry Seen, QueueEntry Fresh)> changed)
{
var file = entry.LeftFile!;
if (FileSide.StampOf(file) is not { } stamp)
{
// Already gone, so there is nothing left to offer to delete.
- gone.Add(entry.Key);
+ gone.Add(entry);
return;
}
@@ -134,24 +142,31 @@ static void Delete(QueueEntry entry, List gone, List changed
Changed(
entry,
- QueueEntry.ForDelete(entry.Key, entry.Name, entry.Solution, file, FileSide.Read(file)),
+ () => QueueEntry.ForDelete(entry.Key, entry.Name, entry.Solution, file, FileSide.Read(file)),
changed);
}
///
- /// Stamped again after the read rather than trusted from before it, because a file that exists
- /// but cannot be opened stamps and does not read: it comes back with no stamp at all, which
- /// differs from the stat every time and would rebuild and re-diff the entry on every pass for
- /// as long as whatever holds the file holds it.
+ /// Re-read, unless a read of it failed a moment ago (). Stamped again
+ /// after the read rather than trusted from before it, because a file that exists but cannot be
+ /// opened stamps and does not read: it comes back with no stamp at all, the same as the entry
+ /// already held, which is kept rather than replaced by an identical one.
///
- static void Changed(QueueEntry entry, QueueEntry fresh, List changed)
+ void Changed(QueueEntry entry, Func read, List<(QueueEntry Seen, QueueEntry Fresh)> changed)
{
+ if (retry.Waiting(entry.Key))
+ {
+ return;
+ }
+
+ var fresh = read();
+ retry.Read(fresh);
if (fresh.LeftStamp == entry.LeftStamp &&
fresh.RightStamp == entry.RightStamp)
{
return;
}
- changed.Add(fresh);
+ changed.Add((entry, fresh));
}
}
diff --git a/src/DiffEngineViewer/ViewerSession.cs b/src/DiffEngineViewer/ViewerSession.cs
index 46242968..14a6ea9c 100644
--- a/src/DiffEngineViewer/ViewerSession.cs
+++ b/src/DiffEngineViewer/ViewerSession.cs
@@ -222,7 +222,7 @@ public static SessionState Sync(
if (selected < 0)
{
- return Open(next);
+ return Reopen(next);
}
return Clamp(next);
@@ -238,8 +238,9 @@ public static SessionState Sync(
/// a re-run had already replaced, and offering a received file that was no longer there.
///
///
- /// Both arguments name keys, and anything they name that is no longer queued is skipped: the
- /// read that produced them ran outside the lock, so a patch or a pair can have arrived since.
+ /// Both arguments name the entries the pass looked at, and any of those no longer queued is
+ /// skipped: the read that produced them ran outside the lock, so a patch or a pair can have
+ /// arrived since, under the same key or not.
/// A pass that changes nothing returns the same state, because this runs several times a
/// second and rebuilding the queue - or clearing the open menu - on every one of them is not
/// housekeeping the reader should be able to feel.
@@ -247,21 +248,29 @@ public static SessionState Sync(
///
public static SessionState Refresh(
SessionState state,
- IReadOnlyCollection gone,
- IReadOnlyList changed)
+ IReadOnlyCollection gone,
+ IReadOnlyList<(QueueEntry Seen, QueueEntry Fresh)> changed)
{
- var replacements = changed.ToDictionary(_ => _.Key);
+ // By the entries the pass looked at, not by their keys: one staged again under the same
+ // key since then is news the pass has not seen, and is left as it arrived.
+ var replacements = new Dictionary(ReferenceEqualityComparer.Instance);
+ foreach (var (seen, fresh) in changed)
+ {
+ replacements[seen] = fresh;
+ }
+
+ var went = new HashSet(gone, ReferenceEqualityComparer.Instance);
var queue = new List(state.Queue.Count);
var any = false;
foreach (var entry in state.Queue)
{
- if (gone.Contains(entry.Key))
+ if (went.Contains(entry))
{
any = true;
continue;
}
- if (replacements.TryGetValue(entry.Key, out var fresh))
+ if (replacements.TryGetValue(entry, out var fresh))
{
any = true;
queue.Add(fresh);
@@ -419,7 +428,10 @@ public static SessionState Drag(
ends.AnchorRow,
ends.AnchorColumn,
ends.FocusRow,
- ends.FocusColumn),
+ ends.FocusColumn)
+ {
+ Text = current.View(false)
+ },
current);
// The identical state when the pointer has not left the cell it was in, which is most
@@ -471,7 +483,10 @@ static SessionState SelectAll(SessionState state)
0,
0,
last,
- RowText.Flatten(rows[last].Text).Length)
+ SelectionText.Cells(RowText.Flatten(rows[last].Text)))
+ {
+ Text = current.View(false)
+ }
};
}
@@ -670,9 +685,9 @@ static SessionState AcceptGroup(SessionState state, MenuState menu, ViewerAction
actions,
discarding: false,
TrackedKeysOf(all),
- // A group accept drops its stale entries, so the queue it hands on cannot be read for
- // them the way the full sweep's can
- refused: notWritten > 0);
+ // Its own members only: a stale one has left the queue, and one that failed is still
+ // in it, beside other entries' statuses from other accepts
+ refused: notWritten + failed > 0);
}
static SessionState DiscardGroup(SessionState state, MenuState menu, ViewerActions actions)
@@ -868,7 +883,7 @@ public static SessionState BeginAcceptAll(SessionState state)
/// Entries with nothing left to apply are passed over rather than claimed: one that has gone
/// since the batch began - settled, discarded, its file taken away - and one a second
/// framework has since made a conflict of. A delete is held rather than claimed once a
- /// snapshot in the batch was not written, for the reason gives.
+ /// snapshot in the batch was not written, for the reason gives.
///
///
public static SessionState ClaimNext(SessionState state)
@@ -1082,6 +1097,26 @@ static SessionState DiscardAllInline(SessionState state, ViewerActions actions)
/// The keys to sweep, for a group header acting on its own members. Null sweeps every tracked
/// entry, which is what the unqualified bulk commands mean.
///
+ ///
+ /// Whether the inline accept this sweep follows left a snapshot unwritten, which holds every
+ /// delete it would carry out.
+ ///
+ /// A snapshot moving inline arrives as two unrelated entries: the patch that writes the literal
+ /// into the source, and a delete of the verified file it replaces. The sweep ran the delete
+ /// whether or not the patch landed, so a patch the applier would not take — a call site that
+ /// cannot host a Snapshot call, a source that moved since the run — cost the snapshot both
+ /// copies at once. Nothing ties a delete to the patch it belongs to, so the whole sweep of
+ /// deletes waits on the whole batch of patches. Blunt, and deliberately so — the entries held
+ /// are still queued, still shown, and still acceptable one at a time. Moves are left alone: a
+ /// received file promoted over a verified one is the snapshot arriving, not the last copy of it
+ /// leaving.
+ ///
+ ///
+ /// Counted by the caller from the attempts it made, never read off the queue. Every status an
+ /// entry carries looks the same there, and reading them held a group's deletes over a failure
+ /// in another solution, left by an accept long before this one.
+ ///
+ ///
static SessionState SweepTracked(
SessionState state,
IReadOnlyList queue,
@@ -1094,7 +1129,6 @@ static SessionState SweepTracked(
var remaining = new List(queue.Count);
var swept = 0;
var kept = 0;
- refused = refused || InlineRefused(queue, discarding);
foreach (var entry in queue)
{
if (entry.Kind is not (QueueEntryKind.Move or QueueEntryKind.Delete) ||
@@ -1144,52 +1178,6 @@ static string WithFiles(string message, int swept, int kept)
const string deleteHeld = "Held: a snapshot in this batch could not be written inline, and this file may be the only copy of it left. Accept it on its own to delete it anyway.";
- ///
- /// Whether this sweep follows an inline accept that something refused.
- ///
- /// A snapshot moving inline arrives as two unrelated entries: the patch that writes the literal
- /// into the source, and a delete of the verified file it replaces. The sweep ran the delete
- /// whether or not the patch landed, so a patch the applier would not take — a call site that
- /// cannot host a Snapshot call, a source that moved since the run — cost the snapshot both
- /// copies at once. The literal was never written and the file it was replacing was gone, which
- /// no re-run recovers: every later run reports the same new snapshot and deletes nothing,
- /// forever.
- ///
- ///
- /// Nothing ties a delete to the patch it belongs to, so the whole sweep of deletes waits on the
- /// whole batch of patches. Blunt, and deliberately so — the entries held are still queued,
- /// still shown, and still acceptable one at a time by anyone who knows the file is redundant.
- /// Moves are left alone: a received file promoted over a verified one is the snapshot arriving,
- /// not the last copy of it leaving.
- ///
- ///
- /// A status on an inline entry is the outcome of an attempt, and this has to read only the
- /// attempts the sweep it is answering for made. A bulk accept skips conflicted entries and
- /// hands them back exactly as they were, so a status on one of those was left by a targeted
- /// accept of a single variant, at some earlier point - and reading it held every pending
- /// delete on every accept-all after it, citing a refusal in a batch that never touched the
- /// entry. Everything else left in the queue with a status was tried and refused just now.
- /// Discarding asks nothing of the patches, so it sweeps as it always did.
- ///
- ///
- static bool InlineRefused(IReadOnlyList queue, bool discarding)
- {
- if (discarding)
- {
- return false;
- }
-
- foreach (var entry in queue)
- {
- if (entry is {Kind: QueueEntryKind.Inline, Conflicted: false, Status: not null})
- {
- return true;
- }
- }
-
- return false;
- }
-
///
/// Accepting a tracked entry is the file operation it describes; discarding one is throwing
/// the received file away, or, for a delete, leaving the file alone and only untracking it —
@@ -1375,7 +1363,7 @@ static IReadOnlyList Project(SessionState state, InlineQueue queue)
// The variants changed, so the entry rebuilds, but what the reader had cycled to
// survives where it still exists.
- entries.Add(QueueEntry.ForInline(pending, entry.SelectedVariant));
+ entries.Add(QueueEntry.ForInline(pending, SameVariant(entry, pending)));
continue;
}
@@ -1462,12 +1450,33 @@ static SessionState Remove(SessionState state, IReadOnlyList queue,
if (selected < 0)
{
- return Open(next);
+ return Reopen(next);
}
return Clamp(next);
}
+ ///
+ /// Where the variant the reader had cycled to is among the rebuilt entry's. Found by what it is
+ /// the output of rather than by where it was: a framework settling, or a re-run agreeing with
+ /// another variant, drops or merges the ones before it, and the index then named a different
+ /// framework's content - silently, and Accept applied that one. The index stands only when no
+ /// variant holds any framework the old one did.
+ ///
+ static int SameVariant(QueueEntry entry, PendingInline pending)
+ {
+ var origins = entry.Variants[entry.SelectedVariant].Origins;
+ for (var index = 0; index < pending.Variants.Count; index++)
+ {
+ if (pending.Variants[index].Origins.Any(origins.Contains))
+ {
+ return index;
+ }
+ }
+
+ return entry.SelectedVariant;
+ }
+
static SessionState Select(SessionState state, int index)
{
if (state.Queue.Count == 0 ||
@@ -1485,7 +1494,11 @@ static SessionState Select(SessionState state, int index)
return Clamp(state);
}
- return Open(state with { Selected = index });
+ // An entry's menu acts on the entry selected when an item is clicked, so one left open over
+ // a move - a focus from the tray or an IDE, a re-sent snapshot - discarded or accepted an
+ // entry it was never opened on. Selecting is the only way the entry changes without the
+ // queue changing, and a queue change already closes the menu.
+ return Open(state with { Selected = index, Menu = null });
}
///
@@ -1517,21 +1530,39 @@ static SessionState Toggle(SessionState state, string key)
}
var folded = state with { Collapsed = collapsed };
- var visible = QueueProjection.VisibleEntries(folded);
+ if (NearestVisible(folded) is { } visible)
+ {
+ return Select(folded, visible);
+ }
+
+ return Clamp(folded);
+ }
+
+ ///
+ /// Where the selection goes when it is under a fold, or null when it is not. The column follows
+ /// the selection, so leaving it there would leave the whole list with nothing highlighted,
+ /// while the panes and Accept went on acting on an entry nobody could see. Forward first,
+ /// because folding a group is usually done on the way down the queue, and an entry that goes
+ /// hands the selection to the one after it.
+ ///
+ /// For a fold, and for the entry being read going: an index kept across that can name the
+ /// first entry of a folded group that follows it.
+ ///
+ ///
+ static int? NearestVisible(SessionState state)
+ {
+ var visible = QueueProjection.VisibleEntries(state);
if (visible.Count == 0 ||
- visible.Contains(folded.Selected))
+ visible.Contains(state.Selected))
{
- return Clamp(folded);
+ return null;
}
- // The selection went under the fold. The column follows the selection, so leaving it there
- // would leave the whole list with nothing highlighted. Forward first, because folding a
- // group is usually done on the way down the queue.
var before = -1;
var after = -1;
foreach (var index in visible)
{
- if (index < folded.Selected)
+ if (index < state.Selected)
{
before = index;
}
@@ -1541,7 +1572,27 @@ static SessionState Toggle(SessionState state, string key)
}
}
- return Select(folded, after >= 0 ? after : before);
+ if (after >= 0)
+ {
+ return after;
+ }
+
+ return before;
+ }
+
+ ///
+ /// The entry being read has gone, and the selection is left on whatever now has its index:
+ /// opened there, or at the nearest entry that can be seen when that one is under a fold.
+ ///
+ static SessionState Reopen(SessionState state)
+ {
+ state = Clamp(state);
+ if (NearestVisible(state) is { } visible)
+ {
+ return Select(state, visible);
+ }
+
+ return Open(state);
}
///
diff --git a/todo.md b/todo.md
index 8c6d602d..75416f5b 100644
--- a/todo.md
+++ b/todo.md
@@ -9,54 +9,14 @@ Open findings from a review of `main` at 4244ebe6 (2026-09-23), rechecked on c37
## Bugs
-The repro tests are on the local branch `review-repros`, one class per area: `ReviewReproSessionTests` and `ReviewReproWatchTests` (viewer model), `ReviewReproWindowsTests`, `ReviewReproTrayTests`, `ReviewReproPatcherTests`, `ReviewReproLibraryTests`. Each test fails on c37bf9e1 except a control (`ControlSpaceIndentedLocalLeavesTheSiblingAlone`) and a measurement (`HowLongADecodeHoldsTheFile`).
+The repro tests are on the local branch `review-repros`, one class per area: `ReviewReproWindowsTests`, `ReviewReproTrayTests`, `ReviewReproPatcherTests`, `ReviewReproLibraryTests`. Each test fails on c37bf9e1 except a control (`ControlSpaceIndentedLocalLeavesTheSiblingAlone`) and a measurement (`HowLongADecodeHoldsTheFile`).
Viewer model
-- [ ] **Removing the current entry can select one hidden inside a collapsed group** (repro)
- - `Remove` (`src/DiffEngineViewer/ViewerSession.cs:1449`) and `Sync` (`:186`) keep the old index and never check `VisibleEntries`, so after an accept, discard, settle or refresh, the panes and Accept act on an entry under a fold that nothing in the list highlights. An attached viewer gets there through `Sync` on every accept.
- - Tests: `Accepting_the_entry_being_read_does_not_select_one_under_a_fold`, `A_listing_without_the_entry_being_read_does_not_select_one_under_a_fold`.
- - Fix as suggested: run `Toggle`'s search (`:1530-1544`) after `Clamp` in both. Tried: both pass and nothing else breaks.
-
-- [ ] **A text selection survives its entry's text being replaced under the same key** (repro)
- - `Describes` (`src/DiffEngineViewer/TextSelection.cs:60-63`) compares key and variant only. A re-run rebuilds the entry (`ViewerSession.cs:53-73`) without touching `Selection`, so the highlight and the copy point into the new rows.
- - Test: `A_rerun_that_replaces_the_text_under_a_selection_ends_the_selection` (copies "added two" where "brown dog" was selected).
- - Fix as suggested: the `View(false)` reference works, since a `with` keeps it and a rebuild does not. It also ends the selection when a conflicted entry gains a variant, which errs the safe way.
-
-- [ ] **An open entry menu acts on whatever is selected when it is clicked, and the selection can move under it** (repro)
- - Menu items act on `state.Current` (`src/DiffEngineViewer/ViewerProgram.cs:445`, discard at `ViewerSession.cs:1263`, the attached key at `ViewerProgram.cs:586`), and `SelectKey`/`Select` (`ViewerSession.cs:309`, `:1488`) never clear `Menu`.
- - Triggers: a wire Focus from the tray menu or an IDE (`MessageHandler.cs:191`); and, attached, with nobody touching anything, a test re-sending an identical patch. The tray folds it into the same entry and stashes a Focus (`OwnedInlineHost.cs:198`) that its next listing carries (`:274`); `Sync` keeps the menu because the entries are unchanged, then `OwnerLink.cs:128` selects.
- - Tests: `A_wire_focus_does_not_retarget_an_open_entry_menu`, `A_focus_riding_the_owners_listing_does_not_retarget_an_open_entry_menu`.
- - Fix: `Menu = null` in `Select`, the only path that moves the selection without changing the queue. Not covered: an arrival landing between a painted frame and a key press, which focusing arrivals are built on.
-
-- [ ] **"Accept all in " holds that solution's deletes because of an unrelated earlier failure** (repro)
- - `AcceptGroup` passes `refused: notWritten > 0` (`ViewerSession.cs:675`) and `SweepTracked` ORs in `InlineRefused` (`:1097`), which scans every inline entry in the queue (`:1182-1188`), not the group's.
- - Test: `An_earlier_failure_in_another_solution_does_not_hold_this_solutions_deletes`.
- - The suggested fix is incomplete: the flag must be `notWritten + failed > 0`, because members that failed stay queued and only the scan catches them today. `InlineRefused` then has no non-discarding caller left.
-
-- [ ] **The variant a reader picked is kept by index rather than identity** (repro)
- - `Project` rebuilds a changed entry with `QueueEntry.ForInline(pending, entry.SelectedVariant)` (`ViewerSession.cs:1378`), which only clamps (`QueueEntry.cs:98`). An origin settle (`ViewerSession.cs:125-132`) or a re-run merging two variants (`InlineQueue.cs:137-182`) moves the reader to another framework's content with nothing to say so, and Accept then applies that one (`ViewerSession.cs:580-586`).
- - Tests: `ASettleOfAnEarlierVariantKeepsTheVariantOnScreen`, `AReRunThatMergesAnEarlierVariantKeepsTheVariantOnScreen`.
- - Fix: keep the variant that holds one of the old variant's origins, and fall back to the index only when none does.
-
-- [ ] **A BMP with a height of `0x80000000` throws in `Math.Abs(int.MinValue)`** (repro)
- - `src/DiffEngineViewer/Images/ImageHeader.cs:181`. `FileSide.Read`'s catch-all (`FileSide.cs:41-44`) turns the `OverflowException` into an unreadable side warning "Negating the minimum value of a twos complement number is invalid", with no stamp, so it is re-read every 200 ms (and, attached, closes an open menu each time; see `ReadChanges` below). No crash.
- - Tests: `ABmpWithTheMinimumHeightIsStillABmp`, `ABmpWithTheMinimumHeightIsNotReportedUnreadable`.
-
-- [ ] **`OwnerLink.ReadChanges` replaces an unreadable file's entry on every pump, which closes an attached viewer's menu within 200 ms** (repro)
- - Both it (`src/DiffEngineViewer/Ipc/OwnerLink.cs:244-279`) and `TrackedWatch` (`TrackedWatch.cs:101-117`) compare the stored stamp, null after a failed read, with a fresh stat, so both re-read and re-diff a locked file five times a second (the diff runs in `QueueEntry.cs:59-63`). Only `TrackedWatch` then drops the rebuilt entry (`Changed`, `:147-156`). `ReadChanges` hands `Sync` a new reference every pump, which defeats #881's `SameEntries` guard.
- - Test: `AnUnreadableTrackedFileLeavesAnAttachedViewersMenuOpen`.
- - The suggested fix is incomplete: porting the guard stops the menu closing but not the re-reads in either path. Storing the stat's stamp on a failed read would leave a briefly locked file unreadable until it changed. A retry backoff would do both.
-
-- [ ] **`TrackedWatch` can drop a pair re-staged under the same key between its stat and its `Refresh`** (repro)
- - `Pump` reads the queue without the lock (`src/DiffEngineViewer/TrackedWatch.cs:65`), stats the files outside it, and applies `Refresh` under `Mutate` (`:85`), which drops or replaces by key alone (`ViewerSession.cs:256-269`). A re-run that deletes then rewrites its received file lands in that gap, and in an owning viewer the pair is gone until the test fails again.
- - Tests: `APassDoesNotDropAPairReStagedAfterItsStat` (6 of 6 runs), `ARefreshDoesNotDropAnEntryThatArrivedAfterTheStat`.
- - Fix as suggested: act only when the current entry is the reference the stat saw.
-
-- [ ] **Selection columns are UTF-16 units, but every head reports cells** (repro for the copy, verified for the heads)
- - All three heads turn the pointer's x into cells (`src/DiffEngineViewer.Windows/ViewerCanvas.cs:273-274`, `native/src/deview.cpp:708-722`, `native/swift/Sources/Deview/ViewerView.swift:212-219`), and `SelectionText` indexes UTF-16 with them (`:63-83`, `:93-113`). GDI+ draws a non-BMP code point as exactly one cell, and ImGui lays out one glyph per code point.
- - Tests: `ADragAcrossOneNonBmpCharacterCopiesAllOfIt` (copies a lone high surrogate), `ADragAfterANonBmpCharacterCopiesWhatWasHighlighted` (copies a low surrogate and "a" for "ab").
- - Counting in Runes is incomplete: CJK falls back to a font 1.83 cells wide on Windows, a combining mark takes none, and Core Text substitutes fonts with their own widths. Either put every code point on the grid or have each head report string indexes from its own layout; at the least, snap the ends to Rune boundaries. `Summary` and select-all's end column (`ViewerSession.cs:474`) count UTF-16 too.
+- [ ] **Selection columns count one code point to a cell, which wide CJK and combining marks do not take** (verified)
+ - Columns now count code points, which fixed the copy and the highlight for characters outside the basic plane: GDI+ draws those one cell wide and ImGui lays out one glyph per code point (`SelectionText.Cells`). Still off: CJK falls back to a font 1.83 cells wide on Windows, a combining mark takes none, and Core Text substitutes fonts with their own widths.
+ - Fix: have each head report string positions from its own layout rather than cells, or put every code point on the grid.
+
Windows head (measured in the test host at 96 DPI; this machine is 120)