-
Notifications
You must be signed in to change notification settings - Fork 6k
Android Embedding PR 12: Add lifecycle methods to FlutterActivity. #7974
Android Embedding PR 12: Add lifecycle methods to FlutterActivity. #7974
Conversation
2fd3ccd
to
5df96a4
Compare
I did a force push after rebasing against master and resolving conflicts from PR 11 |
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.
This seems to have a lot of stubs/TODOs. Is there a reason we'd land it in this state without plumbing through to FlutterEngine?
public void onTrimMemory(int level) { | ||
// Use a trim level delivered while the application is running so the | ||
// framework has a chance to react to the notification. | ||
if (level == TRIM_MEMORY_RUNNING_LOW) { |
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.
Maybe not an issue for this PR, but I suspect we should be doing something with the CRITICAL version of this as well. Maybe @jason-simmons has context?
@dnfield part of the reason for the TODOs is that different files are being filled in different PRs in parallel, so there is some cross dependency. Also, I have been asked to keep these PRs around 100-200 lines long and moving some logic requires changes that might far exceed that. So I'm doing what I can to move in brief pieces with rapid handling of TODOs to remove them after adding them. |
Would it make more sense to merge this one after FlutterEngine is in a place for it? |
The bottleneck is the reviews, so we should merge anything that has been appropriately reviewed so that I can move on to handling the TODOs. |
Is there a different PR we could look to merge first so that more of this could be implemented? This looks like it wouldn't really be useable in its current state. Could we work through the PR to make FlutterEngine do what it needs to do to support this first? |
@dnfield No, there is no other PR. You're currently added to all open PRs and none of them fill out FlutterFragment. I understand this is an unusual process, but it's simply the result of trying to migrate a staging branch as quickly as we can. The goal of this PR is to add all the call forwarding that we know we'll need from FlutterActivity to FlutterFragment, regardless of what FlutterFragment does with it. Do you have any review concerns about the javadocs on those methods, or the need for any of the FlutterActivity call forwards to exist? If you believe the comments are appropriate, and the calls are necessary, then it would seem completely safe to move forward, right? |
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.
Readability LGTM
I'm going to force push after rebasing against master to pull in recent merged work. |
56ccd90
to
0eff43c
Compare
@dnfield The functional TODOs have been resolved. |
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.
LGTM
flutter/engine@99f3f7a9c Fix incorrect transformation matrix (flutter/engine#8001) flutter/engine@c88b09710 Roll src/third_party/skia b7b2da871e95..255569187f27 (23 commits) (flutter/engine#8002) flutter/engine@302e2e9d2 Fix cursor jumping when typing some special characters. (flutter/engine#7964) flutter/engine@fe15149d1 Android Embedding PR 12: Add lifecycle methods to FlutterActivity. (flutter/engine#7974) flutter/engine@6145e9046 Android Embedding PR 13: Integrated text input, keyevent input, and some other channel comms in FlutterView. (flutter/engine#7979)
flutter/engine@99f3f7a9c Fix incorrect transformation matrix (flutter/engine#8001) flutter/engine@c88b09710 Roll src/third_party/skia b7b2da871e95..255569187f27 (23 commits) (flutter/engine#8002) flutter/engine@302e2e9d2 Fix cursor jumping when typing some special characters. (flutter/engine#7964) flutter/engine@fe15149d1 Android Embedding PR 12: Add lifecycle methods to FlutterActivity. (flutter/engine#7974) flutter/engine@6145e9046 Android Embedding PR 13: Integrated text input, keyevent input, and some other channel comms in FlutterView. (flutter/engine#7979)
This work will be applied atop PR11, so don't worry about
FlutterEngine
not being instantiated inFlutterFragment
:#7972