Allow FlutterEngine to be used on back-to-back screens (#31264). #8808
Conversation
mklim
left a comment
There was a problem hiding this comment.
Java style LGTM, just pending some questions on how the fix needs to fit into the lifecycle.
| } | ||
|
|
||
| @Override | ||
| public void onStart() { |
There was a problem hiding this comment.
How often do we want this to repeat? onStart isn't executed every time the activity is brought into the foreground either unfortunately. onResume would be the right fit for that case.
On the flip side, is flutterView.detachFromFlutterEngine(); the mirror of this? If so it should probably be moved into onStop so it's called in equivalent situations, or onPause if this ultimately needs to be moved into onResume.
There was a problem hiding this comment.
Placing this in onResume() would imply that the UI could attach/detach while the Activity is still visible, but developers shouldn't do that. For example, when a dialog pops up, onPause() is invoked. I think the only reason to put this in onResume() would be if we expected to detach in onPause(), but in that case the Flutter UI would disappear out from under the dialog. So I do think onStart() is the correct lifecycle hook.
As for detaching, the reason that we need to attach in onStart() is in case the underlying engine was re-used in another Activity. So when returning to this Activity we re-attach in onStart(). Then there is detaching, and you asked if detaching should be symmetric to attaching. Technically we could detach in onStop() as you suggested, but it also occurred to me that in most cases that detachment is superfluous. In most cases an engine is probably not being re-used, but we'd go through detachment every time the FlutterActivity hits onStop().
I don't know if there is a meaningful performance risk to detaching like that, but I figured we can avoid it by not explicitly detaching until we know the FlutterFragment is detaching from the Activity or being destroyed.
Does that make sense? Do you still think detachment should happen in on onStop() to be symmetric?
There was a problem hiding this comment.
The onStart() vs onResume() change makes sense to me.
For detaching, I guess my remaining question is is there a consequence for the engine not being detached in line with another potential attach() call? Is that a potential issue or is there no larger issue there? If it doesn't matter then it's fine to be asymmetrical, but I was just assuming that there would be some kind of consequence for overlap there so you'd want to make sure it was torn up and down more predictably.
There was a problem hiding this comment.
Looking again at attachment, given that a number of things are setup there, and we may discover that they need to be cleaned up in detachment, I do think the safer thing is to be symmetric. So I'm going to move detachment to onStop().
flutter/engine@ef2bed6...39a950e git log ef2bed6..39a950e --no-merges --oneline 39a950e remove legacy build deps (flutter/engine#8820) c0be4e2 Fix api conformance check (flutter/engine#8817) 30639ee Allow FlutterEngine to be used on back-to-back screens (#31264). (flutter/engine#8808) 45d0c4d Roll src/third_party/skia cd5d14619fe2..68eb8c276355 (4 commits) (flutter/engine#8815) 9856479 Roll src/third_party/dart b6997deb3e..1577b95c93 (18 commits) 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.
Allow FlutterEngine to be used on back-to-back screens ( #31264 ).
The referenced bug occurs when going from one
Activityto anotherActivity, sharing the same FlutterEngine, then going back. The problem was that the point at which the FlutterEngine was being attached to the FlutterView was executed in a lifecycle method that does not repeat. Thus, this PR moves that attachment toonStart()instead ofonCreateView().Even with this solution, a developer may not use the same
FlutterEnginebetween 2Activitys where the 2ndActivityis translucent, or using the sameFlutterEnginein a dialog. This is because such a situation expects that both UIs are simultaneously visible, but aFlutterEnginecannot be attached to 2 surfaces at the same time, let alone render 2 different user interfaces at the same time.