Repository navigation
refactor: move the focus decisions into testable pieces - #34
Merged
Merged
Conversation
The five follow-ups left over from the last cleanup pass. EntryGuard: the rule that stops a window arriving under a still pointer from taking focus was fifteen lines inside hitTestForFocus with its baseline held in an Agent property, and grep found no test touching entryMotionPx at all. It is now EntryGuard in HeedCore with fifteen tests, three of them over an equality that is not an equivalence relation, which is what Target's really is. It did not go into DwellMachine as first sketched: the rule is consulted from inside the hitTest closure the machine calls, so the machine would be consulting itself. DwellMachine now performs the pre-apply confirming re-read itself, and skips it for a candidate the same call hit-tested, which at the default dwell of 0 is every candidate. That was a second full hit test, about eleven AX round trips, for an answer microseconds old, and it double-counted against hitTestFailures so a stuttering app reached the three-strike cooldown in half the real failures. The re-read still runs for a candidate that matured across ticks, which is the case the comment was always about. It returns the refreshed target rather than a yes: one window can report a different element after a dwell, and the stale one can be the one that refuses AXMain. No closure passed to machine.tick may touch machine. A nested access is an exclusivity violation that traps at runtime in every build rather than failing to compile, so the invalidation moved inside the machine. The call graphs of all three closures were checked for it. frontmostApp() reads NSWorkspace once per decision and answers nil for Heed itself, and the value is threaded through the handover questions rather than re-read by each. This buys coherence, not speed: the reads are microseconds, but several AX round trips separate them, so three reads in one decision could legitimately name three different apps and the log would then name an app the hold was not filed under. One Guard list now drives both the tick and --probe, which had been keeping the same six guards by hand in two places and had already drifted once. Ordering is load-bearing and preserved: the window server round trip stays last and stays gated on there being something to act on. verifyFocus sleeps on the activation notification instead of a blind 10ms usleep. NSWorkspace stays the answer, re-read every time round, so a notification that is late or lost costs nothing worse than the old backstop. note() is signalled on the main thread above the existing queue hop: verifyFocus blocks that queue, so signalling from inside it would deadlock the agent for the whole timeout on every focus change. verifyTimeoutMs stays. The wait still needs a budget, so deleting it would have removed the bound without removing the reason for one.
Skipping the confirming re-read for a candidate hit-tested in the same call was wrong. `isAlreadyFocused` runs between the hit test and the point focus is applied, and it reads Accessibility whenever the target's app is already frontmost, which is time enough for the window to have gone. The re-read now always runs, as it did before. What the fold still buys is the decision living in the tested state machine, an invalidation the machine owns, and a signature that hands back the refreshed element rather than a yes. warpAfterHandover took the frontmost app `noteHandover` had sampled, but several Accessibility round trips separate the two, so it could warp the pointer into an app that had since gone to the back and file the hold against it. It reads again for itself, as it did before. The guards read their inputs once per evaluation. `probe` prints a reading and its verdict side by side and was taking each input twice, so a click landing between the two reads could print "0b1" with no SUPPRESSING beside it.
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
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 five follow-ups left over from #33. 290 unit tests pass (265 before), the release build and
make check-packageare green, and the whole branch was exercised live on a two-display Mac.EntryGuard. The rule that stops a window arriving under a still pointer from taking focus was
fifteen lines inside
hitTestForFocuswith its baseline held in an Agent property, and grep foundno test touching
entryMotionPxat all, while its two neighbours in the same decision live inHeedCore with hundreds of lines of tests. It is now
EntryGuardwith fifteen tests, three of themover an equality that is not an equivalence relation, which is what
Target ==really is. It didnot go into
DwellMachineas first sketched: the rule is consulted from inside thehitTestclosure the machine calls, so the machine would have been consulting itself.
The confirming re-read moved into
DwellMachine, which now owns the invalidation and hands backthe refreshed target rather than a yes. One window can report a different
AXUIElementafter adwell, and the stale one can be the one that refuses
AXMain, so the type now enforces what thecode already relied on.
It does not skip that re-read at
dwellMs = 0, which is what the original finding asked for. Iimplemented the skip, and the adversarial review killed it:
isAlreadyFocusedsits between the hittest and the moment focus is applied, and it reads Accessibility whenever the target's app is
already frontmost. That is time enough for the window to have gone, which is exactly what the
re-read is there to catch. The premise that the two reads were microseconds apart holds only when
the pointer has moved to a different app. Measured against ~1-2ms saved per focus change, keeping
the check is the better trade.
No closure passed to
machine.tickmay touchmachine. A nested access is a Swift exclusivityviolation that traps at runtime in every build rather than failing to compile (reproduced: exit
134, "Fatal access conflict detected"). All three closures' call graphs were traced for it, twice.
frontmostApp()reads NSWorkspace once per decision and answers nil for Heed itself. This buyscoherence, not speed: the reads are single-digit microseconds, but several Accessibility round trips
separate them, so three reads in one decision could legitimately name three different apps and the
log would then name an app the hold was not filed under. Callers that genuinely want a different
moment (
adoptPointerWindow,holdingApplies,warpAfterHandover,frontmostForFocus,probe)keep their own reads, and that is commented where it is not obvious.
One
Guardlist now drives both the tick and--probe, which had been keeping the same sixguards by hand in two places and had already drifted once. Ordering is load-bearing and preserved:
the window server round trip stays last and stays gated on there being something to act on. Each
guard's inputs are read at most once per evaluation, so the number
--probeprints and the verdictbeside it cannot disagree.
verifyFocussleeps on the activation notification instead of a blind 10msusleep. NSWorkspacestays the answer, re-read every time round, so a notification that is late or lost costs nothing
worse than the old backstop.
note()is signalled on the main thread above the existing queuehop:
verifyFocusblocks that queue, so signalling from inside it would deadlock the agent for thewhole timeout on every focus change.
verifyTimeoutMsstays, contrary to the original sketch: thewait still needs a budget, so deleting it would have removed the bound without removing the reason
for one.
Verification
Unit tests cover EntryGuard and the confirmation contract. Everything in
Agent.swiftrests on livechecks, run on this branch:
logged.
which is the incoherence the frontmost change closes.
introduced and did not.
ActivationWaitwas extracted verbatim and driven through six checks: early wake, no wake on adifferent pid, no signal leaking past a disarm, and no double-signal into a re-armed wait.
--probeoutput is byte-identical to master apart from the elapsed-seconds readings.Codex reviewed the branch twice. The first pass found three real defects, all mine: the stale-skip
above,
warpAfterHandoveracting on a frontmost app sampled several round trips earlier, and--probereading each guard's state twice so it could print a contradiction. All three are fixed inthe second commit, and the re-review is clean.
Not done, and available separately: refusing a warp into an app that is no longer frontmost. It is a
real improvement but a behaviour change rather than a refactor, it can only ever suppress warps, and
a bug there reads as flakiness rather than as a regression.