Add registration calls for ComponentCallbacks2 #34206
Conversation
|
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 (don't just cc him here, he won't see it! He's on Discord!). 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. |
|
cc @camsim99 |
| if (delegate == null) { | ||
| delegate = new FlutterActivityAndFragmentDelegate(this); | ||
| } |
There was a problem hiding this comment.
This seems strange - maybe we're constructing the test incorrectly?
I'm under the impression that onAttach should never get called twice before onDetach is called in a real case.
There was a problem hiding this comment.
Specifically here, I am passing in a mock delegate that is being blindly overwritten here. The delegate needs to be mocked as it requires a ton of setup to be able to successfully call delegate.onAttach(). This null check allows us to mock it.
There was a problem hiding this comment.
Can we instead have a delegate factory this class uses, perhaps only visible for testing?
There was a problem hiding this comment.
Changed to use a factory.
| new FlutterEngine(spyCtx, new FlutterLoader(), flutterJNI, null, false); | ||
| FlutterEngineCache.getInstance().put("my_cached_engine", flutterEngine); | ||
|
|
||
| FlutterFragment fragment = FlutterFragment.withCachedEngine("my_cached_engine").build(); |
There was a problem hiding this comment.
e.g. here -
FlutterFragment.withCachedEngine("my_cached_engine").withDelegateFactory(someObjectThatAlwaysReturnsTheMockDelegate).build();
| } | ||
|
|
||
| // We track the attached context so we can unregister component callbacks when detaching. | ||
| private Context attachedContext; |
There was a problem hiding this comment.
I suspect you don't need to do this. I think just getContext is fine, and if the context changes because the fragment gets moved to a different activity or something, onAttach should be called (and get registered to another callback)
|
(My comments should not block landing this, but if they can be addressed it might make future work in this area less confusing.) |
| @VisibleForTesting | ||
| /* package */ void setDelegate(@NonNull FlutterActivityAndFragmentDelegate delegate) { | ||
| this.delegate = delegate; | ||
| /* package */ void setDelegateFactory( |
There was a problem hiding this comment.
Remove todo and close issue if not relevant anymore
There was a problem hiding this comment.
This issue still seems to apply.
There was a problem hiding this comment.
The issue itself is to remove the method you're removing here. We still need some way to test this class and this seems like an appropriate way to have some DI.
GaryQian
added
waiting for tree to go green
|
I just thought about this. We can probably remove this call through now too right? https://cs.opensource.google/flutter/engine/+/master:shell/platform/android/io/flutter/embedding/android/FlutterFragmentActivity.java;l=560;drc=7acec05e90011a6ea61c81bbda2c8f182aaa88e9. Otherwise, it'll call twice when using FlutterFragmentActivity? |
|
Yes |
4 participants
Adds registration calls onAttach and onDetach to address flutter/flutter#105346