-
Notifications
You must be signed in to change notification settings - Fork 6k
Re-lands platform brightness support on iOS #10791
Changes from all commits
05ad20c
1bf2e6a
6126ae2
27af2f8
74148fc
e0b9656
7bd9ac8
201f691
e1f0a20
55af42e
b709f95
ed60b1c
8fdb300
ee5e8b8
5c83334
5ac2ada
435a134
13cfeaf
e53c791
c0de8e5
c76d20a
557fb06
7bfdd78
a4053eb
1921433
0e2a1b5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,6 +32,20 @@ @interface FlutterViewController () <FlutterBinaryMessenger> | |
| @property(nonatomic, readwrite, getter=isDisplayingFlutterUI) BOOL displayingFlutterUI; | ||
| @end | ||
|
|
||
| // The following conditional compilation defines an API 13 concept on earlier API targets so that | ||
| // a compiler compiling against API 12 or below does not blow up due to non-existent members. | ||
| #if __IPHONE_OS_VERSION_MAX_ALLOWED < 130000 | ||
| typedef enum UIAccessibilityContrast : NSInteger { | ||
| UIAccessibilityContrastUnspecified = 0, | ||
| UIAccessibilityContrastNormal = 1, | ||
| UIAccessibilityContrastHigh = 2 | ||
| } UIAccessibilityContrast; | ||
|
|
||
| @interface UITraitCollection (MethodsFromNewerSDK) | ||
| - (UIAccessibilityContrast)accessibilityContrast; | ||
| @end | ||
| #endif | ||
|
|
||
| @implementation FlutterViewController { | ||
| std::unique_ptr<fml::WeakPtrFactory<FlutterViewController>> _weakFactory; | ||
| fml::scoped_nsobject<FlutterEngine> _engine; | ||
|
|
@@ -434,6 +448,9 @@ - (void)viewWillAppear:(BOOL)animated { | |
| _engineNeedsLaunch = NO; | ||
| } | ||
|
|
||
| // Send platform settings to Flutter, e.g., platform brightness. | ||
| [self onUserSettingsChanged:nil]; | ||
|
|
||
| // Only recreate surface on subsequent appearances when viewport metrics are known. | ||
| // First time surface creation is done on viewDidLayoutSubviews. | ||
| if (_viewportMetrics.physical_width) { | ||
|
|
@@ -885,10 +902,17 @@ - (void)onLocaleUpdated:(NSNotification*)notification { | |
|
|
||
| #pragma mark - Set user settings | ||
|
|
||
| - (void)traitCollectionDidChange:(UITraitCollection*)previousTraitCollection { | ||
| [super traitCollectionDidChange:previousTraitCollection]; | ||
| [self onUserSettingsChanged:nil]; | ||
| } | ||
|
|
||
| - (void)onUserSettingsChanged:(NSNotification*)notification { | ||
| [[_engine.get() settingsChannel] sendMessage:@{ | ||
| @"textScaleFactor" : @([self textScaleFactor]), | ||
| @"alwaysUse24HourFormat" : @([self isAlwaysUse24HourFormat]), | ||
| @"platformBrightness" : [self brightnessMode], | ||
| @"platformContrast" : [self contrastMode] | ||
| }]; | ||
| } | ||
|
|
||
|
|
@@ -962,6 +986,40 @@ - (BOOL)isAlwaysUse24HourFormat { | |
| return [dateFormat rangeOfString:@"a"].location == NSNotFound; | ||
| } | ||
|
|
||
| // The brightness mode of the platform, e.g., light or dark, expressed as a string that | ||
| // is understood by the Flutter framework. See the settings system channel for more | ||
| // information. | ||
| - (NSString*)brightnessMode { | ||
stuartmorgan-g marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if (@available(iOS 13, *)) { | ||
| UIUserInterfaceStyle style = self.traitCollection.userInterfaceStyle; | ||
|
|
||
| if (style == UIUserInterfaceStyleDark) { | ||
| return @"dark"; | ||
| } else { | ||
| return @"light"; | ||
| } | ||
| } else { | ||
| return @"light"; | ||
| } | ||
| } | ||
|
|
||
| // The contrast mode of the platform, e.g., normal or high, expressed as a string that is | ||
| // understood by the Flutter framework. See the settings system channel for more | ||
| // information. | ||
| - (NSString*)contrastMode { | ||
| if (@available(iOS 13, *)) { | ||
| UIAccessibilityContrast contrast = self.traitCollection.accessibilityContrast; | ||
|
|
||
| if (contrast == UIAccessibilityContrastHigh) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This constant only exists in the iOS 13 SDK, unlike UIUserInterfaceStyleDark which is in 12. This isn't going to compile on the bots. If you want to add this we need to go back to the early discussion of declaring the constant yourself, conditionally, behind an ifdef based on the compiling SDK, so that you can use the constant regardless of which SDK you're compiling against. Same with adding a declaration of accessibilityContrast, which is also 13+
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you have an example of defining such a constant that I can reference and replicate?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I took a stab at only including the accessibility logic when the max API is 13 or above. Please let me know if that's sufficient.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As previously discussed, putting the actual implementation behind ifdefs doesn't do what you want, because that would mean it would only work if someone compiles their own engine locally with the Xcode beta; none of this code would actually be shipped in our library. If this code is important to have now, then per the previous discussion what you need are declarations of the constant and the method that are behind reversed SDK ifdefs (i.e., the local declarations are used when compiling with a stable SDK, and not there to cause duplicate definition errors when using the beta SDK), allowing all of the logic here to compile regardless of SDK. I don't have an example offhand, and would have to dig one up tomorrow. But the declarations that you need behind the ifdefs are exactly the ones that are in the beta SDK: the same name and value for the constant, and the identical signature for the method (declared via a class extension).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. An example to replicate would be great. I wasn't clear on the distinction last time and I haven't been able to locate an example of it in practice.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here are some examples; they are old and on macOS, but the principle is exactly the same. |
||
| return @"high"; | ||
| } else { | ||
| return @"normal"; | ||
| } | ||
| } else { | ||
| return @"normal"; | ||
| } | ||
| } | ||
|
|
||
| #pragma mark - Status Bar touch event handling | ||
|
|
||
| // Standard iOS status bar height in pixels. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,9 +6,29 @@ | |
| #import <XCTest/XCTest.h> | ||
| #import "flutter/shell/platform/darwin/ios/framework/Headers/FlutterViewController.h" | ||
|
|
||
| #include "FlutterBinaryMessenger.h" | ||
|
|
||
| #if !__has_feature(objc_arc) | ||
| #error ARC must be enabled! | ||
| #endif | ||
|
|
||
| @interface FlutterViewControllerTest : XCTestCase | ||
| @end | ||
|
|
||
| // The following conditional compilation defines an API 13 concept on earlier API targets so that | ||
| // a compiler compiling against API 12 or below does not blow up due to non-existent members. | ||
| #if __IPHONE_OS_VERSION_MAX_ALLOWED < 130000 | ||
| typedef enum UIAccessibilityContrast : NSInteger { | ||
| UIAccessibilityContrastUnspecified = 0, | ||
| UIAccessibilityContrastNormal = 1, | ||
| UIAccessibilityContrastHigh = 2 | ||
| } UIAccessibilityContrast; | ||
|
|
||
| @interface UITraitCollection (MethodsFromNewerSDK) | ||
| - (UIAccessibilityContrast)accessibilityContrast; | ||
| @end | ||
| #endif | ||
|
|
||
| @implementation FlutterViewControllerTest | ||
|
|
||
| - (void)testBinaryMessenger { | ||
|
|
@@ -23,4 +43,205 @@ - (void)testBinaryMessenger { | |
| OCMVerify([engine binaryMessenger]); | ||
| } | ||
|
|
||
| #pragma mark - Platform Brightness | ||
|
|
||
| - (void)testItReportsLightPlatformBrightnessByDefault { | ||
| // Setup test. | ||
| id engine = OCMClassMock([FlutterEngine class]); | ||
|
|
||
| id settingsChannel = OCMClassMock([FlutterBasicMessageChannel class]); | ||
| OCMStub([engine settingsChannel]).andReturn(settingsChannel); | ||
|
|
||
| FlutterViewController* vc = [[FlutterViewController alloc] initWithEngine:engine | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are these tests running with ARC? Because these look like leaks. The rest of the engine does not use ARC.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No idea. I was just continuing with what I already found in this file. If you have any specific recommendations for changes, I can make them, but I'm working from a lack of iOS knowledge here.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @chinmaygarde @stuartmorgan do either of you have any specific instructions for what should be done here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dnfield @gspencergoog do either of you know what changes should be made here to avoid leaks? I don't know what changes to make to bring this test code into compliance...
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would turn on ARC for the test file, since that would be easy (it looks like the test project is doing it file-by-file, which seems like an odd approach; I would just enable it for the whole project). Failing that, add an
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @LongCatIsLooong helped me get ARC enabled
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We may want to have ARC off for the project so that we can explicitly test deallocating things...
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe I only enabled it for this file. Do you want me to change something?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, that's fine - my reply was more directed towards @stuartmorgan's comment :)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well since the only test in that file is leaking the view controller, I'm pretty sure it's not deliberately off to test dealloc... |
||
| nibName:nil | ||
| bundle:nil]; | ||
|
|
||
| // Exercise behavior under test. | ||
| [vc traitCollectionDidChange:nil]; | ||
|
|
||
| // Verify behavior. | ||
| OCMVerify([settingsChannel sendMessage:[OCMArg checkWithBlock:^BOOL(id message) { | ||
| return [message[@"platformBrightness"] isEqualToString:@"light"]; | ||
| }]]); | ||
|
|
||
| // Clean up mocks | ||
| [engine stopMocking]; | ||
| [settingsChannel stopMocking]; | ||
| } | ||
|
|
||
| - (void)testItReportsPlatformBrightnessWhenViewWillAppear { | ||
| // Setup test. | ||
| id engine = OCMClassMock([FlutterEngine class]); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have you verified that
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How might I verify this?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just went ahead and explicitly cleaned up every mock in each test. |
||
|
|
||
| id settingsChannel = OCMClassMock([FlutterBasicMessageChannel class]); | ||
| OCMStub([engine settingsChannel]).andReturn(settingsChannel); | ||
|
|
||
| FlutterViewController* vc = [[FlutterViewController alloc] initWithEngine:engine | ||
| nibName:nil | ||
| bundle:nil]; | ||
|
|
||
| // Exercise behavior under test. | ||
| [vc viewWillAppear:false]; | ||
|
|
||
| // Verify behavior. | ||
| OCMVerify([settingsChannel sendMessage:[OCMArg checkWithBlock:^BOOL(id message) { | ||
| return [message[@"platformBrightness"] isEqualToString:@"light"]; | ||
| }]]); | ||
|
|
||
| // Clean up mocks | ||
| [engine stopMocking]; | ||
| [settingsChannel stopMocking]; | ||
| } | ||
|
|
||
| - (void)testItReportsDarkPlatformBrightnessWhenTraitCollectionRequestsIt { | ||
| if (!@available(iOS 13, *)) { | ||
| return; | ||
| } | ||
|
|
||
| // Setup test. | ||
| id engine = OCMClassMock([FlutterEngine class]); | ||
|
|
||
| id settingsChannel = OCMClassMock([FlutterBasicMessageChannel class]); | ||
| OCMStub([engine settingsChannel]).andReturn(settingsChannel); | ||
|
|
||
| FlutterViewController* realVC = [[FlutterViewController alloc] initWithEngine:engine | ||
| nibName:nil | ||
| bundle:nil]; | ||
| id mockTraitCollection = | ||
| [self fakeTraitCollectionWithUserInterfaceStyle:UIUserInterfaceStyleDark]; | ||
|
|
||
| // We partially mock the real FlutterViewController to act as the OS and report | ||
| // the UITraitCollection of our choice. Mocking the object under test is not | ||
| // desirable, but given that the OS does not offer a DI approach to providing | ||
| // our own UITraitCollection, this seems to be the least bad option. | ||
| id partialMockVC = OCMPartialMock(realVC); | ||
| OCMStub([partialMockVC traitCollection]).andReturn(mockTraitCollection); | ||
|
|
||
| // Exercise behavior under test. | ||
| [partialMockVC traitCollectionDidChange:nil]; | ||
|
|
||
| // Verify behavior. | ||
| OCMVerify([settingsChannel sendMessage:[OCMArg checkWithBlock:^BOOL(id message) { | ||
| return [message[@"platformBrightness"] isEqualToString:@"dark"]; | ||
| }]]); | ||
|
|
||
| // Clean up mocks | ||
| [partialMockVC stopMocking]; | ||
| [engine stopMocking]; | ||
| [settingsChannel stopMocking]; | ||
| [mockTraitCollection stopMocking]; | ||
| } | ||
|
|
||
| // Creates a mocked UITraitCollection with nil values for everything except userInterfaceStyle, | ||
| // which is set to the given "style". | ||
| - (UITraitCollection*)fakeTraitCollectionWithUserInterfaceStyle:(UIUserInterfaceStyle)style { | ||
| id mockTraitCollection = OCMClassMock([UITraitCollection class]); | ||
| OCMStub([mockTraitCollection userInterfaceStyle]).andReturn(style); | ||
| return mockTraitCollection; | ||
| } | ||
|
|
||
| #pragma mark - Platform Contrast | ||
|
|
||
| - (void)testItReportsNormalPlatformContrastByDefault { | ||
| if (!@available(iOS 13, *)) { | ||
| return; | ||
| } | ||
|
|
||
| // Setup test. | ||
| id engine = OCMClassMock([FlutterEngine class]); | ||
|
|
||
| id settingsChannel = OCMClassMock([FlutterBasicMessageChannel class]); | ||
| OCMStub([engine settingsChannel]).andReturn(settingsChannel); | ||
|
|
||
| FlutterViewController* vc = [[FlutterViewController alloc] initWithEngine:engine | ||
| nibName:nil | ||
| bundle:nil]; | ||
|
|
||
| // Exercise behavior under test. | ||
| [vc traitCollectionDidChange:nil]; | ||
|
|
||
| // Verify behavior. | ||
| OCMVerify([settingsChannel sendMessage:[OCMArg checkWithBlock:^BOOL(id message) { | ||
| return [message[@"platformContrast"] isEqualToString:@"normal"]; | ||
| }]]); | ||
|
|
||
| // Clean up mocks | ||
| [engine stopMocking]; | ||
| [settingsChannel stopMocking]; | ||
| } | ||
|
|
||
| - (void)testItReportsPlatformContrastWhenViewWillAppear { | ||
| if (!@available(iOS 13, *)) { | ||
| return; | ||
| } | ||
|
|
||
| // Setup test. | ||
| id engine = OCMClassMock([FlutterEngine class]); | ||
|
|
||
| id settingsChannel = OCMClassMock([FlutterBasicMessageChannel class]); | ||
| OCMStub([engine settingsChannel]).andReturn(settingsChannel); | ||
|
|
||
| FlutterViewController* vc = [[FlutterViewController alloc] initWithEngine:engine | ||
| nibName:nil | ||
| bundle:nil]; | ||
|
|
||
| // Exercise behavior under test. | ||
| [vc viewWillAppear:false]; | ||
|
|
||
| // Verify behavior. | ||
| OCMVerify([settingsChannel sendMessage:[OCMArg checkWithBlock:^BOOL(id message) { | ||
| return [message[@"platformContrast"] isEqualToString:@"normal"]; | ||
| }]]); | ||
|
|
||
| // Clean up mocks | ||
| [engine stopMocking]; | ||
| [settingsChannel stopMocking]; | ||
| } | ||
|
|
||
| - (void)testItReportsHighContrastWhenTraitCollectionRequestsIt { | ||
| if (!@available(iOS 13, *)) { | ||
| return; | ||
| } | ||
|
|
||
| // Setup test. | ||
| id engine = OCMClassMock([FlutterEngine class]); | ||
|
|
||
| id settingsChannel = OCMClassMock([FlutterBasicMessageChannel class]); | ||
| OCMStub([engine settingsChannel]).andReturn(settingsChannel); | ||
|
|
||
| FlutterViewController* realVC = [[FlutterViewController alloc] initWithEngine:engine | ||
| nibName:nil | ||
| bundle:nil]; | ||
| id mockTraitCollection = [self fakeTraitCollectionWithContrast:UIAccessibilityContrastHigh]; | ||
|
|
||
| // We partially mock the real FlutterViewController to act as the OS and report | ||
| // the UITraitCollection of our choice. Mocking the object under test is not | ||
| // desirable, but given that the OS does not offer a DI approach to providing | ||
| // our own UITraitCollection, this seems to be the least bad option. | ||
| id partialMockVC = OCMPartialMock(realVC); | ||
| OCMStub([partialMockVC traitCollection]).andReturn(mockTraitCollection); | ||
|
|
||
| // Exercise behavior under test. | ||
| [partialMockVC traitCollectionDidChange:mockTraitCollection]; | ||
|
|
||
| // Verify behavior. | ||
| OCMVerify([settingsChannel sendMessage:[OCMArg checkWithBlock:^BOOL(id message) { | ||
| return [message[@"platformContrast"] isEqualToString:@"high"]; | ||
| }]]); | ||
|
|
||
| // Clean up mocks | ||
| [partialMockVC stopMocking]; | ||
| [engine stopMocking]; | ||
| [settingsChannel stopMocking]; | ||
| [mockTraitCollection stopMocking]; | ||
| } | ||
|
|
||
| // Creates a mocked UITraitCollection with nil values for everything except accessibilityContrast, | ||
| // which is set to the given "contrast". | ||
| - (UITraitCollection*)fakeTraitCollectionWithContrast:(UIAccessibilityContrast)contrast { | ||
| id mockTraitCollection = OCMClassMock([UITraitCollection class]); | ||
| OCMStub([mockTraitCollection accessibilityContrast]).andReturn(contrast); | ||
| return mockTraitCollection; | ||
| } | ||
|
|
||
| @end | ||
Uh oh!
There was an error while loading. Please reload this page.