Make a held-disconnected display discoverable (caption, menu bar mark, notice) - #104
Closed
ncchen99 wants to merge 5 commits into
Closed
Make a held-disconnected display discoverable (caption, menu bar mark, notice)#104ncchen99 wants to merge 5 commits into
ncchen99 wants to merge 5 commits into
Conversation
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.
A disconnected display is switched off at the window server: it shows no signal, and it is absent from System Settings > Displays, so nothing outside this menu can tell the user what happened to it. Since the choice now outlives a replug and a reboot, someone who plugs that monitor back in months later gets a black screen and no explanation, and replugging the cable cannot fix it — which reads as broken hardware rather than as a setting. One caption under the Disconnect row, at the moment the choice is made.
Nothing on screen says a display is being kept off. It shows no signal, System Settings does not list it, and the one place that knows is a menu the user has no reason to open — least of all the user who has forgotten they ever made the choice, which is now a choice that outlives a replug and a reboot. A second dot on the status icon, in the opposite corner and a different colour from the Keep Awake one so both can be read at once. It is deliberately small: it is not an alert, it is the thing a puzzled user clicks — and clicking it is what shows the Reconnect row. The badge factory is now shared between the two rather than duplicated.
The state this feature creates is invisible from outside the app: the display is
switched off at the window server, so it shows no signal, System Settings does
not list it, and replugging the cable changes nothing, because the window server
holds that state and not the cable. Plug that monitor back in months later and
everything about it says the hardware died.
A banner at exactly that moment, carrying the way out with it: the display's own
name, one line saying Crisp is holding it, and a Reconnect button that does it
without opening anything.
Finding the moment took measuring. Unplugging and replugging the HDMI cable of a
display Crisp was holding disconnected, with registry entry IDs beside the
service events as they fired:
[03:24:15] EVENT terminate id=4294977625 <- cable out
[03:24:15] framebuffer 4294969518:- ...same object, name cleared
[03:24:23] EVENT publish id=4294978031 <- cable back in
[03:24:24] framebuffer 4294969518:E241Y G0 ...same object, name restored
The window server never moves: a disabled display does not leave SLSGetDisplayList
and never re-enters the online list, so there is no reconfiguration callback and
no CoreGraphics event to hang this on — the same reason the user sees nothing.
And the framebuffer service is the wrong thing to watch: its object outlives the
cable and only its properties change, so it publishes nothing. DCPAVServiceProxy
is the service actually created and destroyed with the cable. It carries no EDID
of its own, so it is the trigger and the framebuffer is the identity; the name
comes back about 0.4s after the event, hence the delayed reads.
It tells, it never decides. Crisp does not take the disconnect back on its own:
a cable arriving cannot be told apart from a dock re-enumerating everything on
it, or from a DisplayPort monitor being switched off and on at the wall, so
acting on that signal would undo a deliberate choice at moments nobody asked
for. Reconnecting stays the user's action.
Three things keep it from becoming noise. It is silent for a minute after launch
and after each wake, so the displays that come back as a matter of course say
nothing — only one arriving mid-session, which is the case this exists for:
walking up to a monitor, plugging it in, and getting a black screen. At most one
banner per display per day, so a marginal cable cannot produce a stream. And
authorization is requested at the first banner rather than at launch, so the
prompt arrives at the one moment it can be answered from experience, and a user
who never reaches this state is never asked at all.
UserNotifications autolinks; no build-script change needed (verified with otool
on the swiftc path, which is what dev.sh and release.sh use).
Owner
|
Taking the caption, and only that. It says the cost at the moment of the choice, adds no control, and matches the subtitles the panel already uses. Not the dot and not the notification, for now. Keep Awake's dot marks something you switch on for a while and would otherwise forget about; a remembered disconnect is a standing arrangement, and for the people who use it on purpose the dot would sit there every day marking a state they chose. The notification is a new framework, a new permission asked at the most confusing possible moment, and a trigger specific to the DCP path, all for a case nobody has filed yet. Your tell-never-decide reasoning is right, and I would rather leave the door open for the day someone reports the black monitor than add either now. Once #101 lands, rebase this down to the caption commit and I will take it straight away. Or fold it into #101 if that is less work for you; either is fine. |
Owner
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
2 participants
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.
What & why
Follow-up to #93 / #101, and a question rather than a merge request: I'd like your read on whether you want any of this, and which parts.
Thank you for the review on #93 — the Reconnect race you caught was real, and the point about
reconcilebeing where the choice becomes durable is what made the patch small. #99 and #100 were both better than what I would have sent.Stacked on #101. The first two commits here are that PR; only the last three are new. Happy to rebase once #101 lands, or to close this and reopen it as separate PRs.
The problem is one #101 creates. A disconnected display is switched off at the window server: it shows no signal, System Settings does not list it, and replugging the cable changes nothing, because the window server holds that state and not the cable. While the choice only lasted until the display next enumerated, that was survivable. Now that it lasts, someone who comes back to a monitor they disconnected months ago gets a black screen, replugs the cable, power-cycles the monitor, and has every reason to conclude the hardware died. The choice was theirs, but nothing on the machine can tell them so.
Three independent commits, each of which stands alone — take any subset:
1. A caption under the Disconnect row (
65a5c07). One line, at the moment the choice is made: "Stays disconnected until you reconnect it here." The cheapest of the three, and the one I would keep if you only wanted one.2. A dot on the menu bar icon while any display is held off (
84e0463). Opposite corner and a different colour from the Keep Awake one, so both can be read at once; the badge factory is now shared rather than duplicated. Deliberately small — it is not an alert, it is the thing a puzzled user clicks, and clicking it is what shows the Reconnect row.3. A notification with a Reconnect button (
45d710e). The one I am least sure you want, since it is a new framework and a new permission. Two things that I hope make it defensible: authorization is requested at the first banner rather than at launch, so no prompt appears for anyone who never reaches this state; and it tells without deciding — Crisp never takes the disconnect back on its own.Why it does not auto-reconnect on a replug
That was my first instinct and I think it is wrong. A cable arriving cannot be told apart from a dock re-enumerating everything on it (my second monitor is on the same dock, so every dock replug would resurrect it), nor from a DisplayPort monitor being switched off and on at the wall, where the link drops entirely and HDMI's does not — so the same rule would behave differently per connector, in a feature whose value is being predictable. And any signal that resurrects displays automatically can resurrect one that is not really there. So: tell, never decide.
How tested
On a MacBook Pro (M5, macOS 26.6.1), Developer ID signed build, with an Acer E241Y on HDMI through a j5create dock, held disconnected by Crisp.
Finding the trigger took measuring, and the result was not what I assumed. Unplugging and replugging that cable, registry entry IDs beside the service events as they fired:
Two things follow. The window server never moves: a disabled display does not leave
SLSGetDisplayListand never re-enters the online list, so there is no reconfiguration callback and no CoreGraphics event to hang this on — which is exactly why the user sees nothing either. AndIOMobileFramebufferShimis the wrong service to watch: its object outlives the cable and only its properties change, so it publishes nothing (I shipped that version to myself first and it stayed silent through four replugs).DCPAVServiceProxyis the one actually created and destroyed with the cable. It carries no EDID, so it is the trigger and the framebuffer is the identity; the name returns about 0.4s after the event, hence the delayed reads.End to end, on the build with the corrected trigger:
make checkis green (SwiftLint strict, tests, localization keys; both new strings carry zh-Hans).UserNotificationsautolinks — no build-script change needed, verified withotool -Lon the swiftc path thatdev.shandrelease.shuse.One note if the dock-switching rule from #92 ever lands: the notification's Reconnect should also tell that rule the user asked for the panel, the same way the menu's Reconnect does, or the rule will take it away again seconds later. I have that line locally but left it out here, since the service it calls does not exist on this branch.
Checklist
./scripts/release.sh, Developer ID signed)Crisp/Resources/Localizable.xcstringsupdated for both new strings