Hand off presentation properly in VirtualDisplayController.resize()#17897
Conversation
amirh
left a comment
There was a problem hiding this comment.
LGTM (though would love a clarification on my comment below)
| presentation = | ||
| // Initialize a new presentation. We don't want to cancel | ||
| // the old one before we can properly show the new one to | ||
| // take over where it left off. |
There was a problem hiding this comment.
I'm not sure I understand this comment, what side effect does newPresentation.show() has that makes presentation.cancel() safer?
| // Initialize a new presentation. We don't want to cancel | ||
| // the old one before we can properly show the new one to | ||
| // take over where it left off. Cancelling before the show() | ||
| // of the new presentation causes the contents to stop, eg, |
There was a problem hiding this comment.
I'm still confused by this, the view is detached from the presentation at the point we are cancelling it, not sure why would it cause a video to stop or why would showing a different presentation with that view will prevent the video from stopping?
Do you happen to which methods are invoked on the view in which order in the two scenarios? (the one where it stops and the one where it continue playing?)
There was a problem hiding this comment.
So I don't think I know enough about the inner workings of this to write a fully descriptive comment here. Most of this is from what I have gleaned via experimentation getting this to work. My comments here are mainly trying to explain the ordering that needs to happen, but I'm not 100% clear on specifically what in the implementations makes this necessary other that other orderings do not work. I don't want to pick out anything too specific as it may pin the cause incorrectly.
Any suggestions?
|
Let’s just change the language to “that’s what we’ve observed” and add a
link to the issue
…On Thu, Apr 23, 2020 at 2:15 PM Gary Qian ***@***.***> wrote:
cc @chinmaygarde <https://github.com/chinmaygarde>
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#17897 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAH2A5MHQ23U3KUWOLPNTMDROCVYBANCNFSM4MO34TCQ>
.
|
| // PlatformViewsController. We know that all PlatformViewsController does is | ||
| // forward view attachment/detachment calls to it's VirtualDisplayControllers. | ||
| // | ||
| // TODO(mattcarroll): once PlatformViewsController is refactored into testable |
There was a problem hiding this comment.
fwiw, I don't think Matt will be doing this refactor :)
* Update 1.17 engine hash to Dart 2.8.2 stable * Hand off presentation properly in VirtualDisplayController.resize() (#17897) Co-authored-by: Gary Qian <garyq@google.com>
* Update 1.17 engine hash to Dart 2.8.2 stable * Hand off presentation properly in VirtualDisplayController.resize() (flutter-team-archive#17897) Co-authored-by: Gary Qian <garyq@google.com>
* Update 1.17 engine hash to Dart 2.8.2 stable * Hand off presentation properly in VirtualDisplayController.resize() (flutter-team-archive#17897) Co-authored-by: Gary Qian <garyq@google.com>
Based off of #17611 by @dotcink
Fixes flutter/flutter#26345 and flutter/flutter#47154
Previously, we were overwriting the presentation variable with an instance of a new SingleViewPresentation. This caused the old one to remain in memory and crash when callbacks were triggered before GC
This patch creates the new SingleViewPresentation without overwriting the old presentation, shows the new one to maintain smooth transition (without this ordering of calls, content like videos stop playing) and then when the old presentation is no longer needed, calls cancel() and sets the reference to the new presentation.