Fix the smaller items from the review: the viewer's model, the library, the inline patcher, the tray and the three heads - #925
Merged
Conversation
…e copied A context menu leaves out "Copy all" and "Copy <header>" for a side that would copy nothing: a pending delete's left side, the expected side of a new snapshot. It asked by joining every row of the side into one string and taking its length, for both sides, on every right click, which for a large file is megabytes built to be measured and dropped. SelectionText.Any answers the same question from the rows: anything in a line, or a second line, since two empty lines are still a newline. A test holds it to what All finds for the shapes where the two could part.
FileSide.ReadBytes copied the file into a MemoryStream that grew as it went and then copied it out again with ToArray, so a megabyte on disk was more than three allocated on the way in, for every picture and document a pair holds and again for each the WinForms head decodes. The stream says how long the file is, so it is read into an array of that length. The file is shared with whoever writes it, so the length can be out of date by the time it is read: one cut short comes back as what was there, and one that grew is read to its new end, which is what the copy did. A test reads a megabyte and holds what was allocated to the file's length and a little. Not run against the old code, where by its own arithmetic it allocated three times that.
ViewerSession.Project put its entries in display order, and both of its callers then put the tracked files beside them and ordered the whole list again: Rebuild after every inline transition, Sync on every listing an attached viewer takes. The order is by first appearance and arrival, so ordering an ordered list with more appended gives what ordering it all once gives, and the first pass bought nothing. Project now hands back its entries in the queue's order and leaves the ordering to its callers. The existing tests of order, grouping and selection are the check.
A test that keeps failing the same way writes its received file again and sends the pair on every run. The viewer read both files, built a whole entry from them, which is the diff, and only then found in EnqueueTracked that it said what the queued one said. TrackedWatch did the same a moment earlier or later, when it saw the file's new stamp. Both now ask the sides as they are read whether they hold what the queued entry shows (TrackedEntry.MoveAgain and DeleteAgain), and if so hand back that entry with the new stamps, which keeps the rows it already has. An entry is built only when something differs. The handler reads the queued entry outside the lock, as it reads the files, so it may have gone or changed by the time the new one goes in; what goes in is what the files were read as either way. Two tests look at the rows by reference, through the watch and through the handler, after the file's write time moves and its content does not. Not run against the old code, which built a new entry on both paths and so new rows.
A picture decoded on the work queue, or the scaled copy of one, is put in place by whichever asks first: Runtime.present, which then redraws the window, or Renderer.draw, which threw the answer away. A turn of a spinner is a draw clipped to the spinner's rectangle, so a picture that landed between present asking and that draw beginning was painted inside the spinner's rectangle only. The next present was told nothing had landed, the frame was unchanged and there was no spinner left to turn, so the rest of the picture was never drawn. The scaled copy of an enlarged picture lands the same way, and one lost like that left the picture drawn at .low. draw now notes that something landed in a draw whose clip is not the whole window (or in a capture, which is not the window's draw at all), and takeFinished reports it to the next present, which redraws everything. The draw itself asks for nothing, so it cannot loop: the redraw it causes finds nothing newly landed. A capture draws exactly what it drew. Not compiled or run: nothing here builds Swift against AppKit. On a Mac: `DiffEngineViewer --diff a.png b.png` with two large pictures (4000 by 3000 or more), stepping between a few such pairs in a queue a few dozen times. Every picture is drawn whole once its spinner goes, and none is left as a spinner-sized patch of picture in an empty pane. Enlarged one step and below its own size, a picture sharpens within a moment of each zoom and does not stay soft.
`[` and `]` were matched on charactersIgnoringModifiers, as the letters are. On German, French, Nordic, Spanish and Italian layouts a bracket is typed with Option, and with the modifiers left out the event names the digit or letter on the key, so the previous and next page keys did nothing there. They are now matched on event.characters first, which is what was typed. A US layout types them with no modifier and is answered as before. Not compiled or run: nothing here builds Swift against AppKit. On a Mac: add the German input source, open a PDF pair of several pages, and press Option-5 and Option-6: the page goes back and forward, as `[` and `]` do on a US layout.
…t move on An enlarged picture's drag is reported as the centre it leaves, kept inside what the dragged pane's space can show. Both panes share that one centre, and the two pictures need not be the same shape. Where the dragged pane shows the whole of its picture on an axis, the only centre its space holds there is the middle, so the first move of a drag along the other axis put the other pane's picture back to the middle on this one. On an axis the dragged picture cannot move on, the report is now the centre the frame carried, unchanged. "Cannot move" is: the picture's whole extent on that axis, rounded down to a point, is no more than what shows of it, which is to say the space did not cut it short. A picture a fraction of a point larger than what shows counts as unable to move, since `whole` is a fitted size times a zoom step and is seldom a whole number. Only the report changes. Where a picture is drawn is as it was, so a capture draws what it drew. Not compiled or run: nothing here builds Swift against AppKit. On a Mac: `DiffEngineViewer --diff wide.png tall.png`, one picture much wider than tall and the other much taller than wide, zoomed in until the wide one overflows sideways only and the tall one downwards only. Drag the tall one down to its foot, drag the wide one sideways, and the tall one stays at its foot. Before, it jumped back to its middle.
…chmarks rather than show WinForms' dialog WinForms answers an exception thrown inside a window message with a dialog offering Continue and Quit. In a test host that is a run waiting, on the desktop of whoever started it, for a click. Nothing in src set the unhandled exception mode, so both DiffEngineViewer.Windows.Tests and DiffEngineViewer.Windows.Benchmarks could show it. Each now sets UnhandledExceptionMode.ThrowException in a module initializer, before any window can exist, and for the application rather than the thread: Application.SetUnhandledExceptionMode(mode) is thread scoped, and tests run on STA threads of their own, so the one argument overload left every test with the dialog. That was seen: a test that threw inside a message showed it. UnhandledExceptionTests asks WinForms, on a test thread, whether a window lets an exception through (NativeWindow.WndProcShouldBeDebuggable, by reflection) rather than throwing to find out. It failed with the thread scoped call and passes with the application wide one. The existing tests pass unchanged.
Renderer.picturesChanged asks for a redraw when a picture the frame names is not among the ones the last draw looked at, which is how one that has just appeared is drawn. A draw with no room under the rows for the picture returned before looking at it, so it was never among them, and a window shorter than about 176 points was redrawn whole on every present, sixty times a second, to draw no picture. A draw now notes the pictures it had no room for, with each file's write time and length, and picturesChanged takes that as having seen the file. Kept apart from the decoded pictures, where an entry with no image means one ImageIO could not read and is not tried again: this one is decoded as soon as the window is tall enough. A decoded copy of a file rewritten while there was no room goes at the same time, since it would otherwise read as changed on every frame for the same reason. The fix is the second of the two the item offered. A contentMinSize would not cover it: the room a picture has also turns on how many rows are above it. Nothing a capture draws changes: it is one more stat for a picture that is not drawn. Not compiled or run: nothing here builds Swift against AppKit. On a Mac: `DiffEngineViewer --diff a.png b.png`, drag the window as short as it goes so that no picture shows, and watch the process in Activity Monitor: its CPU falls to about nothing, where it held a steady few percent or more. Make the window tall again and both pictures are drawn; rewrite one of the files while it is short, make it tall, and the new picture is the one drawn.
…ght click Two things in the view that nobody has run, both read from the code. A scroll event says which kind of device sent it. One from a trackpad has precise deltas, in points, and those are added up across events until they make a notch. One from a wheel with notches does not, and its delta is in lines scaled by how fast the wheel is turning: a tenth of a line for one click turned slowly. Those went through the same adding up, so a slow wheel did nothing until ten clicks had gone by. An event from such a wheel is now at least one notch the way it turned, and more only where its delta rounds to more. A trackpad is handled as it was. A click with control held is how a menu is asked for on a Mac with one button. AppKit pops a view's menu for it only when the view has an NSMenu to give, and this view's menus come from the managed side a frame later, so the click arrived at mouseDown as an ordinary press: it selected the queue row or began a text selection, and no menu opened. mouseDown now hands a control-click to what rightMouseDown does, which is shared, so it opens the same menus a right click does and the ones the Windows and Linux heads open. If AppKit turns out to send such a click to rightMouseDown already, the new branch is never reached and nothing changes. Not compiled or run: nothing here builds Swift against AppKit. On a Mac with a wheel mouse: turn the wheel one click at a time, slowly, over a long text diff, and every click scrolls; over a picture every click is a step of zoom. A trackpad scrolls as it did. Control-click a queue row and its menu opens under the row; control-click in a pane and the copy menu opens at the pointer; neither selects a row or clears a text selection.
user32 tracks every part of a scroll bar in one modal loop, from the press to the release, and the form ran frames inside it only once it saw a ThumbTrack. An arrow or the trough held down sent its scrolls into a loop nothing was presenting from, so the panes stood still until the button came up and then jumped. The form now runs frames for any scroll that is not the end of one. EndScroll ends every track, whichever part it was, so the exit is paired as before. For a scroll that reaches the bar with no track behind it, and so no EndScroll, the loop stops the timer as it presents its own next frame: it is presenting, so nothing is holding the thread. HoldingTheArrowStillRunsFrames and HoldingTheTroughStillRunsFrames post a real press to the bar of a parked form, hold it half a second and count frames, as the thumb's test does and through the same code. Against the old handler both got none (22 with the change), and the thumb's passed either way. All three check the timer is off once the track ends. AScrollWithNoEndStopsItsFramesAtTheNextPresent covers the unpaired case.
ViewerForm.Map reads the key code and looked at Shift and Control only, so Alt+A accepted, Alt+D discarded and Alt+Q quit. Control had the same leak and was closed; Alt is now answered first, with nothing. Ahead of Control because Alt Gr arrives as Control and Alt together: Alt Gr+C was copy and Alt Gr+0, which types a closing brace on a German keyboard, was a zoom reset. No command is lost by that. The map goes by key code, and a key code does not depend on the modifier a layout needs for a character, so nothing was reachable only through Alt Gr. The form has no menu bar and no access keys of its own, and the context menu takes the keyboard before Map is asked, so nothing relied on an Alt chord being mapped. Alt+F4 was never mapped and still goes to the base. KeyMapTests: every key that is a command alone is none with Alt, and with Alt and Shift; four Alt Gr chords are none; the existing chords are what they were. Five of the six failed with the check taken out.
A key held past the repeat delay sends its key down again and again, and ViewerForm queued a command for each. For Down that is the point. For a and d it accepted or discarded the entry on screen and then each one that took its place, none of which had been read, and an accept writes into source. ProcessCmdKey still has the key message, and bit 30 of its LParam says the key was already down. A repeat of a command ViewerSession.ChangesQueue names is now swallowed; everything else repeats as before. AHeldDiscardIsOneDiscard posts a press and four repeats of d at a queue of three: without the check all three were discarded, with it one. AHeldScrollKeepsItsRepeats posts the same for Down and scrolls five rows.
The status label takes what the buttons leave of the footer, which beside a document's ten is about 230 pixels, and is two lines high. A longer status wrapped and everything past the second line was cut with nothing to say there had been more. The label now has AutoEllipsis, so the cut ends in an ellipsis and the whole status is a tip when the pointer rests on the label. The alignment is left as it was. The item had the label showing the middle of a wrapped status; captured at 100%, with three lines' worth and with five, it showed the first two lines before this change and shows them after, so the start was already what was kept, and moving the status to the left would have moved every window baseline for nothing. WindowsPixelTests.StatusThatDoesNotFit is a new scene: the document fixture with a status five lines long. Its baseline was looked at: the status reads "START lines 1-14 of 40, page 1 of 1, page 1 / differs, 200% zoom, selected 3 lines of sam...", right aligned as before. With AutoEllipsis off the same scene ends "lines of" and fails against it. No existing baseline moved.
…is minimised FormsViewerWindow chose its wait by form.Visible: fifteen milliseconds for a window on screen, a hundred for a hidden one. A minimised window is still Visible, so it ran the loop sixty times a second from the taskbar. It now waits the hidden hundred when minimised too. Nothing else about hidden is copied and no third state is made: the wait ends on any message, which is the click that restores the window, and the loop reads the listener's window commands each time it wakes, so a snapshot that arrives while minimised is queued as before and raises the window within a tenth of a second. FrameWaitTests: the wait for each combination, and a real form, transparent and parked off screen, that is Visible while minimised and gets the hundred. The old expression gave that form fifteen.
…is unseen Runtime.pump is the frame throttle: it waits on nextEvent until a sixtieth of a second has passed. Two things about that wait. It ran to its deadline whatever arrived during it, so a key, a click, a wheel notch or a drag was handed to the managed loop up to a frame after it happened. Once an event has left something for the next poll, the pump now dispatches what else is already queued and returns. An event that leaves nothing, a mouse move or a flags change, does not end the wait, so the loop turns no faster than input arrives. And nothing made it longer for a window nobody can see. The managed loop turns as fast as present returns and has no wait of its own, so ordered out behind a tray, miniaturised or wholly covered it went on at sixty turns a second. The wait is now a tenth of a second then. Any event still ends it at once. What cannot is the managed side wanting the window shown for a patch that arrived over the socket: it acts on that between two presents, and nothing in the C ABI lets it interrupt one, so the window comes forward up to a tenth of a second later than it did. Nothing was added to the ABI, which has no way to say the window is hidden to the managed side either: OwnerLink and TrackedWatch still slow down only for a window the managed side hid itself. Not compiled or run: nothing here builds Swift against AppKit. On a Mac: with a viewer open and idle, miniaturise it, or cover it with another window, and its CPU in Activity Monitor falls to a fraction of what it is when visible; click its Dock icon and it comes back at once and scrolls smoothly. With the viewer wholly covered by another application's window, fail an inline snapshot test: the viewer comes forward with no delay that can be seen. Hold Down in a long diff: the rows scroll at the key's repeat rate.
PendingInline.Key lowercased the path and formatted a string each time it was asked, and every lookup in a queue asks it of each entry it passes: Enqueue, Settle, Accept and Find each walk the list comparing keys. With hundreds pending, a run that enqueues and settles each of them asked hundreds of thousands of times. The key is now kept on the entry, beside the file and line it was built from, and built again when the primary patch no longer holds those. A patch's properties can be set, and `with` copies the kept key onto an entry that may be given other variants, so it is checked rather than trusted. It is held in a field whose equality is always true, so that two entries do not differ for one of them having been asked its key. InlineStaging.SamePath built two whole keys to compare two paths, once per staged trio per clear. It now answers from the lengths and the spelling where those decide it, and folds only where they do not. What it answers is unchanged. No behaviour changes: the existing inline tests are the check, and pass.
An F# call is anchored by the value its literal holds, and the empty string is a value. InlinePatchFile.Build wrote it as an empty field, which TryParse reads back as no value: the viewer headed the pane "expected (new snapshot)", and the patcher went by the line alone. It is now said on a line of its own, `originalValueEmpty: true`, written after every other line and only when the value is the empty string, so every other payload is byte for byte what it was. Not a mark inside the field. The format crosses versions both ways, and a reader that predates this decodes whatever the field holds as base64 and rejects the whole payload when it is not, where a line it has no name for is skipped. So an older reader handed the new payload reads no value, as it did before, and a newer reader handed an older payload finds an empty field and no line, which is still no value, since such a writer sends the same for both. Tests: AnEmptyOriginalValueSurvives fails without the fix, with null where "" was expected. AnEmptyOriginalValueIsALineAnOlderReaderSkips pins the bytes an older reader is handed, OnlyAnEmptyOriginalValueIsMarked that nothing else changed, AnEmptyOriginalValueFieldAloneIsStillNoValue the older writer, and TheEmptyValueLineIsReadInAnyOrderAndYieldsToAValue the order. The unfixed TryParse was run on the new payloads and parsed each, reading no value.
F# lexes inside a block comment, which is what lets one hold commented out code. A string in a comment is a string, so `(* returns "*)" when closed *)` and `(* see "(*" *)` are one comment each, and `(*)` inside one is the operator. The scanner read a comment as text: the first ended at the quoted close with a string opening after it, and the other two never ended. Either way the calls below were inside something, and a patch for one of them was NotFound. Inside a comment the scanner now steps over what fsi was seen to take as a token there: a regular string with its escapes, a verbatim string, a triple quoted string, a char literal, and `(*)`. What was asked of `dotnet fsi`, a shape a script, before any of it was written: - `'"'` and `'\"'` are char literals and open no string, mid word as well (`a'"'`), and a tick that closes no char literal is text, so the string after `it's` is still one. - Only `@"` is verbatim. `$"` is a dollar and then a regular string, with no interpolation holes, and `@$"c:\"` does not end where `$@"c:\"` does. - Backticks are text in a comment: a quote between two pairs opens a string all the same. A double backticked identifier in code is the other half. It holds anything a line can, and F# tests are usually named with one, so ``returns "x" (* when asked`` opened a string or a comment that ran to the end of the file. It is now stepped over whole. It is not recorded as a literal, because it is a name, and the member a patch names is looked for in code. Tests: the shapes are FsCompilerRoundTripTests.Comments and QuotedNames. CommentsAndNamesAreReadAsTheCompilerReadsThem patches a call under each and has fsi compile and run the result, so the compiler is asked about every shape on every run. ACommentIsLexedAsTheCompilerLexesIt and ADoubleBacktickedNameIsSteppedOverWhole in InlinePatcherFsTests are the same shapes without fsi. Against the unfixed scanner all three fail with NotFound.
…r it
A Remove is applied by the test process, so it is applied once for each
framework of a multi-targeted run, and once for each case of a test that
ignores its parameters. With
await Verify(a)
.Snapshot("dup");
await Verify(b).Snapshot("dup");
the first apply took the call with its line, which brought the statement
under it up onto the line the patch names. The second apply then found a
Snapshot call on that line holding the literal it was anchored to, so
RemovedAtHint said nothing had been removed, and Verify(b) lost its
snapshot.
The file as the first apply left it is exactly the file an honest Remove
of Verify(b)'s call would meet: the same line, the same anchor, the same
member. Nothing that reads it afterwards can tell the two apart, so the
first apply has to leave something behind. It now leaves the line the
call started on, empty, with what followed the call still pulled up onto
the line above:
await Verify(a);
await Verify(b).Snapshot("dup");
RemovedAtHint already reads a line with no Snapshot call, under a verify
statement that has none, as the call removed, so the second apply answers
AlreadyApplied.
Only where the line under the call holds a Snapshot call. Anything else
that comes up onto the recorded line is read as removed already, so every
other Remove writes what it wrote before, and no existing expectation in
the patcher's tests moved. One line is kept however many the call ran
over, since the line it started on is the one a patch names. Where what
followed the call opens a literal that runs onto the next line, a line
break there would be content, and the call is taken from where it stands
instead, which keeps the line by leaving on it what followed.
Tests: RemoveAppliedTwiceLeavesTheSiblingUnderIt in InlinePatcherTests
and InlinePatcherFsTests is the item's case, applied twice. Beside it, a
call over several lines, one with no Snapshot call under it, a literal
after the call, and CRLF. Run against the unfixed patcher, in a copy of
the tree outside the repository, the five that keep a line fail on what
the first apply leaves. FsCompilerRoundTripTests compiles and runs the F#
an apply leaves, with the empty line between two statements of a
computation expression and inside a chain.
ProcessCleanup reads every command line from `ps -o pid,command -x` and matches a running diff tool against them. Into a pipe procps prints each whole, but it takes an exported COLUMNS over that, so in a shell with COLUMNS=80 every line came back cut to 80 characters: a tool started with two long paths was never found, and so never killed. `-ww` makes the width unlimited to procps and to the BSD ps on macOS. Checked in a container (procps-ng 4.0.4 on the .NET 10 SDK image): PsColumnsTests.AnExportedWidthDoesNotCutCommandLines passes with -ww, and without it fails with the command line cut at 80 characters. It runs on Linux and macOS only. Apple's ps was not run: its manual says a second -w uses as many columns as necessary. BusyBox's ps rejected the old arguments (-x) and rejects these too, so nothing changes there.
ViewerClient.SendAsync cancels by closing its socket, which is the only thing that unblocks every framework. What the closed socket then threw was not an OperationCanceledException wherever the call in hand takes no token, so it was handled as a connect that failed: the send returned NoOwner and the port was remembered as unowned, with its owner still listening, which silences settles and moves for ten minutes. Two ways in, both reproduced on a private port before the fix. A token already cancelled closed the client before it connected, on net6 and later. A token cancelled while the connect was out did the same on .NET Framework, where the connect takes no token. SendAsync now throws on entry for a token already cancelled, and a socket failure that arrives with the caller's token cancelled is rethrown as the cancellation it is, recording nothing. ViewerClientUnownedTests.ASendCancelledBeforeItStartsSaysNothingAboutThePort failed on net10.0 without the fix, and ASendCancelledWhileConnectingSaysNothingAboutThePort on net48.
ViewerServer's accept loop retries a failed accept, because a peer that resets in the backlog fails one accept and not the listener. It retried with nothing in between, and a failure can persist: a process out of descriptors is refused every accept at once, with the connection left waiting in the backlog, until something is closed. That was run rather than supposed. A .NET 10 listener on Linux under `ulimit -n 64`, with every descriptor taken and one client connecting, failed 9,464 accepts in a second with TooManyOpenSockets and spent 1,011 ms of processor doing it, and accepted the same connection as soon as a descriptor was freed. Windows has the same failure under other names (WSAEMFILE, WSAENOBUFS) and was not made to produce it. The first failure is still retried at once, since the reset is over when it is reported. From the second in a row there is 100 ms between them, which ends early when the listener is stopped. Nothing is added to an accept that works. The loop is now ViewerServer.Serve, given its accept, because a real listener cannot be made to fail one in a test. ViewerProtocolTests.AnAcceptThatKeepsFailingIsWaitedOut counts the accepts made in half a second of failures: 55,297 with the wait taken out, and under 30 with it.
A passing inline verification settles once, and with a tray running that is a connection each. The benchmark sends them to an owner this process binds on a private port, and reports from the operating system's table how many ports the run left in TIME_WAIT. As it stands, on Windows: 301 us and 13.59 KB a settle, and 2,576 ports left waiting after 2,576 settles. Committed ahead of the change to that path so the number can be had again from here. The comment at the top says how to run it without using up the machine's ports.
A passing inline verification settles once, and with an owner present each settle was a connection of its own. The client closes first, so its port then sits in TIME_WAIT: one a settle, for two minutes, out of about 16,000 on Windows. A large enough green run with a tray answering used them up, and the connect that then failed was remembered as an unowned port. Closing hard instead does not help: 500 exchanges left 500 ports waiting either way. Skipping settles while the owner holds nothing was the other fix on offer, and is not safe: an entry another process queues after the listing is one a skipped settle leaves standing. So every settle is still sent, and in order. What changes is the connection. An owner now ends each ordinary reply with `keeps: 1`, which a reader that predates it skips as it skips any name it does not know. A client that sees it opens a second connection, says `keep: 1`, and sends its telling sends (settle, retire, move, delete) down it one after another, each ended by an empty line and answered the same way. An owner that predates this never says the line and is sent a connection each, as before. Asking sends, and the async send a failing snapshot makes, are unchanged. The kept connection is dropped when the owner closes it, which stopping a ViewerServer now does, and the send that finds it gone falls back to the ordinary exchange, which is what finds out who holds the port now. One that times out waiting is given up on without being sent again. SettleBenchmarks, on Windows: 301 us, 13.59 KB and a port left waiting for every settle before; 57 us, 4.69 KB and one port for 2,576 settles after. KeptConnectionTests covers the count of connections an owner accepts, an owner that predates it, an owner that goes and the next one on its port, parallel senders, and a busy owner, on net10.0 and net48 and in an Ubuntu container. DiffEngine.Tests, DiffEngineTray.Tests and DiffEngineViewer.Tests pass in Release. Not run on macOS.
WinForms catches an exception thrown inside a window procedure and shows its own dialog, with Continue and Quit on it, unless told otherwise. Nothing in src told it otherwise, so a tray test that threw there put that dialog on the desktop of whoever was at the machine and waited for a click. The test project's module initializer now sets UnhandledExceptionMode.ThrowException for every thread, before any control exists, so such an exception fails the test it came from. Checked by running DiffEngineTray.Tests with it: 291 pass, so no existing test depends on the default mode.
ExceptionHandler follows an unexpected error with a modal message box asking whether to open an issue, and a yes opens a browser. A test that reached one waited on the desktop for a click. One test avoided it by marking its message as already asked, through reflection. The question now goes through IssueLauncher.Declined, which the test project's module initializer answers itself, keeping what was asked so a test about an error being reported can read it. Checked by running DiffEngineTray.Tests.
ProcessEx.TryGet holds a handle on every process a move names, and DiffRunner names one for an MDI tool too. The only place a tracked process was disposed was KillProcesses, which returns early for a move that cannot be killed, so each of those kept a handle, and a process id Windows could not reuse, until a finaliser got to it. A move now disposes its process, without ending it, wherever it leaves the dictionary for good: accepted, discarded, settled, dropped by the scan, still tracked when the tray exits, or put back after a failed accept to find a newer move in its place. One kept pending keeps its process, which "Accept all open" and "Open diff tool" read. TrackerProcessReleaseTest tracks a process the test started as a move that cannot be killed and takes the move out each way. Four of its five tests failed before the change, the process still on the move; the fifth pins that a move kept pending keeps it.
The scan decides about a move and then compares its two files. A re-run landing in between replaces the move, and the removal was by key, so it took the fresh move and ended the diff tool just opened for it. The removal is now by key and value, and a move that was replaced is left for the next scan. Only IOException was caught around the compare, so a file this account may not read failed the whole scan every two seconds. UnauthorizedAccessException is caught beside it. A scan that failed some other way went to ExceptionHandler, which asks whether to open an issue in a modal box. That was on the timer's thread, and the loop waited in it: no scan ran until it was answered. A failed scan is now logged. TrackerScanTest: a stale pair handed to HandleScanMove took the fresh move before the change, a target denied ReadData threw out of it, and a queue that throws from the scan reached the box (which the test project declines and records). All three failed before and pass now.
"Discard (n)" and its hot key called the inline queue's DiscardAll on the thread the click came in on. Against a queue a viewer owns that is a socket round trip, and a viewer slow to answer held the tray's only UI thread for up to fifteen seconds. The single discard had been moved to a worker for this; the bulk one was left. Clear now discards the tracked files where it is called and returns a task for the snapshots, as the accept-all does. The menu and the hot keys discard the task, and tests await it. The icon is set when the answer is in rather than by the next scan. TrackerSnapshotTest.ClearDoesNotWaitOnTheQueue holds the queue's answer back and asserts the call has returned: with Clear still inline it sat ten seconds in the call and failed. TrackerClearTest and both TrayViewerSyncTest discard-all tests pass awaiting it.
A hot key's action runs inside IMessageFilter.PreFilterMessage. What is thrown from there does not reach Application.ThreadException: it comes out of Application.Run() and the tray ends. KeyRegister now catches around the action and reports through ExceptionHandler, and the message still counts as handled. LinkLauncher.LaunchUrl let Process.Start's failure through. It is called from the options form's links and from the error handler itself, where a throw replaced the error being reported. It is logged now. No action that throws today was found, as the item says; this is the mechanism closed. KeyRegisterTests calls the filter directly, never through a message loop, with an action that throws: the exception came out of the filter before the change. LinkLauncherTests opens a file that does not exist, which threw Win32Exception before.
The process id in a piper payload was held and tracked as the diff tool on the id alone. A library from before ProcessCleanup.StillRunning lists the running tools once per test run and can send the id of one closed since, which Windows may have given to anything else, and an accept then ended whatever that was. ProcessEx.TryGetTool reads the image of the process it holds, through the held handle, and keeps it only when its file name is that of the payload's Exe. By name and not by path, because one executable has several paths and the sender's need not be the system's; that still cannot tell two copies of one tool apart. Anything not known to be the tool is tracked as no process: not ended, and not counted as open. That covers a move naming no tool, a tool started through a script, whose id is the command interpreter's, and an image that cannot be read. An executable shim is the image the sender started, and matches as before. A warning is logged with both names when an id is turned down. TrackerProcessImageTest uses a process the test starts as both the tool and the stranger. Before the change the stranger was tracked under a tool it was not running, under a script, on a re-run, and with no tool named: four of six failed. Four existing tests named their stand-in tool "theExe" and now name it by its image.
An owning tray tags its listing so an attached viewer's poll can be answered "unchanged", and the tracked files' part of the tag was every move and delete built into its wire form, joined and hashed with SHA-256, on every poll: five times a second, to learn that nothing had changed. ITrackedFiles.Version is the tracker's own answer. Everything a listing carries of a move or a delete is fixed when its object is made and a change is another object, so the tracker remembers which objects it held when last asked and compares references. It can report a change where none is visible (a move taken out and put back), which costs one listing, and never the other way round. Not a counter bumped where the dictionaries are written, because one missed write among twenty is a viewer showing a file that has gone. TrayViewerSyncTest: with a hundred moves and a hundred deletes tracked, an ask allocated 1.17 MB before the change and the test bounds it at 2 KB. ATrackedMoveThatChangedOrLeftIsNeverAnsweredUnchanged pins what is relied on: a move that keeps its key and changes target, and one that leaves, both change the tag. The existing test for a delete arriving passes unchanged.
…an as a state read once a frame The shim asked raylib whether a button was down, what the wheel had moved and which keys were down, once a frame. raylib keeps those from GLFW's callbacks as a state, so a press and a release that arrived between two readings had never happened, and of several wheel messages only the last had. A tap on a touchpad is such a press. GLFW's mouse button, scroll, key and character callbacks are now set over raylib's, which are kept and still called. Presses and releases are handed to ImGui in the order they came, at the top of the next frame built; ImGui takes a press and a release handed over together a frame apart, so a row, a button or a menu item is clicked as it would be by a button that was held. Wheel messages are added up, once for ImGui and once for the managed side. Key presses wait in a queue with the character each typed and are handed over one a poll, as the other two heads hand theirs. What a key means is unchanged: by character for the letters, by position for the chords and the navigation keys, and a held letter still repeats as it did. Nothing here asks raylib about a button, the wheel or a pressed key any more, so there is no second account for ImGui's to disagree with. deview_present asks the same queues before it leaves a window alone, and ImGui's own queue, which is where the release of a press handed over with it waits for a frame. Checked in an ubuntu:24.04 container under Xvfb with xdotool, which sends a click as a press and a release at once. Before: none of ten clicks on a queue row was seen, a right-click opened no menu, five wheel notches sent at once scrolled as one, and of Down and Page Down sent together neither acted. After: ten of ten, the menu opens, five notches scroll as five sent apart do, and both keys act. The fifteen Linux pixel baselines are byte for byte what they were, the session that drives the viewer with held clicks and keys photographs the same pictures, and an idle window is still left alone.
…nd paging, and take a non-Latin layout's letters by position Three things about keys in the Linux head. Ctrl+A and Ctrl+C were read by where the key is on a US keyboard, since no character is reported while control is held. On AZERTY, Ctrl+A was the key labelled Q and the one labelled A did nothing. They are now read by what the key types unshifted on the layout in use, which GLFW says. The zoom chords are read the same way first, and then by position as they always were, for a layout whose digits are on Shift. The arrows and Page Up and Page Down acted once however long they were held. They now go on at the rate the window system repeats a key. One repeat of a key waits at a time, so a loop that was held up does not go on scrolling after the key is let go. On a layout with no Latin letters, none of the letter shortcuts could be typed. A key that types a letter outside ASCII is now read as the letter a US keyboard has in its place, but only where no key of the layout types that letter: a layout that has it elsewhere keeps it there and nowhere else. A key GLFW has no number for is queued as any other, so one that types a letter acts by it, as it did before keys were queued. Checked in an ubuntu:24.04 container under Xvfb with xdotool and setxkbmap. On a French layout, before: control with the key that types a did nothing and control with the key that types q selected everything. After: the first selects everything and the second does nothing. On a Russian layout, before: the key where n is did nothing. After: it goes to the next change, and control with the key where a is still selects everything. Down held for a second and a half scrolled one row before and scrolls on now. The fifteen Linux pixel baselines are byte for byte what they were.
ImGui was told where the pointer is on every frame, and raylib goes on answering with the last place it was in the window. A pointer that left over a queue row was still on that row as far as anything drawn could tell: the row stayed lit, and its tooltip came up with the pointer on another window. GLFW's cursor enter callback is now set over raylib's, which is kept and still called, and a pointer that has left is reported to ImGui as nowhere. Not while a button is held: the window system goes on reporting a pointer that was pressed in the window wherever it is taken, and a selection dragged past the window's edge goes on being one. The crossing is one of the things deview_present asks before it leaves a window alone, since raylib's position for the pointer is the same before it and after. Checked in an ubuntu:24.04 container under Xvfb with xdotool. Before, with the pointer taken from a queue row to outside the window, the photograph was the one with the row lit and its tooltip up. After, it is the one from before the pointer came near the row, the tooltip comes back when the pointer does, and a drag begun in a pane and let go outside the window leaves the selection it left before.
The Linux head draws its own context menu, and only a pane's was kept inside the window: the clamp was inside the branch that places that one. A queue row's hung under its row whatever was left there. Under the last row of a queue that fills its column there are 96 pixels at the size the window opens at, and a conflicted entry's menu is 114, so its last item was past the window's edge. Both menus are now kept inside the window, and a row's that would run off the bottom goes over its row instead of under it, so the row it is about can still be read. PixelTests.ContextMenuOnTheLastRow is a new Linux-only scene for it: a queue of 33 with the menu opened on the last, a conflicted entry, at the 41 rows that head measures for a window this size. Captured with the shim as it was, "Copy expected" is cut off by the window's edge; with this, the whole menu is over the row. No other baseline moved: PixelTests.ContextMenu and PixelTests.PaneMenu are byte for byte what they were.
ImGui reads an item's label for more than its text: everything from "##" on is the item's identity and is not drawn. The Linux head handed it names as labels - a queue row, a group's heading, a menu item, a footer button and the two pane headers - so a file, a test or a solution with "##" in its name was cut short there. "Notes##2.received.txt" was "Notes" in the queue, in both pane headers and in the menu's two "Copy" items. A label is never an identity in this head, since every item is given one by its index. One with the mark in it is now drawn by the shim, where the item would have drawn it, over an item that is given no text; a pane's header is cut short with an ellipsis at its column's edge as the table would have cut it. A label without the mark goes to ImGui as it always did, which is every label there has been, so nothing else is drawn differently. PixelTests.NamesWithHashes is a new Linux-only scene: a move, an inline snapshot and a solution with "##" in their names, with the move's menu open. Captured with the shim as it was, every one of them stops at the mark; with this, each is whole. A footer button goes through the same change and is not in the scene, since no button the model makes carries a name.
…d nothing for one too large for a texture Two things about pictures in the Linux head. A picture longer on a side than the largest texture GL takes was handed to it all the same, and came back as a texture with a name and no pixels, which draws as black: a black box the shape of the picture, where a picture this head cannot show is otherwise drawn as nothing and left to its rows. The limit is now asked of GL, by name through GLFW since rlgl reads it only to log it, and a picture past it is not made into a texture. A picture fitted at under half its size was sampled between its own four nearest pixels and no others, so whatever fell between two samples was not there: one pixel lines dropped out and small text came apart. A picture's reduced copies are now made as it is decoded, on the decoder's thread, and it is drawn from them when it is drawn at under half its size. At half its size and over it is sampled as it always was, so no picture drawn near its own size is drawn differently. Two new Linux-only scenes. PixelTests.ImagesReduced is two pictures of a one pixel grid fitted at a third of their size: with the shim as it was, lines of uneven weight with some gone; with this, every line, evenly. And PixelTests.ImageTooLargeForATexture is a picture 16385 pixels wide, one more than the 16384 Mesa's software rasteriser takes, which is what the baselines are pinned to: a black bar with the shim as it was, nothing with this. Both are bitmaps written by the test, since a bitmap's bytes are the same on every runtime. The seventeen baselines there were are byte for byte what they were, PixelTests.Images, ImagesEnlarged and both DocumentPage scenes among them.
…e to go on an axis, in the Linux head The centre an enlarged picture is drawn about is one point for both panes, and a drag reports where it has moved that point to, clamped to how far the dragged pane's own picture can go. The two pictures need not be the same shape. One that is all in view from top to bottom has nowhere to go that way, and its clamp left one answer, the middle, which was reported on every frame of the drag: dragging it sideways took the other pane's picture back to its middle row from wherever it had been dragged to. A pane's picture can move across only when floor(whole width) is more than the width shown, where whole is the fitted size times the zoom and shown is min(floor(space), max(1, floor(whole))): only when the space cut it short. Likewise down. On an axis it can move on, the report is clamped as it was. On one it cannot, the report is the centre the frame was handed for that pane, imageCenterX or imageCenterY, unchanged. That is the rule the macOS head now applies, in the same words. No test can reach a drag, which is the pointer's. Checked in an ubuntu:24.04 container under Xvfb with xdotool: an 800 by 100 picture beside a 400 by 400 one, enlarged to 200%, the right one dragged down and then the left one, which is no taller than its space, dragged down. Before, the second drag moved the right picture; after, the photograph is the one from before it. A drag of the left one sideways still moves both across.
Under X11 a pixel is a pixel whatever the display, and nothing in the Linux head asked how large the desktop wants things. On a display set to twice the size the window was still 1100 by 700 pixels with text 15 to the em: half the size of everything around it. The scale is now asked of GLFW as the window is made, which under X11 is the Xft.dpi resource over 96, and the window is drawn larger by it: the font, ImGui's paddings and spacings, the few lengths the shim gives in pixels, and the size a window opens at when none is remembered, as much of it as the monitor has room for. A remembered placement is in pixels already and is used as it is. Nothing is handed to raylib to scale, so text is rasterised at the size it is shown and everything stays in the pixels the pointer is reported in. Two things had to follow. The grid reported to the managed side was measured between frames, where ImGui answers with the font at the size it was added at, so a window at twice the scale was told it had twice the rows it has: it is now what the last frame built for the window found. And a capture's queue column is the width it starts at in the capture's own cells, where it took the window's, which is in the window's cells and may have been dragged. A capture is never scaled: it makes its own ImGui context, at a scale of 1. Checked in an ubuntu:24.04 container under Xvfb on a 4K screen, with the display's resources set by xrdb. At 96 dots an inch every photograph of a session is byte for byte what it was. At 192, where the window had been the 96 one pixel for pixel, it opens at 2200 by 1400 in the middle of the screen with the same 33 rows of text at twice the size, and a click on a queue row, a selection dragged across a pane and a right-click for a row's menu, each sent to where the thing is at that scale, land on it. At 144 the same at one and a half times. The pixel baselines are byte for byte what they were.
…de.md Every item under Smaller is fixed but one, the macOS viewer started with no console session, where the only documented check may also refuse a session that works today. The section now holds that item and what each fix says it did not reach. claude.md gains what someone working here next has to know: the kept connection the library's telling sends use and why skipping settles was not the fix, the accept loop's wait, where the Linux head's input now comes from and what its window is scaled by, the Windows head's keys and scroll bar, how the tray decides a process id is its tool, the patcher's kept line, and the guard that stops a WinForms test host putting a dialog on the desktop, with the overload that does not do it. deview.h says what a drag reports on an axis its picture cannot move on, which both native heads now do.
SendsFromManyThreadsAreEachAnswered failed on CI's Ubuntu runner with three of its four hundred sends answered wrongly. Its eight senders ran on the thread pool, a send blocks its thread until it is answered, and the owner in the test answers from the same pool. On a runner of four cores the senders were every thread the pool had, the owner could not answer, and a send gave up after its three seconds. That is the test starving its own owner, which a real one, being another process, cannot be. With DOTNET_PROCESSOR_COUNT=2 the test failed every time in just over three seconds; with the senders on threads of their own it passes five times in five at two cores and at one.
Co-authored-by: SimonCropp <122666+SimonCropp@users.noreply.github.com>
AnAcceptThatKeepsFailingIsWaitedOut failed on CI's Ubuntu runner with one accept counted where it wanted the retry as well. It gave the loop half a second and then counted, and on a runner of four cores with the rest of the suite running, the loop's first continuation had not been given a thread in that long. The retry is now waited for, up to thirty seconds, and the accepts that follow are counted over a stretch that is timed rather than assumed, with the bound at twice the rate the wait allows and a few over. What the test is for is unchanged: a loop that spins makes tens of thousands in that stretch. The library's whole suite passes four times in four with the process held to two cores, and twice at one.
…launch left AnInlineSnapshotIsNotCalledQueued failed on CI's Windows runner, in the net48 process, on a payload file in the temp folder that was not there before its launch. The test runs in the net48 process and the net10.0 one at the same time, the temp folder is shared, and each launch writes a payload file and takes it back: one process listed the folder while the other's file was there. The test is from the round before this one and nothing here changed what it tests. It now waits for the files that appeared to go, for up to forty seconds. Another process's file goes when that process's launch fails, which is at once; one this launch left behind stays, and is still what fails it. Both frameworks pass it run side by side, and alone.
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.
The items under
## Smallerintodo.md, from the review ofmainat 991bc48: thirty six of thirty seven, one commit each, so any of them can be reverted alone. The one left open is the macOS viewer started with no console session.Needs attention before merging
macos-14job and none run by a person. Five are event handling or window state that no capture exercises. The check to make on a Mac is in each commit's message.keeps: 1on a reply,keep: 1to open). Skipping settles was not safe. An older owner gets a connection each as before, and an older client skips the new line. Measured: 301 µs and 2,576 ports left behind for 2,576 settles before, 57 µs and one port after. Tested against a stand-in for an older owner, not a released tray..cmd(VS Code, Rider) is therefore not tracked as a process and is not closed on accept; the id held for it before was the command interpreter's.Xft.dpiover 96). Checked under Xvfb at 144 and 192, never on a real HiDPI desktop. At 96 every photograph is byte for byte what it was.native/changed, both the C++ and the Swift, andbuild-native's binaries PR VerifyTests/DiffEngine#926 is merged into this branch. No ABI change.Viewer model (4)
Library (4)
psis asked with-ww, so an exportedCOLUMNSno longer cuts the command linesProcessCleanupreads.Inline patcher (4)
(*)inside one, and a double backticked name in code. Held todotnet fsiover 24 comments and 5 names.Tray (6)
Windows head (5)
aordis one accept or discard; navigation still repeats.Linux head (7, and half of one), built, run and photographed in an
ubuntu:24.04container##in it is drawn whole.Four new Linux-only pixel scenes. All fifteen existing Linux baselines reproduce byte for byte.
macOS head (6 of 7), not compiled or run
[and]match by the character typed, so layouts that need Option for them can turn pages.Tests
dotnet build src --configuration Releaseis clean anddotnet test --solution src/DiffEngine.slnx --configuration Releasepasses on Windows: 3,148 tests, 0 failed, 32 skipped. In the Linux container the viewer's tests pass with pixel snapshots on. One earlier run of the whole suite failed three Windows input tests that passed in the next two runs and alone;todo.mdrecords it.The three WinForms test and benchmark hosts now set
UnhandledExceptionMode.ThrowExceptionfor every thread, so a test that throws inside a window message cannot put WinForms' dialog on the desktop.Also in here
todo.mdloses the smaller items and gains what each fix left.claude.mddescribes the changed behaviour.