Repository navigation
[android] Fix unexpected behavior in |detachFromFlutterEngine|. - #30202
fluttergithubbot merged 6 commits into
Conversation
| if (delegate == null) { | ||
| Log.w(TAG, "FlutterActivity " + hashCode() + " " + event + " called after release."); | ||
| if (delegate == null || !delegate.isAttached()) { | ||
| Log.w(TAG, "FlutterActivity " + hashCode() + " " + event + " called after detach."); |
There was a problem hiding this comment.
delegate becomes null in release, so this is still true.
| public void updateSystemUiOverlays() { | ||
| if (delegate != null) { | ||
| if (delegate != null && delegate.isAttached()) { | ||
| delegate.updateSystemUiOverlays(); |
There was a problem hiding this comment.
updateSystemUiOverlays could check this internally. This way there's no need to expose isAttached in the public API of the delegate.
| + getFlutterEngine() | ||
| + " evicted by another attaching activity"); | ||
| release(); | ||
| delegate.onDestroyView(); |
There was a problem hiding this comment.
please check for delegate != null before calling methods.
There was a problem hiding this comment.
Also, since you are making this change, could release also perform that check?
| private boolean stillAttachedForEvent(String event) { | ||
| if (delegate == null) { | ||
| Log.w(TAG, "FlutterActivity " + hashCode() + " " + event + " called after release."); | ||
| if (delegate == null || !delegate.isAttached()) { |
There was a problem hiding this comment.
don't need to add delegate.isAttached() here. what issue is this addressing?
There was a problem hiding this comment.
I think it is needed. Here is to solve app lifecycle event crosstalk.
| private boolean stillAttachedForEvent(String event) { | ||
| if (delegate == null) { | ||
| Log.w(TAG, "FlutterFragment " + hashCode() + " " + event + " called after release."); | ||
| if (delegate == null || !delegate.isAttached()) { |
| Log.w( | ||
| TAG, "FlutterActivity " + hashCode() + " " + event + " called after release or detach."); |
There was a problem hiding this comment.
can this avoid (either/or)? If someone is looking at this log, it's always better to know exactly what is going on.
I'd suggest to break this log into two.
| + " connection to the engine " | ||
| + getFlutterEngine() | ||
| + " evicted by another attaching activity"); | ||
| release(); |
There was a problem hiding this comment.
what about adding a test to ensure that detachFromFlutterEngine() doesn't release the engine?
| flutterActivity.onPause(); | ||
| flutterActivity.detachFromFlutterEngine(); | ||
| assertNotNull(mockDelegate); | ||
| assertFalse(mockDelegate.isAttached()); |
There was a problem hiding this comment.
this works, but can we also add a test where mockDelegate is indeed null. This ensures that the test itself is working as one would expect.
blasten
added
the
waiting for tree to go green
3 participants
Fixes flutter/flutter#94802
Related issue: flutter/flutter#49950 (comment)
Pre-launch Checklist
writing and running engine tests.
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.