Close the remaining ways a queued snapshot or file is wrong or misreported, and four Windows head and test items - #932
Merged
Merged
Conversation
Two tests built a KeyRegister with the constructor that registers with Windows. Both bind only a key name that does not parse, which stops before anything is asked of the desktop, so nothing reached RegisterHotKey: but that held by what the tests happened to bind rather than by how they were built, and a hot key is taken from every process on the machine. They now build the register over a stand-in that writes down what it is asked, as KeyRegisterTests and OptionsFormLauncherTests do, and assert it was asked nothing. No change to KeyRegister. Checked by running KeyNameTests in Release.
ImageCacheTests wrote every picture to deview-image-cache under the temp folder, and FormsHeadTests.EveryPictureEverDrawnStaysDecoded wrote, moved and then deleted deview-review-cache. Two runs of the suite at once on one machine rewrote and deleted each other's files and failed each other. ImageCacheTests now has a folder made for the class by CreateTempSubdirectory and deleted after it: its tests each write a file of their own name, so within a run they never met. The FormsHeadTests test has a folder for itself, deleted in a finally, where a failure used to leave it behind. Running the suite twice at once then failed RepaintingAPictureComposesItOnce in one of the two, four composes for two. Fixtures.WriteImage, which this project links, wrote its picture on every call: the same bytes under a new write time, which is a rewritten file to whatever had decoded it. Its folder and names stay fixed, as its comment asks, and it now leaves a file alone that already holds the bytes. The rest of the project already used a folder or a file name of its own for each test. Checked by running DiffEngineViewer.Windows.Tests twice at the same time, twice over: one of the first pair failed before the Fixtures change, and all four runs pass after it.
The WinForms canvas drew each pane's header into a rectangle as wide as half the panes and left the clipping to GDI+. A left header too long for its pane was cut at the very pixel the right pane starts on, part way through a character, so in a window about 560 wide the two read as one line: "sample.received.pdf (page 1 ofsample.verified.pdf (page 1 of". The left header's rectangle now stops a gap short of the right pane, as the left picture's does, and a header with more cells than its pane has whole is cut to them with an ellipsis in the last. That is how the Linux head's table cuts its own (RenderTextEllipsis at the column's edge). The gap alone was not enough: four pixels beside a cut character is less than the space between two words. A header that fits is drawn as it was, so of the baselines only FooterThatWraps moves, and it now reads "(page 1 ... sample.verified.pdf (page 1 o...". Its difference was inside the suite's tolerance, so it was taken again by hand and looked at. ALongLeftHeaderStopsShortOfTheRightOne reads the ink of each header back from a paint, and found the two meeting, 0 pixels apart, with the left header drawn the old way.
A capture is handed a screen already built. Built for the whole window, a footer taller than one row left its last rows undrawn, since the canvas draws only what fits over the footer. The window never does that: it reports its grid every frame and is handed a screen sliced to it. FormsViewerWindow.MeasureGrid is that report for a capture: the form sized and the screen's buttons and status laid out, as Capture does them, and the canvas's cells read back. Capture and MeasureGrid share the showing and the sizing, and ViewerForm.Grid is what Drain reports too, so the two cannot come to differ. Capture itself still draws the screen it is given, since IViewerWindow.Capture takes a screen and not a state to build one from. FooterThatWraps asks rather than working the rows out by hand, and pins what it is told: 27 rows, where the 26 written there was a row short. A document's text takes half the body rounded down, nine rows either way, so the baseline is the same bytes. ACaptureIsToldTheRowsItsFooterLeaves holds the measurement to fewer rows under two rows of buttons and a status than under an empty footer. It could not be run against the code before, which had nothing to ask.
… does not stage it again A viewer that could not show what it was started with stages it before it goes: one whose window would not open, and one that found the port held by an owner that then refused the patch. It exited 4 or 1, which the launch gate reads as a launch that failed, and AddInlineAsync answered NoViewerFound, on which Verify stages a trio of its own. So the one snapshot was staged twice, in two VerifyInline directories, until a passing run cleared both. The viewer knew whether it had staged, from the count InlineStaging.Persist returns, and threw that away. ViewerProgram now exits with ViewerExit.Staged (5) where every snapshot it held was written, and with what it used to where one was not, since that one is still nowhere and a failure is what has the launcher keep its patch. A viewer started for a delete or a pair never says it: staging snapshots other processes sent says nothing of the file it was started for. ViewerLaunchGate tells that exit from a failure (ViewerLaunchOutcome.Staged), gives the MaxInstance slot back as it does for a failure, and AddInlineAsync reports InlineResult.Staged, a new member. Verify stages only on NoViewerFound, so it stages nothing for it, with no change on its side. Between versions the code is a failure like any other. An older viewer never returns it and its caller stages as before. An older library reads 5 as it reads any exit that is not zero and stages too, which is what both did until now. So ViewerContract asks for no newer copy. Checked by ViewerLaunchGateTests, against a real process that exits with each code, sync and async, and ViewerProgramTests, against a real project directory. With the gate and the exit put back as they were, the two gate tests fail with Failed where Staged is expected and the viewer test with 4 where 5 is.
…lk accept's write A bulk accept claims a file's snapshots under its host's lock and writes them outside it, and the wait between is the file's own lock, up to ten seconds of it. A snapshot discarded in that time, or settled by a test that started passing, or replaced by a re-run, had already been handed over: it was written with the rest of its file, and then not found to be counted. The reviewer threw it away and found it in the source. InlineApplier.ApplyAll has an overload that takes a question, asked of each patch that edits once the file is patched in memory and before its one write, with the file's lock held. A patch that is no longer wanted is not taken 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, MovedFrom and MovedBy included, which is what InlineQueue.Rebased brings the rest of the file along by. A patch that only edits once another is taken back is asked about then. The unwanted patch is InlineApplyStatus.Withdrawn, a new member only that overload reports, and AcceptInBatch counts nothing for it: not accepted, not failed, and it holds no delete. The viewer's batch asks whether the claimed entry is still queued under the variants it was claimed with, which is how its outcome is found afterwards. It reads the session's state and does not take the session's lock: a single accept arriving over the socket applies inside that lock, so it holds it while it waits for the same file, and a question that took it would leave the two waiting on each other for good. The tray's AcceptEvery asks under its gate, which nothing holds while applying. An applier that takes patches one at a time, a test's, is asked before each. Checked against real files: InlineApplierBatchTests holds the file and every outcome to those of the same batch without the patches taken back, over a batch where patches depend on one another, and WithdrawnSnapshotTests and OwnedInlineHostTest discard, settle and replace an entry between the claim and the write. With the question unasked those fail with the snapshot in the source. TheQuestionDoesNotWaitOnTheSessionsLock times out when the question takes the lock. The threaded ones were run five times with DOTNET_PROCESSOR_COUNT=2. AnEntryDiscardedWhileItsOwnFileIsWrittenIsNotCounted, which pinned the old behaviour, now says the entry is not written.
An accept takes its move out of the tray's pending moves for as long as the move takes, seconds when a file is locked, and marks the delete on its target only once the file is written. In between nothing tracked named the file, so Tracker.HeldReason said the delete was held by nothing. A listing taken then said the same, and a viewer attached to the tray sent the delete's key in a group accept, after the move had written the file. The tracker now counts the files that moves being accepted are onto. A move is in that count from before it leaves the pending moves until it has marked its delete, been put back or been dropped, and HeldReason and a listing's deletes ask it on both sides of their walk of the pending moves, since a move goes from one to the other. Leaving the count is counted as a change for the listing's tag: a move dropped with nothing to move leaves a delete no longer held and the same objects tracked. Checked by asking from inside an accept, where a refused move is reported before it is put back: the tracker directly, and a full listing over a socket from a tray that owns the queue. Both said null before the change.
A delete whose file a move wrote is held from every accept-all, and the hold was let go when the delete was raised again, as the later statement about the file. But 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 later run would use. That let go of the hold, and the next accept-all deleted what had just been accepted. The message alone cannot tell the two apart, so the hold no longer turns on it. The file's stamp is read as the move writes it and kept on the delete, and a delete raised again lets go of the hold only when the file is seen to differ from that: what the hold was keeping is then no longer there. A stamp that could not be read, then or now, keeps the hold. The tray's tracker and an owning viewer's queue decide it the same way, and the viewer decides it from the entry in its queue rather than from the one the arrival was built from, which is read outside the lock and may be from before the move. The price is that a delete a later run truly wants stays held until it is accepted on its own, or discarded and raised afresh. Both reasons shown beside a held delete said "or run the tests again", which is no longer so, and no longer say it. Checked with a tracker, with a session over real files and over none, and over a socket in both arrangements in TrayViewerSyncTest. The tests that held raising a delete again to letting go of it now hold it to the rule above, and fail against the code before it.
A delete a bulk accept leaves pending carries its reason as its status, and every renderer draws a row with a status as an entry that failed: " !" after the label, in the colour of a removed line. Nothing failed. The delete is waiting to be accepted on its own, and only the tooltip said which of the two a row was. The tray's menu marked both with "!" as well. A held delete's row now leads with "~ " and is handed to the heads with no status, so none of them draws it as a failure. In the label, as the conflict marker is, so no head and no ABI field knows of it, and leading, so it is still there in a column too narrow for the name. The reason stays in the tip. Which statuses are holds is known by what they are in a queue this process owns, and said by OwnerLink for someone else's, whose words for a hold are its own. A delete that was tried and could not be deleted is still drawn as the failure it is. The tray's menu marks a held delete with the same character. Checked on the rows and the text rendering of an owning viewer and of an attached one, over a socket in both arrangements, and on the tray's menu items. No text snapshot showed a held delete, so none moved.
A bulk discard in a viewer that owns the queue is a batch, a received file thrown away a step, and was left off the owner's listings because 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: the same progress counts, and a "discarding" line beside them, which AcceptProgress carries as a flag. A window showing the queue says "Discarding n of m", the words the owner's own window uses, and refuses what changes the queue until the batch has gone, as it does for an accept-all. A reader from before the line skips it, as it skips any name it does not know, and takes the batch for an accept: the wrong word, and the same refusals. The line is read after the counts are, so it need not follow them, and with no counts it says nothing. The listing's tag says which kind of batch as well as how far. Checked in the protocol's round trip, including what an older reader is left with; from inside each delete of a discard asked of the owner, where each listing says how far it has got; and over a socket in TrayViewerSyncTest, into a window attached to the owning viewer. The test that held a discard to being on no listing now holds it to this.
The staged exit, the snapshot withdrawn from a bulk write, the four held delete and discard items and the four Windows and test items go, replaced by what each left. claude.md and the docs gain the staged result, the question a bulk accept asks before its write, the hold that outlasts a stale re-raise, the mark for a held delete and a discard on the listings. One comment in AcceptBatch said a discard is on no listing, which is no longer so.
# Conflicts: # src/DiffEngineTray/Tracker.cs # src/DiffEngineViewer/QueueEntry.cs
# Conflicts: # src/DiffEngineTray/Tracker.cs
# Conflicts: # src/DiffEngineViewer.Tests/DerivedFilesTests.cs # src/DiffEngineViewer/ViewerActions.cs
This was referenced Oct 5, 2026
Merged
This was referenced Oct 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two groups from
todo.md: the remaining ways a queued snapshot or file could end up wrong or misreported, and four small Windows head and test items.Needs attention before merging
InlineResult.StagedandInlineApplyStatus.Withdrawn. A consumer whose switch throws on a value it does not know would be affected byStaged. Verify compares againstNoViewerFoundand needs no change.~, leading in the viewer's row and trailing in the tray's menu, where it had the failure's!. The character and positions were chosen, not asked for.FooterThatWrapspins a grid of 60 by 27, which turns on the footer buttons' sizes in the system font. Seen on one machine before this PR'swindowsjob.discarding: truebesideprogress. An older reader skips it and takes a discard for an accept: the wrong word, and it still refuses what changes the queue.Queue correctness
ViewerExit.Staged), the launch gate reportsStaged, andAddInlineAsyncreturnsInlineResult.Staged, so the caller does not stage a second trio. An older viewer never returns 5, and an older library reads it as a failure and stages too, which is today's behaviour, soViewerContractasks nothing for it.InlineApplier.ApplyAllasks, once the file is patched in memory and before its one write, whether each editing patch is still wanted. One discarded or settled while the file was waited for isWithdrawn: the file is patched again without it, and a batch counts it as nothing. The viewer answers without the session's lock, because a wire accept applies inside that lock; a test pins the deadlock that taking it would be.Windows head and tests
FooterThatWraps.FormsViewerWindow.MeasureGrid). A capture still draws the screen it is handed, so this is half of that item.Fixtures.WriteImageleaves a file alone that holds the same bytes. Two runs of the suite at once now pass.KeyNameTestsbuilds its registers on the stand-in desktop.Tests
dotnet build src --configuration Releaseis clean anddotnet test --solution src/DiffEngine.slnx --configuration Releasepasses on Windows: 3,439 tests, 0 failed, 35 skipped, and again with the process held to two cores.todo.md,claude.mdand the docs are updated. Nothing undernative/changed, so no binaries PR will open.