Add flutter settings channel and window brightness to macOS shell #8810
Conversation
|
I don't know enough about the code to LGTM, but I like the goal, and it looks reasonable as far as I can tell. Thanks @jonahwilliams. |
| selector:@selector(onSettingsChanged:) | ||
| name:@"AppleInterfaceThemeChangedNotification" | ||
| object:nil]; | ||
| [controller onSettingChanged:nil]; |
There was a problem hiding this comment.
init methods should not call member methods; that's in fact why CommonInit is a free function rather than a method. See http://google.github.io/styleguide/objcguide.html#avoid-messaging-the-current-object-within-initializers-and--dealloc
Apart from the subtle issues, this call isn't actually doing anything useful, because _settingsChannel hasn't been created at this point (nor has the engine been started).
What you'll want to do is make a new sendInitialSettings (or similar) method, put the onSettingsChanged: call there, and call it from launchEngineInternalWithAssetsPath:... after the engine has been started successfully.
There was a problem hiding this comment.
I've moved the sendInitialSettings call to after the engine initialization. This method also contains the initial subscription logic - I'm unsure if this is the correct location.
| [self dispatchMouseEvent:event phase:kHover]; | ||
| } | ||
|
|
||
| - (void)onSettingsChanged:(NSNotification*)notification { |
There was a problem hiding this comment.
This method needs a declaration comment, per style guide. The macOS shell uses the style of pre-declaring all internal methods and putting the declaration there, so please follow that style. (~line 90 in this file)
Also, the method implementations are grouped by type and ordered the same way they are declared, to make it easier to navigate the file; this is the section for NSResponder overrides (see #pragma mark). This should go at the end of the Private methods pragma section (corresponding to where you will be adding the declaration).
| [[NSUserDefaults standardUserDefaults] stringForKey:@"AppleInterfaceStyle"]; | ||
| [_settingsChannel sendMessage:@{ | ||
| @"platformBrightness" : [brightness isEqualToString:@"Dark"] ? @"dark" : @"light", | ||
| // The values below are hard-coded, but the iOS implementation has heuristics |
There was a problem hiding this comment.
Please file a bug for this and make the comment a TODO referencing the bug.
b4a76a8 to
610d7b1
Compare
stuartmorgan-g
left a comment
There was a problem hiding this comment.
Coupling the initial setting message with the registration for changes seems ideal to me, so the structure of this LGTM.
Looking at this again I was surprised it's using string values rather than constants, so I did a bit of looking into dark mode. I left some comments below about TODOs, but other than adding them let's go ahead and landing this as-is since nothing would change structurally about the code when switching to the documented APIs.
| } | ||
|
|
||
| - (void)onSettingsChanged:(NSNotification*)notification { | ||
| NSString* brightness = |
There was a problem hiding this comment.
Could you add a TODO to switch to using NSAppearance here?
| } | ||
|
|
||
| - (void)sendInitialSettings { | ||
| [[NSDistributedNotificationCenter defaultCenter] |
There was a problem hiding this comment.
And based on https://developer.apple.com/documentation/appkit/supporting_dark_mode_in_your_interface?language=objc a TODO to switch this to KVO on effectiveAppearance
…hell (flutter/engine#8810) (#32018) flutter/engine@4808446...1bcbaf7 git log 4808446..1bcbaf7 --no-merges --oneline 1bcbaf7 Add flutter settings channel and window brightness to macOS shell (flutter/engine#8810) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff (amirha@google.com), and stop the roller if necessary.
Adds the flutter/settings message channel to macOS and use it to send the platform brightness. This allows us to support dark mode on Mojave by connecting this setting to https://docs.flutter.dev/flutter/dart-ui/Window/platformBrightness.html.
The MaterialApp will automatically tween between the light and dark theme when this is changed thanks to the existing work of @matthew-carroll