macOS: Set the theme on the NSWindow, instead of application-wide#3736
macOS: Set the theme on the NSWindow, instead of application-wide#3736
NSWindow, instead of application-wide#3736Conversation
c056a77 to
102ffb3
Compare
0fcebe1 to
00030a4
Compare
| _context: *mut c_void, | ||
| ) { | ||
| trace_scope!("observeValueForKeyPath:ofObject:change:context:"); | ||
| if key_path == Some(ns_string!("effectiveAppearance")) { |
There was a problem hiding this comment.
Given that entire function is inside the if probably should just early return.
There was a problem hiding this comment.
At some point in the future we might get more key_paths that we're observing here, and then it makes sense to handle each in their own if - that's the whole reason why I'm checking the keypath, in reality we don't have to right now, since we're only observing one.
There was a problem hiding this comment.
In the future, I'd prefer each if case like that to be in its own function/method, but for now, I'd use early return, because it's the best thing you can do now.
|
|
||
| - On Web, let events wake up event loop immediately when using | ||
| `ControlFlow::Poll`. | ||
| - On macOS, set the window theme on the `NSWindow` instead of application-wide. |
There was a problem hiding this comment.
I'd consider that this is a fix, given that it was not behaving as on any other platform, though, it's a change in behavior which could be excluded from the point release... Not sure about it.
There was a problem hiding this comment.
I think it's a "change", but I'd be fine with including it in a point release.
While it could technically break applications that relied on the window theme being set for the whole application by setting it on a single window, I think that's neither likely nor something that we intended to support in the first place.
src/event.rs
Outdated
| /// This only reports a change if the window theme was not overridden by the application. | ||
| /// |
There was a problem hiding this comment.
I think the previous sentence says the same thing. system changes the window theme is not a user changes the theme with Window::set_theme.
There was a problem hiding this comment.
I've changed this to reference Window::set_theme specifically instead, though I do think it still makes sense to be explicit about this. Open for a better wording?
| _context: *mut c_void, | ||
| ) { | ||
| trace_scope!("observeValueForKeyPath:ofObject:change:context:"); | ||
| if key_path == Some(ns_string!("effectiveAppearance")) { |
There was a problem hiding this comment.
In the future, I'd prefer each if case like that to be in its own function/method, but for now, I'd use early return, because it's the best thing you can do now.
ca983f6 to
b8153fe
Compare
This new implementation uses: - The NSAppearanceCustomization protocol for retrieving the appearance of the window, instead of using the application-wide `-[NSApplication effectiveAppearance]`. - Key-Value observing for observing the `effectiveAppearance` to compute the `ThemeChanged` event, instead of using the undocumented `AppleInterfaceThemeChangedNotification` notification. This also fixes `WindowBuilder::with_theme` not having any effect, and the conversion between `Theme` and `NSAppearance` is made a bit more robust.
b8153fe to
5810bb2
Compare
|
After this change, I can observe the If |
| /// Returns `None` if the current theme is set as the system default, or if it cannot be | ||
| /// determined on the current platform. |
There was a problem hiding this comment.
This was overlooked. I think earlier conversations about the ThemeChanged event and set_theme() were accidentally equated with the getter.
Web definitely does support theme() and does actually return the current theme even if its the default.
Windows, the only other platform implementing this API, also returns the current theme, even without an override.
So this documentation/implementation is not correct.
I would argue that the solution would probably be to introduce a second method for returning the system theme, while leaving this method exactly in place as it is.
Windows would have to be adjusted and Web would always return None in Window::theme().
I will open a separate issue for this.
Thanks @tronical.
|
See #3837. |
This new implementation uses:
-[NSApplication effectiveAppearance].effectiveAppearanceto compute theThemeChangedevent, instead of using the undocumentedAppleInterfaceThemeChangedNotificationnotification.This also fixes
WindowBuilder::with_themenot having any effect (fixes alacritty/alacritty#8046), and the conversion betweenThemeandNSAppearanceis made a bit more robust.changelogmodule if knowledge of this change could be valuable to users