This repository has been archived by the owner on Aug 8, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Prevent map view on external display from sleeping in background #13701
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Have you consider adding a flag along the lines of
enableExternalDisplay
orenableSecondaryExternalDisplay
? Although I don't have an specific use case in mind, it may be good let the developers control this.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’m leaning towards not making this behavior configurable. Note that this PR only affects whether external displays can continue to render after the main display falls asleep; I’m pretty certain map views can already render on external displays.
I think an
enableExternalDisplay
option would lead to surprising behavior. Suppose an application has non-mirroring support for external displays, such as what this PR adds to iosapp, and the external display’s window contains not only a map view but other views. Perhaps these other views are programmatically synchronized to the map view, with dynamic text based on the current camera. IfenableExternalDisplay
is disabled, then the UI will appear to work normally, but as soon as the main display falls asleep, the map view portion of the UI will halt and eventually go black. All the while, the rest of the UI will continue to update, because the map view’s model continues to function even while rendering is disabled.That said, I’m open to adding an explicit option if we find that this PR regresses the crash fix and we can’t figure out a way around 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.
I’d agree with @1ec5 here — the developer should probably be responsible for handling how mirroring/other-screens are managed (and we should try to transparently support those without configuration).
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.
@1ec5 @friedbunny that makes sense, thank you for explaining your thoughts. I got a final question: If a map is being displayed on a second screen; would it be useful have delegates that accounts for that case?
e.g.
mapViewWillRenderSecondScreen
/mapViewWillStopRenderingSecondScreen
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.
☝🏼discard my comment above.