-
Notifications
You must be signed in to change notification settings - Fork 59
Update AppKit Components & Examples #784
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
Conversation
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.
Pull request overview
Updates the AppKit hosting integration to better participate in Auto Layout sizing, window lifecycle coordination, and example app setup.
Changes:
- Added sizing/intrinsic-content-size logic and window/content-size constraint management to
NSHostingView. - Extended
NSHostingControllerto exposesceneBridgingOptions, participate in sizing viapreferredContentSize, and set its view during common init. - Updated AppKit app delegate + hosting example to construct windows via
NSWindow(contentViewController:)and keep strong references to window controllers.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| Sources/OpenSwiftUI/Integration/Hosting/AppKit/View/NSHostingView.swift | Adds constraint/sizing behaviors, window notifications, and environment/container updates. |
| Sources/OpenSwiftUI/Integration/Hosting/AppKit/Controller/NSHostingController.swift | Wires hosting view as controller’s view and exposes additional sizing/scene-bridging APIs. |
| Sources/OpenSwiftUI/App/App/AppKit/AppKitAppDelegate.swift | Updates window creation to use contentViewController and retains the window controller. |
| Example/HostingExample/ViewController.swift | Switches macOS example to a window-controller-based setup using NSHostingController. |
| Example/HostingExample/AppDelegate.swift | Removes now-duplicated window controller and relies on the new macOS setup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| center.addObserver(self, selector: #selector(windowDidChangeMain), name: NSWindow.didBecomeMainNotification, object: newWindow) | ||
| center.addObserver(self, selector: #selector(windowDidChangeMain), name: NSWindow.didResignMainNotification, object: newWindow) | ||
| center.addObserver(self, selector: #selector(windowDidChangeKey), name: NSWindow.didBecomeKeyNotification, object: newWindow) | ||
| center.addObserver(self, selector: #selector(windowDidChangeKey), name: NSWindow.didResignKeyNotification, object: newWindow) | ||
| // TODO: NSWindowDidOrderOnScreenNotification / NSWindowDidOrderOffScreenNotification → windowDidChangeVisibility | ||
| center.addObserver(self, selector: #selector(windowWillBeginSheet), name: NSWindow.willBeginSheetNotification, object: newWindow) | ||
| center.addObserver(self, selector: #selector(windowDidEndSheet), name: NSWindow.didEndSheetNotification, object: newWindow) | ||
| center.addObserver(self, selector: #selector(windowDidChangeScreen), name: NSWindow.didChangeScreenNotification, object: newWindow) |
Copilot
AI
Feb 8, 2026
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.
The observers added in viewWillMove(toWindow:) use #selector(windowDidChangeMain)/etc, but the corresponding handler methods should be #selector(windowDidChangeMain(_:)) (and so on) to match NotificationCenter’s required method signature. Update both the selector and method signatures consistently.
| center.addObserver(self, selector: #selector(windowDidChangeMain), name: NSWindow.didBecomeMainNotification, object: newWindow) | |
| center.addObserver(self, selector: #selector(windowDidChangeMain), name: NSWindow.didResignMainNotification, object: newWindow) | |
| center.addObserver(self, selector: #selector(windowDidChangeKey), name: NSWindow.didBecomeKeyNotification, object: newWindow) | |
| center.addObserver(self, selector: #selector(windowDidChangeKey), name: NSWindow.didResignKeyNotification, object: newWindow) | |
| // TODO: NSWindowDidOrderOnScreenNotification / NSWindowDidOrderOffScreenNotification → windowDidChangeVisibility | |
| center.addObserver(self, selector: #selector(windowWillBeginSheet), name: NSWindow.willBeginSheetNotification, object: newWindow) | |
| center.addObserver(self, selector: #selector(windowDidEndSheet), name: NSWindow.didEndSheetNotification, object: newWindow) | |
| center.addObserver(self, selector: #selector(windowDidChangeScreen), name: NSWindow.didChangeScreenNotification, object: newWindow) | |
| center.addObserver(self, selector: #selector(windowDidChangeMain(_:)), name: NSWindow.didBecomeMainNotification, object: newWindow) | |
| center.addObserver(self, selector: #selector(windowDidChangeMain(_:)), name: NSWindow.didResignMainNotification, object: newWindow) | |
| center.addObserver(self, selector: #selector(windowDidChangeKey(_:)), name: NSWindow.didBecomeKeyNotification, object: newWindow) | |
| center.addObserver(self, selector: #selector(windowDidChangeKey(_:)), name: NSWindow.didResignKeyNotification, object: newWindow) | |
| // TODO: NSWindowDidOrderOnScreenNotification / NSWindowDidOrderOffScreenNotification → windowDidChangeVisibility | |
| center.addObserver(self, selector: #selector(windowWillBeginSheet(_:)), name: NSWindow.willBeginSheetNotification, object: newWindow) | |
| center.addObserver(self, selector: #selector(windowDidEndSheet(_:)), name: NSWindow.didEndSheetNotification, object: newWindow) | |
| center.addObserver(self, selector: #selector(windowDidChangeScreen(_:)), name: NSWindow.didChangeScreenNotification, object: newWindow) |
🤖 Augment PR SummarySummary: This PR updates the AppKit hosting components and the macOS hosting example to better align window/controller setup and begin fleshing out sizing/window-bridging behavior. Changes:
Technical Notes: The new AppKit hosting behavior introduces more Auto Layout integration and window-awareness (constraints + content size extrema), and begins scaffolding “scene/window bridging” options for future toolbar/title integration. 🤖 Was this summary useful? React with 👍 or 👎 |
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.
| guard sizingOptions.contains(.intrinsicContentSize), | ||
| !checkForReentrantLayout() | ||
| else { | ||
| return cachedIntrinsicContentSize |
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.
intrinsicContentSize returns cachedIntrinsicContentSize when .intrinsicContentSize isn’t in sizingOptions; if sizingOptions is toggled from enabled → disabled, the cached value could keep advertising an intrinsic size unexpectedly. Consider ensuring the “disabled” path reports no intrinsic metric (or clears the cache) so sizingOptions = [] truly opts out.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| } | ||
|
|
||
| if let newWindow { | ||
| center.addObserver(self, selector: #selector(windowDidChangeMain), name: NSWindow.didBecomeMainNotification, object: newWindow) |
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.
The view registers for multiple NSWindow notifications via NotificationCenter in viewWillMove(toWindow:); if the view is deallocated without a final move to nil, those observers can still fire and message a freed object. Consider also removing observers in deinit as a safety net.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| } | ||
|
|
||
| // TODO: screen.displayID | ||
| let displayID = CGMainDisplayID() |
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.
startAsyncRendering() always creates the display link using CGMainDisplayID() without checking window?.screen, which can schedule rendering even when the view isn’t in a window (and can target the wrong display’s refresh characteristics). It seems safer to gate display-link creation on having an attached window/screen (or otherwise deliberately document why main display is correct).
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| /// window, the default value for this property will be `.all`, which | ||
| /// includes the options for `.toolbars` and `.title`. Otherwise, the | ||
| /// default value is `[]`. | ||
| public var sceneBridgingOptions: NSHostingSceneBridgingOptions { |
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.
The docs for sceneBridgingOptions say the default becomes .all when used as a window’s contentViewController, but the current implementation only forwards to host.sceneBridgingOptions (which defaults to []). This looks like a docs/behavior mismatch that could confuse callers relying on the documented default.
Severity: low
Other Locations
Sources/OpenSwiftUI/Integration/Hosting/AppKit/View/NSHostingView.swift:102
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #784 +/- ##
==========================================
+ Coverage 26.19% 26.57% +0.37%
==========================================
Files 650 650
Lines 40232 40529 +297
==========================================
+ Hits 10539 10769 +230
- Misses 29693 29760 +67 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
fd8e792 to
c3e21c9
Compare
No description provided.