-
-
Notifications
You must be signed in to change notification settings - Fork 92
Fix null pointer reference on MacOS #174
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Fix null pointer reference on MacOS #174
Conversation
Use ScreenCapturekit directly to get window dimensions.
WalkthroughReplaced Cocoa-based window scale and dimension retrieval with ScreenCaptureKit queries in the mac target. Window branches now find the window by id from SC content, compute scale from the containing display's mode, and compute dimensions from the SC frame with fallbacks. Display branches unchanged. Changes
Sequence Diagram(s)sequenceDiagram
actor Caller
participant MacTarget as mac::get_scale_factor
participant SC as ScreenCaptureKit
Caller->>MacTarget: get_scale_factor(Target::Window)
MacTarget->>SC: query current content (windows, displays)
SC-->>MacTarget: windows list, displays list
alt window found and display resolved
MacTarget->>MacTarget: compute scale = pixel_width / width
MacTarget-->>Caller: scale
else fallback
MacTarget-->>Caller: 1.0
end
sequenceDiagram
actor Caller
participant MacTarget as mac::get_target_dimensions
participant SC as ScreenCaptureKit
Caller->>MacTarget: get_target_dimensions(Target::Window)
MacTarget->>SC: query current content (windows)
SC-->>MacTarget: windows list
alt window found
MacTarget->>MacTarget: frame = window.frame * scale
MacTarget-->>Caller: (width, height)
else fallback
MacTarget-->>Caller: (800, 600)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/targets/mac/mod.rs (2)
134-134
: Naming nit:cg_win_id
is misleading (this is an SC window id).Rename to avoid confusion with CoreGraphics ids.
Example:
- let cg_win_id = window.raw_handle; + let sc_win_id = window.raw_handle;…and update references accordingly within this block.
89-123
: Optional: reduce duplicate SC enumeration and improve locality.Both
get_scale_factor
andget_target_dimensions
callShareableContent::current()
and iterate windows/displays. Consider a small helper to resolve “window -> containing display mode” once, returning(frame_in_points, mode)
so both scale and pixel size derive from the same snapshot. This also avoids TOCTOU between two independent enumerations.If you want, I can draft a minimal helper like:
fn resolve_window_and_display(window_id) -> Option<(CGRect, cg::DirectDisplayId, cg::DisplayMode)>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/targets/mac/mod.rs
(2 hunks)
🔇 Additional comments (1)
src/targets/mac/mod.rs (1)
89-123
: Confirm coordinate space consistency between SCWindow.frame and CGDisplay bounds.You’re comparing
sc_window.frame()
againstCGDirectDisplay
bounds to choose the containing display. On macOS this should be in global display coordinates (points), but issues can arise with rotated displays, mirroring, or unusual arrangements. Please validate on multi-display + rotation setups.I can provide a small diagnostic that logs
frame
and the matched displaybounds
at runtime to confirm assumptions if you want.
Fixes #173 null pointer reference when capturing a single window on MacOS.
Use ScreenCapturekit directly to get window dimensions.
Summary by CodeRabbit