-
Notifications
You must be signed in to change notification settings - Fork 149
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
Pause rendering when entering background #1086
Conversation
e6b8523
to
7ecc932
Compare
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.
Looking good. Can you also make the change to how the display link is initialized?
Tests will need to be more thorough if possible:
- verify notifications are registered/unregistered correctly
- unit test UIWindow.parentScene
…in iOS 13+ and falls back to older tech when running on older iOS versions
f056bfc
to
a28a09f
Compare
func testCarPlayWindowReturnsCorrectParentScene() { | ||
let window = CPWindow() | ||
|
||
XCTAssertEqual(window.parentScene, window.templateApplicationScene) |
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.
What are the actual values? Are they both nil? If so, that might not be a very good test.
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.
Yeah, it's tricky to create a good test setup here as it is not possible to create a UIScene
object(including its decendant CPTemplateApplicationScene
. In this test indeed the expectation is that both of these values are nil
.
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.
Not for this PR, but I wonder whether we could set up a test suite that runs in a host app with CarPlay entitlements.
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.
Good point, this will enable for tests like this to be run properly, let's get back to it next time we touch CarPlay related stuff.
Tests/MapboxMapsTests/Foundation/Extensions/UIWindow+ParentSceneTests.swift
Outdated
Show resolved
Hide resolved
Tests/MapboxMapsTests/Foundation/Extensions/UIWindow+ParentSceneTests.swift
Show resolved
Hide resolved
var notificationCenter: MockNotificationCenter! | ||
var bundle: MockBundle! |
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.
Please nil these in tearDown
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.
notificationCenter too please
let notificationCenter = MockNotificationCenter() | ||
let bundle = MockBundle() | ||
bundle.infoDictionaryStub.defaultReturnValue = [:] | ||
dependencyProvider.makeNotificationCenterStub.defaultReturnValue = notificationCenter | ||
dependencyProvider.makeBundleStub.defaultReturnValue = bundle | ||
let mapView = MapView( | ||
frame: CGRect(origin: .zero, size: CGSize(width: 100, height: 100)), | ||
mapInitOptions: MapInitOptions(), | ||
dependencyProvider: dependencyProvider) | ||
|
||
window.addSubview(mapView) |
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.
Is this necessary? seems to duplicate what's already happening in setUp
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.
Similarly in the other tests.
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.
I setup a new instance of MapView
here so I can affect which lifecycle notifications map view subscribes to(by altering bundle's infoDictionaryStub
). In this particular test there is no need to have this setup here as it matches with the one from setUp
. I put it here for consistency with testSceneLifecycleNotificationSubscribedWhenDidMoveToNewWindow
.
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.
Still on the right track! Let me know if you want to talk through any of the feedback.
a3e7118
to
a5b56f0
Compare
|
||
@available(iOS 13.0, *) | ||
@objc private func sceneWillEnterForeground(_ notification: Notification) { | ||
guard notification.object as? UIScene == window?.parentScene else { return } |
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.
I don't think these checks are necessary since we only subscribe for a specific object.
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.
Removing them would allow you to share handlers with the app lifecycle notifications
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.
I added them here to protect from a situation when map view moves to a window without scene attached(there is no clear documentation on specifics of this, so I'm assuming it's possible). The map view will subscribe to lifecycle notifications with nil
object in this case. And this check supposed to be a safeguard against it.
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.
So the scenario is, on iOS 13+ with scenes enabled, map view moves to a window that has a nil scene?
In that case, we would basically not get pausing and resuming until the window's scene became non-nil, right?
Not sure if this scenario is even possible.
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.
Yeah, in this scenario we won't get pausing and resuming, it either this or pausing and resuming by notifications from a random scene(if there would be no checks). But this scenario is definitely in the realm of possibility - windowScene is an optional readwrite property.
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.
Approving, but can you take care of one more thing before merging? #1086 (comment)
Nice work!
a5b56f0
to
37e25fe
Compare
notificationCenter.addObserver(self, | ||
selector: #selector(appWillEnterForeground), | ||
name: UIApplication.willEnterForegroundNotification, | ||
object: nil) |
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.
Per mapbox/mapbox-gl-native#1869, this may be too early for mbgl to wake up and start rendering again:
UIApplicationWillEnterForegroundNotification is posted before UIApplicationDidBecomeActiveNotification, while the application is still in the background.
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.
#1402 replaces this with UIScene.didActivateNotification
.
notificationCenter.addObserver(self, | ||
selector: #selector(sceneWillEnterForeground(_:)), | ||
name: UIScene.willEnterForegroundNotification, | ||
object: window?.parentScene) |
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.
As with UIApplication.willEnterForegroundNotification
, UIScene.willEnterForegroundNotification
is probably premature as well. UIScene.didActivateNotification
would be more reliable.
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.
#1402 replaces this with UIApplication.didBecomeActiveNotification
.
MapView
will now listen to eitherUIApplication
orUIScene
lifecycle event notifications(depending on project setup). As application/scene moves to background/foreground, map view'sdisplayLink
will be paused/resumed accordingly.On iOS 11 and 12
UIApplication
is the only source of these events. Starting from iOS 13 Scene API can be enabled by including UIApplicationSceneManifest key toInfo.plist
.Additionally this PR adds
reduceMemoryUse()
call when transitioning to background as its documentation advises.Fixes #978, #1053
Pull request checklist:
## main
heading near the top).