Make FlutterFragment usable without requiring it to be attached to an Android Activity. #27332

ajmalk · 2021-07-12T17:44:41Z

call getContext instead of getActivity in FlutterActivityAndFragmentDelegate.onCreateView

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Android Activity. - call getContext instead of getActivity in FlutterActivityAndFragmentDelegate.onCreateView

flutter-dashboard · 2021-07-12T17:44:44Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

blasten

could we add a test to verify that onCreateView doesn't call getActivity, but instead getContext?

ajmalk · 2021-07-12T21:38:57Z

could we add a test to verify that onCreateView doesn't call getActivity, but instead getContext?

Yes I was looking into this but I can't seem to get the tests to run locally. Are there any docs to help with that? Currently I'm getting:

There was 1 failure:
1) initializationError(org.junit.runner.JUnitCommandLineParseResult)
java.lang.IllegalArgumentException: Could not find class [FlutterActivityAndFragmentDelegateTest.java]
        at org.junit.runner.JUnitCommandLineParseResult.parseParameters(JUnitCommandLineParseResult.java:100)
        at org.junit.runner.JUnitCommandLineParseResult.parseArgs(JUnitCommandLineParseResult.java:50)
        at org.junit.runner.JUnitCommandLineParseResult.parse(JUnitCommandLineParseResult.java:44)
        at org.junit.runner.JUnitCore.runMain(JUnitCore.java:72)
        at org.junit.runner.JUnitCore.main(JUnitCore.java:36)
Caused by: java.lang.ClassNotFoundException: FlutterActivityAndFragmentDelegateTest.java
        at java.net.URLClassLoader.findClass(URLClassLoader.java:382)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:418)
        at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:355)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:351)
        at java.lang.Class.forName0(Native Method)
        at java.lang.Class.forName(Class.java:348)
        at org.junit.internal.Classes.getClass(Classes.java:42)
        at org.junit.internal.Classes.getClass(Classes.java:27)
        at org.junit.runner.JUnitCommandLineParseResult.parseParameters(JUnitCommandLineParseResult.java:98)
        ... 4 more

When I try to run testing/run_tests.py --type=java --java-filter=FlutterActivityAndFragmentDelegateTest.java

blasten · 2021-07-12T21:58:49Z

what about testing/run_tests.py --type=java --java-filter=io.flutter.embedding.android.FlutterActivityAndFragmentDelegateTest?

blasten · 2021-07-12T22:34:20Z

actually, I'm seeing the same issue. cc @dnfield Is this related to recent changes?

dnfield · 2021-07-12T22:59:26Z

Make sure you build the target(s) first - run_tests.py used to do some of that for you but doesn't anymore because it messed up things on LUCI.

/cc @zanderso fyi

ajmalk · 2021-07-12T23:04:35Z

Do you mean just run ninja -C out/android_debug_unopt? I did that and it built fine. Tried running it again and it says "no work to do".

dnfield · 2021-07-12T23:22:06Z

Make sure to also build the roboletric_tests target there - ninja -C out/android_debug_unopt robolectric_tests.

Since the test script doesn't build that target for you anymore, we should really add it to the default Android build rule, at least for the case of armv7 debug.

ajmalk · 2021-07-12T23:52:53Z

The build ran correctly (I think). But I'm still the same java.lang.IllegalArgumentException when I try to run the test.

dnfield · 2021-07-12T23:55:26Z

That error sounds like there's something missing in the way the test is set up or decorated.

ajmalk · 2021-07-13T02:57:52Z

It's not a test I added fwiw. Are you saying it's broken at head? Are unit test not run by your presubmit? What should I do?

dnfield · 2021-07-13T02:58:59Z

I am able to run these tests at head :/. They run on the Linux android debug shard

ajmalk · 2021-07-13T03:00:30Z

Oh hmm. I'm trying to run on mac. I suppose I could try and run it on linux. Do you think that will help?

dnfield · 2021-07-13T04:20:32Z

I ran it locally on mac.

ajmalk · 2021-07-13T17:07:25Z

Hmm. I think some sort of instructions for setup would be helpful.

dnfield · 2021-07-13T17:14:27Z

This works for me from the src directory:

git -C flutter fetch upstream
git -C flutter merge upstream/master
gclient sync
./flutter/tools/gn --android --unopt --runtime-mode=debug
ninja -C out/android_debug_unopt
./flutter/testing/run_tests.py --type-java

dnfield · 2021-07-13T17:15:42Z

The only thing I can think of - this is all using JDK 1.8. For better or worse we use your system JDK and do not vend a JDK with the engine repo. I think JDK 1.9 should work, but I'm willing to bet JDK 1.10/11/12 will not.

dnfield · 2021-07-13T17:16:03Z

So for me, javac -version shows javac 1.8.0_221

ajmalk · 2021-07-13T19:15:13Z

ok I changed the java version and that finally worked! Thanks. I'll send over the unit tests soon.

ajmalk · 2021-07-13T19:20:26Z

For reference I ran this from the src dir:

./flutter/tools/gn --android --unopt --runtime-mode=debug
ninja -C out/android_debug_unopt
ninja -C out/android_debug_unopt robolectric_tests
./flutter/testing/run_tests.py --type=java --java-filter=io.flutter.embedding.android.FlutterActivityAndFragmentDelegateTest

ajmalk · 2021-07-13T19:21:29Z

And I had to install Java 8 by downloading it from the oracle website and manually editing my PATH.

…ity.

ajmalk · 2021-07-13T23:11:56Z

Ok. I made sure there's test coverage. I opted to modify an existing test instead of adding a new one since that seemed appropriate in this case. Should I add/modify docs somewhere so people know FlutterFragment can be used without an Activity? Or is too niche a use case?

dnfield

LGTM.

If you want to update docs that's fine, but AFAIK this use case was always intended to be supported and this code just slipped through the cracks.

dnfield · 2021-07-13T23:29:23Z

Waiting to apply the label on review from @blasten

blasten

LGTM

…ed to an Android Activity. (#27332)" This reverts commit 317166d.

…ed to an Android Activity. (#27332)" (#27382) This reverts commit 317166d.

…ed to an Android Activity. (flutter/engine#27332)

…e attached to an Android Activity. (flutter#27332)" (flutter#27382)" This reverts commit 093c4ed.

… Android Activity. (flutter#27332)

…ed to an Android Activity. (flutter#27332)" (flutter#27382) This reverts commit 317166d.

… Android Activity. (flutter#27332)

…ed to an Android Activity. (flutter#27332)" (flutter#27382) This reverts commit 317166d.

Make FlutterFragment usable without requiring it to be attached to an

a200c80

Android Activity. - call getContext instead of getActivity in FlutterActivityAndFragmentDelegate.onCreateView

flutter-dashboard bot added the platform-android label Jul 12, 2021

flutter-dashboard bot added the needs tests label Jul 12, 2021

google-cla bot added the cla: yes label Jul 12, 2021

blasten reviewed Jul 12, 2021

View reviewed changes

dnfield mentioned this pull request Jul 12, 2021

Build the robolectric tests by default for debug armv7 #27345

Merged

dnfield mentioned this pull request Jul 13, 2021

Vend the JDK with the engine flutter/flutter#86377

Closed

Make sure tests fully cover the case where there is no attached activ…

ca4bbbc

…ity.

dnfield approved these changes Jul 13, 2021

View reviewed changes

blasten approved these changes Jul 13, 2021

View reviewed changes

blasten added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jul 13, 2021

fluttergithubbot merged commit 317166d into flutter:master Jul 14, 2021

zanderso added a commit that referenced this pull request Jul 14, 2021

Revert "Make FlutterFragment usable without requiring it to be attach…

8a563b0

…ed to an Android Activity. (#27332)" This reverts commit 317166d.

zanderso mentioned this pull request Jul 14, 2021

Revert "Make FlutterFragment usable without requiring it to be attached to an Android Activity." #27382

Merged

zanderso added a commit that referenced this pull request Jul 14, 2021

Revert "Make FlutterFragment usable without requiring it to be attach…

093c4ed

…ed to an Android Activity. (#27332)" (#27382) This reverts commit 317166d.

engine-flutter-autoroll mentioned this pull request Jul 14, 2021

Roll Engine from bde2f528888b to 093c4ed1a3d7 (3 revisions) flutter/flutter#86406

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 14, 2021

317166d Make FlutterFragment usable without requiring it to be attach…

8570b21

…ed to an Android Activity. (flutter/engine#27332)

dnfield added a commit to dnfield/engine that referenced this pull request Jul 14, 2021

Revert "Revert "Make FlutterFragment usable without requiring it to b…

afaeefc

…e attached to an Android Activity. (flutter#27332)" (flutter#27382)" This reverts commit 093c4ed.

ajmalk mentioned this pull request Jul 14, 2021

Make FlutterFragment usable without requiring it to be attached to an Android Activity. (Attempt 2) #27397

Merged

8 tasks

moffatman pushed a commit to moffatman/engine that referenced this pull request Aug 5, 2021

Make FlutterFragment usable without requiring it to be attached to an…

4f88093

… Android Activity. (flutter#27332)

moffatman pushed a commit to moffatman/engine that referenced this pull request Aug 5, 2021

Revert "Make FlutterFragment usable without requiring it to be attach…

1ffe168

…ed to an Android Activity. (flutter#27332)" (flutter#27382) This reverts commit 317166d.

naudzghebre pushed a commit to naudzghebre/engine that referenced this pull request Sep 2, 2021

Make FlutterFragment usable without requiring it to be attached to an…

f4fd712

… Android Activity. (flutter#27332)

naudzghebre pushed a commit to naudzghebre/engine that referenced this pull request Sep 2, 2021

Revert "Make FlutterFragment usable without requiring it to be attach…

71d0ac0

…ed to an Android Activity. (flutter#27332)" (flutter#27382) This reverts commit 317166d.

Make FlutterFragment usable without requiring it to be attached to an Android Activity. #27332

Make FlutterFragment usable without requiring it to be attached to an Android Activity. #27332

Uh oh!

Conversation

ajmalk commented Jul 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Jul 12, 2021

Uh oh!

blasten left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajmalk commented Jul 12, 2021

Uh oh!

blasten commented Jul 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blasten commented Jul 12, 2021

Uh oh!

dnfield commented Jul 12, 2021

Uh oh!

ajmalk commented Jul 12, 2021

Uh oh!

dnfield commented Jul 12, 2021

Uh oh!

ajmalk commented Jul 12, 2021

Uh oh!

dnfield commented Jul 12, 2021

Uh oh!

ajmalk commented Jul 13, 2021

Uh oh!

dnfield commented Jul 13, 2021

Uh oh!

ajmalk commented Jul 13, 2021

Uh oh!

dnfield commented Jul 13, 2021

Uh oh!

ajmalk commented Jul 13, 2021

Uh oh!

dnfield commented Jul 13, 2021

Uh oh!

dnfield commented Jul 13, 2021

Uh oh!

dnfield commented Jul 13, 2021

Uh oh!

ajmalk commented Jul 13, 2021

Uh oh!

ajmalk commented Jul 13, 2021

Uh oh!

ajmalk commented Jul 13, 2021

Uh oh!

ajmalk commented Jul 13, 2021

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

dnfield commented Jul 13, 2021

Uh oh!

blasten left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ajmalk commented Jul 12, 2021 •

edited

Loading

blasten left a comment •

edited

Loading

blasten commented Jul 12, 2021 •

edited

Loading