Skip to content

Remember a disconnected display instead of forgetting it on sight - #101

Merged
didriksg merged 5 commits into
didriksg:mainfrom
ncchen99:keep-display-disconnected
Sep 6, 2026
Merged

Remember a disconnected display instead of forgetting it on sight#101
didriksg merged 5 commits into
didriksg:mainfrom
ncchen99:keep-display-disconnected

Conversation

@ncchen99

@ncchen99 ncchen99 commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

What & why

Fixes #93.

A disconnect is a choice about one particular display, stored by UUID — but reconcile dropped the record the moment that display showed up online again, so the choice lasted only until the next time macOS enumerated it. For a monitor that is cabled but switched off at the wall that is every boot: it enumerates as an entirely ordinary display, takes a slot in the arrangement, and the same disconnect has to be redone by hand.

reconcile now re-applies the disconnect for a record whose display is back, instead of deleting the record. Launch, relaunch, reboot and replug all arrive through the path DisplayManager already calls on every refresh, so there is no second mechanism and no new setting.

Per your note on the issue, the changed default rather than an opt-in.

The old "never disconnect on launch" promise is kept a different way:

  • only displays the user disconnected themselves are ever touched;
  • wouldLeaveNoActiveDisplay still refuses to take the last screen, restoreIfNoActiveDisplay is still the backstop;
  • a re-apply that is refused, or that leaves the display verifiably still on screen, forgets the record exactly as before — reconcile keeps its invariant that no record exists for a display that is on screen;
  • the result is read from enumeration, not from the transaction, for the reason verifyBackOnline gives: a lying success would otherwise have this retry at every refresh;
  • Intel keeps the old behaviour outright, having no working disconnect to re-apply.

The Reconnect race you caught. reconnect() clears the record only once setEnabled(true) returns, and a reconfiguration callback inside that window runs refreshDisplays, and so reconcile, which would find the display online with its record still in place and switch it straight back off. reconnectInFlight — the same shape as softReconnectInFlight — names the displays a reconnect is running on, so reconcile reads intent from there rather than from a record that has not been cleared yet. restoreIfNoActiveDisplay goes through reconnect() too, so the backstop is covered by the same guard.

Re-entrancy. reapplyInFlight is per-UUID rather than a single flag: a burst of callbacks refreshes the display list several times over, and two records can legitimately be mid-re-apply at once. It is restoreInFlight's job in the same place, just not shared between unrelated displays.

reapplyOnWake is gone. It re-disconnected whatever macOS had re-enabled during sleep, which is now just one case of a display coming back online; the wake chain's own refreshDisplays runs reconcile like any other. One path instead of two.

That is a behaviour change on the wake path and not only a deletion, so stating it plainly: the chain no longer awaits the re-disconnect, and it goes from two refreshes to one. The re-apply is now started by reconcile and runs on its own, so wake returns before the display is back off, and a display macOS re-enabled later in the wake settle is caught by the reconfiguration callback's refresh rather than by that second explicit pass. It self-corrects either way, but the timing is not what it was.

How tested

Live on a MacBook Pro (M5, macOS 26.6.1) with a BenQ GW2780 over DisplayPort through a j5create JCD552 dock, using the built-in panel as the remembered display. Crisp quit for the first half so the re-apply is the only thing that could have acted; the display was re-enabled from outside Crisp with SLSConfigureDisplayEnabled, standing in for macOS bringing it back at boot:

Crisp quit → display re-enabled externally → still online at t+1s, t+2s, t+3s
Crisp launched                              → disconnected again within 1s
record kept, not dropped

Then the same with Crisp already running (the display-appears case rather than the launch case): re-enabled externally, disconnected again inside a second, record intact.

Then the ordinary things, on a build installed and running for a day: full reboot, sleep and wake, and unplugging the dock and plugging it back in. The remembered display stayed out of the arrangement across all of them, the built-in came back on undock, and the external's stored HiDPI mode was reapplied as before.

make check is green (SwiftLint strict, tests, localization keys).

Not verified live: the Reconnect race itself. Hitting the window means landing a reconfiguration callback inside reconnect's await on purpose, which I could not stage from outside the app; the guard is reasoned from the same MainActor ordering softReconnectInFlight relies on.

Checklist

  • Builds locally (./scripts/release.sh v0.0.0-ci equivalent via make build)
  • No new user-facing strings
  • No UI change

@ncchen99

ncchen99 commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

One consequence of "forget the record if the re-apply doesn't take" that I should put on the record, because it happened to me while I was running the branch rather than in theory.

I keep two records: the built-in panel, and an Acer that is cabled over HDMI but switched off. After a session of reboots, sleeps and dock replugs, the built-in record was intact (and had its isBuiltin refreshed by the re-apply), while the Acer record was gone. The Acer's hardware side is flaky — an enable on its ID answers CGCompleteDisplayConfiguration 1001 right now — so the most likely path is that it enumerated online during a replug, the re-apply failed, and the record was dropped by design. I could not confirm it from the unified log, since this file logs nothing yet (#100 fixes that, which would have answered it outright).

So the honest shape of the tradeoff: a transient failure during a wake or replug storm silently costs the user the choice, and the symptom is exactly the annoyance the issue is about — disconnect it again. The alternative is keeping the record and retrying on the next refresh, which risks the other failure: the Reconnect row naming a display that is on screen, and your invariant gone.

I left the patch as it is, since dropping is what preserves the invariant and it matches what reconcile did before. If you would rather it survived a bad moment, a bounded retry — keep the record, try again on the next refresh, forget it only after the second failure — is a small change and I am happy to push it. Your call on which failure is the better one to have.

@ncchen99

ncchen99 commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

Following up on my own comment above: the tradeoff was worse than I described, and I've pushed a fix.

I framed dropping the record as costing the user their choice. It can cost them the display. setEnabled(false) can report an error and take anyway, and the old code read the record's fate from that result: error → forget. The display is then switched off at the window server with no record, so no Reconnect row names it, and replugging does not undo it — the window server is holding that state, not the cable. Nothing in the UI can reach it.

That is what happened to the Acer I mentioned. Full sequence, now that I have it from the reporter's side rather than by inference:

  1. record present, display cabled but the cable turned out to be unplugged
  2. Reconnect from the menu → fails, record correctly stays
  3. HDMI plugged back in → display enumerates online → re-apply runs → record dropped
  4. row gone from the menu; dock replug, cable replug and power-cycling the monitor all change nothing
  5. SLSGetDisplayList still lists it, CGGetOnlineDisplayList does not: switched off, unreferenced
id 2 vendor=0x09d1 online=true   ← BenQ, the one lit screen
id 1 vendor=0x0610 online=false  ← built-in, disconnected on purpose, record intact
id 3 vendor=0x0472 online=false  ← Acer, disconnected, record gone: stranded

Recovering it needed SLSConfigureDisplayEnabled from outside the app.

The fix reads the outcome from enumeration instead of from the transaction, which is what the file already does in the other direction: still online after the attempt, refusals included → forget the record, as before, so your invariant holds; off → keep it, whatever the result said, because in that state the record is the only handle on the display.

That also answers the retry question from my last comment: no retry needed. The failure I was worried about — a transient error during a wake or replug storm — is now indistinguishable from success as far as the record is concerned, because both leave the display off, and off is the state the record exists to describe.

Worth noting reenableUnintentionallyDisabled did not catch this. It is scoped to displays outside the intentional set, which the Acer had just been dropped into, but it only runs from softReconnect's exhausted retries and stranded recovery — nothing calls it on an ordinary refresh. I am not proposing to widen it here, only recording that the safety net below this one does not cover the hole.

@didriksg didriksg left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed in full, and applied on top of #99 and #100 to check the combination builds: it does, with one trivial conflict in reconnect() where both sides add lines before setEnabled(true), your in-flight guard and my log line, and both are wanted.

The race fix is right. reconnectInFlight covers the window, reconcile skips it, reapplyRemembered re-checks on entry, and the record is cleared before the defer fires with no suspension point in between. Deciding the record's fate by enumeration rather than the return value is the right call, and your comment on why a dropped record strands the display is exactly the reasoning that belongs in this file.

One thing I would change before merging, and it comes from numbers measured today rather than from reading. After the disable, reapplyRemembered polls for 1 second before deciding. On direct-attached hardware here a disable reported success after 628 ms and an enable commit took 888 ms, so a healthy transaction already uses most of that second. This file budgets 4 seconds for the same wait in softReconnect, because a display link handshake runs 2 to 4 s. And setEnabled gives up at 10 s while the commit keeps running, which #33's capture shows is real on a hub: a 29.5 s WindowServer stall. Put those together on slow hardware and the sequence is: the transaction reports failure at 10 s, the 1 s poll sees the display still lit, the record is dropped, and then the commit lands and switches the display off with no record left. That is the stranding your own comment warns about, reached through the timeout path. Matching softReconnect's 4 s window and re-checking enumeration once more before dropping would close it. The long stall in #33 is on enable, and nobody has yet seen a disable block that long, so treat the 10 s branch as reasoned rather than reproduced; the 1 s against 2 to 4 s mismatch stands either way.

One decision I would rather see made than fall out as a side effect. If a remembered display is the only screen attached at boot, wouldLeaveNoActiveDisplay refuses, the display stays lit, and the record is dropped. A single boot in that configuration silently forgets the choice, which is the complaint #93 opened with. Keeping the invariant that the list never claims a display is off while it is on screen is defensible, but it deserves a sentence in the doc comment saying that is the intended trade.

Minor, for the description only: the wake path no longer awaits the re-disconnect and goes from two refreshes to one. It self-corrects through the reconfiguration callback, but it is a behaviour change.

Merge order on this file is #99, #100, then this, so it wants a rebase once those two land, plus the conflict above.

@ncchen99

ncchen99 commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto #99, #100, #106, #109 and #110. The reconnect() conflict resolved the way you described, both lines kept, your log line before the in-flight insert so a capture records the request before anything can observe it. #110 is a visible improvement from this side too: the reconnects that used to sit for seconds now land immediately, which also means the window below is reached far less often than it was.

The 1 second window. Changed, and the reasoning holds up against the file rather than just against the numbers: softReconnect budgets 4 s for the same wait and says why in place, and one second is most of a single healthy transaction on its own. It is 4 s now, plus a 2 s second look on the branch that forgets the record, since that is the direction where being wrong costs a display rather than a preference.

What I would rather say outright than have you find: that narrows the window, it does not close it. Nothing bounds the commit, and the 29.5 s in #33 is longer than any wait that belongs on a refresh path. A disable slow enough still reports failure, still finds the display lit when the looks run out, and still lands afterwards on a record that is gone.

The three cases that reach the drop branch are not equally ambiguous, though:

what happened display still lit dropping the record
wouldLeaveNoActiveDisplay refused yes safe: nothing was ever issued
disable reported success yes safe: the transaction is finished and it did not take
disable reported failure yes ambiguous: genuinely failed, or the wrapper gave up and the commit is still in WindowServer's hands

Only the third strands anything, and no length of window resolves it, because what would resolve it is knowing whether the commit is still running. The change that closes it properly is narrow: in that case only, keep the record and let the next reconcile decide. The cost is that the list can name a lit display for the length of one refresh, which is a bounded violation of your invariant instead of the permanent one it exists to prevent.

I have not pushed that, for two reasons. It is your invariant to spend. And the timeout path is reasoned rather than reproduced, exactly as you framed it: the long stall in #33 is on enable, and nothing has yet shown a disable blocking that way. Say the word and it is small. setEnabled would need to tell its wrapper timeout apart from a real CGBeginDisplayConfiguration failure, which collapse to the same .configurationFailed(.failure) today; it already measures the elapsed time that separates them, it just does not return it.

The boot-with-one-screen trade is now stated in reconcile's doc comment rather than left to fall out of the code: with the remembered display as the only screen attached, the last-screen guard refuses and a single boot in that configuration forgets the choice, which is the complaint #93 opened with, in the one arrangement where honouring it would mean booting to a black machine. Written as the trade it is, so the next person reading it does not take it for an oversight and "fix" it.

Description updated for the wake path: not just a deletion, the chain no longer awaits the re-disconnect and goes from two refreshes to one, so wake returns before the display is back off and a later re-enable is caught by the reconfiguration callback instead of by that second explicit pass.

Pushed as a third commit rather than squashed into the other two, so the response to the review reads on its own.

make check green on the rebase.

@didriksg

didriksg commented Sep 3, 2026

Copy link
Copy Markdown
Owner

Read the rebase and the third commit, and built it here: compile, strict lint and the x86_64 typecheck are green on 13f69db. The 4 s window plus the 2 s second look reads right, and the boot trade is better stated in the doc comment than it would have been in a review thread.

One thing the rebase could not have known about, since #109 landed after your branch was written. disconnect() on main now snapshots the other displays' modes before the disable and puts them back afterwards, because WindowServer applies its stored arrangement for the smaller display set the moment a display is disabled (#108: my other external dropped from 2560x1440 at 165 to 1920x1080 at 60 and the built-in changed scale). reapplyRemembered calls setEnabled(false) directly, so a re-applied disconnect at boot, relaunch, wake or replug goes through that drop with nothing to put the modes back, and at boot it would happen every time. The fix is the two calls disconnect() uses: currentModes(excluding: liveID) before the disable, restoreModes after it.

On the ambiguous branch: yes, for the timeout only. Give setEnabled a distinct case for the wrapper giving up (it already has the elapsed time) and keep the record in that one case. A real failure still drops it, so a display whose disable genuinely keeps failing cannot pull a fresh transaction out of every refresh. The list naming a lit display for one refresh is a cost I will take over a stranded display.

I will run it on my desk once those are in: disconnect, relaunch, wake, with the other displays' modes checked each time. With one screen attached here right now the re-apply refuses by design, so that waits for the next time the lid is open.

@ncchen99

ncchen99 commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Both in, as 48ad7c8.

The modes. reapplyRemembered now takes the same snapshot and starts the same restore as disconnect(). One deviation from the literal instruction, which I would rather flag than bury: the restore is started immediately after setEnabled returns, not after the verify. The verify can hold for up to six seconds on the slow path, and restoreModes polls rather than sleeping precisely so the flip is as short as it can be — waiting for the verify would hand the user those six seconds at the wrong resolution. It only ever acts on a display that actually moved, so starting it after an attempt that turns out not to have taken costs nothing.

The ambiguous branch, timeout only, as you specified. setEnabled's wrapper now returns a distinct ToggleError.timedOut instead of collapsing into configurationFailed(.failure), and that is the one case where the record survives for the next refresh to decide. Every other failure drops it, so a display that genuinely cannot be switched off still cannot pull a fresh transaction out of every refresh. New case means a new localized string; added to the catalog by hand rather than letting Xcode rewrite it.

Run here, since you are down to one screen: BenQ GW2780 over DisplayPort through the dock, built-in as the remembered display. Crisp quit, the built-in re-enabled from outside with SLSConfigureDisplayEnabled so the re-apply is the only thing that can act, then launched:

built-in enabled externally, Crisp not running
  id 3 BenQ     ON   1600x900 @60  (px 3200x1800, modeID 94)
  id 1 built-in ON   1512x982 @120

Crisp launched
  display: disable 1: reported success after 573 ms
  id 3 BenQ     ON   1600x900 @60  (px 3200x1800, modeID 94)
  id 1 built-in off

BenQ sampled every 0.8 s for 11 s across the whole re-apply: modeID 94 throughout.
Both records intact afterwards, isBuiltin refreshed on the built-in's.

So: the re-apply path works and the restore is wired into it, but I did not reproduce #108 on this pair — WindowServer never moved the BenQ when the built-in went off, so restoreModes found nothing to put back and correctly did nothing. The restoring branch itself is unexercised here. Yours is the setup that produces the move, so that half still wants your desk rather than mine.

The timeout branch is unreproduced too, for the same reason it was reasoned rather than measured in your review: nothing here makes a disable block. With #110 in, disables on this hardware report success in about 570 ms.

make check green.

@ncchen99

ncchen99 commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

A measured number for the timeout branch, which we both had down as reasoned rather than reproduced. It turned up while I was chasing the count question on #92, so it is from the wake path rather than from a hub.

Two runs of the same sequence — built-in held disconnected, two externals, sleep, pull the dock while asleep, wake — and the same operation, setEnabled(true) on the built-in from the blackout rescue:

run 1, issued in DarkWake (before the user's keypress reached full wake)
  19:59:50.992  rescue decides
  20:00:09.703  slow enable 1: commit took 6690 ms
  20:00:10.443  enable 1: reported success after 7430 ms

run 2, issued after full wake
  20:10:09.340  rescue decides
  20:10:09.803  enable 1: reported success after 463 ms

Same machine, same display, sixteen times the duration. CGCompleteDisplayConfiguration blocked for 6.7 s because the transaction was issued while the machine was in DarkWake with the panel still down; it did not complete until the spacebar took the system to full wake. That is 7.4 s against the wrapper's 10 s, on ordinary hardware with nothing wrong, and the gap that produced it is under the user's thumb — a couple more seconds before they touch a key and it crosses.

Two caveats. It is an enable, not a disable, so it is not direct evidence for the branch we changed. And the rescue is what issued it, not reapplyRemembered. But it is the same wrapper over the same call, and the wake path is exactly where reapplyRemembered runs: a display that comes back at wake gets its re-apply while WindowServer is still settling, which is the condition that produced this. I would treat the 10 s timeout as reachable in practice now rather than theoretical, which makes the timedOut case earning its keep more likely than I assumed when I wrote it.

Nothing to change in the PR from this — the handling is already what you specified. Recording it because you asked for the 10 s branch to be treated as reasoned, and it now has a number next to it.

Also, for whenever you get the lid open: the mode restore in reapplyRemembered is still unexercised. I got #108 to reproduce here on the manual path — display 2 moved to 756x1344 after the disconnect, restoring 900x1600 @120: ok, so #109 works on this hardware — but I could not provoke the same move through the re-apply path in three attempts with two externals plus the built-in. Plausibly because that first restore left WindowServer's stored arrangement for the smaller set already correct, so there is nothing left to shuffle. The code path is shared, but the restoring branch has still only been seen working from disconnect().

@didriksg

didriksg commented Sep 3, 2026

Copy link
Copy Markdown
Owner

Ran 48ad7c8 tonight with the lid open, the built-in and two externals, merged onto main at a585c95. Records written straight into the defaults, the displays switched on from outside with a bare SkyLight probe, and the mode restore watched in the log. Everything I could drive worked, and the restoring branch you could not reach is now seen from this path.

What passed:

  • Cold launch with a record for display 3: disable 3: reported success after 2299 ms, display 2 dropped to 1920x1080 a second later and the log has display 2 moved to 1920x1080 after the disconnect, restoring 2560x1440 @165: ok. So the External display changes resolution while another display is disconnected through Crisp #108 drop does recur at launch here, and the restore puts it back.
  • Display 3 switched on from outside while remembered off: re-disconnected within two seconds, record intact with the same id.
  • Relaunch with display 3 held off: record survives, nothing re-applied, and the Log the blackout restore's stand-downs #111 line says 2 active display(s), standing down.
  • Two records (both externals) at launch: both re-applied, only the built-in left, both records intact.
  • A third record for the built-in on top of those: refused as the last screen and dropped, the other two kept. The trade in the doc comment is what happens.

One thing I need changed, and it is a measurement, not a reading of the diff. On the resurface path the snapshot is taken too late. Before display 3 came back the built-in was at 1352x878. The enable itself moved it to 1512x982, because that is what WindowServer has stored for the built-in plus display 3 set. reapplyRemembered then snapshotted 1512x982, disabled display 3, WindowServer put the built-in back to 1352x878 on its own, and the restore undid that: display 1 moved to 1352x878 after the disconnect, restoring 1512x982 @120: ok. Net effect: a display the user has disconnected blinked on for two seconds and the built-in changed scale. The same sequence ran in reverse when display 2 came back. This is exactly the wake case too, since macOS re-enables the display at wake and reconcile is now the wake path, so a user with a remembered disconnect would get the full-set modes pinned on their other displays after every wake. What the user was actually in is the arrangement from the last refresh where the remembered display was still offline, and WindowServer restores that one itself when the display goes off again. So: snapshot the other displays' modes at that earlier refresh and restore to those, and keep the live snapshot only at launch, where there is nothing earlier. On this desk that would have left the built-in at 1352x878 both times.

Smaller, from the two-record run: the two re-applies each took their own snapshot and each ran restoreModes, so the same restore of display 1 landed twice, 19 ms apart. Harmless here, but one snapshot per reconcile pass shared between the records would drop the redundant transaction.

For the record, not a change: display 3's DDC/CI controller was wedged tonight (6 s I2C timeouts), and #110's hold did what it is for: disable 3: waited 12873 ms for DDC to go idle before the launch re-apply, no WindowServer stall over 270 ms across the whole run. It means a remembered display can stay lit for up to 15 s after launch on a bad channel, which is the price of not freezing the machine.

Not driven: the Reconnect row (needs a click by hand, still to do), wake, and the timeout branch, which your DarkWake number covers better than anything I can provoke here.

A disconnect is a choice about one particular display, and Crisp already stores
it by UUID — but reconcile() dropped the record the moment that display showed
up online again, so the choice only ever lasted until the next time macOS
enumerated it. For a monitor that is cabled but switched off at the wall, that
is every single boot: it enumerates as an entirely ordinary display (its EDID
EEPROM and hot-plug detect stay powered in HDMI standby), takes a slot in the
arrangement, and the same disconnect has to be redone by hand. See issue didriksg#93.

reconcile() now re-applies the disconnect for a record whose display is back,
rather than deleting the record, which covers launch, relaunch, reboot and
replug through the one path DisplayManager already calls on every refresh. No
new setting: the choice was made and stored when the user disconnected the
display, and the Reconnect row is still the way to take it back.

That does reverse the old "never disconnect on launch" default, so it keeps
that promise a different way — nothing can be blacked out that the user did not
disconnect themselves, wouldLeaveNoActiveDisplay still refuses to take the last
screen, and restoreIfNoActiveDisplay is still underneath as the backstop. A
re-apply that is refused, fails, or does not verifiably drop the display out of
the online list forgets the record exactly as before, so reconcile keeps its
invariant: no record for a display that is on screen. The re-apply is also
verified by enumeration rather than by the API's return value, since a lying
success would otherwise have it retry at every refresh. Intel keeps the old
behaviour outright, having no working disconnect to re-apply.

Two things follow from reconcile being able to configure displays now.

reconnect() clears the record only once setEnabled(true) returns, and a
reconfiguration callback inside that window runs refreshDisplays, and therefore
reconcile, which would find the display online with its record still in place
and switch it straight back off — the user clicks Reconnect and nothing
happens. An in-flight set, the same shape as softReconnectInFlight, names the
displays a reconnect is running on so reconcile reads intent from there rather
than from a record that has not been cleared yet. restoreIfNoActiveDisplay goes
through reconnect() too, so the backstop is covered by the same guard.

And reapplyOnWake is gone. It re-disconnected whatever macOS had re-enabled
during sleep, which is now just a special case of a display coming back online;
the wake chain's own refreshDisplays runs reconcile like any other, so the
second mechanism was doing the first one's work over again.
The re-apply decided the record's fate from the transaction: anything other than
a verified disable dropped it. That is right for a display that is still lit —
the list must not claim a display is disconnected while the user is looking at
it — and wrong for the case that reads identically from the return value: a
disable that reports an error and takes anyway. The display is then switched off
at the window server with its record gone, so no Reconnect row names it, and
replugging cannot undo it because the window server holds that state, not the
cable. The display is stranded with no way back through the UI.

Not theoretical: it happened to a monitor of mine while running this branch. It
had been left disconnected, was cabled but switched off at the wall, and after a
dock replug it turned up SLS-disabled with no record and nothing in the app able
to reach it. Re-enabling it needed SLSConfigureDisplayEnabled from outside Crisp.

So the outcome is now read from enumeration rather than from the result, which
is what the rest of this file already does in the other direction (see
verifyBackOnline). Still online after the attempt, refusals included: forget the
record, same as before. Off: keep it, whatever the transaction claimed, because
the record is the only handle on a display in that state.
The re-apply polled for 1 second after the disable and then read the
record's fate from what it saw. This file budgets 4 seconds for the same
wait in softReconnect, because a display link handshake runs 2-4s, and a
healthy transaction already uses most of one second on direct-attached
hardware. setEnabled's wrapper gives up at 10s without being able to
cancel the commit underneath it, and didriksg#33 has WindowServer holding one for
29.5s, so on slow hardware the sequence was: failure reported at 10s, the
1s look finds the display still lit, the record is dropped, and then the
commit lands and switches the display off with nothing left to name it.
That is the stranding the comment two lines below warns about, reached
through the timeout instead of through a lying error.

The wait now matches softReconnect's 4 seconds, and the branch that
forgets the record confirms with a second look before acting, since that
is the direction with the expensive mistake.

Also states the boot-time trade in reconcile's doc comment rather than
leaving it to fall out: with the remembered display as the only screen
attached, the last-screen guard refuses and the record is forgotten, so a
single boot in that configuration loses the choice. Keeping the list from
ever naming a display the user is looking at is worth that.
Two things from the review of the rebase.

didriksg#109 landed after this branch was written. `disconnect()` now snapshots the
other displays' modes before the disable and puts back any WindowServer moved
when it applied its stored arrangement for the smaller display set (didriksg#108).
`reapplyRemembered` calls `setEnabled(false)` directly, so a re-applied
disconnect went through that same drop with nothing to put the modes back, and
at boot it would have done so every time. It now takes the same snapshot and
starts the same restore. Started right after the disable rather than after the
verify, for the reason restoreModes polls instead of sleeping: the flip the
user sees should be as short as it can be, and the verify can hold for seconds.

And the ambiguous branch, for the timeout only. `setEnabled`'s 10s wrapper
stops waiting without being able to cancel the commit, so its failure is the
one that is not evidence: the transaction may still be in the window server's
hands, and didriksg#33 has one held for 29.5s. That case now has its own `ToggleError`
rather than collapsing into `configurationFailed(.failure)`, and the record
survives it for the next refresh to decide. Everything else is unchanged: a
disable that genuinely fails still drops the record, so a display that cannot
be switched off cannot pull a fresh transaction out of every refresh.
Two fixes from @didriksg's run of the branch, both in the re-apply's mode
restore.

The snapshot was taken too late. On the resurface path the enable that brings
the remembered display back has already moved the other displays to
WindowServer's stored arrangement for the larger set by the time
reapplyRemembered runs, so a live snapshot captures the moved state; the
disable then has WindowServer put the others back on its own, and the restore
undid that correction. Measured on his desk: the built-in went 1352x878 ->
1512x982 on the enable, back to 1352x878 when the display went off, and the
restore pushed it to 1512x982 again. Net effect, a display the user had
disconnected blinked on for two seconds and the built-in changed scale, and
the same would happen after every wake.

So the modes now come from the last reconcile pass where no remembered display
was online, which is the arrangement the user is actually in and the one
WindowServer restores by itself. That baseline is refreshed on every such pass,
so a resolution picked in between is what goes back. The live snapshot stays
for launch only, where there is no earlier pass to read.

And one restore per pass instead of one per record: two records each took their
own snapshot and each ran restoreModes, landing the same restore twice 19 ms
apart. The pass takes the snapshot once and whichever disable lands first
consumes it. Started after a disable is issued, as before, since before that
there is nothing to poll for and restoreModes' window is finite.

currentModes now excludes a set rather than a single ID, since a pass can take
more than one display off.

make check green.
@ncchen99
ncchen99 force-pushed the keep-display-disconnected branch from 48ad7c8 to 2376c4e Compare September 5, 2026 11:45
@ncchen99

ncchen99 commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

Both in, as 2376c4e, rebased onto main at 4275339 (clean, no conflicts). make check green before and after the rebase.

The snapshot now comes from the last reconcile pass where no remembered display was online, kept in baselineModes and refreshed on every such pass, so a resolution picked in between is what goes back. The live snapshot stays for launch only, where there is no earlier pass. And one snapshot per pass: reconcile takes it once and whichever disable lands first consumes it, so the pass runs one restoreModes however many records it re-applied. currentModes excludes a set now rather than a single ID, since a pass can take more than one display off.

I could reproduce #108 through the re-apply path this time. What I was missing in the three attempts before is that the two arrangements have to actually differ: with all three displays here the built-in sits at 1512x982, and so did the two-display arrangement, so nothing ever moved. Setting the built-in to 1352x878 while the Acer was off pins the smaller set at that, and then the move happens every time. Ground truth first, with Crisp quit and a bare SkyLight probe doing the toggling:

Acer off        built-in 1352x878 @120 (modeID 42)
enable Acer     built-in 1512x982 @120 (modeID 54)   ← WindowServer, three-display arrangement
disable Acer    built-in 1352x878 @120 (modeID 42)   ← WindowServer puts it back on its own

One record, resurface path. Record for the Acer, Acer switched on from outside while Crisp is running, built-in at 1352x878 before it comes back.

053a00c (before)
  19:36:26.974  disable 3: reported success after 206 ms
  19:36:27.305  display 1 moved to 1352x878 after the disconnect, restoring 1512x982 @120: ok
  final: built-in 1512x982   ← the state you described

2376c4e (after)
  19:37:59.794  disable 3: reported success after 378 ms
  (no restore line)
  final: built-in 1352x878   ← where the user was

Nothing moved from the restore's point of view in the second run, because the baseline and what WindowServer put back are the same modes, so restoreModes found no display off its target and did nothing. Record intact in both runs.

Two records at launch. Records for the built-in and the Acer, both displays online at launch, the BenQ the only screen left afterwards. Its one-display arrangement pinned to 1280x720 so it differs from the 1600x900 of the three-display set.

053a00c (before)
  19:44:04.628  disable 1: reported success after 959 ms
  19:44:05.146  disable 3: reported success after 1477 ms
  19:44:05.337  display 2 moved to 1280x720 after the disconnect, restoring 1600x900 @60: ok
  19:44:05.337  display 2 moved to 1280x720 after the disconnect, restoring 1600x900 @60: ok

2376c4e (after)
  19:41:17.221  disable 1: reported success after 930 ms
  19:41:17.756  disable 3: reported success after 1465 ms
  19:41:17.946  display 2 moved to 1280x720 after the disconnect, restoring 1600x900 @60: ok

Two lines at the same millisecond before, one after, with the disables 500 ms apart in both runs and the single restore covering both. This one is the launch case, so it is the live snapshot doing the work, as you specified.

Both builds were the same scratch bundle, same desk, same records written straight into the defaults, and the displays toggled from outside with the probe so the re-apply is the only thing acting.

Not driven: wake. The undock-while-asleep run I did tonight for #112 was on plain main without this branch, and I would rather run wake against this once the phantom question there is settled than mix the two.

@didriksg didriksg left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ran 2376c4e tonight on top of main at 6a46e69, lid open, the built-in and two externals, the same way as last time: records written into the defaults, displays switched on from outside with a bare SkyLight probe, the display log watched. make check is green on the merge, and so is an x86_64 typecheck. Everything passed, including the wake you left alone.

Resurface, one record. Record for display 5, launched with it off, then switched on from outside. The re-apply fired 370 ms after the enable, the disable reported success after 364 ms, and no restore ran: the built-in sits at 1512x982 in both arrangements here, and WindowServer put the other external back on its own, so restoreModes found nothing off its target. Record intact. That's your "after" run, reproduced.

Two records at launch. Records for 3 and 5, both online when Crisp launched. Both re-applied within 3 ms of each other, the disables landed at 1.6 s and 2.3 s (the DDC hold from #110 waited 1.3 and 1.6 s in front of them), and one restore line: display 1 moved to 1352x878, put back to 1512x982. One line, not two. Both records intact.

Wake. Both records in place, both externals off, 82 s of real sleep from the Apple menu, woken with a key. WindowServer re-asserted 3 and 5 off at wake by itself (setEnabled:0 for both, 70 ms after the built-in came up), so nothing resurfaced, the wake refresh ran reconcile with nothing to do, and the records survived. On this Mac macOS doesn't switch a disabled display back on at wake, so the path you removed from AppDelegate had nothing to do here either. Where it does, it goes through the same reconcile as the resurface case above.

Reconnect row, which neither of us had clicked on this branch. Record for display 5, display off, Reconnect clicked in the panel: the enable reported success after 703 ms, the display came on and stayed on, the record was gone, and no re-apply followed. That's the reconnectInFlight guard doing its job.

The timeout branch I read rather than drove; nothing here holds a commit for 10 s.

One thing I added on my side while testing, and will put up as a small PR once this is in so you don't need another round: two notice lines in the display category, "remembered disconnect for id N: re-applying" (or "refused, it is the last active display") and "record dropped for id N: still lit". Without them a capture shows a disable with nothing saying why, and captures from other people are how the #33 class of report gets settled.

Noted, not asked: a display that times out on every attempt gets a fresh transaction on every reconfiguration while it stays lit, since the record is kept on a timeout. refreshDisplays runs only from the add, remove and move callbacks, launch, wake and two panel actions, so it's bounded by how often the display itself reappears, which is what a remembered disconnect should do anyway.

Merging. Three rounds with numbers in every one of them is more than most PRs get, and it shows in the code.

ncchen99 added a commit to ncchen99/Crisp that referenced this pull request Sep 7, 2026
… Mac

Opt-in and off by default, hidden on a Mac with no built-in panel (didriksg#92). When an
external display arrives the built-in is disconnected; when the last one leaves it
comes back. A blackout watch sits underneath: if every screen goes dark while the
rule owns the built-in's state, it brings a display back rather than leaving the
machine unusable.

Rebuilt on main rather than replayed. The branch predated didriksg#101 and didriksg#133, so two
things changed on the way:

- The wake path is `reconcile`, not the `reapplyOnWake` this branch added; the
  guard that keeps a stale built-in disconnect from being re-applied while nothing
  external is up now filters `reconcile`'s pending list instead.
- `usablePhysicalDisplayIDs` was the same test as main's `viewableActiveDisplays`,
  written before it existed. Dropped, and the existing one is now internal. The two
  must stay one test: a display that does not count as a screen for the count
  cannot count as an external for the rule.

make check green.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feature request: remember a display's disconnected state across relaunch, wake and reboot

2 participants