Preview card follows mouse across monitors, with a pin-to-display option - #128
Open
BradleyAllanDavis wants to merge 5 commits into
Open
Conversation
|
@BradleyAllanDavis is attempting to deploy a commit to the knox projects Team on Vercel. A member of the Team first needs to authorize it. |
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
Comment on lines
+35
to
+37
| if let targetDisplayID, let index = ActiveDisplayResolver.screencaptureDisplayIndex(for: targetDisplayID) { | ||
| args.append(contentsOf: ["-D", String(index)]) | ||
| } |
Contributor
There was a problem hiding this comment.
The whole point of this fix is to stop screencapture from silently grabbing the main display when the user meant a different one. This else path — when a target was expected but no -D index could be resolved (e.g. the display disconnected in the race between resolution and the 200 ms sleep) — falls back to exactly that silent main-display grab. Worth a log so it's diagnosable when it does happen.
Suggested change
| if let targetDisplayID, let index = ActiveDisplayResolver.screencaptureDisplayIndex(for: targetDisplayID) { | |
| args.append(contentsOf: ["-D", String(index)]) | |
| } | |
| if let targetDisplayID, let index = ActiveDisplayResolver.screencaptureDisplayIndex(for: targetDisplayID) { | |
| args.append(contentsOf: ["-D", String(index)]) | |
| } else { | |
| print("BetterShot: could not resolve target display for -D; screencapture will fall back to the main display") | |
| } |
…ay option Adds a Settings > Capture > Preview Thumbnail toggle: the preview card either follows whichever display the mouse is on (the existing default, now centralized in one resolver) or pins to a specific display the user picks. Along the way, found and fixed a real bug this surfaced: captureFullscreen() never used the resolved target screen at all -- plain `screencapture -x -t png` always grabs the main display regardless of which one is active. On a multi-monitor setup, triggering a fullscreen capture with the mouse on a secondary display silently saved the wrong display's content while the preview card still showed up on the right one. Fixed by threading the resolved screen through to a `-D<n>` argument (translated from CGDirectDisplayID to screencapture's ordinal via CGGetActiveDisplayList). Verified end-to-end on all 4 physical displays via a temporarily instrumented build: correct CGDirectDisplayID resolved, correct screencapture ordinal, correct single-display output dimensions (cross-checked against the app's own beautifier padding math and independent manual screencapture -D<n> runs), and pinned mode correctly overrides mouse position for both the capture and the preview.
…ew card's Copy/Save buttons onDrag and onTapGesture were attached to the whole card ZStack, which also contains the hover buttons drawn on top of it (Copy, Save, corner actions). That put a native drag-source recognizer over the same pixels as the buttons. A physical mouse almost always moves a pixel or two between mouseDown and mouseUp, which is enough to cross AppKit's drag-start threshold before the button's own tap gesture resolves — starting a real NSDraggingSession under the cursor that snaps back on release when there's no valid drop target. That snap-back is what read as the pointer jumping around and made the Copy button hard to hit; a trackpad tap doesn't move enough to trigger it, which is why it was mouse-only. Moved onDrag/onTapGesture down onto the base Image view (a sibling of the button overlay, not its ancestor), so the drag-source region no longer overlaps the buttons. Verified with `just check` (xcodegen + Debug build, BUILD SUCCEEDED, no new warnings).
Reproduced on Bradley's setup: a Retina main display (scale 2.0) plus three non-Retina externals (scale 1.0). The preview card's NSPanel was created once and reused across captures -- show() only called createPanel() the first time, and every later capture just repositioned the existing panel via setFrame(). Moving an existing window between screens with different backing scale factors is a known AppKit trouble spot: the window can repaint at its new position while its cached hit-testing state still points at the screen it was last shown on, so a click lands somewhere other than where the buttons are drawn on screen. That matches exactly what Bradley saw: rock solid on the Retina main display (where the panel, first created there, never crossed a scale boundary), cursor-jumping/ missed clicks on every external. show()/dismiss() now always tear the panel down and build a fresh one, so it's created directly on its final screen before it's ever ordered on-screen, and never carries state across a scale-factor transition. Verified: just check (xcodegen + Debug build) succeeded. Needs Bradley to confirm on an actual external monitor -- can't reproduce multi-monitor locally.
Previously the card only picked a screen once, at capture time, and then stayed there even if the mouse moved to a different display afterward -- Bradley's original request was for it to keep following. While the card is visible and Settings > Capture > Preview Thumbnail is set to follow the mouse (not pinned to a specific display), a global + local NSEvent mouseMoved monitor watches for the mouse crossing onto a different screen and, when it does, tears the panel down and rebuilds it there. Deliberately NOT a live setFrame reposition of the same window -- that's the exact pattern the previous commit fixed away from (moving an existing window across a scale-factor boundary desyncs its hit-testing from where it repaints), and doing it continuously as the mouse crosses monitors would hit that bug far more often than a single capture-time reposition ever did. A pinned display is left alone entirely; the monitor is only installed when follow-mouse is enabled. Verified: just check succeeded. Not yet confirmed live on Bradley's actual multi-monitor setup -- needs his testing.
BradleyAllanDavis
force-pushed
the
feat/preview-follow-mouse-display
branch
from
September 5, 2026 13:51
e7774fd to
8484891
Compare
…n capture Addresses tembo's PR review comment: the else path (no -D index resolved) silently fell through to screencapture grabbing the main display -- exactly the silent-wrong-display bug this fix exists to prevent. Only expected in a narrow race (target display disconnected between resolution and the 200ms sleep), but it should be diagnosable when it happens.
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.
Adds a Settings > Capture > Preview Thumbnail toggle: the preview card either follows whichever display the mouse is on (the existing default, now centralized in one resolver) or pins to a specific display the user picks.
Along the way, found and fixed a real bug this surfaced:
captureFullscreen()never used the resolved target screen at all — plainscreencapture -x -t pngalways grabs the main display regardless of which one is active. On a multi-monitor setup, triggering a fullscreen capture with the mouse on a secondary display silently saved the wrong display's content while the preview card still showed up on the right one. Fixed by threading the resolved screen through to a-Dargument, translated fromCGDirectDisplayIDtoscreencapture's ordinal viaCGGetActiveDisplayList.Verified end-to-end on a real 4-monitor Mac via a temporarily-instrumented build: correct
CGDirectDisplayIDresolved per display, correctscreencaptureordinal, correct single-display output dimensions (cross-checked against the app's own beautifier padding math and independent manualscreencapture -Druns), and pinned mode correctly overrides mouse position for both the capture and the preview.Two more real bugs found and fixed using this in daily driving, both pre-existing and unrelated to the toggle itself:
.onDrag/.onTapGesturewere scoped to the whole preview card, which also contains the hover buttons (Copy, Save, etc.) drawn on top of it. A physical mouse's few pixels of jitter between mouseDown and mouseUp was enough to start a native drag session under the buttons before their own tap gesture won the gesture arena, which then snapped back on release with no drop target — read as the cursor jumping around near Copy/Save. Fixed by moving both modifiers onto the base image, a sibling of the button overlay rather than its ancestor.NSPanelwas created once and reused across captures, only repositioned viasetFrame(). On a mixed-DPI desktop (Retina main + non-Retina externals), moving an existing window across a scale-factor boundary is a known AppKit trouble spot — it can repaint at the new position while its cached hit-testing state still points at the old screen, so clicks land somewhere other than where the buttons are drawn. Fixed by always tearing the panel down and building a fresh one on its final screen, never reusing.Also added the live-follow behavior this toggle was originally meant to have: while the card is visible and set to follow the mouse (not pinned), it now keeps moving to whichever display the mouse is actually on, not just the display at capture time — a global+local
NSEventmouse-moved monitor tears the panel down and rebuilds it on a screen change, deliberately not a livesetFramereposition (the same AppKit hazard as the panel-reuse fix above).Addressed @tembo's review comment: the
-Dresolution'selsepath silently fell through to grabbing the main display — exactly the bug the fix above exists to prevent. Added a diagnostic log for the narrow race where it can still happen (target display disconnects between resolution and the capture delay).