Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

matthew-carroll
Copy link
Contributor

This work will be applied atop PR11, so don't worry about FlutterEngine not being instantiated in FlutterFragment:
#7972

@matthew-carroll matthew-carroll force-pushed the android_embedding_refactor_pr12_add_lifecycle_to_flutteractivity branch from 2fd3ccd to 5df96a4 Compare February 28, 2019 05:11
@matthew-carroll
Copy link
Contributor Author

I did a force push after rebasing against master and resolving conflicts from PR 11

Copy link
Contributor

@dnfield dnfield left a 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) {
Copy link
Contributor

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?

@matthew-carroll
Copy link
Contributor Author

@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.

@dnfield
Copy link
Contributor

dnfield commented Feb 28, 2019

Would it make more sense to merge this one after FlutterEngine is in a place for it?

@matthew-carroll
Copy link
Contributor Author

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.

@dnfield
Copy link
Contributor

dnfield commented Feb 28, 2019

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?

@matthew-carroll
Copy link
Contributor Author

@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?

Copy link

@DaveShuckerow DaveShuckerow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Readability LGTM

@matthew-carroll
Copy link
Contributor Author

I'm going to force push after rebasing against master to pull in recent merged work.

@matthew-carroll matthew-carroll force-pushed the android_embedding_refactor_pr12_add_lifecycle_to_flutteractivity branch from 56ccd90 to 0eff43c Compare February 28, 2019 23:33
@matthew-carroll
Copy link
Contributor Author

@dnfield The functional TODOs have been resolved. FlutterFragment still has more behavior to come, but I think the open-ended stuff should now be filled.

Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@matthew-carroll matthew-carroll merged commit fe15149 into flutter:master Mar 1, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 1, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 1, 2019
cbracken added a commit to cbracken/flutter that referenced this pull request Mar 1, 2019
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)
cbracken added a commit to flutter/flutter that referenced this pull request Mar 1, 2019
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)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants