Viewer one window - #848
Merged
Merged
Conversation
The viewer is not MDI, so DiffRunner gave it a process per pair the way it does every other tool: a run failing several image snapshots opened a window each. That shape was not incidental. Kill closes a pair's window by matching the command line it was launched with, and it skips MDI tools entirely, so a shared window would have left a test that started passing nothing to close - or closed the window every other pending pair was drawn in. The pair was already a row somewhere. DiffRunner tracks a pending move for every launch, and with no tray that move goes to the queue owner, so the entry existed and a second window was drawn beside it. Diff is Move plus the window Move deliberately withholds: every other tool has already opened its own for the pair by the time its move arrives, and when the viewer is the tool there is none. So the verb tracks the pair and raises the owner over it - a viewer that holds a window selects the entry, and a tray that does not starts one and hands it the same selection. DiffRunner takes that route as soon as it knows the tool is the viewer, before the process bookkeeping that has nothing to work on: no instance showing this pair to find, no window to replace, no MaxInstance slot to spend on a window that already exists, and no process id for the tray to kill on accept. Kill sends Settle for the move key instead, and a tracked key settles by being dropped, on both owners: the received file is already gone by then, so accepting or discarding the row would only fail on it. An owner too old to know the verb gets a plain Move rather than a second process it would refuse as well. Rows follow their files now, which they did not before and had not needed to. An owned queue is push only - a socket message or a launch argument puts an entry in it and nothing revisits it - so a row described the moment it arrived and nothing after: content a re-run had already replaced, and a received file that was no longer there. OwnerLink has always done the equivalent for a displayed queue, so TrackedWatch is the same stat, the same FileStamp test and the same cadence for an owned one. It stops short of the tray's rule about a pair whose files became byte equal, because that check is there for an external diff tool converging them and here the viewer is the diff tool. Three things change for a reader. Accepting moves the received file over the target rather than copying it, which is what accepting a move has always meant. Received is always the left pane, so the target-on-left ordering no longer applies to these. And the window closes once the last row goes, which was already the rule for an emptied queue and is now reachable by the files themselves going away. A hand run DiffEngineViewer left right is untouched: its own window, no port, no queue, since a queue nothing can join is the whole intent there.
A pair whose diff tool is the viewer goes to a running tray as an ordinary move, carrying the exe and arguments the tray re-runs for "Open diff tool". Those arguments now say --diff, so the relaunch joins the queue the pair is already in rather than opening a window of its own beside it - which is the arrangement the whole route exists to remove. Nothing covered that, and it is a string built in one assembly and parsed in another, which is the shape that rots quietly. Asserted by putting it back through both parsers it actually has to survive: CommandLineToArgvW, which is what Windows will split it with, and then the viewer's own CommandLine. The received file is staged under a directory with a space in its name, because that is the case where the quoting has to be right rather than merely present. The move is also pinned as not killable and carrying no process id. There is no window of its own to kill, and the one it is drawn in is holding every other pending pair, so a tray that killed it on accept would take them all.
DiffRunner.Kill's viewer branch had nothing on it. What it does - address the queue owner rather than a process, and drop the row rather than acting on a file that has already gone - now lives beside the launch half in PendingFiles.SettleDiff, where a test can drive it over a real socket. The dispatch in Kill stays uncovered, because tool resolution cannot be steered to a DiffTool identity from a test: the public AddTool overloads all register with a null one. It is the same predicate the launch side branches on two lines away. The refusal fallback was reasoned about and never run. An owner that answers and says no is one too old to know the verb, and the pair goes over as a plain move instead - a row with nothing raised over it. Both the sync and async routes are pinned now, which matters because they take different client calls to tell a refusal from an absence. The auto-refresh and MDI table describes what DiffEngine does with a tool it has launched before: relaunch it, kill it, or leave it to refresh itself. None of those happen for the viewer any more, so a reader working out what to expect from "Is MDI: False" was being told the wrong thing. Said on the tool and once under the table. ITrackedFiles said a viewer that owns the queue never has tracked files, which stopped being true when moves and deletes started going to the queue owner with no tray running, and is now how every failing pair arrives.
Nothing owns the port at the start of a failing run, so every caller that finds it free is entitled to start a viewer, and a parallel run reaches that point once per failing snapshot. Twenty pairs failing at once measured as twenty processes: one bound the port, and the other nineteen handed their work over and exited. The queue came out right - the racing resolution is what makes it right - but it was twenty process starts to open one window, and every one of them reported StartedNewInstance when there was one instance. MaxInstance used to hold this down to five, and stopped applying when the viewer stopped opening a window per pair, so nothing was capping it any more. The gate replaces it for this tool: the first caller through starts a viewer and holds the gate until that viewer answers, and everyone behind it finds an owner and never launches. Fifty pairs failing at once now measures as one process and one StartedNewInstance, against fifty of each before. What the gate holds is the decision, not the work. Inside it is a connect that asks whether anyone is there; the send that hands the payload over happens outside, so the callers that find an owner still reach it at once. Sending inside instead turned twenty process starts into nineteen serialised round trips, which measured slower than the problem. The inline and delete routes go through it too. The race is not new there and was never as visible - a run failing twenty inline snapshots has always started twenty viewers - but it is the same race, and leaving one of the three unfixed would only mean rediscovering it. The gate's ownership probe is a parameter rather than a call into ViewerClient, because the alternative is a test that arranges twenty concurrent connects against a port being bound underneath them and then reads the answer out of the operating system's timing. That test existed for a while and was not reliable. One test still uses the real probe against a real bound port, so the default the call sites take is not only ever exercised through a stand-in.
The tray works out the exe and arguments for itself whenever a tracked move arrives without them, which is what a move over the viewer port does - the wire carries two paths and nothing else. It took them off the tool definition, and the viewer's definition still declares the plain two path form, so a pair reopened that way got a window of its own while the queue it belongs to was on screen behind it. It also got CanKill from IsMdi, which for the viewer means the tray would kill the window every other pending pair is drawn in. Neither is reachable often - it needs a tray owning the inline queue and a move that did not come through AddDiff, which the refusal fallback is - but the two callers disagreeing about how to reopen the same pair is the kind of thing that only ever gets worse. PendingFiles.RelaunchFor answers it once, and both ask. The tray's own call is not covered, for the reason the Kill dispatch is not: it goes through resolution by extension, and which tool that finds depends on what is installed. What it now calls is covered with a constructed viewer, an ordinary tool and an MDI one.
Nothing in DiffEngine reaches ViewerMode.File any more, which makes it look like something left behind rather than something kept. It is kept: it is the blocking one-pair-per-invocation shape a difftool caller needs, where queue mode's second invocation forwards and exits and the caller races on through the rest of the files; it is the only place accepting means copy rather than move, which is what two arbitrary files a person named deserve; and Fixtures.File() is the state around thirty test call sites are built on, so collapsing it would re-approve every renderer, scroll and pixel snapshot with a pending column those tests are not about. Written down because the next person to read ScreenBuilder will see four mode checks guarding a path nothing takes, and the reasons are not in the code.
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.
No description provided.