fix(display): follow screen-parameter changes - #188
Open
shsw228 wants to merge 3 commits into
Open
Conversation
Showing or hiding the menu bar changes how much of a display is usable, but it is not a display reconfiguration: CGDisplayRegisterReconfigurationCallback does not fire. macOS reports it through NSApplicationDidChangeScreenParameters, which was received and then discarded on the assumption that the display callback covered everything. Nothing else re-reads the screen list, and AppKit caches it per process, so the daemon kept serving the geometry it had at startup. Toggling the setting had no effect on the layout until the daemon was restarted, and subscribers were told nothing at all -- verified by discarding the notification again on this branch: zero display events, and the reported main display stayed at 2560x1080 @ (0,0) while the OS reserved 30px. Report the notification on the same channel the reconfiguration callback already uses, which turns DisplayReconfigEvent into a two-variant DisplayChangeSignal. Both paths then converge before anything is re-read: CFRunLoopSource coalesces signals raised before the run loop services it, so a notification and the callbacks it accompanies collapse into one callback, and the handler drains the queue and reconciles once. That matters beyond the notification: CoreGraphics delivers one signal per affected display and the reconciliation covers the whole configuration, so a single hotplug produced eight callbacks and eight full retiles, seven of them against a configuration that had not settled yet. Measured after the change: a display mode change logs one "Reconciling displays" for the notification plus two callbacks that previously produced two. Routing it through the existing channel keeps the observer free of display plumbing: the sender and the run loop source are already held by the display module, so the notification needs neither passed in, and the handler needs no side-channel flag to tell it what woke it.
CoreGraphics invokes the reconfiguration callback twice per affected display: once before the change with kCGDisplayBeginConfigurationFlag set, and once after with the flags describing what changed. Both were forwarded. During the "begin" phase CGDisplayBounds still describes the outgoing configuration, so that pass laid windows out against metrics that were about to be replaced. The post-change pass corrects it, so the visible effect is a transient, but it doubles the reconciliation work per hotplug. Drop the pre-change notification. Over an unplug and replug all eight callbacks that reached the handler carried post-change flags (0x1220, 0x1008, 0x111a, 0x133e, 0x1000), none with the begin bit.
macOS emits several screen-parameter notifications around a single change, and some describe a configuration that is already in state. handle_display_change retiled unconditionally, so those spurious notifications moved every window on every display for nothing. Compare the configuration read from the OS against the one held in state and report no change when they match. The comparison goes through Rect::from_bounds, the same conversion sync_all uses to store the frame, so the two cannot drift apart. Report it as None rather than an empty DisplayChangeResult. The caller cannot tell the two apart otherwise: it treated an empty displays_to_retile as "no particular display, so do them all" and fell through to a full retile, which is exactly what this is meant to avoid. With that distinction in the type, the retile branch loses its fallback and drives the list it is handed, which every branch fills in exhaustively. The early return also covers event emission. display_updated was emitted for every display on every reconcile, so a Dock autohide toggle -- a screen parameter change that leaves the top edge alone -- woke every subscriber with identical payloads. This changes observable behaviour: no retile runs and no events are emitted for a no-op reconcile. Measured around a display mode change: the trailing notification 800ms later now takes the early exit instead of retiling.
Author
|
Heads up on the failing The single error is a pre-existing lint in code this PR does not touch:
The trigger looks like the toolchain floating rather than a code change: The fix is one place: Effect::ExecCommandTracked { command, path } => {
let pid = manipulator.exec_command_tracked(&command, &path)?;
state
.borrow_mut()
.tracked_processes
.push(crate::core::TrackedProcess { pid, _command: command.clone() });
tracing::info!("Tracked process started: {} (pid={})", command, pid);
}Happy to send that as a separate PR so this one stays scoped to the display change — just say the word. Pinning the toolchain would also stop stable bumps from turning unrelated PRs red. |
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.
Fixes #186.
What
NSApplicationDidChangeScreenParameterswas received and then discarded, on the assumption thatCGDisplayRegisterReconfigurationCallbackcovered every geometry change. It does not, so changes reported only through the notification were never noticed: the daemon kept serving the geometry it read at startup, and subscribers were told nothing at all.The notification is reported on the channel the reconfiguration callback already uses, which turns
DisplayReconfigEventinto a two-variantDisplayChangeSignal. Both paths then signal the sameCFRunLoopSource, so a real reconfiguration — which fires the notification and the callback together — is delivered as one callback for the pair, and the reconciliation runs once from a single place.The sender and the run loop source are already held by the display module, so the workspace observer needs neither passed in and the handler needs no side-channel flag to tell it what woke it.
Commits
fix(display): follow screen-parameter changesCFRunLoopSourceso a burst reconciles once; a single hotplug previously produced eight callbacks and eight full retilesperf(display): ignore pre-change reconfiguration callbackskCGDisplayBeginConfigurationFlagpass, whereCGDisplayBoundsstill describes the outgoing configurationperf(display): skip the retile when the configuration is unchangedOption<DisplayChangeResult>The last commit changes
handle_display_changeto returnOption<DisplayChangeResult>rather than an empty result.The caller could not otherwise tell "nothing changed" from "no particular display needs a retile": it read an empty
displays_to_retileas "no specific target, so do them all" and fell through to a full retile — the exact work the commit exists to avoid. It also emitteddisplay_updatedfor every display on every reconcile, regardless of the result.With the distinction in the type, the callback returns early on
None, and the retile branch loses its fallback and drives the list it is handed. Every branch that returnsSomefills that list in exhaustively, so an empty list means no display needs one.Behaviour change
No retile runs and no events are emitted for a no-op reconcile. Subscribers that relied on
display_updatedarriving on every screen-parameter notification, including ones that changed nothing, will see fewer events.Verification
cargo test --workspace312 passing, and each of the three commits builds on its own;cargo clippy --all-targetsreports no new warnings.Reconciling displays→Display configuration unchanged, nothing to do, no retile, 0 events. Before this change the same toggle produced a full retile and 4display_updatedevents carrying identical payloads.2560x1080 @ (0,0)⇄2560x1050 @ (0,30)and windows follow, without a daemon restart.handle_display_changereports no change; that the caller then does nothing is only shown by the run above.Environment
macOS 27.0 (26A5378n), Apple Silicon. Independent from the PR for #185.