diff --git a/claude.md b/claude.md index 762343e0..b1d84005 100644 --- a/claude.md +++ b/claude.md @@ -163,8 +163,12 @@ write. Every patch is told what `Apply` would have told it in turn, and `Apply` case of the same code. One thing can only differ: a write that fails fails every patch that edited, and any other that would have had to edit the file as it was read, while the rest keep what is true of the file. Both batches use it, a file at a time (`AcceptBatch.Together` in the viewer, -`OwnedInlineHost.AcceptEvery` in the tray), so the moment up to which a snapshot can still be -withdrawn from a bulk accept is its file's turn rather than its own. A `SourceScan` rents its map +`OwnedInlineHost.AcceptEvery` in the tray). Each patch that edits is asked about once the file is +patched in memory and before its one write, with the file's lock held: one no longer wanted, +discarded or settled while the file was waited for, is `InlineApplyStatus.Withdrawn`, the file is +patched again from what was read without it, and a batch counts it as nothing. The viewer answers +from `host.State` without the session's lock, because a wire accept applies inside that lock and +would wait on the file while the question waited on it; the tray answers under its gate. A `SourceScan` rents its map from the pool and is disposed for that reason, and keeps its spans as sorted lists rather than hash tables. A batch lexes a file once and carries the scan from one editing patch to the next (`SourceScan.Edited`): lexing starts again at the edit's line, never at a line that follows a @@ -228,7 +232,10 @@ the comment there about not caching "nothing staged" asks for. `Application.DoEvents` rather than `Application.Run`, so the shared loop stays shared. Only the grid is owner drawn: the footer, the context menu, the pane scrollbar and the tooltips are real controls, so they get the OS's keyboard handling, theming and screen reader support. The menu is - still projected from the same `Screen.Menu` the other heads draw. A row is handed to GDI+ cut to + still projected from the same `Screen.Menu` the other heads draw. A pane's header too long for + its pane is cut to whole cells with an ellipsis in the last, the left one a gap short of the + right pane (`ViewerCanvas.HeaderShown`), as the Linux head's table cuts its own. A capture's + caller asks `FormsViewerWindow.MeasureGrid` for the grid a screen's footer leaves. A row is handed to GDI+ cut to the cells its pane has, and one more (`RowText.Shown`, read from the front of the row and cut before it is segmented): GDI+ lays out every character it is given before it clips any, so 72 rows of 2,000 character lines were 15 ms a paint, and a megabyte line 24. A picture zoomed to @@ -353,8 +360,10 @@ the comment there about not caching "nothing staged" asks for. would not take staying in the queue with what the applier said, and it counts as still needing review only its own members. The bulk discards are the same batch with `Discarding` set: snapshots and pending deletes go as it begins, since nothing of theirs is on disk, and each - move's received file is thrown away outside the lock. A discard under way is not on an owner's - listings. No inline transition rebuilds the whole list any more. An arrival, a settle, a + move's received file is thrown away outside the lock. A discard under way is on an owner's + listings as the same counts and a `discarding` line, which an older reader skips and so takes + for an accept; an attached window says "Discarding n of m" and refuses what changes the queue. + No inline transition rebuilds the whole list any more. An arrival, a settle, a discard and a single accept read what changed off the `InlineQueue` before and after, whose untouched items come back as the same instances, and edit the list where it stands (`TryRebuildChanged`), with `RebuildWhole` as the fallback and as what the tests hold it to. @@ -664,7 +673,11 @@ the comment there about not caching "nothing staged" asks for. anything held the queue is `Failed`, so the caller stages; it used to be waited on for the whole of `BindWait` and reported as launched, and an inline snapshot was then in no queue and not staged either. A clean exit is left to the wait, since a viewer that hands its work to an owner - exits with zero. `ViewerContract` is the other half: resolution passes over a copy older than + exits with zero. A viewer that could not show its patch and staged everything it held exits 5 + (`ViewerExit.Staged`): the gate reports `Staged`, `AddInlineAsync` answers + `InlineResult.Staged`, and the caller stages nothing, where both used to stage a trio. It is a + failure to any older library and never returned by an older viewer, so `ViewerContract` asks + nothing for it, and a viewer started for a delete or a pair never says it. `ViewerContract` is the other half: resolution passes over a copy older than 20.5.0, which exits on `--payload`, when a newer one is further down the search order. - Unless that diff tool is the viewer, which is the `Diff` verb and `--diff `. Then the premise above is false — there is no window for the pair yet — so it is tracked exactly @@ -715,8 +728,14 @@ the comment there about not caching "nothing staged" asks for. passes that path as it was given. A tool started through a script, or one that hands over to another process and exits, cannot be tracked from here at all. - A move that writes a file marks the delete pending on it (`TrackedDelete.Written`), however - the move was accepted, and no accept-all carries a marked delete out until a run raises it - again; accepting it on its own still does. `Tracker.HeldReason` is what the menu and the debug + the move was accepted, and no accept-all carries a marked delete out until it is raised again + over a file written since (`TrackedDelete.WrittenAs`, `QueueEntry.WrittenAs`): raised over the + file as the move left it, it stays held, since a process that decided before the move sends + the same message. Accepting it on its own still does. A move counts as still to write its file + from before it leaves `moves` until it lands (`Tracker.accepting`), so a listing taken mid + accept holds its delete. A held delete's row leads with `~` in its label and is handed to the + heads with no status, so none draws it as a failure (`QueueProjection.HeldMark`, + `QueueEntry.Held`), and the tray's menu marks it `~` where a failure is `!`. `Tracker.HeldReason` is what the menu and the debug view show, and it rides a full listing as a `held: key|reason` line of its own (`ViewerResponseDelete.Held`), since the `delete` line is parsed by field count and an older reader skips a line it does not know. An attached viewer shows it as the entry's status and diff --git a/docs/inline.md b/docs/inline.md index efe6f5cc..f6b20ff3 100644 --- a/docs/inline.md +++ b/docs/inline.md @@ -75,7 +75,7 @@ DiffEngineViewer --inline --source --line < the.inlinepat For the producing side — a test library with a failing inline snapshot: -* `DiffRunner.AddInlineAsync(patch)` queues a patch with whatever owns the port, launching the bundled viewer when nothing does. Returns `Queued`, `Disabled` (build servers, continuous testing and AI CLIs included), or `NoViewerFound` — the caller's cue to stage files and fall back to a text diff. A viewer that was started and exited with a failure before it held the queue is `NoViewerFound` as well: one too old for the launch, or with no runtime to run on, took nothing. +* `DiffRunner.AddInlineAsync(patch)` queues a patch with whatever owns the port, launching the bundled viewer when nothing does. Returns `Queued`, `Disabled` (build servers, continuous testing and AI CLIs included), or `NoViewerFound` — the caller's cue to stage files and fall back to a text diff. A viewer that was started and exited with a failure before it held the queue is `NoViewerFound` as well: one too old for the launch, or with no runtime to run on, took nothing. `Staged` is a viewer that was started, could not show the snapshot, and staged it itself under the source project's `obj/VerifyInline`: the caller should not stage it a second time. * `DiffRunner.SettleInline(sourceFile, line)` drops the pending entry for a call site, for when a previously failing test passes. Unknown entries and an absent owner are no-ops, so call it freely. The settle carries the running framework, so a multi-targeted run only settles its own variant of a conflicted entry. That framework is the running process's, which makes this the test run's verb and only the test run's: a surface applying a patch of its own wants `SettleAppliedInline`, [below](#applying-a-patch-from-another-surface). Pass `memberName` and `value` too: `value` is what the passing call's expected argument holds, as the library compared it (for F#, after `SourceLanguage.SnapshotValue`). Once an accept above a call site moves it, its line no longer names its entry and the member is the fallback. `value` narrows that fallback to an entry the value settles, one anchored to it or waiting to become it. Without `value`, a passing call can settle the entry of a failing sibling in the same member. The line can also come to name another call's entry, so one found under it that was queued from a different member is left alone unless `value` settles it. A failing re-run of a call site that has moved is recognised the same way, by its member, test and anchor, and updates its entry rather than queueing a second one beside it. * `InlineStaging.Settle(sourceFile, line, memberName)` clears what the running framework staged for a call site that now passes. `InlinePatchFile.Write` labels a patch that carries no framework with the running one, so a framework that passes does not clear what another one staged. `InlineStaging.Clear` still clears every framework's, which is what retiring a call site wants. * An absent owner is also remembered. A port found with nothing listening is taken as still unowned for ten minutes, and the sends that only tell the owner something — settle, retire, a move or delete to track, the first attempt to queue a patch — return without connecting while that stands. A refused loopback connection is not free on Windows: the firewall's stealth mode, on by default, drops the reset a closed port would answer with, so each refusal takes two seconds, and a green run settling once per inline verification was spending minutes on them. Anything that has to reach an owner probes for itself before launching a viewer, and that probe, like every listing, always connects and corrects the memory with what it finds. diff --git a/docs/mdsource/inline.source.md b/docs/mdsource/inline.source.md index be28af05..7642f4a4 100644 --- a/docs/mdsource/inline.source.md +++ b/docs/mdsource/inline.source.md @@ -68,7 +68,7 @@ DiffEngineViewer --inline --source --line < the.inlinepat For the producing side — a test library with a failing inline snapshot: -* `DiffRunner.AddInlineAsync(patch)` queues a patch with whatever owns the port, launching the bundled viewer when nothing does. Returns `Queued`, `Disabled` (build servers, continuous testing and AI CLIs included), or `NoViewerFound` — the caller's cue to stage files and fall back to a text diff. A viewer that was started and exited with a failure before it held the queue is `NoViewerFound` as well: one too old for the launch, or with no runtime to run on, took nothing. +* `DiffRunner.AddInlineAsync(patch)` queues a patch with whatever owns the port, launching the bundled viewer when nothing does. Returns `Queued`, `Disabled` (build servers, continuous testing and AI CLIs included), or `NoViewerFound` — the caller's cue to stage files and fall back to a text diff. A viewer that was started and exited with a failure before it held the queue is `NoViewerFound` as well: one too old for the launch, or with no runtime to run on, took nothing. `Staged` is a viewer that was started, could not show the snapshot, and staged it itself under the source project's `obj/VerifyInline`: the caller should not stage it a second time. * `DiffRunner.SettleInline(sourceFile, line)` drops the pending entry for a call site, for when a previously failing test passes. Unknown entries and an absent owner are no-ops, so call it freely. The settle carries the running framework, so a multi-targeted run only settles its own variant of a conflicted entry. That framework is the running process's, which makes this the test run's verb and only the test run's: a surface applying a patch of its own wants `SettleAppliedInline`, [below](#applying-a-patch-from-another-surface). Pass `memberName` and `value` too: `value` is what the passing call's expected argument holds, as the library compared it (for F#, after `SourceLanguage.SnapshotValue`). Once an accept above a call site moves it, its line no longer names its entry and the member is the fallback. `value` narrows that fallback to an entry the value settles, one anchored to it or waiting to become it. Without `value`, a passing call can settle the entry of a failing sibling in the same member. The line can also come to name another call's entry, so one found under it that was queued from a different member is left alone unless `value` settles it. A failing re-run of a call site that has moved is recognised the same way, by its member, test and anchor, and updates its entry rather than queueing a second one beside it. * `InlineStaging.Settle(sourceFile, line, memberName)` clears what the running framework staged for a call site that now passes. `InlinePatchFile.Write` labels a patch that carries no framework with the running one, so a framework that passes does not clear what another one staged. `InlineStaging.Clear` still clears every framework's, which is what retiring a call site wants. * An absent owner is also remembered. A port found with nothing listening is taken as still unowned for ten minutes, and the sends that only tell the owner something — settle, retire, a move or delete to track, the first attempt to queue a patch — return without connecting while that stands. A refused loopback connection is not free on Windows: the firewall's stealth mode, on by default, drops the reset a closed port would answer with, so each refusal takes two seconds, and a green run settling once per inline verification was spending minutes on them. Anything that has to reach an owner probes for itself before launching a viewer, and that probe, like every listing, always connects and corrects the memory with what it finds. diff --git a/docs/mdsource/tray.source.md b/docs/mdsource/tray.source.md index 4f8b0819..9130becd 100644 --- a/docs/mdsource/tray.source.md +++ b/docs/mdsource/tray.source.md @@ -63,7 +63,7 @@ Exiting the tray writes any still-pending inline snapshots back to disk, under t "Accept all" will accept all pending moves, deletes and inline snapshots. Snapshots whose target frameworks disagree about the content are skipped rather than picked between; resolve those in the viewer. -The deletes it carries out are the ones that were pending when it began. A delete can be the last copy of a snapshot that is moving inline, so the deletes are held back, and the tray says so, when a snapshot could not be written or when a viewer that owns the queue did not answer. A delete of a file that an accepted move has written is left pending rather than carried out, and stays that way through later "Accept all"s: it is marked `!` in the menu, with the reason. It goes when it is accepted on its own, or when a test run raises the delete again. +The deletes it carries out are the ones that were pending when it began. A delete can be the last copy of a snapshot that is moving inline, so the deletes are held back, and the tray says so, when a snapshot could not be written or when a viewer that owns the queue did not answer. A delete of a file that an accepted move has written is left pending rather than carried out, and stays that way through later "Accept all"s: it is marked `~` in the menu, with the reason. It goes when it is accepted on its own, or discarded and raised by a later run. A test run raising it again does not release it unless the file has changed since the move wrote it. A long queue takes a while to accept. An open [DiffEngineViewer](/docs/viewer.md) window shows how far it has got, with each snapshot leaving the list as it lands. diff --git a/docs/mdsource/viewer.source.md b/docs/mdsource/viewer.source.md index 27f27c16..a88186dc 100644 --- a/docs/mdsource/viewer.source.md +++ b/docs/mdsource/viewer.source.md @@ -189,7 +189,7 @@ Rows that came from files follow those files. A re-run that rewrites a received The list sits in a column on the left. Drag the divider beside it to widen the column when the file names are longer than it is. When the list outgrows the window it follows the selection, keeping the selected row visible. -Row labels are the shortest thing that tells one entry from another, so hovering one fills in what it left out: the whole path, the test behind a call site, every framework behind a conflict, and the failure behind a `!`. A row with nothing to add shows no tooltip at all. +Row labels are the shortest thing that tells one entry from another, so hovering one fills in what it left out: the whole path, the test behind a call site, every framework behind a conflict, the failure behind a `!`, and why a delete marked `~` is held. A row with nothing to add shows no tooltip at all. The panes carry a scrollbar, which moves with the keys and the wheel. diff --git a/docs/tray.md b/docs/tray.md index 71dbb43e..9be642b8 100644 --- a/docs/tray.md +++ b/docs/tray.md @@ -70,7 +70,7 @@ Exiting the tray writes any still-pending inline snapshots back to disk, under t "Accept all" will accept all pending moves, deletes and inline snapshots. Snapshots whose target frameworks disagree about the content are skipped rather than picked between; resolve those in the viewer. -The deletes it carries out are the ones that were pending when it began. A delete can be the last copy of a snapshot that is moving inline, so the deletes are held back, and the tray says so, when a snapshot could not be written or when a viewer that owns the queue did not answer. A delete of a file that an accepted move has written is left pending rather than carried out, and stays that way through later "Accept all"s: it is marked `!` in the menu, with the reason. It goes when it is accepted on its own, or when a test run raises the delete again. +The deletes it carries out are the ones that were pending when it began. A delete can be the last copy of a snapshot that is moving inline, so the deletes are held back, and the tray says so, when a snapshot could not be written or when a viewer that owns the queue did not answer. A delete of a file that an accepted move has written is left pending rather than carried out, and stays that way through later "Accept all"s: it is marked `~` in the menu, with the reason. It goes when it is accepted on its own, or discarded and raised by a later run. A test run raising it again does not release it unless the file has changed since the move wrote it. A long queue takes a while to accept. An open [DiffEngineViewer](/docs/viewer.md) window shows how far it has got, with each snapshot leaving the list as it lands. diff --git a/docs/viewer.md b/docs/viewer.md index e5060551..5dd841bf 100644 --- a/docs/viewer.md +++ b/docs/viewer.md @@ -196,7 +196,7 @@ Rows that came from files follow those files. A re-run that rewrites a received The list sits in a column on the left. Drag the divider beside it to widen the column when the file names are longer than it is. When the list outgrows the window it follows the selection, keeping the selected row visible. -Row labels are the shortest thing that tells one entry from another, so hovering one fills in what it left out: the whole path, the test behind a call site, every framework behind a conflict, and the failure behind a `!`. A row with nothing to add shows no tooltip at all. +Row labels are the shortest thing that tells one entry from another, so hovering one fills in what it left out: the whole path, the test behind a call site, every framework behind a conflict, the failure behind a `!`, and why a delete marked `~` is held. A row with nothing to add shows no tooltip at all. The panes carry a scrollbar, which moves with the keys and the wheel. diff --git a/src/DiffEngine.Tests/InlineApplierBatchTests.cs b/src/DiffEngine.Tests/InlineApplierBatchTests.cs index 48506c78..d6ad2fc2 100644 --- a/src/DiffEngine.Tests/InlineApplierBatchTests.cs +++ b/src/DiffEngine.Tests/InlineApplierBatchTests.cs @@ -364,6 +364,263 @@ Task M1() public async Task AnEmptyBatchIsNoResults() => await Assert.That(InlineApplier.ApplyAll([])).IsEmpty(); + /// + /// A patch that stops being wanted while its file is waited for is not in what is written. + /// A queue owner hands a file's snapshots over together, and one discarded, or settled by a + /// test that started passing, before the write went into the source with the rest. The file + /// is what it would be had the patch never been handed over, and so is what every other + /// patch is told, the lines each moved included: whoever holds what is left of the file + /// brings it along by those. + /// + [Test] + public async Task AnUnwantedPatchIsNotInWhatIsWritten() + { + using var asked = new TempSource(Members(4)); + using var without = new TempSource(Members(4)); + var swaps = 0; + + var results = InlineApplier.ApplyAll( + [Set(asked.FullName, 0), Set(asked.FullName, 1), Set(asked.FullName, 2), Set(asked.FullName, 3)], + (temporary, destination) => + { + swaps++; + File.Replace(temporary, destination, null); + }, + _ => _ != 1); + var control = InlineApplier.ApplyAll([Set(without.FullName, 0), Set(without.FullName, 2), Set(without.FullName, 3)]); + + await Assert.That(Statuses(results)).IsEqualTo("Applied, Withdrawn, Applied, Applied"); + await Assert.That(asked.Text).IsEqualTo(without.Text); + await Assert.That(asked.Text).Contains("\"old 1\""); + await Assert.That(asked.Text).DoesNotContain("new 1"); + await Assert.That(Outcomes([results[0], results[2], results[3]])).IsEqualTo(Outcomes(control)); + await Assert.That(results[1].MovedBy).IsEqualTo(0); + await Assert.That(swaps).IsEqualTo(1); + // Every member is where the results taken in order say it is, the one left alone too + var lines = asked.Text.Split('\n'); + for (var member = 0; member < 4; member++) + { + var line = results.Aggregate(member + 3, (current, result) => result.Rebase(current)); + await Assert.That(lines[line - 1]).Contains($"void M{member}()"); + } + } + + /// + /// The same over a batch where a patch reads as it does because of one before it: the same + /// patch twice, a second patch for a call site already taken, a call site that was never + /// there. Whichever are taken back, the file and every outcome left are those of the batch + /// without them. A patch that made no edit is not asked about, since there is nothing of it + /// to leave out, and keeps the answer it had. + /// + [Test] + [Arguments("0")] + [Arguments("1")] + [Arguments("2")] + [Arguments("3")] + [Arguments("4")] + [Arguments("5")] + [Arguments("6")] + [Arguments("7")] + [Arguments("0 1")] + [Arguments("1 2")] + [Arguments("0 3")] + [Arguments("4 6 7")] + [Arguments("0 1 2 3 4 5 6 7")] + public async Task WithoutTheUnwantedIsWhatNeverHandingThemOverLeaves(string takenBack) + { + var unwanted = takenBack.Split(' ').Select(int.Parse).ToList(); + using var asked = new TempSource(Members(6)); + using var without = new TempSource(Members(6)); + + static InlinePatch[] Patches(string path) => + [ + Set(path, 0), + Set(path, 3), + // The same patch again, which finds its own literal already there + Set(path, 3), + // The call site the first one took, with different content: its anchor has gone + Set(path, 0, content: "something else\nagain"), + Set(path, 1), + // Never was in the file + Set(path, 2, anchor: "\"not in the source\""), + Set(path, 5), + Set(path, 4) + ]; + + var questions = new List(); + var results = InlineApplier.ApplyAll( + Patches(asked.FullName), + _ => + { + questions.Add(_); + return !unwanted.Contains(_); + }); + var kept = Enumerable.Range(0, 8).Where(_ => !unwanted.Contains(_)).ToList(); + var all = Patches(without.FullName); + var control = InlineApplier.ApplyAll(kept.Select(_ => all[_]).ToList()); + + await Assert.That(asked.Text).IsEqualTo(without.Text); + await Assert.That(Outcomes(kept.Select(_ => results[_]))).IsEqualTo(Outcomes(control)); + // Once each at most, and only of a patch with an edit to leave out + await Assert.That(questions.Distinct().Count()).IsEqualTo(questions.Count); + foreach (var index in unwanted) + { + if (questions.Contains(index)) + { + await Assert.That(results[index].Status).IsEqualTo(InlineApplyStatus.Withdrawn); + } + else + { + await Assert.That(results[index].Status).IsNotEqualTo(InlineApplyStatus.Applied); + await Assert.That(results[index].Status).IsNotEqualTo(InlineApplyStatus.Withdrawn); + } + } + } + + /// + /// The second of two patches for one call site makes no edit while the first is there, and + /// is the one that edits once the first is taken back. It had not been asked about, so it + /// is asked then, and left out as well when it is not wanted either. + /// + [Test] + public async Task APatchThatOnlyEditsOnceAnotherIsTakenBackIsAskedAboutToo() + { + using var file = new TempSource(Members(3)); + using var without = new TempSource(Members(3)); + var questions = new List(); + + var results = InlineApplier.ApplyAll( + [Set(file.FullName, 1), Set(file.FullName, 1), Set(file.FullName, 2)], + _ => + { + questions.Add(_); + return _ == 2; + }); + var control = InlineApplier.ApplyAll([Set(without.FullName, 2)]); + + await Assert.That(string.Join(", ", questions)).IsEqualTo("0, 2, 1"); + await Assert.That(Statuses(results)).IsEqualTo("Withdrawn, Withdrawn, Applied"); + await Assert.That(file.Text).IsEqualTo(without.Text); + await Assert.That(file.Text).Contains("\"old 1\""); + await Assert.That(Outcomes([results[2]])).IsEqualTo(Outcomes(control)); + } + + /// + /// The question is asked once the file is patched in memory and before it is written, which + /// is as late as it can be asked: the file on disk is still as it was read. + /// + [Test] + public async Task TheQuestionIsAskedBeforeAnythingIsWritten() + { + using var file = new TempSource(Members(3)); + var before = file.Text; + var onDisk = new List(); + var swaps = 0; + + InlineApplier.ApplyAll( + [Set(file.FullName, 0), Set(file.FullName, 1), Set(file.FullName, 2)], + (temporary, destination) => + { + swaps++; + File.Replace(temporary, destination, null); + }, + _ => + { + onDisk.Add($"{swaps} {file.Text == before}"); + return true; + }); + + await Assert.That(string.Join(", ", onDisk)).IsEqualTo("0 True, 0 True, 0 True"); + await Assert.That(swaps).IsEqualTo(1); + } + + /// + /// With every edit taken back there is nothing to write, and the file an editor has open is + /// not touched. + /// + [Test] + public async Task NothingWantedIsNothingWritten() + { + using var file = new TempSource(Members(2)); + var before = file.Text; + var swaps = 0; + + var results = InlineApplier.ApplyAll( + [Set(file.FullName, 0), Set(file.FullName, 1, content: "old 1"), Set(file.FullName, 1)], + (_, _) => swaps++, + _ => false); + + await Assert.That(Statuses(results)).IsEqualTo("Withdrawn, AlreadyApplied, Withdrawn"); + await Assert.That(swaps).IsEqualTo(0); + await Assert.That(file.Text).IsEqualTo(before); + } + + /// + /// A patch taken back is asked about where it was in what the caller handed over, whichever + /// file it is for, and the other files are written as they were going to be. + /// + [Test] + public async Task APatchIsAskedAboutByWhereTheCallerPutIt() + { + using var first = new TempSource(Members(2)); + using var second = new TempSource(Members(2)); + + var results = InlineApplier.ApplyAll( + [ + Set(first.FullName, 0), + Set(second.FullName, 1), + Set(first.FullName, 1), + Set(second.FullName, 0) + ], + _ => _ != 2); + + await Assert.That(Statuses(results)).IsEqualTo("Applied, Applied, Withdrawn, Applied"); + await Assert.That(first.Text).Contains("new 0"); + await Assert.That(first.Text).Contains("\"old 1\""); + await Assert.That(second.Text).Contains("new 0"); + await Assert.That(second.Text).Contains("new 1"); + } + + /// + /// A write that fails fails what it was carrying, and a patch taken back before it was not + /// among that: nothing of it was going to be written either way. + /// + [Test] + public async Task AWriteThatFailsLeavesAPatchTakenBackAsItWas() + { + using var file = new TempSource(Members(3)); + var before = file.Text; + + var results = InlineApplier.ApplyAll( + [Set(file.FullName, 0), Set(file.FullName, 1), Set(file.FullName, 2)], + (_, _) => throw new IOException("The process cannot access the file."), + _ => _ != 1); + + await Assert.That(Statuses(results)).IsEqualTo("Failed, Withdrawn, Failed"); + await Assert.That(file.Text).IsEqualTo(before); + } + + /// + /// A question that throws is taken as yes: the patch was handed over to be written, and the + /// outcomes of the patches beside it are not lost to it. + /// + [Test] + public async Task AQuestionThatThrowsIsTakenAsWanted() + { + using var file = new TempSource(Members(2)); + + var results = InlineApplier.ApplyAll( + [Set(file.FullName, 0), Set(file.FullName, 1)], + _ => throw new InvalidOperationException("the queue went away")); + + await Assert.That(Statuses(results)).IsEqualTo("Applied, Applied"); + await Assert.That(file.Text).Contains("new 1"); + } + + // Everything a host reads off a result, the lines it moved included + static string Outcomes(IEnumerable results) => + string.Join("\n", results.Select(_ => $"{_.Status} {_.MovedFrom} {_.MovedBy} {_.Message ?? "-"}")); + /// /// A batch lexes its file once and carries the scan from each patch to the next. What each /// patch is told, and what the source comes to, has to be what lexing the whole text again diff --git a/src/DiffEngine.Tests/InlineQueueTests.cs b/src/DiffEngine.Tests/InlineQueueTests.cs index 68f60b45..63ebef95 100644 --- a/src/DiffEngine.Tests/InlineQueueTests.cs +++ b/src/DiffEngine.Tests/InlineQueueTests.cs @@ -784,6 +784,24 @@ public async Task ABatchStepSkipsAnEntryReplacedWhileItApplied() await Assert.That(tally).IsEqualTo(new()); } + /// + /// A patch withdrawn as its file was about to be written is neither an accept nor a failure: + /// nothing of it was written, and nothing went wrong. The batch counts nothing for it, so it + /// holds no delete, and an entry still here under it is left as it is. + /// + [Test] + public async Task ABatchStepCountsNothingForAPatchThatWasWithdrawn() + { + var queue = InlineQueue.Empty.Enqueue(Patch()); + var tally = new AcceptAllTally(); + + var after = queue.AcceptInBatch(queue.Items.Single(), InlineApplyResult.Withdrawn, ref tally); + + await Assert.That(after).IsSameReferenceAs(queue); + await Assert.That(tally).IsEqualTo(new()); + await Assert.That(tally.Refused).IsFalse(); + } + /// /// 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 diff --git a/src/DiffEngine.Tests/ViewerLaunchGateTests.cs b/src/DiffEngine.Tests/ViewerLaunchGateTests.cs index 5b73640b..3f30028e 100644 --- a/src/DiffEngine.Tests/ViewerLaunchGateTests.cs +++ b/src/DiffEngine.Tests/ViewerLaunchGateTests.cs @@ -446,6 +446,101 @@ public async Task AFailedLaunchGivesItsSlotBackAsync() } } + /// + /// A viewer that could not show what it was started with stages it before it goes, and its + /// exit used to say only that it failed. So the caller, told that, staged the same snapshot + /// again: two trios in two VerifyInline directories until a passing run cleared both. It has + /// an exit of its own now, and the gate tells it from a failure. No window came of it either + /// way, so the slot goes back as a failed launch's does. + /// + [Test] + public async Task AViewerThatStagedWhatItWasGivenIsToldFromOneThatFailed() + { + var previous = ViewerLaunchGate.BindWait; + // Long enough that waiting it out would show in this test's duration. + ViewerLaunchGate.BindWait = TimeSpan.FromSeconds(20); + try + { + var givenBack = 0; + var elapsed = Stopwatch.StartNew(); + + var outcome = ViewerLaunchGate.Launch( + retry: () => true, + launch: () => Exited(ViewerExit.Staged), + isOwned: () => false, + canLaunch: () => true, + giveBack: () => givenBack++); + + await Assert.That(outcome).IsEqualTo(ViewerLaunchOutcome.Staged); + await Assert.That(givenBack).IsEqualTo(1); + await Assert.That(elapsed.Elapsed).IsLessThan(TimeSpan.FromSeconds(10)); + } + finally + { + ViewerLaunchGate.BindWait = previous; + } + } + + /// + [Test] + public async Task AViewerThatStagedWhatItWasGivenIsToldFromOneThatFailedAsync() + { + var previous = ViewerLaunchGate.BindWait; + ViewerLaunchGate.BindWait = TimeSpan.FromSeconds(20); + try + { + var givenBack = 0; + var elapsed = Stopwatch.StartNew(); + + var outcome = await ViewerLaunchGate.LaunchAsync( + retry: () => Task.FromResult(true), + launch: () => Task.FromResult(Exited(ViewerExit.Staged)), + Cancel.None, + isOwned: () => false, + canLaunch: () => true, + giveBack: () => givenBack++); + + await Assert.That(outcome).IsEqualTo(ViewerLaunchOutcome.Staged); + await Assert.That(givenBack).IsEqualTo(1); + await Assert.That(elapsed.Elapsed).IsLessThan(TimeSpan.FromSeconds(10)); + } + finally + { + ViewerLaunchGate.BindWait = previous; + } + } + + /// + /// The exits a viewer had before that one are failures still, each of them: 1 for an owner + /// that would not take the work, where what was staged could not be written, 2 for arguments + /// it does not know, 3 for a throw, 4 for a window that would not open with something left + /// unstaged. A caller told any of those keeps what it sent. + /// + [Test] + [Arguments(1)] + [Arguments(2)] + [Arguments(3)] + [Arguments(4)] + public async Task EveryOtherExitIsStillAFailure(int code) + { + var outcome = ViewerLaunchGate.Launch( + retry: () => true, + launch: () => Exited(code), + isOwned: () => false, + canLaunch: () => true); + + await Assert.That(outcome).IsEqualTo(ViewerLaunchOutcome.Failed); + } + + /// + /// What Verify is told of a staged launch is neither of the answers it had. Not that nobody + /// has the snapshot, which is the one answer Verify stages a trio of its own on, and not that + /// a viewer is holding it. + /// + [Test] + public async Task AStagedLaunchIsNeitherQueuedNorNowhere() => + await Assert.That(DiffRunner.InlineResultFor(ViewerLaunchOutcome.Staged)).IsEqualTo(InlineResult.Staged); + /// /// A launch that worked keeps its slot, which is what the cap counts. /// diff --git a/src/DiffEngine.Tests/ViewerProtocolTests.cs b/src/DiffEngine.Tests/ViewerProtocolTests.cs index f9a105b6..dc8ddf8a 100644 --- a/src/DiffEngine.Tests/ViewerProtocolTests.cs +++ b/src/DiffEngine.Tests/ViewerProtocolTests.cs @@ -984,6 +984,40 @@ public async Task AcceptProgressRidesOnAListing() await Assert.That(parsed!.Progress).IsEqualTo(new(3, 40)); } + /// + /// A bulk discard is listed as the counts an accept-all is, and a line beside them saying + /// which it is. A reader from before the line skips it, as it skips any name it does not + /// know, and is left with a batch under way: it words it as an accept, and refuses what + /// changes the queue until it is done, which is what matters. + /// + [Test] + public async Task ADiscardsProgressSaysItIsOneOnALineOfItsOwn() + { + var discarding = new AcceptProgress(1, 3) + { + Discarding = true + }; + var text = ViewerResponse.Listing([], progress: discarding).Build(); + + await Assert.That(text).Contains("progress: 1|3\ndiscarding: true\n"); + await Assert.That(ViewerResponse.TryParse(text, out var parsed)).IsTrue(); + await Assert.That(parsed!.Progress).IsEqualTo(discarding); + await Assert.That(parsed.Progress!.Describe()).IsEqualTo("Discarding 2 of 3"); + + // What an older reader makes of it: the same text, less the line it does not know + await Assert.That(ViewerResponse.TryParse(text.Replace("discarding: true\n", ""), out var older)).IsTrue(); + await Assert.That(older!.Progress).IsEqualTo(new(1, 3)); + + // The line can come before the counts, and with none it says nothing + await Assert.That(ViewerResponse.TryParse(text.Replace("progress: 1|3\ndiscarding: true\n", "discarding: true\nprogress: 1|3\n"), out var swapped)).IsTrue(); + await Assert.That(swapped!.Progress).IsEqualTo(discarding); + await Assert.That(ViewerResponse.TryParse(text.Replace("progress: 1|3\n", ""), out var alone)).IsTrue(); + await Assert.That(alone!.Progress).IsNull(); + + // An accept-all's listing is what it was + await Assert.That(ViewerResponse.Listing([], progress: new(1, 3)).Build()).DoesNotContain("discarding"); + } + [Test] public async Task AListingWithNoAcceptRunningSaysNothingOfProgress() { diff --git a/src/DiffEngine/DiffRunner_Inline.cs b/src/DiffEngine/DiffRunner_Inline.cs index 62537248..b623a435 100644 --- a/src/DiffEngine/DiffRunner_Inline.cs +++ b/src/DiffEngine/DiffRunner_Inline.cs @@ -19,7 +19,16 @@ public enum InlineResult /// thing from the caller's side, since in each case the snapshot is pending nowhere. Callers /// that want a fallback should use it here. /// - NoViewerFound + NoViewerFound, + + /// + /// No window is showing the snapshot, and it is staged: the viewer that was started could not + /// show it, and wrote it under a VerifyInline directory in the source project's + /// obj before it went (), where accept tooling finds + /// it. A caller that stages on has nothing to stage here, and + /// doing so would leave the one snapshot staged twice. + /// + Staged } public static partial class DiffRunner @@ -89,7 +98,7 @@ public static async Task AddInlineAsync(InlinePatch patch, Cancel async () => await ViewerClient.SendAsync(new(ViewerVerb.Inline, Body: payload), cancel) == SendOutcome.Accepted, () => ViewerLauncher.LaunchAsync(patch, payload, file, cancel), cancel); - if (launched == ViewerLaunchOutcome.Failed) + if (launched is ViewerLaunchOutcome.Failed or ViewerLaunchOutcome.Staged) { // Nothing is left that could read it: no viewer was started, or the one that was has // exited. One that got as far as reading it also deleted it, and then this finds @@ -115,11 +124,17 @@ public static async Task AddInlineAsync(InlinePatch patch, Cancel /// took nothing. That is a copy too old for the arguments this library gives it, or one with /// no runtime to run on. /// + /// + /// One that exited having staged the patch is neither. Nothing holds it in a queue, so it is + /// not Queued, and it is not pending nowhere: reported as no viewer, the caller staged it a + /// second time, in its own directory, and the two trios stood until a passing run cleared both. + /// /// internal static InlineResult InlineResultFor(ViewerLaunchOutcome outcome) => outcome switch { ViewerLaunchOutcome.Launched or ViewerLaunchOutcome.Taken => InlineResult.Queued, + ViewerLaunchOutcome.Staged => InlineResult.Staged, _ => InlineResult.NoViewerFound }; diff --git a/src/DiffEngine/Inline/InlineApplier.cs b/src/DiffEngine/Inline/InlineApplier.cs index e9cced12..9c790161 100644 --- a/src/DiffEngine/Inline/InlineApplier.cs +++ b/src/DiffEngine/Inline/InlineApplier.cs @@ -52,11 +52,47 @@ public static InlineApplyResult Apply(InlinePatch patch) => public static IReadOnlyList ApplyAll(IReadOnlyList patches) => ApplyAll(patches, Swap); + /// + /// for a caller whose patches can stop + /// being wanted while they wait: a queue owner's bulk accept, whose queue goes on being + /// settled and discarded from while a file is read, patched and written. + /// + /// A patch is handed over when its file's turn comes and written at the end of it, and the + /// wait between is the file's lock, up to ten seconds of it. A snapshot discarded in that + /// time, or settled by a test that started passing, was written with the rest of its file: + /// the reviewer threw it away and found it in the source. So once a file is patched in + /// memory, and before its one write, each patch that edited is asked about, by its position + /// in . One that is no longer wanted is not in what is written. + /// + /// + /// Not by taking its edit back out, since the patches after it were applied to source that + /// held it: the file is patched again from what was read, without it. So every outcome is + /// what it would have been had that patch never been handed over, the lines each edit moved + /// included, and is true of the file that is written. The patch itself is + /// . + /// + /// + /// The question is asked with the file's lock and mutex held, on the thread that applies. It + /// must not wait on anything that can be waiting to apply to the same file. + /// + /// + /// The patches, in the order they are to be applied. + /// + /// Whether the patch at a position is still to be written. Asked only of a patch that would + /// edit its file, and at most once. + /// + internal static IReadOnlyList ApplyAll(IReadOnlyList patches, Func wanted) => + ApplyAll(patches, Swap, wanted); + /// The patches, in the order they are to be applied. /// /// The swap. Supplied by the tests, which count how many there were and make one fail. /// - internal static IReadOnlyList ApplyAll(IReadOnlyList patches, Action replace) + /// + /// Whether the patch at a position is still to be written, or null for a caller whose patches + /// are all wanted. + /// + internal static IReadOnlyList ApplyAll(IReadOnlyList patches, Action replace, Func? wanted = null) { var results = new InlineApplyResult[patches.Count]; // Each file's patches in the order they were given, and the files in the order they were @@ -86,7 +122,10 @@ internal static IReadOnlyList ApplyAll(IReadOnlyList patches[_]).ToList(), write: true, anchorOnly: false, replace); + // Asked by where a patch is in the file's own list, and answered by where it was in + // the caller's + Func? fileWanted = wanted is null ? null : _ => wanted(indexes[_]); + var applied = Run(fullPath, indexes.Select(_ => patches[_]).ToList(), write: true, anchorOnly: false, replace, fileWanted); for (var position = 0; position < indexes.Count; position++) { results[indexes[position]] = applied[position]; @@ -181,7 +220,13 @@ static bool TryResolve(InlinePatch patch, out string fullPath, [NotNullWhen(fals /// outcome for each. A failure that is about the file rather than about a patch - a lock that /// could not be taken, a file that could not be read - is every patch's outcome. /// - static InlineApplyResult[] Run(string fullPath, IReadOnlyList patches, bool write, bool anchorOnly, Action replace) + static InlineApplyResult[] Run( + string fullPath, + IReadOnlyList patches, + bool write, + bool anchorOnly, + Action replace, + Func? wanted = null) { var normalizedPath = fullPath.ToLowerInvariant(); // A dry run of a file an earlier one read, and nothing has written since, is answered @@ -226,7 +271,7 @@ static InlineApplyResult[] Run(string fullPath, IReadOnlyList patch return All(patches, InlineApplyResult.Failed($"Timed out waiting for the inline patch mutex for: {fullPath}")); } - return LockedApply(fullPath, normalizedPath, patches, write, anchorOnly, replace); + return LockedApply(fullPath, normalizedPath, patches, write, anchorOnly, replace, wanted); } finally { @@ -249,7 +294,14 @@ static InlineApplyResult[] All(IReadOnlyList patches, InlineApplyRe return results; } - static InlineApplyResult[] LockedApply(string fullPath, string normalizedPath, IReadOnlyList patches, bool write, bool anchorOnly, Action replace) + static InlineApplyResult[] LockedApply( + string fullPath, + string normalizedPath, + IReadOnlyList patches, + bool write, + bool anchorOnly, + Action replace, + Func? wanted) { // Asked here rather than before the lock, because the swap at the end of this method takes // the path away for the instant it takes to rename over it. Asked outside, an applier @@ -317,6 +369,13 @@ static InlineApplyResult[] LockedApply(string fullPath, string normalizedPath, I var results = new InlineApplyResult[patches.Count]; var read = source; var firstToWrite = PatchInTurn(language, ref source, patches, fullPath, results); + if (wanted is not null && + firstToWrite >= 0) + { + // As late as there is: everything from here to the write is this thread's own work + firstToWrite = WithoutTheUnwanted(language, read, ref source, patches, fullPath, results, wanted, firstToWrite); + } + if (firstToWrite < 0) { return results; @@ -386,6 +445,12 @@ static void AfterAFailedWrite( { for (var index = firstToWrite; index < results.Length; index++) { + // Taken back before the write was tried, and no more in the file for its failing + if (results[index].Status == InlineApplyStatus.Withdrawn) + { + continue; + } + if (results[index].Status == InlineApplyStatus.Applied) { results[index] = failed; @@ -411,6 +476,118 @@ static void AfterAFailedWrite( } } + /// + /// Asks, of each patch whose edit the write is about to carry, whether it is still wanted, + /// and leaves the source and the outcomes as they would be had the ones that are not never + /// been handed over. Returns the first patch whose edit a write has still to carry, or -1 + /// when none is left. + /// + /// The patches are applied again, from the source as it was read, without the unwanted ones. + /// An edit cannot be taken back out on its own: each patch after it was judged against source + /// that held it, at a line it had moved, and may have been already applied only because of + /// it, or not found only because it had taken the anchor. Applied again, each is told what is + /// true of the file that will be written, and says which lines it moved in that file. + /// + /// + /// Which can make an edit of a patch that had made none, the second of two for one call site + /// when the first is taken back. That one has not been asked about, so it is asked, and the + /// file patched once more if it is unwanted too. Each patch is asked once, so this ends. + /// + /// + /// The language the source is in. + /// The source as it was read. + /// The patched source, replaced when a patch is taken back out. + /// The patches, in the order they are to be applied. + /// The file, for a failure to name. + /// The outcomes, replaced when a patch is taken back out. + /// Whether the patch at a position is still to be written. + /// The first patch whose edit the write has to carry. + static int WithoutTheUnwanted( + SourceLanguage language, + string read, + ref string source, + IReadOnlyList patches, + string fullPath, + InlineApplyResult[] results, + Func wanted, + int firstToWrite) + { + var asked = new bool[results.Length]; + var withdrawn = new bool[results.Length]; + while (firstToWrite >= 0) + { + var found = false; + for (var index = firstToWrite; index < results.Length; index++) + { + if (asked[index] || + results[index].Status != InlineApplyStatus.Applied) + { + continue; + } + + asked[index] = true; + if (!IsWanted(wanted, index)) + { + withdrawn[index] = true; + found = true; + } + } + + if (!found) + { + break; + } + + var kept = new List(results.Length); + for (var index = 0; index < results.Length; index++) + { + if (withdrawn[index]) + { + results[index] = InlineApplyResult.Withdrawn; + } + else + { + kept.Add(index); + } + } + + var keptPatches = new InlinePatch[kept.Count]; + for (var position = 0; position < keptPatches.Length; position++) + { + keptPatches[position] = patches[kept[position]]; + } + + var keptResults = new InlineApplyResult[kept.Count]; + source = read; + var first = PatchInTurn(language, ref source, keptPatches, fullPath, keptResults); + for (var position = 0; position < keptResults.Length; position++) + { + results[kept[position]] = keptResults[position]; + } + + firstToWrite = first < 0 ? -1 : kept[first]; + } + + return firstToWrite; + } + + /// + /// A question that throws is taken as yes. The patch was handed over to be written, and + /// nothing has said otherwise; thrown on from here, the outcomes of the patches beside it + /// would be lost with it. + /// + static bool IsWanted(Func wanted, int index) + { + try + { + return wanted(index); + } + catch (Exception) + { + return true; + } + } + /// /// Applies the patches of one file to its source in memory, each to what the one before it /// left, and says what became of each. Returns the first patch whose edit a write has to diff --git a/src/DiffEngine/Inline/InlineApplyResult.cs b/src/DiffEngine/Inline/InlineApplyResult.cs index 1c8f217c..57f3b583 100644 --- a/src/DiffEngine/Inline/InlineApplyResult.cs +++ b/src/DiffEngine/Inline/InlineApplyResult.cs @@ -20,7 +20,19 @@ public enum InlineApplyStatus /// /// IO, locking, or validation failure. See . /// - Failed + Failed, + + /// + /// The patch stopped being wanted while it was waiting to be written, and nothing of it is in + /// the file. Not a failure and not an accept: whoever asked for it had already taken it back. + /// + /// Only from an apply that was handed a question to ask, which is a queue owner's bulk accept: + /// a snapshot discarded, or settled by a test that started passing, after its patch was + /// claimed and before its file was written. and + /// never report it. + /// + /// + Withdrawn } public sealed class InlineApplyResult @@ -78,6 +90,9 @@ internal static InlineApplyResult AppliedMoving(int from, int by) => public static readonly InlineApplyResult Applied = new(InlineApplyStatus.Applied, null, null); public static readonly InlineApplyResult AlreadyApplied = new(InlineApplyStatus.AlreadyApplied, null, null); + /// + internal static readonly InlineApplyResult Withdrawn = new(InlineApplyStatus.Withdrawn, null, null); + public static InlineApplyResult NotFound(string message) => new(InlineApplyStatus.NotFound, message, null); diff --git a/src/DiffEngine/Inline/InlineQueue.cs b/src/DiffEngine/Inline/InlineQueue.cs index 2c4051ff..9b0c9668 100644 --- a/src/DiffEngine/Inline/InlineQueue.cs +++ b/src/DiffEngine/Inline/InlineQueue.cs @@ -658,9 +658,21 @@ public InlineQueue AcceptAll( /// is this, once per outcome. An entry that changed while its patch was applying is left alone /// and not counted, found by its variants the way the two phase accept finds it. /// + /// + /// Nor is one whose patch was withdrawn (): the host + /// said, as the file was about to be written, that the entry was no longer the one it had + /// claimed, so nothing of it was written and there is no accept or failure to count. The + /// entry is normally gone by now, which is why it was unwanted. One that is here all the + /// same is left as it is. + /// /// internal InlineQueue AcceptInBatch(PendingInline entry, InlineApplyResult result, ref AcceptAllTally tally) { + if (result.Status == InlineApplyStatus.Withdrawn) + { + return this; + } + var items = Items.ToList(); var index = items.FindIndex(_ => ReferenceEquals(_.Variants, entry.Variants)); if (index < 0) diff --git a/src/DiffEngine/Protocol/AcceptProgress.cs b/src/DiffEngine/Protocol/AcceptProgress.cs index ccc45811..e9d9040e 100644 --- a/src/DiffEngine/Protocol/AcceptProgress.cs +++ b/src/DiffEngine/Protocol/AcceptProgress.cs @@ -18,12 +18,26 @@ namespace DiffEngine; /// record AcceptProgress(int Done, int Total) { + /// + /// Whether the batch is a bulk discard rather than an accept-all: the same steps, each one + /// throwing a received file away. + /// + /// An owning viewer's discard was on no listing, since the progress line says an accept is + /// under way to whoever reads it. So a window attached to that viewer saw the queue shrink + /// with nothing saying why, and refused nothing meanwhile. It is listed now, as the same + /// counts and a line of its own beside them (). A reader from + /// before that line skips it and takes the batch for an accept, which is the wrong word and + /// the right behaviour: it refuses what changes the queue until the batch is done. + /// + /// + public bool Discarding { get; init; } + /// /// The entry being worked on rather than the count finished, which is how a progress line /// reads: the first entry is "1 of 40" while it is being applied, not "0 of 40". /// public string Describe() => - $"Accepting {Math.Min(Done + 1, Total)} of {Total}"; + $"{(Discarding ? "Discarding" : "Accepting")} {Math.Min(Done + 1, Total)} of {Total}"; /// /// One more entry dealt with, however it went. diff --git a/src/DiffEngine/Protocol/ViewerExit.cs b/src/DiffEngine/Protocol/ViewerExit.cs new file mode 100644 index 00000000..2b34fb02 --- /dev/null +++ b/src/DiffEngine/Protocol/ViewerExit.cs @@ -0,0 +1,32 @@ +namespace DiffEngine; + +/// +/// What a viewer's exit code says to the process that started it. +/// +/// Here, beside the wire, because it is the same kind of agreement between the same two parties: +/// ViewerLaunchGate reads what ViewerProgram returns, and the copy of the viewer +/// that resolves is not always the one this library was built beside. Neither is named as a +/// reference, since this file is compiled into both and each has only its own half. +/// +/// +static class ViewerExit +{ + /// + /// The viewer could not show what it was started with, and every inline snapshot it held is + /// staged where accept tooling finds it (). So whoever + /// started it has nothing to stage itself. + /// + /// A failure all the same, which is what makes it safe between versions. A viewer from before + /// it never returns it, and its caller stages as it always did. A library from before it reads + /// it as it reads any exit that is not zero, as a launch that failed, and stages a second trio + /// beside the viewer's: what both did before there was a code for it. So no copy has to be + /// passed over for this, and ViewerContract asks for nothing more. + /// + /// + /// Only ever the answer to a launch with an inline patch. A viewer started for a delete or a + /// pair that could not open its window has not dealt with what it was given by staging + /// someone else's snapshots, and says it failed. + /// + /// + public const int Staged = 5; +} diff --git a/src/DiffEngine/Protocol/ViewerResponse.cs b/src/DiffEngine/Protocol/ViewerResponse.cs index 33102b91..3daa5c69 100644 --- a/src/DiffEngine/Protocol/ViewerResponse.cs +++ b/src/DiffEngine/Protocol/ViewerResponse.cs @@ -100,7 +100,8 @@ record ViewerResponse( /// /// How far the accept-all the owner is running has got, on a listing taken while one is, and /// null the rest of the time. A reader that predates it skips the line, as it skips any name it - /// does not know. + /// does not know. A bulk discard's too, with a discarding line beside the counts + /// (). /// public AcceptProgress? Progress { get; init; } @@ -170,6 +171,13 @@ public string Build() { // Plain too: two counts builder.Append($"progress: {Progress.Build()}\n"); + if (Progress.Discarding) + { + // A line of its own rather than a third count, which a reader that predates it + // would refuse the whole listing over. That reader skips this, and is left with + // a batch under way + builder.Append("discarding: true\n"); + } } if (Written is { } written) @@ -249,6 +257,7 @@ public static bool TryParse(string text, [NotNullWhen(true)] out ViewerResponse? WindowCommand? window = null; string? windowKey = null; AcceptProgress? progress = null; + var discarding = false; bool? written = null; string? tag = null; var unchanged = false; @@ -286,6 +295,9 @@ public static bool TryParse(string text, [NotNullWhen(true)] out ViewerResponse? return false; } + continue; + case "discarding": + discarding = value == "true"; continue; case "written": written = value == "true"; @@ -436,7 +448,11 @@ public static bool TryParse(string text, [NotNullWhen(true)] out ViewerResponse? { Moves = moves, Deletes = deletes, - Progress = progress, + // After the loop, so the line does not have to follow the counts it is about. With + // no counts it says nothing + Progress = discarding && progress is not null + ? progress with { Discarding = true } + : progress, Written = written, Tag = tag, Unchanged = unchanged diff --git a/src/DiffEngine/Viewer/ViewerLaunchGate.cs b/src/DiffEngine/Viewer/ViewerLaunchGate.cs index 4edbbbac..4db3e13a 100644 --- a/src/DiffEngine/Viewer/ViewerLaunchGate.cs +++ b/src/DiffEngine/Viewer/ViewerLaunchGate.cs @@ -26,7 +26,15 @@ enum ViewerLaunchOutcome /// Nobody was there to take it and had no slot left, so nothing was /// started. /// - Capped + Capped, + + /// + /// What was started could not show what it was given, and staged it before it went + /// (). No window came of it, as with , and + /// unlike that the work is somewhere: on disk, where accept tooling finds it. A caller that + /// stages what nobody took has nothing to stage. + /// + Staged } /// @@ -64,7 +72,8 @@ enum ViewerLaunchOutcome /// already up is not a new instance and spends nothing, which is why the caller cannot ask: it /// would charge all twenty of the callers above for the one window between them. Asked after the /// ownership probe, so the nineteen that find an owner still forward their work when the cap is -/// long since reached. And given back when the launch fails, since no window came of it. +/// long since reached. And given back when the launch fails, or when the viewer staged what it +/// was given and went, since no window came of either. /// /// static class ViewerLaunchGate @@ -118,14 +127,12 @@ public static ViewerLaunchOutcome Launch( isOwned ??= () => ViewerClient.IsOwned(); giveBack ??= canLaunch is null ? MaxInstance.GiveBack : () => { }; canLaunch ??= () => !MaxInstance.Reached(); - bool owned; gate.Wait(); try { // Asked rather than sent, so the decision to launch costs a connect rather than a // round trip with a payload on it. - owned = isOwned(); - if (!owned) + if (!isOwned()) { if (!canLaunch()) { @@ -133,12 +140,16 @@ public static ViewerLaunchOutcome Launch( } using var viewer = launch(); - if (viewer is null || - !WaitForBind(viewer, isOwned)) + var waited = viewer is null + ? ViewerLaunchOutcome.Failed + : WaitForBind(viewer, isOwned); + if (waited != ViewerLaunchOutcome.Launched) { + // Staged or failed, no window came of it giveBack(); - return ViewerLaunchOutcome.Failed; } + + return waited; } } finally @@ -146,11 +157,6 @@ public static ViewerLaunchOutcome Launch( gate.Release(); } - if (!owned) - { - return ViewerLaunchOutcome.Launched; - } - return retry() ? ViewerLaunchOutcome.Taken : ViewerLaunchOutcome.Failed; } @@ -176,12 +182,10 @@ public static async Task LaunchAsync( isOwned ??= () => ViewerClient.IsOwned(); giveBack ??= canLaunch is null ? MaxInstance.GiveBack : () => { }; canLaunch ??= () => !MaxInstance.Reached(); - bool owned; await gate.WaitAsync(cancel).ConfigureAwait(false); try { - owned = isOwned(); - if (!owned) + if (!isOwned()) { if (!canLaunch()) { @@ -189,12 +193,15 @@ public static async Task LaunchAsync( } using var viewer = await Task.Run(launch, cancel).ConfigureAwait(false); - if (viewer is null || - !await WaitForBindAsync(viewer, isOwned, cancel).ConfigureAwait(false)) + var waited = viewer is null + ? ViewerLaunchOutcome.Failed + : await WaitForBindAsync(viewer, isOwned, cancel).ConfigureAwait(false); + if (waited != ViewerLaunchOutcome.Launched) { giveBack(); - return ViewerLaunchOutcome.Failed; } + + return waited; } } finally @@ -202,11 +209,6 @@ public static async Task LaunchAsync( gate.Release(); } - if (!owned) - { - return ViewerLaunchOutcome.Launched; - } - return await retry() ? ViewerLaunchOutcome.Taken : ViewerLaunchOutcome.Failed; } @@ -216,61 +218,71 @@ public static async Task LaunchAsync( /// the launch all the same, because it did happen: the work went over on the command line or /// in a payload file, and the cost of giving up early is one more viewer, which is where this began. /// - /// False when the viewer gave up first, which is the one launch that did not happen. It used to - /// be waited on for the whole of with the gate held and then reported + /// Failed when the viewer gave up first, which is the one launch that did not happen. It used + /// to be waited on for the whole of with the gate held and then reported /// like any other, so an inline snapshot was said to be queued when it was nowhere: not in a /// queue, and not staged either, since a caller stages only what it is told nobody took. /// + /// + /// Staged when it gave up having staged what it held. That was Failed as well, and the caller + /// staged the same snapshot again beside it. + /// /// - static bool WaitForBind(Process viewer, Func isOwned) + static ViewerLaunchOutcome WaitForBind(Process viewer, Func isOwned) { var elapsed = Stopwatch.StartNew(); while (elapsed.Elapsed < BindWait) { // Before the probe, so that an owner some other process started is not taken for the // viewer this one did: that owner was never handed the work - if (GaveUp(viewer)) + if (GaveUp(viewer) is { } how) { - return false; + return how; } if (isOwned()) { - return true; + return ViewerLaunchOutcome.Launched; } Thread.Sleep(poll); } - return true; + return ViewerLaunchOutcome.Launched; } /// - static async Task WaitForBindAsync(Process viewer, Func isOwned, Cancel cancel) + static async Task WaitForBindAsync(Process viewer, Func isOwned, Cancel cancel) { var elapsed = Stopwatch.StartNew(); while (elapsed.Elapsed < BindWait) { - if (GaveUp(viewer)) + if (GaveUp(viewer) is { } how) { - return false; + return how; } if (isOwned()) { - return true; + return ViewerLaunchOutcome.Launched; } await Task.Delay(poll, cancel).ConfigureAwait(false); } - return true; + return ViewerLaunchOutcome.Launched; } /// - /// Whether the viewer this call started has exited and said it failed, so it took nothing and - /// never will. A copy from before the arguments it was given exits on the first it does not - /// know, and an apphost with no runtime to run exits before any of the viewer's own code. + /// How the viewer this call started gave up, where it has exited and said it failed, so it + /// took nothing and never will: null while it has not. A copy from before the arguments it was + /// given exits on the first it does not know, and an apphost with no runtime to run exits + /// before any of the viewer's own code. + /// + /// One exit is told from the rest (): the viewer could not + /// show what it held and staged it. Only a viewer says that, so an older copy and an apphost + /// that never reached the viewer's code are failures, as they were. + /// /// /// A clean exit is not this, and is left to the wait. A viewer that finds the port already /// bound hands its work to whoever holds it and exits with zero, and the next probe finds that @@ -282,17 +294,24 @@ static async Task WaitForBindAsync(Process viewer, Func isOwned, Can /// wait as it was before there was a process to ask. /// /// - static bool GaveUp(Process viewer) + static ViewerLaunchOutcome? GaveUp(Process viewer) { try { - return viewer.HasExited && - viewer.ExitCode != 0; + if (!viewer.HasExited || + viewer.ExitCode == 0) + { + return null; + } + + return viewer.ExitCode == ViewerExit.Staged + ? ViewerLaunchOutcome.Staged + : ViewerLaunchOutcome.Failed; } catch (Exception exception) when (exception is InvalidOperationException or System.ComponentModel.Win32Exception or NotSupportedException) { - return false; + return null; } } diff --git a/src/DiffEngineTray.Tests/KeyNameTests.cs b/src/DiffEngineTray.Tests/KeyNameTests.cs index 0193e078..f3c9a025 100644 --- a/src/DiffEngineTray.Tests/KeyNameTests.cs +++ b/src/DiffEngineTray.Tests/KeyNameTests.cs @@ -74,19 +74,22 @@ public async Task Rejects_nothing() [Test] public async Task A_bad_key_leaves_the_hot_key_unbound() { - using var register = new KeyRegister(0); - - // Nothing is registered with the OS for a key that is not one, so the handle above is - // never used and no hot key is taken from the machine running this - var bound = register.TryAddBinding( - KeyBindingIds.AcceptAll, - shift: true, - control: false, - alt: false, - "Ctrl+A", - () => throw new("Not bound, so never invoked")); + var desktop = new Desktop(); + bool bound; + using (var register = desktop.Register()) + { + bound = register.TryAddBinding( + KeyBindingIds.AcceptAll, + shift: true, + control: false, + alt: false, + "Ctrl+A", + () => throw new("Not bound, so never invoked")); + } await Assert.That(bound).IsFalse(); + // A key that is not one is never taken as far as the desktop + await Assert.That(desktop.Asked).IsEmpty(); } /// @@ -105,12 +108,36 @@ public async Task A_bad_key_does_not_stop_the_tray_starting() } }; await using var tracker = new RecordingTracker(); - using var register = new KeyRegister(0); + var desktop = new Desktop(); var warnings = new List(); - Program.ReBindKeys(settings, register, tracker, warnings.Add); + using (var register = desktop.Register()) + { + Program.ReBindKeys(settings, register, tracker, warnings.Add); + } await Assert.That(warnings).HasSingleItem(); await Assert.That(warnings[0]).Contains("Ctrl+A"); + await Assert.That(desktop.Asked).IsEmpty(); + } + + /// + /// What a register asks of the desktop, answered here and written down. These tests bind only + /// names that are no key, which stop before anything is asked, but a register built on the + /// real desktop would take a hot key from the whole machine the day one of them changed. + /// + class Desktop + { + public List<(int id, KeyModifiers modifiers, Keys key)> Asked { get; } = []; + + public KeyRegister Register() => + new( + IntPtr.Zero, + (_, id, modifiers, key) => + { + Asked.Add((id, modifiers, key)); + return true; + }, + (_, _) => true); } } diff --git a/src/DiffEngineTray.Tests/MenuBuilderTest.cs b/src/DiffEngineTray.Tests/MenuBuilderTest.cs index 16fd1074..0a318d12 100644 --- a/src/DiffEngineTray.Tests/MenuBuilderTest.cs +++ b/src/DiffEngineTray.Tests/MenuBuilderTest.cs @@ -138,7 +138,8 @@ public async Task ADeleteAcceptAllWouldKeepCarriesItsReason() .OfType() .ToList(); var held = deletes.Single(_ => _.Text!.StartsWith("Sample.Test.verified.txt")); - await Assert.That(held.Text).IsEqualTo("Sample.Test.verified.txt !"); + // Its own mark, and not the one a snapshot that failed to apply carries + await Assert.That(held.Text).IsEqualTo("Sample.Test.verified.txt ~"); await Assert.That(held.ToolTipText).IsEqualTo(Tracker.WroteItsFile); var label = held.DropDownItems .OfType() diff --git a/src/DiffEngineTray.Tests/OwnedInlineHostTest.cs b/src/DiffEngineTray.Tests/OwnedInlineHostTest.cs index 19780af3..aa42c006 100644 --- a/src/DiffEngineTray.Tests/OwnedInlineHostTest.cs +++ b/src/DiffEngineTray.Tests/OwnedInlineHostTest.cs @@ -1098,12 +1098,13 @@ public async Task AnEntryDiscardedDuringAnAcceptAllIsNotWritten() /// /// The turn is a file's, because a file's snapshots are written together in one write. So one - /// discarded while its own file is being written had already been handed over with the rest - /// of the file, and is written with them. It is gone from the queue as it was asked to be, and - /// is not counted as accepted: the outcome is of an entry that is no longer there. + /// discarded while its own file is waited for had already been handed over with the rest of + /// the file, and was written with them: gone from the queue as it was asked to be, and in the + /// source all the same. Each is asked about as the file is about to be written now, and one + /// that is no longer queued is left out. It is not counted as accepted, as it never was. /// [Test] - public async Task AnEntryDiscardedWhileItsOwnFileIsWrittenIsNotCounted() + public async Task AnEntryDiscardedWhileItsOwnFileIsWaitedForIsNotWritten() { using var held = new HeldApply(1); var applied = new List(); @@ -1126,11 +1127,131 @@ public async Task AnEntryDiscardedWhileItsOwnFileIsWrittenIsNotCounted() held.Release(); var response = await accepting; - await Assert.That(applied.Count).IsEqualTo(2); + await Assert.That(applied).HasSingleItem(); + await Assert.That(applied[0].LineHint).IsEqualTo(1); await Assert.That(response.Message).IsEqualTo("Accepted 1"); await Assert.That(owner.Send(new(ViewerVerb.ListFull)).Items).IsEmpty(); } + /// + /// A test that started passing settles its entry, and the source already holds what it + /// passes with. The patch claimed for it would have put the failing run's content over that. + /// + [Test] + public async Task AnEntrySettledWhileItsOwnFileIsWaitedForIsNotWritten() + { + using var held = new HeldApply(1); + var applied = new List(); + using var owner = new Owner( + patch => + { + lock (applied) + { + applied.Add(patch); + } + + return held.Apply(patch); + }); + owner.Queue(line: 1); + owner.Queue(line: 2); + owner.Queue(line: 3); + + var accepting = Task.Run(() => owner.Send(new(ViewerVerb.AcceptAll), TimeSpan.FromSeconds(30))); + held.WaitUntilHeld(); + owner.Send(new(ViewerVerb.Settle, InlineKey.For(@"c:\repo\SampleTests.cs", 2))); + held.Release(); + var response = await accepting; + + await Assert.That(applied.Select(_ => _.LineHint)).IsEquivalentTo([1, 3]); + await Assert.That(response.Message).IsEqualTo("Accepted 2"); + await Assert.That(owner.Send(new(ViewerVerb.ListFull)).Items).IsEmpty(); + } + + /// + /// One a re-run replaced while its file was waited for is no longer the entry that was + /// claimed either. The content it was claimed with is stale, and it used to be written and + /// then not recorded. It keeps what the re-run sent, still pending. + /// + [Test] + public async Task AnEntryReplacedWhileItsOwnFileIsWaitedForIsNotWritten() + { + using var held = new HeldApply(1); + var applied = new List(); + using var owner = new Owner( + patch => + { + lock (applied) + { + applied.Add(patch); + } + + return held.Apply(patch); + }); + owner.Queue(line: 1); + owner.Queue(line: 2, content: "first run"); + + var accepting = Task.Run(() => owner.Send(new(ViewerVerb.AcceptAll), TimeSpan.FromSeconds(30))); + held.WaitUntilHeld(); + owner.Queue(line: 2, content: "second run"); + held.Release(); + var response = await accepting; + + await Assert.That(applied).HasSingleItem(); + await Assert.That(response.Message).IsEqualTo("Accepted 1"); + var left = owner.Host.Queued().Single(); + await Assert.That(left.Patch.NewContent).IsEqualTo("second run"); + await Assert.That(left.Status).IsNull(); + } + + /// + /// Against a real file: the one discarded while the first of its file was being written is + /// not in the source afterwards, and the one accepted is. + /// + [Test] + public async Task AnEntryDiscardedWhileItsFileIsWaitedForIsNotInTheSource() + { + var directory = Path.Combine(Path.GetTempPath(), $"OwnedInlineHostTest_{Guid.NewGuid():N}"); + Directory.CreateDirectory(directory); + try + { + var source = Path.Combine(directory, "SampleTests.cs"); + await File.WriteAllTextAsync( + source, + """ + class C + { + void One() => Verify(value).Snapshot("old"); + void Two() => Verify(value).Snapshot("old"); + } + """); + using var held = new HeldApply(1); + using var owner = new Owner( + patch => + { + held.Apply(patch); + return InlineApplier.Apply(patch); + }); + owner.Queue(source, 3, content: "one"); + owner.Queue(source, 4, content: "two"); + + var accepting = Task.Run(() => owner.Send(new(ViewerVerb.AcceptAll), TimeSpan.FromSeconds(30))); + held.WaitUntilHeld(); + owner.Send(new(ViewerVerb.Discard, InlineKey.For(source, 4))); + held.Release(); + var response = await accepting; + + await Assert.That(response.Message).IsEqualTo("Accepted 1"); + var written = await File.ReadAllTextAsync(source); + await Assert.That(written).Contains("One() => Verify(value).Snapshot(\"one\")"); + await Assert.That(written).Contains("Two() => Verify(value).Snapshot(\"old\")"); + await Assert.That(owner.Send(new(ViewerVerb.ListFull)).Items).IsEmpty(); + } + finally + { + Directory.Delete(directory, true); + } + } + /// /// The menu's accept-all reaches the queue through the tray's own host rather than the wire, /// and a viewer displaying the queue follows that one the same way. diff --git a/src/DiffEngineTray.Tests/TrackerMoveOntoDeleteTest.cs b/src/DiffEngineTray.Tests/TrackerMoveOntoDeleteTest.cs index 60e94bbc..971acefd 100644 --- a/src/DiffEngineTray.Tests/TrackerMoveOntoDeleteTest.cs +++ b/src/DiffEngineTray.Tests/TrackerMoveOntoDeleteTest.cs @@ -157,18 +157,68 @@ public async Task AHeldDeleteIsCarriedOutWhenAcceptedOnItsOwn() } /// - /// A test run that raises the delete again has looked at the file the move wrote and still - /// says nothing produces it. That is the later statement, and the next sweep carries it out. + /// A delete raised again says nothing about when it was decided. A run has a process for each + /// target framework, and one that looked at the file before the move was accepted raises the + /// delete after it, in the same words a run that looked at what the move wrote would use. + /// Raising it again used to let go of the hold, so that process cost the snapshot the move + /// had just put there at the next "Accept all". /// [Test] - public async Task ADeleteRaisedAgainAfterTheWriteIsCarriedOut() + public async Task ADeleteRaisedAgainOverTheFileTheMoveLeftIsStillHeld() { await using var tracker = new RecordingTracker(); + ITrackedFiles tracked = tracker; tracker.AddMove(received, verified, "theExe", "theArguments", true, null); var delete = tracker.AddDelete(verified); await tracker.AcceptAll(); + var before = tracked.Version(); + + tracker.AddDelete(verified); + await Assert.That(tracker.HeldReason(delete)).IsEqualTo(Tracker.WroteItsFile); + // Nothing a listing carries has changed + await Assert.That(tracked.Version()).IsEqualTo(before); + await tracker.AcceptAll(); + + await Assert.That(await File.ReadAllTextAsync(verified)).IsEqualTo("received"); + await Assert.That(tracker.Deletes).HasSingleItem(); + } + /// + /// The hold keeps what the move put there. Once the file is seen to be something else, there + /// is nothing of the move's left for it to keep, and a delete raised then is carried out. + /// + [Test] + public async Task ADeleteRaisedAgainOverAFileWrittenSinceIsCarriedOut() + { + await using var tracker = new RecordingTracker(); + tracker.AddMove(received, verified, "theExe", "theArguments", true, null); + var delete = tracker.AddDelete(verified); + await tracker.AcceptAll(); + await File.WriteAllTextAsync(verified, "written by something else since"); + + tracker.AddDelete(verified); + await Assert.That(tracker.HeldReason(delete)).IsNull(); + await tracker.AcceptAll(); + + await Assert.That(File.Exists(verified)).IsFalse(); + await tracker.AssertEmpty(); + } + + /// + /// Discarded and raised afresh, it is a delete like any other: the hold was the tracked + /// delete's, and that one has gone. + /// + [Test] + public async Task AHeldDeleteDiscardedAndRaisedAfreshIsCarriedOut() + { + await using var tracker = new RecordingTracker(); + ITrackedFiles tracked = tracker; + tracker.AddMove(received, verified, "theExe", "theArguments", true, null); tracker.AddDelete(verified); + await tracker.AcceptAll(); + tracked.Discard(TrackedKeys.ForDelete(verified)); + + var delete = tracker.AddDelete(verified); await Assert.That(tracker.HeldReason(delete)).IsNull(); await tracker.AcceptAll(); @@ -196,6 +246,72 @@ public async Task ADeleteWaitingOnAMoveIsCarriedOutOnceTheMoveIsDiscarded() await tracker.AssertEmpty(); } + /// + /// An accept takes its move out of the pending ones for as long as the move takes, and the + /// delete on its target was held by nothing meanwhile: not by a pending move, which had left, + /// and not by a write, which had yet to be marked. Asked from where a refused move is reported, + /// which is inside the accept, before the move is put back. + /// + [Test] + public async Task ADeleteIsHeldWhileTheMoveOntoItsFileIsBeingAccepted() + { + RecordingTracker? tracker = null; + TrackedDelete? delete = null; + var pendingMoves = -1; + string? held = null; + string? listed = null; + tracker = new( + acceptFailed: _ => + { + pendingMoves = tracker!.Moves.Count; + held = tracker.HeldReason(delete!); + listed = ((ITrackedFiles) tracker).Deletes().Single().Held; + }); + await using var disposing = tracker; + var move = tracker.AddMove(received, verified, "theExe", "theArguments", true, null); + delete = tracker.AddDelete(verified); + // A target that cannot be written, so the move is refused, and said to be, without a wait + File.SetAttributes(verified, FileAttributes.ReadOnly); + try + { + tracker.Accept(move); + } + finally + { + File.SetAttributes(verified, FileAttributes.Normal); + } + + await Assert.That(pendingMoves).IsEqualTo(0); + await Assert.That(held).IsEqualTo(Tracker.AwaitsItsFile); + await Assert.That(listed).IsEqualTo(Tracker.AwaitsItsFile); + // Put back, and held for the move that is pending again + await Assert.That(tracker.Moves).HasSingleItem(); + await Assert.That(tracker.HeldReason(delete)).IsEqualTo(Tracker.AwaitsItsFile); + } + + /// + /// A move with nothing left to move is dropped having written nothing, and from then on its + /// delete is held by nothing. The listing's tag has to move for that, since the delete is the + /// same object it was while the move was being accepted. + /// + [Test] + public async Task AMoveDroppedWhileBeingAcceptedLetsGoOfItsDelete() + { + await using var tracker = new RecordingTracker(); + ITrackedFiles tracked = tracker; + var move = tracker.AddMove(received, verified, "theExe", "theArguments", true, null); + var delete = tracker.AddDelete(verified); + File.Delete(received); + var before = tracked.Version(); + + tracker.Accept(move); + + await Assert.That(tracker.Moves).IsEmpty(); + await Assert.That(tracker.HeldReason(delete)).IsNull(); + await Assert.That(tracked.Deletes().Single().Held).IsNull(); + await Assert.That(tracked.Version()).IsNotEqualTo(before); + } + /// /// Where the reason is read by somebody who was not looking when the balloon went by: beside /// the delete in the debug view, as it is in the menu. diff --git a/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs b/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs index 51f4aff5..dd439589 100644 --- a/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs +++ b/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs @@ -13,6 +13,7 @@ // The viewer's own half of the app. using CommandKind = viewer::CommandKind; using OwnerLink = viewer::OwnerLink; +using QueueProjection = viewer::QueueProjection; using SessionHost = viewer::SessionHost; using SessionMessageHandler = viewer::MessageHandler; using SessionState = viewer::SessionState; @@ -215,7 +216,7 @@ public async Task ViewerAcceptAllKeepsTheFileATrackedMoveJustWrote() /// and the window attached to it showed that delete like any other, with "1 kept" for an /// answer when it asked for an accept-all. The hold rides the listing, and is the entry's /// status: why while the move is pending, why once it has been accepted, and nothing once a - /// run raises the delete again - a change to a tracked object that is still the same object, + /// run raises the delete again over a file written since - a change to a tracked object that is still the same object, /// which the listing's tag has to move for all the same. /// [Test] @@ -234,12 +235,64 @@ public async Task AnAttachedViewerSaysWhyTheTrayHoldsADelete() var viewer = pair.Pump(); await Assert.That(viewer.Message).IsEqualTo($"Accepted 0, plus 1 files (1 kept). {Tracker.DeletesKept([delete])}"); await Assert.That(viewer.Queue.Single().Status).IsEqualTo(Tracker.WroteItsFile); + // Marked as held, where the tray's words for why were all the window had and drew the + // row as a failure. The tray's menu marks it with the same character (MenuBuilderTest) + var row = QueueProjection.Rows(viewer).Single(); + await Assert.That(row.Label).StartsWith(QueueProjection.HeldMark); + await Assert.That(row.Status).IsNull(); + await Assert.That(row.Tooltip!).Contains(Tracker.WroteItsFile); + await Assert.That(MenuBuilder.HeldMark.Trim()).IsEqualTo(QueueProjection.HeldMark.Trim()); + + // Raised again over the file as the move left it, which a process that decided before + // the move was accepted does too: still held + pair.Tracker.AddDelete(move.Target); + + await Assert.That(pair.Pump().Queue.Single().Status).IsEqualTo(Tracker.WroteItsFile); + await File.WriteAllTextAsync(move.Target, "written by something else since"); pair.Tracker.AddDelete(move.Target); await Assert.That(pair.Pump().Queue.Single().Status).IsNull(); } + /// + /// A listing taken while the tray has a move out being accepted. The move is not among the + /// pending ones and has yet to say it wrote the file, and the listing said the delete on its + /// target was held by nothing: a window attached to the tray then sent that delete's key in a + /// group accept, after the move had written the file. + /// + [Test] + public async Task AListingTakenWhileAMoveIsBeingAcceptedHoldsItsDelete() + { + TrayOwned? owned = null; + string? held = null; + var listedMoves = -1; + owned = new( + acceptFailed: _ => + { + var listing = owned!.Send(new(ViewerVerb.ListFull)); + listedMoves = listing.Moves.Count; + held = listing.Deletes.Single().Held; + }); + await using var pair = owned; + var move = pair.AddMove(); + await File.WriteAllTextAsync(move.Target, "verified"); + pair.Tracker.AddDelete(move.Target); + // A target that cannot be written, so the move is refused, and said to be, without a wait + File.SetAttributes(move.Target, FileAttributes.ReadOnly); + try + { + pair.Tracker.Accept(pair.Tracker.Moves.Single()); + } + finally + { + File.SetAttributes(move.Target, FileAttributes.Normal); + } + + await Assert.That(listedMoves).IsEqualTo(0); + await Assert.That(held).IsEqualTo(Tracker.AwaitsItsFile); + } + /// /// A move accepted from the tray's own menu, with a window attached: the delete it leaves held /// says so in the window on the next listing. @@ -1177,6 +1230,61 @@ public async Task AnOwningViewerHoldsADeleteItsMoveWroteAndSaysWhy() await Assert.That(await File.ReadAllTextAsync(move.Target)).IsEqualTo("received"); await Assert.That(response.Message).IsEqualTo($"Accepted 0, plus 1 files (1 kept). {ViewerSession.DeletesKept}"); await Assert.That(pair.Send(new(ViewerVerb.ListFull)).Deletes.Single().Held).IsEqualTo(ViewerSession.WroteItsFile); + // And marks it in its own window as held, not as a failure + var row = QueueProjection.Rows(pair.Viewer).Single(); + await Assert.That(row.Label).StartsWith(QueueProjection.HeldMark); + await Assert.That(row.Status).IsNull(); + + // Raised again over the file as the move left it, which a process that decided before + // the move was accepted does too: still held, as a tray holds it + await DiffRunner.AddDeleteAsync(move.Target); + + await Assert.That(pair.Send(new(ViewerVerb.ListFull)).Deletes.Single().Held).IsEqualTo(ViewerSession.WroteItsFile); + + await File.WriteAllTextAsync(move.Target, "written by something else since"); + await DiffRunner.AddDeleteAsync(move.Target); + + await Assert.That(pair.Send(new(ViewerVerb.ListFull)).Deletes.Single().Held).IsNull(); + } + + /// + /// A viewer that owns the queue throws its pending received files away a step at a time, and + /// none of its listings said it was doing so: a window attached to it, or a tray, saw the + /// queue shrink for no reason given. Listed from inside each delete, which is the discard + /// part way through, over the socket and into a window displaying that queue. + /// + [Test] + public async Task AnOwningViewersDiscardIsOnItsListings() + { + ViewerOwned? owned = null; + SessionHost? window = null; + OwnerLink? link = null; + var listed = new List<(int Done, int Total, bool Discarding)>(); + var said = new List(); + owned = new( + deleting: _ => + { + var progress = owned!.Send(new(ViewerVerb.ListFull)).Progress; + listed.Add(progress is null ? (-1, -1, false) : (progress.Done, progress.Total, progress.Discarding)); + link!.Pump(); + said.Add(window!.State.Progress?.Describe() ?? "nothing"); + }); + await using var pair = owned; + using var noTray = new NoTray(); + window = new(SessionState.Start(ViewerMode.Inline)); + link = new(window, pair.Port); + foreach (var move in new[] { pair.StageMove(), pair.StageMove() }) + { + await File.WriteAllTextAsync(move.Target, "verified"); + PendingFiles.AddMove(move.Temp, move.Target, null, null, false, null); + } + + var response = pair.Send(new(ViewerVerb.DiscardAll)); + + await Assert.That(response.Ok).IsTrue(); + await Assert.That(listed).IsEquivalentTo([(0, 2, true), (1, 2, true)]); + await Assert.That(said).IsEquivalentTo(["Discarding 1 of 2", "Discarding 2 of 2"]); + await Assert.That(pair.Send(new(ViewerVerb.ListFull)).Progress).IsNull(); } /// @@ -1275,7 +1383,12 @@ record TrackedDeleteFile(string Key, string File); /// sealed class TrayOwned : IAsyncDisposable { - public TrayOwned(Func? applier = null) + /// What applying a snapshot answers, when not that it was applied. + /// + /// Told of a move that was refused, from inside its accept: the one place a test can + /// stand while a move is out being accepted. + /// + public TrayOwned(Func? applier = null, Action? acceptFailed = null) { Host = OwnedInlineHost.TryOwn( Warnings.Add, @@ -1287,7 +1400,7 @@ public TrayOwned(Func? applier = null) return applier?.Invoke(patch) ?? InlineApplyResult.Applied; }) ?? throw new("Could not bind an ephemeral port."); - Tracker = new(inlineFailed: Failures.Add, inline: Host); + Tracker = new(acceptFailed: acceptFailed, inlineFailed: Failures.Add, inline: Host); // Wired the way Program does, and before serving starts: a queue change arriving over // the socket has to reach the listing the tray menu and the icon read, not wait for the // next two second scan. @@ -1415,7 +1528,12 @@ public async ValueTask DisposeAsync() /// sealed class ViewerOwned : IAsyncDisposable { - public ViewerOwned(Func? applier = null) + /// What applying a snapshot answers, when not that it was applied. + /// + /// Told of a file as it is about to be deleted, which for a bulk discard is the batch + /// part way through. + /// + public ViewerOwned(Func? applier = null, Action? deleting = null) { if (!ViewerSideServer.TryBind(0, out var bound)) { @@ -1442,7 +1560,11 @@ public ViewerOwned(Func? applier = null) // The real ones, so accepting a pending file here is the file operation itself // rather than a recording of one. MoveFile = ViewerActions.Real.MoveFile, - DeleteFile = ViewerActions.Real.DeleteFile + DeleteFile = _ => + { + deleting?.Invoke(_); + ViewerActions.Real.DeleteFile(_); + } }; var handler = new SessionMessageHandler(Window, actions, Windows.Enqueue); listening = server.Listen(handler.Handle, cancel.Token); diff --git a/src/DiffEngineTray/MenuBuilder.cs b/src/DiffEngineTray/MenuBuilder.cs index 6b55b5ce..bcb06ba5 100644 --- a/src/DiffEngineTray/MenuBuilder.cs +++ b/src/DiffEngineTray/MenuBuilder.cs @@ -261,14 +261,24 @@ static string ForMenu(string name, string status) static string Shortened(string text) => text.Length <= 60 ? text : $"{text[..59].TrimEnd()}…"; + /// + /// What a delete "Accept all" would leave pending is marked with. Not the ! of a + /// snapshot that failed to apply, which it used to share: nothing failed here, and the delete + /// is waiting to be accepted on its own. The character the viewer's queue column leads a held + /// delete's row with. + /// + internal const string HeldMark = " ~"; + + /// The pending delete. /// - /// Why "Accept all" leaves this delete pending, when it does. Marked and said the way a - /// snapshot that was not written is: a delete an accept-all had just passed over looked like - /// every other, and pressing "Accept all" again was the natural thing to try. + /// Why "Accept all" leaves this delete pending, when it does. Marked, and said the way a + /// snapshot that was not written says why: a delete an accept-all had just passed over looked + /// like every other, and pressing "Accept all" again was the natural thing to try. /// + /// Accepts this delete on its own. static ToolStripDropDownButton BuildDelete(TrackedDelete delete, string? held, Action accept) { - var marker = held == null ? "" : " !"; + var marker = held == null ? "" : HeldMark; var menu = new ToolStripDropDownButton($"{delete.Name}{marker}") { DropDownDirection = ToolStripDropDownDirection.Left diff --git a/src/DiffEngineTray/OwnedInlineHost.cs b/src/DiffEngineTray/OwnedInlineHost.cs index abf37d3a..fb97ec5b 100644 --- a/src/DiffEngineTray/OwnedInlineHost.cs +++ b/src/DiffEngineTray/OwnedInlineHost.cs @@ -60,11 +60,14 @@ sealed class OwnedInlineHost : readonly Lock accepting = new(); /// - /// Several snapshots of one source file, written with one read and one write and answered in - /// the order given: what a bulk accept hands a file's snapshots to. The applier a test - /// supplied, asked of each in turn, when there is one. + /// The snapshots of one source file, written with one read and one write and answered in + /// the order given: what a bulk accept hands a file's snapshots to, with the question it + /// asks of each as the file is about to be written, whether the snapshot is still wanted + /// (). The + /// applier a test supplied, asked of each in turn, when there is one: each is then a write of + /// its own, and is asked about before it. /// - readonly Func, IReadOnlyList> together; + readonly Func, Func, IReadOnlyList> together; OwnedInlineHost( ViewerServer server, @@ -82,7 +85,16 @@ sealed class OwnedInlineHost : } else { - together = _ => _.Select(applier).ToList(); + together = (patches, wanted) => + { + var results = new List(patches.Count); + for (var index = 0; index < patches.Count; index++) + { + results.Add(wanted(index) ? applier(patches[index]) : InlineApplyResult.Withdrawn); + } + + return results; + }; } } @@ -646,6 +658,12 @@ void IQueueOwner.Window(WindowCommand command, string? key) /// one read and one write, each with its own outcome. That is still looked up when its turn /// comes, by the first of the file's, and the wait it is looked up ahead of is the one write. /// + /// + /// That wait is the file's lock, which another process can hold for seconds, and a snapshot + /// discarded or settled in it had already been handed over: it was written with the rest of + /// its file. So each is asked about again as the file is about to be written, and one that is + /// no longer the entry that was claimed is left out of the write and counted as nothing. + /// /// /// /// The tracked files the caller sweeps once the snapshots are done, for the progress total. @@ -707,9 +725,29 @@ string AcceptEvery(int files, out bool refused) } } - var results = claimed.Count == 1 - ? [applier(claimed[0].Patch)] - : together(claimed.Select(_ => _.Patch).ToList()); + // Asked as the file is about to be written, with its lock held, so under the gate and + // not across anything: nothing here applies with the gate held, so nothing holding + // the gate can be waiting for that file. An entry is still the one claimed while the + // queue holds its variants, which is how its outcome is recorded below. Without the + // question, one discarded or settled while the file was waited for was written with + // the rest, and then not found to be counted + bool Wanted(int index) + { + lock (gate) + { + foreach (var pending in queue.Items) + { + if (ReferenceEquals(pending.Variants, claimed[index].Variants)) + { + return true; + } + } + + return false; + } + } + + var results = together(claimed.Select(_ => _.Patch).ToList(), Wanted); if (results.Count != claimed.Count) { throw new InvalidOperationException($"{claimed.Count} snapshots were applied together and {results.Count} outcomes came back."); diff --git a/src/DiffEngineTray/TrackedDelete.cs b/src/DiffEngineTray/TrackedDelete.cs index f67a143b..b81a6890 100644 --- a/src/DiffEngineTray/TrackedDelete.cs +++ b/src/DiffEngineTray/TrackedDelete.cs @@ -24,8 +24,9 @@ public TrackedDelete(string file, string? group, string? source = null) /// /// Set once a move has written this file while the delete was pending, and from then on no - /// accept-all carries the delete out: see . Cleared when a - /// test run raises the delete again, which is a statement made after the write. + /// accept-all carries the delete out: see . Cleared when the + /// delete is raised again over a file that is no longer what the move left + /// (), and not by one raised over the file as it was written. /// /// The one thing here that changes, and a listing carries what follows from it, so whoever /// changes it has to say so to : see the tracker's count of @@ -33,4 +34,10 @@ public TrackedDelete(string file, string? group, string? source = null) /// /// public bool Written { get; set; } + + /// + /// The file's length and write time as the move left it, read as was + /// set, or null when they could not be read. What a delete raised again is asked against. + /// + public (long Length, DateTime Written)? WrittenAs { get; set; } } diff --git a/src/DiffEngineTray/Tracker.cs b/src/DiffEngineTray/Tracker.cs index ec3ba47e..8274274d 100644 --- a/src/DiffEngineTray/Tracker.cs +++ b/src/DiffEngineTray/Tracker.cs @@ -687,26 +687,70 @@ public TrackedDelete AddDelete(string file, string? source = null) => updateValueFactory: (_, existing) => { Log.Information("DeleteUpdated. File:{file}", file); + // Raised again, which says nothing about when it was decided. A run has a process + // for each target framework, and one that looked at the file before the move was + // accepted sends the same message after it as a run that looked at what the move + // wrote. Taken for the later statement, the first let go of the hold, and the + // next "Accept all" deleted what had just been accepted. So the hold stands while + // the file is as the move left it, and goes only when it is seen to be something + // else: what the hold was keeping is then no longer there to keep + var held = existing.Written && + !ChangedSinceWritten(existing); + // A listing carries what a delete was derived from, and the objects tracked are // what says whether a listing has changed, so one raised again under another - // source is another delete. A new one is not marked either, which is right: it - // was raised by a run that looked at the file as it is now + // source is another delete. It keeps the hold, for the reason above if (!string.Equals(existing.Source, source, StringComparison.OrdinalIgnoreCase)) { - return new(existing.File, existing.Group, source); + var replacement = new TrackedDelete(existing.File, existing.Group, source); + if (held) + { + replacement.Written = true; + replacement.WrittenAs = existing.WrittenAs; + } + + return replacement; } - // Raised again, so by a run that looked at the file as it is now. Whatever a move - // wrote there since the delete was first raised, this is the later statement - if (existing.Written) + if (existing.Written && + !held) { existing.Written = false; + existing.WrittenAs = null; Interlocked.Increment(ref restores); } return existing; }); + /// + /// Whether a delete's file is seen to differ from what the move left. Not when either look + /// at it failed: a hold is let go on what is known, and kept on what is not. + /// + static bool ChangedSinceWritten(TrackedDelete delete) => + delete.WrittenAs is { } written && + Stamp(delete.File) is { } now && + now != written; + + static (long Length, DateTime Written)? Stamp(string file) + { + try + { + var info = new FileInfo(file); + if (!info.Exists) + { + return null; + } + + return (info.Length, info.LastWriteTimeUtc); + } + catch (Exception exception) + when (exception is IOException or UnauthorizedAccessException) + { + return null; + } + } + /// /// Why an accept-all leaves a delete pending, where it would: null when it would carry it /// out. For the menu and the debug view, which say it beside the delete, and for the sweeps, @@ -715,7 +759,56 @@ public TrackedDelete AddDelete(string file, string? source = null) => public string? HeldReason(TrackedDelete delete) => HeldReason( delete, - moves.Values.Any(_ => string.Equals(_.Target, delete.File, StringComparison.OrdinalIgnoreCase))); + // Asked on both sides of the walk, since a move is in one or the other and goes + // between them: into this as an accept begins, and back out of it as one ends + accepting.ContainsKey(delete.File) || + moves.Values.Any(_ => string.Equals(_.Target, delete.File, StringComparison.OrdinalIgnoreCase)) || + accepting.ContainsKey(delete.File)); + + /// + /// The files that moves being accepted right now are onto, and how many moves each. + /// + /// An accept takes its move out of for as long as the move takes, which is + /// seconds when a file is locked, and only says the file was written + /// () once it has been. In between, nothing tracked named the file, + /// so a delete pending on it was held for neither reason: a listing taken then said so, and a + /// viewer showing this queue sent the delete's key in a group accept, after the move had + /// written the file. So a move counts as still to write its file from before it leaves + /// until it has been marked written, dropped or put back. + /// + /// + readonly ConcurrentDictionary accepting = new(StringComparer.OrdinalIgnoreCase); + + /// + /// Takes a move out to be accepted, having first said its file is being written. False when + /// the move had already gone, and nothing is then being written. + /// + bool TakeToAccept(TrackedMove move, [NotNullWhen(true)] out TrackedMove? removed) + { + accepting.AddOrUpdate(move.Target, 1, (_, count) => count + 1); + if (moves.TryRemove(move.Temp, out removed)) + { + return true; + } + + Landed(move); + return false; + } + + /// + /// The accept of a move is over, however it went: its delete has been marked written, or the + /// move is back among the pending ones, or it was dropped having written nothing. Counted as + /// a change, since the last of those leaves a delete no longer held and nothing else about + /// what is tracked different from a moment before. + /// + void Landed(TrackedMove move) + { + accepting.AddOrUpdate(move.Target, 0, (_, count) => count - 1); + // Only ever an entry at none, which another accept of the same file may have raised + // again by now and is then left + accepting.TryRemove(new(move.Target, 0)); + Interlocked.Increment(ref restores); + } static string? HeldReason(TrackedDelete delete, bool awaited) { @@ -746,8 +839,13 @@ public TrackedDelete AddDelete(string file, string? source = null) => /// other, and a second "Accept all" deleted the snapshot the first had just accepted. The same /// went for a move accepted on its own and an accept-all after it. /// + /// + /// It does not offer running the tests again, which used to let go of the hold: see + /// . The price is that a delete a later run truly wants stays held + /// until it is accepted on its own. + /// /// - public const string WroteItsFile = "Kept by 'Accept all': a move was accepted onto this file after the delete was raised, so deleting it would remove what was just accepted. Accept the delete on its own to delete the file anyway, or run the tests again."; + public const string WroteItsFile = "Kept by 'Accept all': a move was accepted onto this file after the delete was raised, so deleting it would remove what was just accepted. Accept the delete on its own to delete the file anyway."; /// /// A move still pending is going to write the file. Not remembered: it is true for as long as @@ -760,6 +858,8 @@ void MarkWritten(string target) if (deletes.TryGetValue(target, out var delete) && !delete.Written) { + // The stamp first, so nobody finds the flag set and the stamp yet to be + delete.WrittenAs = Stamp(target); delete.Written = true; // A listing says why a delete is held, so this is a change to what one carries Interlocked.Increment(ref restores); @@ -835,19 +935,26 @@ HashSet AcceptMoves(IEnumerable toAccept) void AcceptMove(TrackedMove move, AcceptBatch batch) { - if (!moves.TryRemove(move.Temp, out var removed)) + if (!TakeToAccept(move, out var removed)) { return; } - if (InnerMove(removed, batch)) + try { - Release(removed); - return; - } + if (InnerMove(removed, batch)) + { + Release(removed); + return; + } - // Keep the move pending so accepting can be retried - Restore(removed); + // Keep the move pending so accepting can be retried + Restore(removed); + } + finally + { + Landed(removed); + } } /// @@ -1352,11 +1459,24 @@ IReadOnlyList ITrackedFiles.Deletes() // The files the pending moves are onto, gathered once: HeldReason walks the moves for // the one delete it is asked about, which here would be every move for every delete var awaited = new HashSet(StringComparer.OrdinalIgnoreCase); + // The moves being accepted on both sides of the pending ones, since a move is in one or + // the other and goes between them: into the first as an accept begins, and back out of + // it as one ends + foreach (var target in accepting.Keys) + { + awaited.Add(target); + } + foreach (var move in moves) { awaited.Add(move.Value.Target); } + foreach (var target in accepting.Keys) + { + awaited.Add(target); + } + return [ .. deletes.Values @@ -1676,19 +1796,26 @@ int ITrackedFiles.DiscardAll() => (bool ok, string? message) AcceptWithoutPrompting(TrackedMove move, AcceptBatch batch) { - if (!moves.TryRemove(move.Temp, out var removed)) + if (!TakeToAccept(move, out var removed)) { return (false, null); } - if (InnerMove(removed, batch)) + try { - Release(removed); - return (true, $"Accepted {removed.Name}"); - } + if (InnerMove(removed, batch)) + { + Release(removed); + return (true, $"Accepted {removed.Name}"); + } - Restore(removed); - return (false, $"Files for '{removed.Name}' are locked. Accept from the tray menu to resolve."); + Restore(removed); + return (false, $"Files for '{removed.Name}' are locked. Accept from the tray menu to resolve."); + } + finally + { + Landed(removed); + } } /// diff --git a/src/DiffEngineViewer.Tests/AttachedViewerTests.cs b/src/DiffEngineViewer.Tests/AttachedViewerTests.cs index 05cb3c6f..fa04d6fd 100644 --- a/src/DiffEngineViewer.Tests/AttachedViewerTests.cs +++ b/src/DiffEngineViewer.Tests/AttachedViewerTests.cs @@ -418,9 +418,10 @@ public async Task UnchangedFilesAreNotReReadAndAChangeRefreshes() /// /// A delete the owner's accept-all would leave is marked in the owner's own menu, and looked - /// like any other here. What the owner says of it is the entry's status, so the row carries - /// the mark a failure does and its tip says why. The same entry from pump to pump while the - /// owner says the same, and unmarked again once the owner lets go of it. + /// like any other here. What the owner says of it is the entry's status, and its tip says + /// why. The row leads with the mark of a held delete and is not the row of a failure, which + /// it was drawn as when the status was all a row had to go by. The same entry from pump to + /// pump while the owner says the same, and unmarked again once the owner lets go of it. /// [Test] public async Task ADeleteTheOwnerHoldsSaysWhy() @@ -450,8 +451,10 @@ public async Task ADeleteTheOwnerHoldsSaysWhy() var first = host.State.Queue.Single(); await Assert.That(first.Status).IsEqualTo(held); var row = QueueProjection.Rows(host.State).Single(_ => _.Kind == QueueRowKind.Entry); - await Assert.That(row.Status).IsEqualTo(held); + await Assert.That(row.Label).IsEqualTo("~ extra.verified.txt"); + await Assert.That(row.Status).IsNull(); await Assert.That(row.Tooltip!).Contains(held); + await Assert.That(AsciiRenderer.Render(ScreenBuilder.Build(host.State))).DoesNotContain("extra.verified.txt !"); link.Pump(); await Assert.That(host.State.Queue.Single()).IsSameReferenceAs(first); @@ -465,6 +468,7 @@ public async Task ADeleteTheOwnerHoldsSaysWhy() held = null; link.Pump(); await Assert.That(host.State.Queue.Single().Status).IsNull(); + await Assert.That(QueueProjection.Rows(host.State).Single(_ => _.Kind == QueueRowKind.Entry).Label).IsEqualTo("extra.verified.txt"); await cancel.CancelAsync(); } } diff --git a/src/DiffEngineViewer.Tests/DiscardBatchTests.cs b/src/DiffEngineViewer.Tests/DiscardBatchTests.cs index 95283792..b56a6d80 100644 --- a/src/DiffEngineViewer.Tests/DiscardBatchTests.cs +++ b/src/DiffEngineViewer.Tests/DiscardBatchTests.cs @@ -144,10 +144,63 @@ public async Task AWindowOnlyBeginsItsDiscard() await Assert.That(state.Queue.Count).IsEqualTo(2); var screen = ScreenBuilder.Build(state); await Assert.That(screen.Buttons.Where(_ => _.Command is CommandKind.Accept or CommandKind.Discard or CommandKind.AcceptAll).Any(_ => _.Enabled)).IsFalse(); - // Not something a listing says: the wire's progress is an accept's, in so many words - await Assert.That(state.ListedProgress).IsNull(); + // And a listing says so, as a discard + await Assert.That(state.ListedProgress).IsEqualTo(Discarding(0, 2)); } + /// + /// A discard under way was on none of its owner's listings, so whoever displayed the queue + /// saw it shrink with nothing saying why, and refused nothing meanwhile. Asked from inside a + /// delete, which is the batch part way through: each listing says how far the discard has + /// got and that it is one, under a tag of its own, and the one after it says nothing. + /// + [Test] + public async Task AListingDuringADiscardSaysHowFarItHasGot() + { + var host = new SessionHost(Mixed()); + var listed = new List(); + var tags = new List(); + IQueueOwner? owner = null; + var actions = Deleting(_ => + { + listed.Add(owner!.Listing(true).Progress ?? new(-1, -1)); + tags.Add(owner.ListingTag() ?? ""); + }); + owner = new MessageHandler(host, actions, _ => { }); + + owner.DiscardAll(); + + await Assert.That(listed).IsEquivalentTo([Discarding(0, 2), Discarding(1, 2)]); + await Assert.That(owner.Listing(true).Progress).IsNull(); + tags.Add(owner.ListingTag() ?? ""); + await Assert.That(tags.Distinct().Count()).IsEqualTo(3); + } + + /// + /// What a window displaying that queue does with it: says discarding, in the words the owner's + /// own window uses, and refuses what changes the queue until the owner is done. + /// + [Test] + public async Task AnAttachedWindowFollowsTheOwnersDiscard() + { + var attached = Fixtures.Attached(Fixtures.Pending(), Fixtures.Move("One.Test (txt)"), Fixtures.Move("Two.Test (txt)")); + var state = ViewerSession.Sync(attached, Fixtures.Pending(), [..attached.Queue], null, Discarding(0, 2)); + + var screen = ScreenBuilder.Build(state); + await Assert.That(screen.Status).IsEqualTo("Discarding 1 of 2"); + await Assert.That(screen.Buttons.Where(_ => _.Command is CommandKind.Accept or CommandKind.Discard or CommandKind.AcceptAll).Any(_ => _.Enabled)).IsFalse(); + await Assert.That(ViewerProgram.Apply(state, Input(CommandKind.Discard), null, new Window()).Queue).IsSameReferenceAs(state.Queue); + + var done = ViewerSession.Sync(state, Fixtures.Pending(), [state.Queue[1]], null); + await Assert.That(done.Progress).IsNull(); + } + + static AcceptProgress Discarding(int done, int total) => + new(done, total) + { + Discarding = true + }; + // A snapshot, two pending moves and a pending delete static SessionState Mixed() { diff --git a/src/DiffEngineViewer.Tests/Fixtures.cs b/src/DiffEngineViewer.Tests/Fixtures.cs index 1e9e4f15..53253b88 100644 --- a/src/DiffEngineViewer.Tests/Fixtures.cs +++ b/src/DiffEngineViewer.Tests/Fixtures.cs @@ -416,7 +416,23 @@ static string WriteImage(string name, byte[] content) var directory = Path.Combine(Path.GetTempPath(), "deview-fixture-images"); Directory.CreateDirectory(directory); var path = Path.Combine(directory, name); - System.IO.File.WriteAllBytes(path, content); + // Left alone when it already holds these bytes, which after a machine's first run it + // does. Written on every call, the same picture under a new write time was a file + // rewritten to whatever had decoded it: a test in this process or in another run of the + // suite that painted it twice composed it twice, and one reading it met the write. + try + { + if (!System.IO.File.Exists(path) || + !System.IO.File.ReadAllBytes(path).AsSpan().SequenceEqual(content)) + { + System.IO.File.WriteAllBytes(path, content); + } + } + catch (IOException) + { + // Being written by another test, and every writer writes the same bytes + } + return path; } diff --git a/src/DiffEngineViewer.Tests/MoveOntoDeleteTests.cs b/src/DiffEngineViewer.Tests/MoveOntoDeleteTests.cs index 35216785..49cd47d2 100644 --- a/src/DiffEngineViewer.Tests/MoveOntoDeleteTests.cs +++ b/src/DiffEngineViewer.Tests/MoveOntoDeleteTests.cs @@ -85,6 +85,37 @@ public async Task A_delete_whose_file_a_single_accept_wrote_is_held_by_every_acc await Assert.That(state.Queue.Single().Kind).IsEqualTo(QueueEntryKind.Delete); } + /// + /// A held delete's reason is its status, and every head draws a row with a status as an entry + /// that failed. Nothing failed, so the row leads with a mark of its own and carries no + /// status; why it is held stays in its tip. A delete that was tried and could not be deleted + /// is still the failure it was. + /// + [Test] + public async Task A_held_delete_is_marked_apart_from_a_failed_one() + { + var disk = new Disk(); + var state = Queued(Fixtures.Move(), Delete()); + state = ViewerSession.Apply(state, CommandKind.AcceptAll, disk.Actions); + + var held = QueueProjection.Rows(state).Single(); + await Assert.That(held.Label).IsEqualTo("~ sample.verified.txt"); + await Assert.That(held.Status).IsNull(); + await Assert.That(held.Tooltip!).Contains(ViewerSession.WroteItsFile); + // Leading, so it is there in a column too narrow for the name + await Assert.That(AsciiRenderer.Render(ScreenBuilder.Build(state))).Contains("| > ~ sample.verified"); + + var locked = disk.Actions with + { + DeleteFile = static _ => throw new("The file is locked.") + }; + state = ViewerSession.Apply(state, CommandKind.Accept, locked); + + var failed = QueueProjection.Rows(state).Single(); + await Assert.That(failed.Label).IsEqualTo("sample.verified.txt"); + await Assert.That(failed.Status).IsEqualTo("The file is locked."); + } + /// /// Accepted on its own it is carried out, held or not: that is the reviewer saying the file /// is redundant. @@ -103,27 +134,99 @@ public async Task A_held_delete_accepted_on_its_own_is_carried_out() } /// - /// Raised again, so by a run that looked at the file as it is now: the later statement, and - /// the hold is let go. Both ways an arrival is taken, since the file a move wrote may or may - /// not hold what the delete's entry was showing. + /// A delete raised again says nothing about when it was decided: a process of the same run + /// that looked at the file before the move was accepted raises it after, as a later run + /// would. Raising it again used to let go of the hold, and the next accept-all then deleted + /// what had just been accepted. Both ways an arrival is taken, since the entry it is built + /// from may or may not show what the queued one does. Nothing here is a file, so no stamp + /// was read, and a stamp that is not known is not a file seen to differ. /// [Test] [Arguments(Fixtures.Expected)] [Arguments("something else")] - public async Task A_delete_raised_again_is_no_longer_held(string contentNow) + public async Task A_delete_raised_again_is_still_held(string contentNow) { var disk = new Disk(); var state = Queued(Fixtures.Move(), Delete()); state = ViewerSession.Apply(state, CommandKind.AcceptAll, disk.Actions); state = ViewerSession.EnqueueTracked(state, Delete(contentNow)); - await Assert.That(state.Queue.Single().Status).IsNull(); - await Assert.That(ViewerSession.HeldReason(state.Queue, state.Queue.Single())).IsNull(); + await Assert.That(state.Queue.Single().Status).IsEqualTo(ViewerSession.WroteItsFile); + await Assert.That(ViewerSession.HeldReason(state.Queue, state.Queue.Single())).IsEqualTo(ViewerSession.WroteItsFile); state = ViewerSession.Apply(state, CommandKind.AcceptAll, disk.Actions); - await Assert.That(state.Queue).IsEmpty(); - await Assert.That(disk.Files).IsEmpty(); + await Assert.That(state.Queue.Single().Kind).IsEqualTo(QueueEntryKind.Delete); + await Assert.That(disk.Files[target]).IsEqualTo("received"); + } + + /// + /// The same with files, where the stamps are: raised again over the file as the move left it + /// the delete is still held, however many times, and raised over a file written since it is + /// a delete like any other, since nothing of what the move put there is left to keep. + /// + [Test] + public async Task A_delete_raised_again_is_let_go_only_over_a_file_written_since() + { + var directory = Path.Combine(Path.GetTempPath(), $"MoveOntoDeleteTests_{Guid.NewGuid():N}"); + Directory.CreateDirectory(directory); + try + { + var received = Path.Combine(directory, "sample.received.txt"); + var verified = Path.Combine(directory, "sample.verified.txt"); + await File.WriteAllTextAsync(received, "received"); + await File.WriteAllTextAsync(verified, "verified"); + var actions = Fixtures.Applied with + { + MoveFile = ViewerActions.Real.MoveFile, + DeleteFile = ViewerActions.Real.DeleteFile + }; + var state = Queued(TrackedEntry.ForMove(received, verified), TrackedEntry.ForDelete(verified)); + state = ViewerSession.Apply(state, CommandKind.AcceptAll, actions); + await Assert.That(state.Queue.Single().WrittenAs).IsNotNull(); + + // Twice: the first is built from what the file holds now, and the second finds an + // entry already showing that + for (var raise = 0; raise < 2; raise++) + { + state = ViewerSession.EnqueueTracked(state, TrackedEntry.DeleteAgain(state.Queue.Single(), verified)); + await Assert.That(state.Queue.Single().RightText).IsEqualTo("received"); + await Assert.That(state.Queue.Single().Status).IsEqualTo(ViewerSession.WroteItsFile); + await Assert.That(ViewerSession.HeldReason(state.Queue, state.Queue.Single())).IsEqualTo(ViewerSession.WroteItsFile); + } + + await File.WriteAllTextAsync(verified, "written by something else since"); + state = ViewerSession.EnqueueTracked(state, TrackedEntry.DeleteAgain(state.Queue.Single(), verified)); + await Assert.That(state.Queue.Single().Status).IsNull(); + await Assert.That(ViewerSession.HeldReason(state.Queue, state.Queue.Single())).IsNull(); + + state = ViewerSession.Apply(state, CommandKind.AcceptAll, actions); + + await Assert.That(state.Queue).IsEmpty(); + await Assert.That(File.Exists(verified)).IsFalse(); + } + finally + { + Directory.Delete(directory, true); + } + } + + /// + /// The entry an arrival is built from is read outside the lock, and may be from before the + /// move was carried out. The hold is the queued entry's, whatever the arrival says of it. + /// + [Test] + public async Task A_delete_raised_again_from_an_entry_read_before_the_move_is_still_held() + { + var disk = new Disk(); + var state = Queued(Fixtures.Move(), Delete()); + var readEarlier = state.Queue.Single(_ => _.Kind == QueueEntryKind.Delete); + state = ViewerSession.Apply(state, CommandKind.AcceptAll, disk.Actions); + + state = ViewerSession.EnqueueTracked(state, readEarlier); + + await Assert.That(ViewerSession.HeldReason(state.Queue, state.Queue.Single())).IsEqualTo(ViewerSession.WroteItsFile); + await Assert.That(state.Queue.Single().Status).IsEqualTo(ViewerSession.WroteItsFile); } /// diff --git a/src/DiffEngineViewer.Tests/ViewerProgramTests.cs b/src/DiffEngineViewer.Tests/ViewerProgramTests.cs index 02bf6f31..04c46575 100644 --- a/src/DiffEngineViewer.Tests/ViewerProgramTests.cs +++ b/src/DiffEngineViewer.Tests/ViewerProgramTests.cs @@ -50,10 +50,57 @@ public async Task AViewerWithNoWindowStillStagesWhatItHolds() var code = ViewerProgram.Run(new(state), server: null, link: null, NoWindow); + // And says so, where it used to say only that it failed: whoever launched it stages what + // it sent on a failure, and that was a second trio beside this one + await Assert.That(code).IsEqualTo(ViewerExit.Staged); + await Assert.That(project.StagedFiles().Count(_ => _.EndsWith(".inlinepatch"))).IsEqualTo(1); + } + + /// + /// Staged is said only where it is so. A snapshot whose source has no project above it has + /// nowhere to be staged, and one of two left unwritten is still nowhere: the exit is the + /// failure it always was, which is what has the launcher keep the patch it sent. + /// + [Test] + public async Task AViewerWithNoWindowThatCouldNotStageEverythingSaysItFailed() + { + using var project = new TempProject(); + var source = project.Source("SampleTests.cs"); + var nowhere = Path.Combine(Path.GetTempPath(), $"viewer-persist-{Guid.NewGuid():N}.cs"); + var state = Fixtures.Inline( + Fixtures.Patch(source: source, framework: "net10.0"), + Fixtures.Patch(source: nowhere, framework: "net10.0")); + + var code = ViewerProgram.Run(new(state), server: null, link: null, NoWindow); + await Assert.That(code).IsEqualTo(4); await Assert.That(project.StagedFiles().Count(_ => _.EndsWith(".inlinepatch"))).IsEqualTo(1); } + /// + /// A viewer with nothing of a snapshot in it has staged nothing, whatever it was started for. + /// + [Test] + public async Task AViewerWithNoWindowAndNoSnapshotsSaysItFailed() + { + var code = ViewerProgram.Run(new(Fixtures.File()), server: null, link: null, NoWindow); + + await Assert.That(code).IsEqualTo(4); + } + + /// + /// A viewer started for a delete or a pair can be holding snapshots other processes sent it + /// by the time its window fails, and staging those says nothing of the file it was started + /// for. Its launcher is told the launch failed. + /// + [Test] + public async Task AViewerStartedForAFileNeverSaysStaged() + { + await Assert.That(ViewerProgram.ForAFile(ViewerExit.Staged)).IsEqualTo(4); + await Assert.That(ViewerProgram.ForAFile(0)).IsEqualTo(0); + await Assert.That(ViewerProgram.ForAFile(1)).IsEqualTo(1); + } + /// /// A loop that throws ends the way one that returns does. The throw used to unwind straight to /// Main's catch, past the persist, and the queue went with the process. diff --git a/src/DiffEngineViewer.Tests/WithdrawnSnapshotTests.cs b/src/DiffEngineViewer.Tests/WithdrawnSnapshotTests.cs new file mode 100644 index 00000000..30b5c1f5 --- /dev/null +++ b/src/DiffEngineViewer.Tests/WithdrawnSnapshotTests.cs @@ -0,0 +1,271 @@ +/// +/// A batch claims a file's snapshots under the session's lock and writes them outside it, and the +/// wait between is the file's own lock, up to ten seconds. The queue goes on changing meanwhile. +/// A snapshot discarded then, or settled by a test that started passing, had already been handed +/// over, and was written with the rest of its file and counted nowhere: thrown away by the +/// reviewer and in the source all the same. It is asked about as the file is about to be written +/// now, and left out when it is no longer the entry that was claimed. +/// +public class WithdrawnSnapshotTests +{ + [Test] + public async Task ASnapshotDiscardedWhileItsFileIsWaitedForIsNotWritten() + { + using var file = new SourceFile(); + var host = new SessionHost( + Fixtures.Inline( + Fixtures.Patch(file.Path, 3, "\"a\"", "one"), + Fixtures.Patch(file.Path, 4, "\"b\"", "two"), + Fixtures.Patch(file.Path, 5, "\"c\"", "three"))); + var discarded = host.State.Queue.Single(_ => _.Patch!.LineHint == 4).Key; + var actions = Meanwhile( + () => host.Mutate(_ => ViewerSession.Apply(ViewerSession.SelectKey(_, discarded), CommandKind.Discard, ViewerActions.Real))); + host.Mutate(ViewerSession.BeginAcceptAll); + + var message = new AcceptAllRunner(host, actions).Drive(); + + var written = file.Text; + await Assert.That(written).Contains("One() => Verify(value).Snapshot(\"one\")"); + await Assert.That(written).Contains("Two() => Verify(value).Snapshot(\"b\")"); + await Assert.That(written).Contains("Three() => Verify(value).Snapshot(\"three\")"); + // Neither accepted nor failed: it was taken back + await Assert.That(message).IsEqualTo("Accepted 2"); + await Assert.That(host.State.Queue).IsEmpty(); + await Assert.That(host.State.Batch).IsNull(); + } + + /// + /// A test that started passing settles its entry, and its source already holds what it + /// passes with: the patch claimed for it would put the failing run's content over that. + /// What is left of the file is taken to where its call sites are in the file that was + /// written, which the snapshot left out moved nothing in. + /// + [Test] + public async Task ASnapshotSettledWhileItsFileIsWaitedForIsNotWrittenAndMovesNothing() + { + using var file = new SourceFile(); + var host = new SessionHost( + Fixtures.Inline( + Fixtures.Patch(file.Path, 3, "\"a\"", "one\nmore"), + Fixtures.Patch(file.Path, 4, "\"b\"", "two\nmore\nand more\nand again"), + Fixtures.Patch(file.Path, 5, "\"c\"", "three"), + // A conflict, which no bulk accept takes: what is left of the file afterwards + Fixtures.Patch(file.Path, 6, "\"d\"", "four", framework: "net8.0"), + Fixtures.Patch(file.Path, 6, "\"d\"", "vier", framework: "net9.0"))); + var settled = host.State.Queue.Single(_ => _.Patch!.LineHint == 4).Key; + var actions = Meanwhile(() => host.Mutate(_ => ViewerSession.Settle(_, settled))); + host.Mutate(ViewerSession.BeginAcceptAll); + + var message = new AcceptAllRunner(host, actions).Drive(); + + var written = file.Text; + await Assert.That(written).Contains("Two() => Verify(value).Snapshot(\"b\")"); + await Assert.That(written).DoesNotContain("and again"); + await Assert.That(written).Contains("more"); + await Assert.That(written).Contains("Three() => Verify(value).Snapshot(\"three\")"); + await Assert.That(message).IsEqualTo("Accepted 2, 1 conflict needs review"); + var left = host.State.Queue.Single(); + await Assert.That(left.Conflicted).IsTrue(); + var lines = written.Split('\n'); + await Assert.That(lines[left.Patch!.LineHint - 1]).Contains("void Four()"); + await Assert.That(left.Key).IsEqualTo(InlineKey.For(file.Path, left.Patch.LineHint)); + } + + /// + /// A re-run that replaces a claimed snapshot is the other way an entry stops being the one + /// that was claimed. The content it was claimed with is stale, and was written over the + /// source and then not recorded, because the entry had changed. It is not written now, and + /// the entry keeps what the re-run sent. With an applier that takes them one at a time, + /// each is asked about before it is applied. + /// + [Test] + public async Task ASnapshotReplacedWhileItsFileIsWaitedForIsNotApplied() + { + var host = new SessionHost( + Fixtures.Inline( + Fixtures.Patch(), + Fixtures.Patch("SampleTests.cs", 88, "\"one\"", "two"), + Fixtures.Patch("SampleTests.cs", 90, "\"three\"", "four"))); + var applied = new List(); + var actions = Fixtures.Applied with + { + ApplyInline = _ => + { + applied.Add(_.LineHint); + if (_.LineHint == 42) + { + host.Mutate(state => ViewerSession.EnqueueInline(state, Fixtures.Patch("SampleTests.cs", 88, "\"one\"", "third run"))); + } + + return InlineApplyResult.Applied; + } + }; + host.Mutate(ViewerSession.BeginAcceptAll); + + var message = new AcceptAllRunner(host, actions).Drive(); + + await Assert.That(applied).IsEquivalentTo([42, 90]); + await Assert.That(message).IsEqualTo("Accepted 2"); + var left = host.State.Queue.Single(); + await Assert.That(left.Name).IsEqualTo("SampleTests.cs:88"); + await Assert.That(left.LeftText).IsEqualTo("third run"); + await Assert.That(left.Status).IsNull(); + } + + /// + /// A batch inside one transition asks nothing, since nothing else can have touched the queue + /// between its claim and its write: every claimed snapshot is applied, as before. + /// + [Test] + public async Task ABatchInOneTransitionAsksNothing() + { + var applied = new List(); + var actions = Fixtures.Applied with + { + ApplyInline = _ => + { + applied.Add(_.LineHint); + return InlineApplyResult.Applied; + }, + ApplyInlineWanted = (_, _) => throw new("Nothing was there to ask.") + }; + var state = Fixtures.Inline( + Fixtures.Patch(), + Fixtures.Patch("SampleTests.cs", 88, "\"one\"", "two")); + + var done = ViewerSession.Apply(state, CommandKind.AcceptAll, actions); + + await Assert.That(applied).IsEquivalentTo([42, 88]); + await Assert.That(done.Message).IsEqualTo("Accepted 2"); + } + + /// + /// The question is asked with the source file's lock held, on the thread that applies. A + /// single accept arriving over the socket applies inside the session's lock, so it holds + /// that while it waits for the file. Were the question to take the session's lock, each + /// thread would be waiting on what the other holds, for good. It reads the state, which + /// takes no lock, and both finish. + /// + [Test] + public async Task TheQuestionDoesNotWaitOnTheSessionsLock() + { + using var file = new SourceFile(); + var host = new SessionHost(Fixtures.Inline(Fixtures.Patch(file.Path, 3, "\"a\"", "one"))); + using var asking = new ManualResetEventSlim(); + using var holding = new ManualResetEventSlim(); + var actions = ViewerActions.Real with + { + ApplyInlineWanted = (patches, wanted) => InlineApplier.ApplyAll( + patches, + _ => + { + // The file's lock is held here. Not answered until the other thread has the + // session's lock and is on its way to this file + asking.Set(); + holding.Wait(TimeSpan.FromSeconds(30)); + return wanted(_); + }) + }; + host.Mutate(ViewerSession.BeginAcceptAll); + + // On threads of their own: both block, and the pool is what resumes this test + var batch = Task.Factory.StartNew( + () => new AcceptAllRunner(host, actions).Drive(), + Cancel.None, + TaskCreationOptions.LongRunning, + TaskScheduler.Default); + var single = Task.Factory.StartNew( + () => + { + asking.Wait(TimeSpan.FromSeconds(30)); + // What MessageHandler does with a wire accept: the apply is inside the mutation + return host.Mutate( + _ => + { + holding.Set(); + InlineApplier.Apply(Fixtures.Patch(file.Path, 4, "\"b\"", "two")); + return _; + }); + }, + Cancel.None, + TaskCreationOptions.LongRunning, + TaskScheduler.Default); + + await Task.WhenAll(batch, single).WaitAsync(TimeSpan.FromSeconds(60)); + + await Assert.That(await batch).IsEqualTo("Accepted 1"); + var written = file.Text; + await Assert.That(written).Contains("Snapshot(\"one\")"); + await Assert.That(written).Contains("Snapshot(\"two\")"); + } + + /// + /// The viewer's real actions, with something done to the queue at the moment a claim has + /// been handed over and its file not yet read: what another thread does while the batch + /// waits for the file. On both ways of handing a file's snapshots over, so the moment is + /// the same whether or not the batch then asks about them. + /// + static ViewerActions Meanwhile(Action change) => + ViewerActions.Real with + { + ApplyInline = _ => + { + change(); + return InlineApplier.Apply(_); + }, + ApplyInlineTogether = _ => + { + change(); + return InlineApplier.ApplyAll(_); + }, + ApplyInlineWanted = (patches, wanted) => + { + change(); + return InlineApplier.ApplyAll(patches, wanted); + } + }; + + /// + /// A real source file with four snapshots, one a line, the first on line 3. + /// + sealed class SourceFile : IDisposable + { + readonly string directory = System.IO.Path.Combine( + System.IO.Path.GetTempPath(), + $"WithdrawnSnapshotTests_{Guid.NewGuid():N}"); + + public SourceFile() + { + Directory.CreateDirectory(directory); + Path = System.IO.Path.Combine(directory, "SampleTests.cs"); + File.WriteAllText( + Path, + """ + class C + { + void One() => Verify(value).Snapshot("a"); + void Two() => Verify(value).Snapshot("b"); + void Three() => Verify(value).Snapshot("c"); + void Four() => Verify(value).Snapshot("d"); + } + """); + } + + public string Path { get; } + + public string Text => File.ReadAllText(Path); + + public void Dispose() + { + try + { + Directory.Delete(directory, true); + } + catch (Exception exception) + when (exception is IOException or UnauthorizedAccessException) + { + // Best effort cleanup of the temp directory + } + } + } +} diff --git a/src/DiffEngineViewer.Windows.Tests/FormsHeadTests.cs b/src/DiffEngineViewer.Windows.Tests/FormsHeadTests.cs index 2ea37526..88f4b5fb 100644 --- a/src/DiffEngineViewer.Windows.Tests/FormsHeadTests.cs +++ b/src/DiffEngineViewer.Windows.Tests/FormsHeadTests.cs @@ -85,6 +85,73 @@ public async Task HighlightAtColumn66CoversItsGlyph() await Assert.That(ink.Max()).IsLessThan(band.Right); } + /// + /// A left header too long for its pane ends short of where the right one starts. It was cut + /// at the very pixel the right pane begins on, so in a narrow window the two read as one + /// line. Each is drawn alone here, with nothing else bright on the canvas, so where its ink + /// is can be read back. + /// + [Test] + public async Task ALongLeftHeaderStopsShortOfTheRightOne() + { + // Full blocks, which the font draws from one edge of a cell to the other, so where a + // header's ink starts and stops is where its cells do + var header = new string('█', 200); + var screen = ScreenBuilder.Build(ViewerSession.Resize(Fixtures.File(), 60, 26)) with + { + Title = "", + Subtitle = "" + }; + var none = new Pane("", [], 0, 0); + using var host = new CanvasHost(560, 560); + + var left = Bounds( + host.Draw( + screen with + { + Left = none with + { + Header = header + }, + Right = none + }), + _ => _.GetBrightness() > 0.6f); + var right = Bounds( + host.Draw( + screen with + { + Left = none, + Right = none with + { + Header = header + } + }), + _ => _.GetBrightness() > 0.6f); + + await Assert.That(left).IsNotNull(); + await Assert.That(right).IsNotNull(); + Console.WriteLine($"left header's ink ends at {left!.Value.Right}, right header's starts at {right!.Value.Left}"); + // No less than the gap the canvas keeps between its columns, which is four pixels. The + // ellipsis is in the last whole cell, so whatever part of a cell is left over is more + await Assert.That(right.Value.Left - left.Value.Right).IsGreaterThanOrEqualTo(4); + } + + /// + /// A header is cut to whole cells with an ellipsis in the last of them, and one that fits is + /// left as it is. A character two cells wide is kept whole or not at all. + /// + [Test] + public async Task AHeaderTooLongForItsPaneEndsInAnEllipsis() + { + await Assert.That(ViewerCanvas.HeaderShown("a.txt (new)", 11)).IsEqualTo("a.txt (new)"); + await Assert.That(ViewerCanvas.HeaderShown("a.txt (new)", 10)).IsEqualTo("a.txt (ne…"); + await Assert.That(ViewerCanvas.HeaderShown("文件.txt", 6)).IsEqualTo("文件.…"); + // Three cells before the ellipsis, and the second character would be half in the fourth + await Assert.That(ViewerCanvas.HeaderShown("文件.txt", 4)).IsEqualTo("文…"); + await Assert.That(ViewerCanvas.HeaderShown("文件.txt", 3)).IsEqualTo("文…"); + await Assert.That(ViewerCanvas.HeaderShown("a.txt", 0)).IsEqualTo("…"); + } + /// /// A bar after characters the font does not draw a cell wide, selected at the column the grid /// puts it in: its ink has to be inside the highlight. Drawn as one string, GDI+ put the bar @@ -262,28 +329,36 @@ public async Task AMegabyteRowIsPaintedFromItsStart() [Test] public async Task EveryPictureEverDrawnStaysDecoded() { - var directory = TempDirectory("deview-review-cache"); - using var host = new CanvasHost(); - for (var index = 0; index < 10; index++) + // A folder of this test's own: under one fixed name, two runs on a machine at once moved + // and deleted each other's pictures + var directory = Directory.CreateTempSubdirectory("deview-review-cache-").FullName; + try { - var received = Path.Combine(directory, $"Test{index}.received.png"); - var verified = Path.Combine(directory, $"Test{index}.verified.png"); - await File.WriteAllBytesAsync(received, SamplePng.Build(400, 300, 200, 40, 40)); - await File.WriteAllBytesAsync(verified, SamplePng.Build(400, 300, 40, 40, 200)); - var entry = QueueEntry.ForFiles(received, verified, FileSide.Read(received), FileSide.Read(verified)); - var state = ViewerSession.Resize( - ViewerSession.EnqueueFile(SessionState.Start(ViewerMode.File, columns, rows), entry), - columns, - rows); - host.Draw(ScreenBuilder.Build(state)); - ViewerActions.Real.MoveFile(received, verified); - } + using var host = new CanvasHost(); + for (var index = 0; index < 10; index++) + { + var received = Path.Combine(directory, $"Test{index}.received.png"); + var verified = Path.Combine(directory, $"Test{index}.verified.png"); + await File.WriteAllBytesAsync(received, SamplePng.Build(400, 300, 200, 40, 40)); + await File.WriteAllBytesAsync(verified, SamplePng.Build(400, 300, 40, 40, 200)); + var entry = QueueEntry.ForFiles(received, verified, FileSide.Read(received), FileSide.Read(verified)); + var state = ViewerSession.Resize( + ViewerSession.EnqueueFile(SessionState.Start(ViewerMode.File, columns, rows), entry), + columns, + rows); + host.Draw(ScreenBuilder.Build(state)); + ViewerActions.Real.MoveFile(received, verified); + } - host.Draw(ScreenBuilder.Build(ViewerSession.Resize(Fixtures.File(), columns, rows))); - var (count, bytes) = host.Canvas.CachedImages(); - Directory.Delete(directory, true); - Console.WriteLine($"{count} decoded pictures held, {bytes / 1024} KB of pixels, on a screen showing none"); - await Assert.That(count).IsLessThanOrEqualTo(2); + host.Draw(ScreenBuilder.Build(ViewerSession.Resize(Fixtures.File(), columns, rows))); + var (count, bytes) = host.Canvas.CachedImages(); + Console.WriteLine($"{count} decoded pictures held, {bytes / 1024} KB of pixels, on a screen showing none"); + await Assert.That(count).IsLessThanOrEqualTo(2); + } + finally + { + Directory.Delete(directory, true); + } } /// @@ -1204,13 +1279,6 @@ static string Lines(int count, int changedAt = -1) return builder.ToString(); } - static string TempDirectory(string name) - { - var path = Path.Combine(Path.GetTempPath(), name); - Directory.CreateDirectory(path); - return path; - } - static T Field(object target, string name) => (T) target.GetType().GetField(name, BindingFlags.Instance | BindingFlags.NonPublic)!.GetValue(target)!; diff --git a/src/DiffEngineViewer.Windows.Tests/ImageCacheTests.cs b/src/DiffEngineViewer.Windows.Tests/ImageCacheTests.cs index 78ca4243..0376f135 100644 --- a/src/DiffEngineViewer.Windows.Tests/ImageCacheTests.cs +++ b/src/DiffEngineViewer.Windows.Tests/ImageCacheTests.cs @@ -473,22 +473,28 @@ public async Task Take() public async Task MissingFile() { using var cache = new ImageCache(); - await Assert.That(cache.Get(Path.Combine(Directory(), "gone.png"), null)).IsNull(); + await Assert.That(cache.Get(Path.Combine(directory, "gone.png"), null)).IsNull(); } static string Write(string name, byte[] content) { - var path = Path.Combine(Directory(), name); + var path = Path.Combine(directory, name); File.WriteAllBytes(path, content); return path; } - static string Directory() - { - var path = Path.Combine(Path.GetTempPath(), "deview-image-cache"); - System.IO.Directory.CreateDirectory(path); - return path; - } + // A folder for this run alone. Each test writes a file of its own name, so the tests of one + // run never meet, but under one fixed name two runs on a machine at once wrote, rewrote and + // deleted each other's pictures. + static string directory = ""; + + [Before(Class)] + public static void CreateDirectory() => + directory = Directory.CreateTempSubdirectory("deview-image-cache-").FullName; + + [After(Class)] + public static void DeleteDirectory() => + Directory.Delete(directory, true); /// /// A picture rewritten with different pixels at the same length and @@ -498,7 +504,7 @@ static string Directory() [Test] public async Task ARewriteWithTheSameStampKeepsTheOldPicture() { - var path = Path.Combine(Directory(), "Same.received.png"); + var path = Path.Combine(directory, "Same.received.png"); await File.WriteAllBytesAsync(path, SamplePng.Build(8, 6, 200, 40, 40)); var stamp = File.GetLastWriteTimeUtc(path); using var cache = new ImageCache(); diff --git a/src/DiffEngineViewer.Windows.Tests/WindowsPixelTests.FooterThatWraps.verified.png b/src/DiffEngineViewer.Windows.Tests/WindowsPixelTests.FooterThatWraps.verified.png index 5d74dc2e..a0850a6a 100644 Binary files a/src/DiffEngineViewer.Windows.Tests/WindowsPixelTests.FooterThatWraps.verified.png and b/src/DiffEngineViewer.Windows.Tests/WindowsPixelTests.FooterThatWraps.verified.png differ diff --git a/src/DiffEngineViewer.Windows.Tests/WindowsPixelTests.cs b/src/DiffEngineViewer.Windows.Tests/WindowsPixelTests.cs index 07fe6f3d..6845ff9c 100644 --- a/src/DiffEngineViewer.Windows.Tests/WindowsPixelTests.cs +++ b/src/DiffEngineViewer.Windows.Tests/WindowsPixelTests.cs @@ -36,12 +36,12 @@ public class WindowsPixelTests const int rows = 37; - static IViewerWindow? window; + static FormsViewerWindow? window; [Before(Class)] public static void Open() { - window = FormsViewerWindow.Open("DiffEngineViewer", width, height, hidden: true, placement: null, out var error); + window = (FormsViewerWindow?) FormsViewerWindow.Open("DiffEngineViewer", width, height, hidden: true, placement: null, out var error); if (window is null) { throw new(error!); @@ -209,22 +209,52 @@ screen with /// the left. In one row the buttons past the window's edge could not be reached at all. /// /// Fewer rows than the window of the other scenes holds, as the canvas reports fewer once - /// the footer is this tall. + /// the footer is this tall. The window is asked how many, as the loop asks it every frame, + /// and the screen is built for those. Built for the whole window its last rows are under + /// the footer, and built for a number worked out by hand, as this was, it was a row short + /// of what the window has. /// /// [Test] - public Task FooterThatWraps() + public async Task FooterThatWraps() { - var screen = ScreenBuilder.Build(ViewerSession.Resize(Fixtures.Document(), 60, 26)); - return Capture( - screen with + var grid = window!.MeasureGrid(Wrapping(60, 100), 560, 560); + + // Pinned, as the grid of the other scenes is, so the baseline does not move with a + // measurement: this says the pin is what the window reports + await Assert.That(grid).IsEqualTo((60, 27)); + await Capture(Wrapping(grid.Columns, grid.Rows), 560, 560); + } + + /// + /// The grid a capture is told is what its footer leaves: a footer of two rows of buttons and + /// a status under them leaves fewer rows than the same window with nothing in its footer, + /// and no fewer columns. + /// + [Test] + public async Task ACaptureIsToldTheRowsItsFooterLeaves() + { + var wrapping = Wrapping(60, 100); + var bare = window!.MeasureGrid( + wrapping with { - Status = longStatus + Buttons = [], + Status = "" }, 560, 560); + var under = window.MeasureGrid(wrapping, 560, 560); + + await Assert.That(under.Columns).IsEqualTo(bare.Columns); + await Assert.That(under.Rows).IsLessThan(bare.Rows); } + static Screen Wrapping(int columns, int rows) => + ScreenBuilder.Build(ViewerSession.Resize(Fixtures.Document(), columns, rows)) with + { + Status = longStatus + }; + static Task Capture(SessionState state) => Capture(ScreenBuilder.Build(ViewerSession.Resize(state, columns, rows))); diff --git a/src/DiffEngineViewer.Windows/FormsViewerWindow.cs b/src/DiffEngineViewer.Windows/FormsViewerWindow.cs index 99d7b891..75e46d11 100644 --- a/src/DiffEngineViewer.Windows/FormsViewerWindow.cs +++ b/src/DiffEngineViewer.Windows/FormsViewerWindow.cs @@ -174,6 +174,42 @@ public bool Capture(Screen screen, int width, int height, string pngPath) return false; } + return Sized( + screen, + width, + height, + () => + { + // Invalidate only marks dirty; the paint has to have happened before the bitmap. + form.Surface.Refresh(); + + using var bitmap = new Bitmap(width, height); + form.Surface.DrawToBitmap(bitmap, new(0, 0, width, height)); + bitmap.Save(pngPath, DrawingImageFormat.Png); + return true; + }); + } + + /// + /// The grid a capture of at this size draws: the window's cells + /// once the footer that screen's buttons and status come to has been laid out. + /// + /// A capture is handed a screen already built, and built for the whole window it shows only + /// the rows that fit over its footer: a footer of two rows of buttons and a status under + /// them left the last rows of the screen undrawn. The window never does that, since it + /// reports its grid every frame and is handed a screen sliced to it. This is that report + /// for a capture, so its caller can build the screen again for the rows there are. What a + /// status says can turn on the rows, so the footer is the first screen's. + /// + /// + internal (int Columns, int Rows) MeasureGrid(Screen screen, int width, int height) => + Sized(screen, width, height, () => form.Grid); + + /// + /// With the form showing at this size, as a capture has it. + /// + T Sized(Screen screen, int width, int height, Func read) + { // DrawToBitmap sends a paint message, and a window that has never been shown does not // answer one: the result is a correctly sized image of nothing. Shown off to the side // rather than at the default position, and without being activated: off to the side it @@ -197,12 +233,7 @@ public bool Capture(Screen screen, int width, int height, string pngPath) form.ClientSize = new(width, height); form.Apply(screen); form.PerformLayout(); - // Invalidate only marks dirty; the paint has to have happened before the bitmap. - form.Surface.Refresh(); - - using var bitmap = new Bitmap(width, height); - form.Surface.DrawToBitmap(bitmap, new(0, 0, width, height)); - bitmap.Save(pngPath, DrawingImageFormat.Png); + return read(); } finally { @@ -213,8 +244,6 @@ public bool Capture(Screen screen, int width, int height, string pngPath) form.Parked = false; } } - - return true; } public void Dispose() diff --git a/src/DiffEngineViewer.Windows/ViewerCanvas.cs b/src/DiffEngineViewer.Windows/ViewerCanvas.cs index 575f0684..4923e6a6 100644 --- a/src/DiffEngineViewer.Windows/ViewerCanvas.cs +++ b/src/DiffEngineViewer.Windows/ViewerCanvas.cs @@ -482,8 +482,9 @@ protected override void OnPaint(PaintEventArgs e) Painter.Draw(graphics, $"Pending ({screen.PendingCount})", font, Palette.Text, Cellular(padding, headerTop, queue, lineHeight)); } - Painter.Draw(graphics, screen.Left.Header, font, Palette.Text, Cellular(panesLeft, headerTop, half, lineHeight)); - Painter.Draw(graphics, screen.Right.Header, font, Palette.Text, Cellular(panesLeft + half, headerTop, half, lineHeight)); + // The left header stops a gap short of the right one, as the left picture does + DrawHeader(graphics, screen.Left.Header, Cellular(panesLeft, headerTop, half - gap, lineHeight)); + DrawHeader(graphics, screen.Right.Header, Cellular(panesLeft + half, headerTop, panesWidth - half, lineHeight)); DrawRule(graphics, headerTop + lineHeight + gap); var bodyTop = BodyTop; @@ -1007,6 +1008,35 @@ void DrawTitle(Graphics graphics, int lineHeight) Painter.Draw(graphics, screen.Subtitle, font, Palette.Dim, Cellular(Width - padding - width, padding, width, lineHeight)); } + /// + /// A pane's header, which one too long for its pane ends in an ellipsis, in the last whole + /// cell the pane has, as the Linux head's table cuts its own. Clipped at the pane's edge and + /// nothing more, the left one stopped at the very pixel the right one starts on, part way + /// through a character, and the two read as one line: "(page 1 ofsample.verified.pdf". + /// + void DrawHeader(Graphics graphics, string header, RectangleF bounds) => + Painter.Draw(graphics, HeaderShown(header, (int) (bounds.Width / Advance)), font, Palette.Text, bounds); + + internal static string HeaderShown(string header, int cells) + { + if (CellGrid.Cells(header) <= cells) + { + return header; + } + + // A row is cut after the character its last cell falls in, which for one two cells wide + // is a cell past what was asked for. Here that cell is the ellipsis's, so that character + // goes instead: it starts in the cell before, which is where the cut then lands. + var room = Math.Max(0, cells - 1); + var kept = RowText.Shown(header, room); + if (CellGrid.Cells(kept) > room) + { + kept = RowText.Shown(header, room - 1); + } + + return kept + "…"; + } + void DrawQueueItem(Graphics graphics, int index, Rectangle bounds) { if (index >= screen!.Queue.Count) diff --git a/src/DiffEngineViewer.Windows/ViewerForm.cs b/src/DiffEngineViewer.Windows/ViewerForm.cs index 2f46b763..fc5eda77 100644 --- a/src/DiffEngineViewer.Windows/ViewerForm.cs +++ b/src/DiffEngineViewer.Windows/ViewerForm.cs @@ -814,6 +814,15 @@ void ApplyButtons(Screen screen) } } + /// + /// The cells the canvas has now, under whatever the footer has come to. ScreenBuilder + /// subtracts Chrome to get the body, so adding it back asks for exactly the rows the canvas + /// can draw rather than a guess from a fixed cell height. + /// + [DesignerSerializationVisibility(DesignerSerializationVisibility.Hidden)] + public (int Columns, int Rows) Grid => + (canvas.ColumnCapacity, canvas.BodyCapacity + ScreenBuilder.Chrome); + public ViewerInput Drain() { var drag = canvas.TakeDrag(); @@ -830,10 +839,8 @@ public ViewerInput Drain() ClickedQueueItem: next.QueueItem, ScrollDelta: scrollDelta, CloseRequested: closeRequested, - Columns: canvas.ColumnCapacity, - // ScreenBuilder subtracts Chrome to get the body, so adding it back asks for exactly - // the rows the canvas can draw rather than a guess from a fixed cell height. - Rows: canvas.BodyCapacity + ScreenBuilder.Chrome, + Columns: Grid.Columns, + Rows: Grid.Rows, RightClickedQueueItem: next.RightClickedQueueItem, ClickedMenuItem: next.MenuItem, MenuClosed: next.MenuClosed, diff --git a/src/DiffEngineViewer/AcceptAllRunner.cs b/src/DiffEngineViewer/AcceptAllRunner.cs index 23ff1b2f..8b64d614 100644 --- a/src/DiffEngineViewer/AcceptAllRunner.cs +++ b/src/DiffEngineViewer/AcceptAllRunner.cs @@ -60,7 +60,10 @@ public void Start() Func record; try { - record = ViewerSession.ApplyClaimed(claimed, actions); + // With the session to ask, so a snapshot taken back while its file was + // waited for is not written. A read of the state and not a mutation: + // see ApplyClaimed + record = ViewerSession.ApplyClaimed(claimed, actions, () => host.State); } catch (Exception exception) { diff --git a/src/DiffEngineViewer/AcceptBatch.cs b/src/DiffEngineViewer/AcceptBatch.cs index 6c05936a..4f365364 100644 --- a/src/DiffEngineViewer/AcceptBatch.cs +++ b/src/DiffEngineViewer/AcceptBatch.cs @@ -106,7 +106,7 @@ Only is null || /// /// What the status line says while the batch runs. An accept's is /// , the words a window displaying someone else's batch - /// uses too; a discard's is this process's alone, since it is not put on a listing. + /// uses too, as it uses a discard's, which is on a listing as the same counts. /// public string Describe() { diff --git a/src/DiffEngineViewer/Ipc/MessageHandler.cs b/src/DiffEngineViewer/Ipc/MessageHandler.cs index c1c92417..85912c04 100644 --- a/src/DiffEngineViewer/Ipc/MessageHandler.cs +++ b/src/DiffEngineViewer/Ipc/MessageHandler.cs @@ -159,7 +159,10 @@ string IQueueOwner.ListingTag() generation++; } - return $"{instance}.{generation}.{state.ListedProgress?.Build()}"; + // Which kind of batch as well as how far: a discard begun as an accept-all ends can + // stand at the same counts over the same queue + var progress = state.ListedProgress; + return $"{instance}.{generation}.{progress?.Build()}{(progress?.Discarding == true ? ".discarding" : "")}"; } } diff --git a/src/DiffEngineViewer/Ipc/OwnerLink.cs b/src/DiffEngineViewer/Ipc/OwnerLink.cs index a3cf0633..8a0d6c74 100644 --- a/src/DiffEngineViewer/Ipc/OwnerLink.cs +++ b/src/DiffEngineViewer/Ipc/OwnerLink.cs @@ -434,13 +434,20 @@ List ReadChanges(ViewerResponse response) delete.SourceKey, delete.File, FileSide.Read(delete.File, documents))); - // Why the owner's accept-all would leave it, where it would, said on the entry as a - // failure is. A delete's status is nothing else here: this process applies nothing, - // so nothing of its own is ever recorded against an entry. The same entry when the - // owner says what it said before, for the reason Read gives - if (entry.Status != delete.Held) + // Why the owner's accept-all would leave it, where it would, said on the entry where a + // failure is, and marked as a hold rather than as one. A delete's status is nothing + // else here: this process applies nothing, so nothing of its own is ever recorded + // against an entry. The same entry when the owner says what it said before, for the + // reason Read gives + var isHold = delete.Held is not null; + if (entry.Status != delete.Held || + entry.StatusIsHold != isHold) { - entry = entry with { Status = delete.Held }; + entry = entry with + { + Status = delete.Held, + StatusIsHold = isHold + }; } // As on a move above diff --git a/src/DiffEngineViewer/QueueEntry.cs b/src/DiffEngineViewer/QueueEntry.cs index 69c64b96..ae9259db 100644 --- a/src/DiffEngineViewer/QueueEntry.cs +++ b/src/DiffEngineViewer/QueueEntry.cs @@ -177,11 +177,37 @@ public bool ShowsProperties(DrawingView drawing) => /// /// On a pending delete this process owns: a move has written its file since the delete was /// raised, and from then on no bulk accept carries the delete out (see - /// ). Gone when a run raises the delete again, which - /// is a statement made after the write. What TrackedDelete.Written is to the tray. + /// ). Gone when the delete is raised again over a + /// file that is no longer what the move left (), and not when it is + /// raised over the file as it was written. What TrackedDelete.Written is to the tray. /// public bool Written { get; init; } + /// + /// The file's stamp as the move left it, read when the move was carried out, or null when it + /// could not be read. What a delete raised again is asked against. + /// + public FileStamp? WrittenAs { get; init; } + + /// + /// On a pending delete of someone else's queue: is why its owner's + /// accept-all would leave it, which is all a delete's status ever is in a viewer that applies + /// nothing itself (). The owner's words are its own, so they cannot be + /// told from a failure by what they say. + /// + public bool StatusIsHold { get; init; } + + /// + /// Whether this is a delete a bulk accept leaves pending and says why, + /// rather than an entry that was tried and failed. The two are marked apart in the queue + /// column (): one is waiting to be accepted on its own, and the + /// other has something wrong with it. + /// + public bool Held => + Kind == QueueEntryKind.Delete && + Status is not null && + (StatusIsHold || ViewerSession.IsHold(Status)); + /// /// On a move or a delete: the key of the pending move it was derived from, or null when it /// stands alone. A page of a document, say, whose document is pending too. diff --git a/src/DiffEngineViewer/QueueProjection.cs b/src/DiffEngineViewer/QueueProjection.cs index a26ada96..7bf7b4d2 100644 --- a/src/DiffEngineViewer/QueueProjection.cs +++ b/src/DiffEngineViewer/QueueProjection.cs @@ -716,18 +716,43 @@ static QueueItem EntryRow( SessionState state, bool underTestHeader = false) => new( - entry.Conflicted ? $"{indent}* {text}" : $"{indent}{text}", + $"{indent}{Mark(entry)}{text}", index == state.Selected, - entry.Status, + // What every head marks as a failure, which a hold is not. Why it is held is still + // in the tip + entry.Held ? null : entry.Status, QueueRowKind.Entry, index) { Tooltip = Tooltip(entry, text, underTestHeader) }; + /// + /// What a held delete's label leads with. + /// + /// A delete a bulk accept left pending carried its reason as its status, and a row with a + /// status is drawn by every head as an entry that failed. Nothing failed: the delete is + /// waiting to be accepted on its own, and only the tooltip said which of the two a row was. + /// In the label, as the conflict marker is and for its reason, so no head and no ABI field + /// knows of it. A delete has one variant, so the two never meet. + /// + /// + public const string HeldMark = "~ "; + + static string Mark(QueueEntry entry) + { + if (entry.Conflicted) + { + return "* "; + } + + return entry.Held ? HeldMark : ""; + } + /// /// What the row cannot say for itself: the whole path behind a bare file name, the test behind - /// a call site, every framework behind one variant, and the failure behind a !. + /// a call site, every framework behind one variant, the failure behind a !, and why a + /// delete marked ~ is held. /// /// Null when all of that is already on the row. A tip that repeats its label has told the /// reader nothing, so on those rows there is no tip at all rather than an empty one. diff --git a/src/DiffEngineViewer/SessionState.cs b/src/DiffEngineViewer/SessionState.cs index 4cf881ba..99b422c7 100644 --- a/src/DiffEngineViewer/SessionState.cs +++ b/src/DiffEngineViewer/SessionState.cs @@ -200,12 +200,14 @@ public SessionState Showing(DrawingView view) Batch?.Progress ?? OwnerProgress; /// - /// The progress an owner puts on its listings: its accept-all's. Not a bulk discard's, which - /// the wire has no words for: is read by whoever displays the - /// queue as an accept under way, and says so on their status line. + /// The progress an owner puts on its listings: its accept-all's, or its bulk discard's said + /// to be one (). Whoever displays the queue words + /// their status line from it and refuses what changes the queue until it has gone. /// public AcceptProgress? ListedProgress => - Batch is { Discarding: true } ? null : Progress; + Batch is { Discarding: true } discard + ? discard.Progress with { Discarding = true } + : Progress; public QueueEntry? Current => Selected >= 0 && Selected < Queue.Count ? Queue[Selected] : null; diff --git a/src/DiffEngineViewer/TrackedEntry.cs b/src/DiffEngineViewer/TrackedEntry.cs index 294dcd41..dcf23563 100644 --- a/src/DiffEngineViewer/TrackedEntry.cs +++ b/src/DiffEngineViewer/TrackedEntry.cs @@ -129,10 +129,12 @@ public static QueueEntry DeleteAgain(QueueEntry queued, string file, DocumentPlu // Held because a move wrote this file, which is the very thing that has it read again // with something else in it: what it holds is another thing and why it is held is not. - // A run raising the delete again is what lets go of it (ViewerSession.EnqueueTracked) + // Whether a delete raised again lets go of it is asked where it is queued + // (ViewerSession.EnqueueTracked) return fresh with { Written = true, + WrittenAs = queued.WrittenAs, Status = queued.Status }; } diff --git a/src/DiffEngineViewer/ViewerActions.cs b/src/DiffEngineViewer/ViewerActions.cs index 4e506675..589812b0 100644 --- a/src/DiffEngineViewer/ViewerActions.cs +++ b/src/DiffEngineViewer/ViewerActions.cs @@ -36,18 +36,51 @@ record ViewerActions( /// public Func, IReadOnlyList>? ApplyInlineTogether { get; init; } - public IReadOnlyList ApplyTogether(IReadOnlyList patches) + /// + /// with a question: whether the patch at a position is + /// still wanted, asked once the file is patched in memory and before it is written, and a + /// patch that is not is left out of the write and answered + /// . See + /// . + /// + /// For one snapshot as much as for several: the wait a discard or a settle can land in is the + /// one for its file's lock, whoever else is in the file. + /// + /// + /// Null leaves the question unasked, which is what a caller that supplied only the others + /// gets, as before there was one: only knows the moment to ask. + /// + /// + public Func, Func, IReadOnlyList>? ApplyInlineWanted { get; init; } + + /// The snapshots of one source file, in the order they are applied. + /// + /// Whether the patch at a position is still to be written, or null where nothing can have + /// taken one back: a batch carried out inside one transition. + /// + public IReadOnlyList ApplyTogether(IReadOnlyList patches, Func? wanted = null) { + if (wanted is not null && + ApplyInlineWanted is { } asking) + { + return asking(patches, wanted); + } + if (ApplyInlineTogether != null && patches.Count > 1) { return ApplyInlineTogether(patches); } + // One at a time each is a write of its own, so the moment before each is the moment to + // ask about it var results = new List(patches.Count); - foreach (var patch in patches) + for (var index = 0; index < patches.Count; index++) { - results.Add(ApplyInline(patch)); + results.Add( + wanted is null || wanted(index) + ? ApplyInline(patches[index]) + : InlineApplyResult.Withdrawn); } return results; @@ -60,7 +93,8 @@ public IReadOnlyList ApplyTogether(IReadOnlyList { MoveFile = Move, DeleteFile = File.Delete, - ApplyInlineTogether = InlineApplier.ApplyAll + ApplyInlineTogether = InlineApplier.ApplyAll, + ApplyInlineWanted = InlineApplier.ApplyAll }; /// diff --git a/src/DiffEngineViewer/ViewerProgram.cs b/src/DiffEngineViewer/ViewerProgram.cs index 9f8b075d..fd69fdc0 100644 --- a/src/DiffEngineViewer/ViewerProgram.cs +++ b/src/DiffEngineViewer/ViewerProgram.cs @@ -72,9 +72,10 @@ static int RunInline(string? payloadFile, OpenWindow open, DocumentPlugin? docum // between the bind and the send. Whoever launched this was told the patch was // taken, and a refusal used to be read as a hand over, so it is staged rather // than dropped: this process is the only place it exists. - InlineStaging.Persist([new(patch)]); + var staged = InlineStaging.Persist([new(patch)]); Console.Error.WriteLine("A viewer holds the port but did not accept the patch."); - return 1; + // Said apart from a failure, so whoever launched this does not stage it again + return staged > 0 ? ViewerExit.Staged : 1; } return 0; @@ -166,10 +167,23 @@ static int RunDelete(string file, OpenWindow open, DocumentPlugin? documents, Vi var start = ViewerSession.EnqueueTracked( SessionState.Start(ViewerMode.Inline), TrackedEntry.ForDelete(file, documents)); - return Run(new(start), server, null, open, documents, preferences); + return ForAFile(Run(new(start), server, null, open, documents, preferences)); } } + /// + /// What a viewer started for a delete or a pair exits with. Never + /// : snapshots that reached it before its window failed are + /// staged, and that says nothing of the file it was started for, which no window is showing. + /// + internal static int ForAFile(int code) => + code == ViewerExit.Staged ? noWindow : code; + + /// + /// The window could not be opened, and not everything this viewer held is known to be staged. + /// + const int noWindow = 4; + /// /// One failing pair, owning the queue so more can join it. /// @@ -204,7 +218,7 @@ static int RunDiff(string temp, string target, OpenWindow open, DocumentPlugin? var start = ViewerSession.EnqueueTracked( SessionState.Start(ViewerMode.Inline), TrackedEntry.ForMove(temp, target, documents)); - return Run(new(start), server, null, open, documents, preferences); + return ForAFile(Run(new(start), server, null, open, documents, preferences)); } } @@ -281,8 +295,15 @@ internal static int Run( // memory. Staged instead, where accept tooling finds it. A display that is not there // or a native library that will not load are both ordinary on Linux, and each used to // cost every inline snapshot of the run. - PersistOwned(host.State, link); - return 4; + var held = host.State; + var staged = PersistOwned(held, link); + // Whoever launched this with a patch stages it itself when told the launch failed, a + // second trio beside the one just written. So where everything held is staged, the + // exit says that instead. Only then: one that could not be written is still nowhere, + // and a failure is the answer that has the launcher keep what it sent. + return staged > 0 && staged == OwnedVariants(held) + ? ViewerExit.Staged + : noWindow; } // One for the loop and for a head's modal loop, which draw the same window on one thread. @@ -402,6 +423,24 @@ internal static int PersistOwned(SessionState state, OwnerLink? link) .Select(_ => new PendingInline(_.Variants, _.Status))); } + /// + /// How many trios writes when every one of them can be written: + /// one for each variant of each snapshot. + /// + static int OwnedVariants(SessionState state) + { + var variants = 0; + foreach (var entry in state.Queue) + { + if (entry.Kind == QueueEntryKind.Inline) + { + variants += entry.Variants.Count; + } + } + + return variants; + } + /// /// How the window is now, for the next one to open as. Asked whenever the window is about to /// stop being on screen, rather than once at exit: a viewer hidden behind a tray stays hidden diff --git a/src/DiffEngineViewer/ViewerSession.cs b/src/DiffEngineViewer/ViewerSession.cs index 2ef5a346..c5df8352 100644 --- a/src/DiffEngineViewer/ViewerSession.cs +++ b/src/DiffEngineViewer/ViewerSession.cs @@ -165,17 +165,9 @@ public static SessionState EnqueueTracked(SessionState state, QueueEntry entry) { state = Withdraw(state, entry.TargetFile!); } - else if (entry.Written) + else { - // A delete raised again, built from the entry that was queued for it - // (TrackedEntry.DeleteAgain). Raised by a run that looked at the file as it is now, so - // whatever a move wrote there since the delete was first raised, this is the later - // statement - entry = entry with - { - Written = false, - Status = null - }; + entry = RaisedAgain(state.Queue, entry); } var existing = IndexOf(state.Queue, entry.Key); @@ -213,6 +205,55 @@ public static SessionState EnqueueTracked(SessionState state, QueueEntry entry) return Seen(next); } + /// + /// A delete that has arrived, with the hold of the one queued for the same file where that + /// still stands, and with none otherwise. + /// + /// A delete raised again says nothing about when it was decided. A run has a process for each + /// target framework, and one that looked at the file before the move was accepted raises the + /// delete after it, in the same words a run that looked at what the move wrote would use. + /// Taken for the later statement, the first let go of the hold, and the next accept-all + /// deleted what had just been accepted. So the hold stands while the file is as the move left + /// it, and goes only when it is seen to be something else: what the hold was keeping is then + /// no longer there to keep. A stamp that could not be read, then or now, is not a file seen + /// to differ. The tray's tracker decides it the same way (Tracker.AddDelete). + /// + /// + /// Asked of the entry in the queue rather than of what the arrival was built from + /// (), which was read outside the lock and may be from + /// before the move was carried out. + /// + /// + static QueueEntry RaisedAgain(IReadOnlyList queue, QueueEntry arrived) + { + var index = IndexOf(queue, arrived.Key); + if (index >= 0 && + queue[index] is { Kind: QueueEntryKind.Delete, Written: true } queued && + !(queued.WrittenAs is { } written && + arrived.LeftStamp is { } now && + now != written)) + { + return arrived with + { + Written = true, + WrittenAs = queued.WrittenAs, + Status = queued.Status + }; + } + + if (!arrived.Written) + { + return arrived; + } + + return arrived with + { + Written = false, + WrittenAs = null, + Status = null + }; + } + /// /// Drops the delete pending on a file a move has just arrived for. /// @@ -258,10 +299,11 @@ static SessionState Restaged(SessionState state, int index, QueueEntry entry) { LeftStamp = entry.LeftStamp, RightStamp = entry.RightStamp, - // A delete raised again is no longer held for what a move wrote before it was: see - // EnqueueTracked. Nothing but a delete is ever marked - Written = false, - Status = queued.Written ? null : queued.Status + // Whether a delete raised again is still held for what a move wrote: see RaisedAgain, + // which has said so on the arrival. Nothing but a delete is ever marked + Written = entry.Written, + WrittenAs = entry.WrittenAs, + Status = queued.Written && !entry.Written ? null : queued.Status }; return Clamp(state with { @@ -1546,7 +1588,19 @@ static IReadOnlyList TakeSameFile(IReadOnlyList queue, Q /// /// The state returned, which says what was claimed. /// What applies it. - public static Func ApplyClaimed(SessionState claimed, ViewerActions actions) + /// + /// The session's state as it is when asked, for a batch carried out a step at a time while + /// other threads change the queue. With it, a snapshot is asked about as its file is about to + /// be written (), and one that has been settled, discarded or + /// replaced since it was claimed is left out of the write. Null for a batch carried out + /// inside one transition, where nothing else can have touched the queue. + /// + /// Read, and never by taking the session's lock. The question is asked with the source + /// file's lock held, and a single accept arriving over the socket holds the session's lock + /// while it waits for that same file: each would wait on the other for good. + /// + /// + public static Func ApplyClaimed(SessionState claimed, ViewerActions actions, Func? current = null) { if (claimed.Batch is not { Current: { } entry } batch) { @@ -1555,12 +1609,13 @@ public static Func ApplyClaimed(SessionState claimed if (entry.Kind != QueueEntryKind.Inline) { - var failure = TryApplyTracked(entry, actions, batch.Discarding); - return _ => RecordTracked(_, entry, failure); + var failure = TryApplyTracked(entry, actions, batch.Discarding, out var wrote); + return _ => RecordTracked(_, entry, failure, wrote); } List entries = [entry, ..batch.Together]; - var results = actions.ApplyTogether(entries.Select(_ => _.Patch!).ToList()); + Func? wanted = current is null ? null : _ => StillClaimed(current().Queue, entries[_]); + var results = actions.ApplyTogether(entries.Select(_ => _.Patch!).ToList(), wanted); if (results.Count != entries.Count) { throw new InvalidOperationException($"{entries.Count} snapshots were applied together and {results.Count} outcomes came back."); @@ -1569,6 +1624,32 @@ public static Func ApplyClaimed(SessionState claimed return _ => RecordInline(_, entries, results); } + /// + /// Whether a snapshot a batch claimed is still in the queue as it was claimed: found by its + /// variants, which is how recording its outcome finds it. + /// + /// Not there, it was discarded, or settled by a test that started passing, and its patch + /// would put back a literal nobody wants. There under other variants, a re-run replaced its + /// content, a second framework made a conflict of it, or one of its frameworks settled: what + /// is pending now is another entry, which a bulk accept would not have recorded an outcome + /// against either. Written anyway, both went into the source with the rest of their file and + /// were counted nowhere. + /// + /// + static bool StillClaimed(IReadOnlyList queue, QueueEntry claimed) + { + foreach (var entry in queue) + { + if (entry.Kind == QueueEntryKind.Inline && + ReferenceEquals(entry.Variants, claimed.Variants)) + { + return true; + } + } + + return false; + } + /// /// The transition for a claim whose apply threw rather than answering. InlineApplier answers /// every failure it knows of, so this is an applier that did not, and a batch left holding @@ -1754,7 +1835,11 @@ state with /// one a re-run staged over it since is news, and neither the file operation nor its failure /// was about that one. /// - static SessionState RecordTracked(SessionState state, QueueEntry entry, string? failure) + /// The state to record it in. + /// The entry that was claimed. + /// Why it could not be carried out, or null when it was. + /// For a move carried out, the stamp of the file it wrote. + static SessionState RecordTracked(SessionState state, QueueEntry entry, string? failure, FileStamp? wrote = null) { if (state.Batch is not { } batch) { @@ -1769,7 +1854,7 @@ static SessionState RecordTracked(SessionState state, QueueEntry entry, string? // Whether or not its entry is still there: the file was written either way if (!batch.Discarding) { - queue = MarkWritten(queue, entry); + queue = MarkWritten(queue, entry, wrote); } return Remove( @@ -1884,7 +1969,13 @@ static string OfDocument(string did, string document, int swept, int kept) /// as written and says why it is held from here on (). The same list /// when there is none, or when what was carried out was not a move. /// - static IReadOnlyList MarkWritten(IReadOnlyList queue, QueueEntry accepted) + /// The list as it stands. + /// The entry that was carried out. + /// + /// The stamp of the file the move wrote, for the delete to remember + /// (). + /// + static IReadOnlyList MarkWritten(IReadOnlyList queue, QueueEntry accepted, FileStamp? wrote) { if (accepted is not { Kind: QueueEntryKind.Move, TargetFile: { } target }) { @@ -1902,6 +1993,7 @@ static IReadOnlyList MarkWritten(IReadOnlyList queue, Qu marked[index] = entry with { Written = true, + WrittenAs = wrote, Status = WroteItsFile }; } @@ -1963,6 +2055,14 @@ 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 a status is one of the reasons a bulk accept of a queue this process owns leaves a + /// delete pending, rather than what a failed accept said. For the mark on its row + /// (). + /// + public static bool IsHold(string status) => + status is deleteHeld or WroteItsFile or AwaitsItsFile; + /// /// Why a bulk accept leaves a pending delete where it is, when it would: null when it would /// carry it out. The tray's Tracker.HeldReason, for a queue this process owns, and what @@ -2006,8 +2106,13 @@ static string WithFiles(string message, int swept, int kept) /// wrote the file, so that a second accept-all, or one after the move was accepted on its own, /// does not delete what was just accepted. /// + /// + /// It does not offer running the tests again, which used to let go of the hold: see + /// . The price is that a delete a later run truly wants stays held + /// until it is accepted on its own. + /// /// - public const string WroteItsFile = "Held: a move was accepted onto this file after the delete was raised, so deleting it would remove what was just accepted. Accept it on its own to delete it anyway, or run the tests again."; + public const string WroteItsFile = "Held: a move was accepted onto this file after the delete was raised, so deleting it would remove what was just accepted. Accept it on its own to delete it anyway."; /// /// A move still pending is going to write the file. Not remembered: it is true for as long as @@ -2031,8 +2136,17 @@ static string WithFiles(string message, int swept, int kept) /// inline apply does. /// /// - static string? TryApplyTracked(QueueEntry entry, ViewerActions actions, bool discarding) + /// The move or delete. + /// What carries it out. + /// Whether it is being discarded rather than accepted. + /// + /// For a move that was accepted, the stamp of the file it wrote, read here because this is + /// where the file is touched: what a delete pending on that file is later asked against + /// (). Null for anything else, and when it could not be read. + /// + static string? TryApplyTracked(QueueEntry entry, ViewerActions actions, bool discarding, out FileStamp? wrote) { + wrote = null; try { if (discarding) @@ -2048,6 +2162,7 @@ static string WithFiles(string message, int swept, int kept) if (entry.Kind == QueueEntryKind.Move) { actions.MoveFile(entry.LeftFile!, entry.TargetFile!); + wrote = FileSide.StampOf(entry.TargetFile!); } else { @@ -2075,7 +2190,7 @@ static SessionState ApplyTracked( bool discarding, string done) { - if (TryApplyTracked(entry, actions, discarding) is { } failure) + if (TryApplyTracked(entry, actions, discarding, out var wrote) is { } failure) { var queue = state.Queue .Select(_ => _.Key == entry.Key ? _ with { Status = failure } : _) @@ -2091,7 +2206,7 @@ static SessionState ApplyTracked( IReadOnlyList left = state.Queue.Where(_ => _.Key != entry.Key).ToList(); if (!discarding) { - left = MarkWritten(left, entry); + left = MarkWritten(left, entry, wrote); } return Remove(state, left, done); diff --git a/todo.md b/todo.md index 3ab470fd..7da2b314 100644 --- a/todo.md +++ b/todo.md @@ -8,7 +8,8 @@ An item with no tag was said by whoever made the fix it follows from. A tag says ## Library - [ ] A viewer that is alive and never binds the port is still reported as launched after `BindWait`, and its payload file stays. That is the apphost's "install .NET" dialog, which does not exit. At the gate it cannot be told from a viewer that is only slow. -- [ ] A viewer that exits 1 or 4 has tried to stage the patch itself, and the caller, told the launch failed, stages it too: two trios in different `VerifyInline` folders until a passing run clears both. The gate cannot tell staged from tried, because the viewer discards `InlineStaging.Persist`'s count: it needs an exit code of its own for "staged", from `ViewerProgram`. +- [ ] A viewer that staged its patch exits 5 and `AddInlineAsync` answers `InlineResult.Staged`, a new public member: a consumer whose switch throws on a value it does not know would be affected. Verify gives no message and no staged path for it, so its failure says nothing about where the snapshot went: the viewer's trio is under the source project's `obj/VerifyInline`, not Verify's intermediate directory. Not run end to end through a real viewer process exiting 5, only as the gate against a process that exits with each code and the viewer's own exit. +- [ ] A viewer that binds the port and then fails to open its window can be probed in between. The gate then says launched and the caller hears `Queued`, though the snapshot is staged and not queued. - [ ] On macOS and Linux a tool started without ShellExecute still inherits the test host's streams. Nothing in the definitions tells a terminal tool, which needs them, from a windowed one: Neovim is declared `UseShellExecute: true` like the rest. - [ ] `DiffRunner.LaunchProcess` still starts a third party tool in the test host's working directory, which then cannot be deleted while the tool is open. Left alone because a tool resolves relative arguments against it and `ProcessCleanup` matches on those same strings. - [ ] None of the four tools started through `WindowsProcess.StartInheritingNothing` (Word and Excel comparers, Cursor, VS Code) was itself run. A console exe, a windowed exe and a `.cmd` stood in for them. @@ -42,10 +43,10 @@ An item with no tag was said by whoever made the fix it follows from. A tag says ## Tray -- [ ] The hold on a delete is let go when the delete is raised again, which another framework's process of the same run, having decided before the move was accepted, can do. True of the tray's tracker and of an owning viewer's queue (`ViewerSession.EnqueueTracked`) alike. -- [ ] A tray's listing taken while a move is out of `moves` being accepted, before `MarkWritten`, says the delete on its target is not held. A group accept sent from an attached viewer inside that one poll interval still sends the delete's key, after the move has written the file. `Tracker.HeldReason` does not know of a move in flight. (read, not run) -- [ ] An owning viewer's delete kept by a batch because a move was still pending keeps that status text if the move is later discarded. `ViewerSession.HeldReason`, the listing and the next batch are right; only the row's tooltip is stale. -- [ ] A held delete's row carries the same ` !` mark as a failed entry in every renderer, on an owning and an attached viewer. Only the tooltip tells them apart. +- [ ] A delete held because a move wrote its file stays held when a later run raises it again over the unchanged file: a process that decided before the move and a run after it send the same message. It goes when accepted on its own, discarded and raised afresh, or raised over a file written since. And that last lets it go whoever raised it: a process that decided before the move, arriving after someone edited the file, releases it too. +- [ ] An owning viewer's delete kept by a batch because a move was still pending keeps that status text, and its `~` mark, if the move is later discarded. `ViewerSession.HeldReason`, the listing and the next batch are right; only the row is stale. +- [ ] A held delete is marked `~`, leading in the viewer's row, where the queue column cuts what trails, and trailing in the tray's menu. The character and the two positions were chosen, not asked for. +- [ ] A tray driving a viewer-owned queue (`RemoteInlineHost`) reads no progress from its listings, so its menu offers accept and discard during the viewer's accept-all or bulk discard. A discard-all in a tray that owns the queue is not a batch and has no progress. An older reader of a listing takes a discard under way for an accept: the wrong word, and it still refuses what changes the queue. - [ ] A move arriving in an owning viewer withdraws the delete pending on its target, which can be the entry on screen, and closes an open menu as any removal does. The withdrawal and the mark compare paths as the file system does (`InlineKey.SamePath`), case sensitive on Linux, where the tray's rule ignores case throughout. Not run off Windows. - [ ] A batch that begins between Verify raising a delete and queueing its patch can still carry out the delete without the patch. Closing that needs the two tied together on the wire. - [ ] The session ending, which stages the queue and removes the version marker, was exercised by sending `WM_QUERYENDSESSION` and `WM_ENDSESSION` to the window, not by logging off, and its wiring in `Program.Inner` is read, not run. @@ -53,7 +54,6 @@ 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. -- [ ] `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. @@ -61,12 +61,12 @@ An item with no tag was said by whoever made the fix it follows from. A tag says - [ ] A pair is still read and diffed before the viewer answers the test process that sent it (`MessageHandler.TrackMove`). The diff is bounded, so what is left is two reads: a million lines a side, shuffled, is about two of the sender's three seconds. Answering first and filling the entry in afterwards was looked at and left: `Refresh` does not open an entry at its first change, a second arrival of the pair while unread would be compared against a placeholder, the watch that would fill it has a budget and a hidden cadence, and every small pair would flash an empty pane. - [ ] A queue of more than a hundred pending files is looked at a hundred a pass, so a row that is not on screen follows its file within `count / 100` passes. Looking every pass at the rows the queue column shows would be better, and was judged not worth the watch knowing the body's height and what is folded. -- [ ] A snapshot discarded, or settled by a test that started passing, while its own source file is being written by a bulk accept is written with the rest of the file. It is not counted. Closing it takes `InlineApplier.ApplyAll` asking, for each patch after the file is patched in memory and before its one write, whether it is still wanted, with the core answering from whether the claimed entry is still queued. +- [ ] A snapshot discarded or settled after a bulk accept has asked whether it is still wanted, and before the write lands, is still written. The gap is now the encode and the swap, not the wait for the file's lock. The viewer's answer is a scan of the queue for each patch asked about, not indexed, and a question that throws is taken as wanted. +- [ ] A single accept of another snapshot in the same file, landing while a batch waits for that file, takes the batch's claimed entries to other lines. They are then no longer the entries claimed, so they are left out of the write and stay queued for a second accept. Before, they were written and stayed queued anyway. - [ ] A change to one entry still costs by the queue's length, a tenth of what it did: 0.1 to 0.24 ms at 2,000 entries, in asking `InlineQueue`, which copies its list and builds a key an item. The first snapshot of a solution, and a removal that leaves a solution with only files, still rebuild the whole list, and an attached viewer's `Sync` still projects the whole queue for each listing that changed. - [ ] Queue labels are kept against the queue's list, so a list changed after it is put in a state would show stale labels. Nothing in the core does that. - [ ] `ScreenPayload` cuts a row at half the window and a cell, on both native heads splitting the panes equally, which was read and not run. A head that stops doing that has to change `Screen.PaneCells`. A changed frame of CJK rows is still 1.7 ms at 4K, in classifying each cluster. - [ ] Past its budget a diff goes by the lines that occur once on each side. Texts made mostly of repeated lines have few, and are still split wherever the search stopped. -- [ ] A bulk discard under way in an owning viewer is not on its listings, so an attached window or the tray sees the queue shrink with no progress and refuses nothing meanwhile. A discard-all from the tray during an accept-all now waits for it to end. - [ ] `ScreenBuilder.Build` for an entry of 100,000 lines is 1.5 to 2.5 ms a screen. (noticed, cause not found) - [ ] The rows a native head reports now depend on its footer, and the status line depends on the rows ("lines 1-N"). At a width where one character of status decides whether it gets a line of its own, the two could alternate frame to frame. Not seen; the WinForms head has always had the same loop. - [ ] A drag in the pane that can go less far, from a centre beyond its own range, moves only the other pane's picture until the centre is back inside its range. All three heads. @@ -82,12 +82,12 @@ An item with no tag was said by whoever made the fix it follows from. A tag says ## Viewer, Windows head - [ ] A status loses what is past three lines under the buttons, behind an ellipsis and a tooltip. The footer was not looked at on a scaled display; its tests enlarge the font instead. -- [ ] 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. +- [ ] A capture still draws the screen it is handed, so a caller that builds for the whole window loses its last rows under a tall footer. `FormsViewerWindow.MeasureGrid` says how many there are, and only `FooterThatWraps` asks. `IViewerWindow.Capture` would have to take a state, or the grid, for it to hold by construction. That test pins the grid reported, 60 by 27, which turns on the footer buttons' sizes in the system font: seen on one machine before CI. - [ ] 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. - [ ] 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) +- [ ] `Fixtures.SolutionFile` and `Fixtures.WriteImage` still use fixed folders under temp (`deview-fixtures`, `deview-fixture-images`), on purpose. `WriteImage` no longer rewrites a file that holds the same bytes; a machine's first two runs at once can still meet at the first write. +- [ ] The pixel suite's 0.9999 let a header cut about 70 pixels differently pass against the old baseline of `FooterThatWraps`, which was taken again by hand. A header's ellipsis is drawn by GDI+ in the head's font, and was not asked of `FontCoverage`. - [ ] 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. - [ ] A window wholly behind another is not reported as unseen; only a minimised one is. @@ -121,6 +121,7 @@ An item with no tag was said by whoever made the fix it follows from. A tag says - Text outside Latin: `--diff` two files of Chinese and hold Down. The same symbol for each frame: the new row alone. - An enlarged picture: `--diff` two 2880 by 1800 screenshots, `+` once, and drag. Time under `Renderer.enlarged` for each frame: a blit, and one 1560 by 975 bitmap a pane about a tenth of a second after the step. - [ ] To confirm on a Mac, from the two rounds of smaller fixes: ten changes, eight of them event handling or window state that no capture exercises. The check for each is in its commit's message. +- [ ] A left pane header too long for its pane is drawn into the whole half and meets the right one, as the Windows head's did (`Renderer.swift`, the two `text(frame.*.header …)` calls). (read, not seen) - [ ] To confirm on a Mac, from the ABI round. CI's job only captures, so a green build exercises none of the three: - A PDF pair in a window narrow enough for four rows of buttons has its last body row drawn above the footer, and "lines 1-N" in the status drops as the footer grows. - Miniaturise or wholly cover an owning viewer and rewrite a pending received file: the pane follows about a second later rather than within 200 ms, and at once after the window is uncovered.