fix(capture): tolerate menu-bar item bounds that overshoot NSScreen.frame#614
Conversation
…rame Status-item windows on macOS 26 routinely report bounds that extend a couple of pixels past `NSScreen.frame.maxX` (observed on the Clock and Thaw's own status item: bounds = (1029, 0, 443, 33) on a 1470-wide display, two pixels past the right edge). The strict `frame.contains` requirement introduced in 2ddc419 rejected every such capture as "no display fully contains effectiveBounds", so the SCK path silently returned nil and the affected icons disappeared from Settings → Menu Bar Layout and the Search bar. Replaces the contains-both guard with a largest-intersection-area pick: each display's intersection with `unionBounds` is measured and the display holding the largest area wins. Edge-overshoot cases capture against the display that holds the bulk of the window and SCK clips the trailing few pixels (visually identical to the legacy `SLWindowListCreateImageFromArray` behaviour, which also clipped at the display edge). Cross-display spans pick the majority side instead of "any silent partial". Truly orphan windows whose `unionBounds` doesn't intersect any display still return nil with a warning, so the defensive intent of the original change is preserved for the case that actually warranted it.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe ChangesDisplay Intersection-Based Selection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
2 participants
What does this PR do?
Fixes a regression introduced by #613 where the strict
display.frame.contains(...)precondition inBridging.captureWindowsImageSCKrejected status-item windows whose bounds report a few pixels pastNSScreen.frame.maxX(observed on the Clock and Thaw's own status item). The SCK capture silently returnednil, so the affected icons disappeared from Settings → Menu Bar Layout and the Search bar. Replaces the contains guard with a largest-intersection-area display selection that tolerates small edge overshoots while still rejecting truly orphan windows.PR Type
Does this PR introduce a breaking change?
If yes, please describe the impact and migration path:
N/A. Function signature and external behaviour for healthy captures are unchanged. The fix only relaxes a precondition that was rejecting valid captures.
What is the current behavior?
After #613,
Bridging.captureWindowsImageSCKselects the host display viacontent.displays.first(where: { $0.frame.contains(effectiveBounds) && $0.frame.contains(unionBounds) }). macOS 26's WindowServer routinely reports status-item bounds that overshootNSScreen.frame.maxXby a couple of pixels (observed example: Clock at(1029, 0, 443, 33)on a 1470-wide display: the bounds end at x=1472, two pixels past the display's right edge). The strictcontainsrejects every such case as "no display fully contains effectiveBounds", returnsnil, and the downstream consumer (Layout settings preview, Search panel icon strip) renders nothing for that item. The Clock and Thaw's own status item are the most consistently affected because they sit at the trailing end of the menu bar.Issue Number: N/A
What is the new behavior?
captureWindowsImageSCKnow picks the display whose intersection withunionBoundshas the largest area. The display holding the bulk of the window wins for edge-overshoot cases (SCK still clips the trailing few pixels atdisplay.frame.maxX, which matches the legacySLWindowListCreateImageFromArraybehaviour that also clipped at the display edge, so the captured image is visually identical). Cross-display spans pick the majority side instead of accepting whichever display arbitrarily intersected first. Truly orphan windows whoseunionBoundsdoesn't intersect any display still returnnilwith a warning, so the defensive intent of #613's change is preserved for the case that warranted it. Diagnostic warning is updated to report botheffectiveBoundsandunionBoundsso future rejections are debuggable.PR Checklist
Other information
Reproduction was driven from the user-facing report: open Settings → Menu Bar Layout and check that the Clock and Thaw status items render in the layout preview; open the menu-bar Search panel and check that those icons appear in the row. Before this fix, both surfaces displayed empty slots for those items. Diagnostic confirmation in
~/Library/Logs/Thaw/thaw_*.logshowed the rejection linescaptureWindowsImageSCK: no display fully contains effectiveBounds=(1029.0, 0.0, 443.0, 33.0) and unionBounds=(1029.0, 0.0, 443.0, 33.0)firing once per affected item per refresh tick. After the fix, those warnings disappear andcaptureWindowsImageSCK: captured N windows -> WxHpxlines replace them.Notes for reviewers
Largest-intersection was preferred over reverting to the original
intersects-with-fallback chain because the fallback chain ended incontent.displays.first(any display), which was looser than necessary and could have picked an unrelated display. Largest-intersection is genuinely better than both: it tolerates the macOS-26 overshoot we observed empirically, picks the right display for cross-monitor spans, and rejects truly orphan windows so callers still fall back / skip cleanly. The diagnostic warning now includes botheffectiveBoundsandunionBoundsso future regressions report enough state to investigate without instrumentation.Summary by CodeRabbit
Bug Fixes