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

Call PlatformView.dispose when removing hybrid composition platform views #21790

Merged
merged 1 commit into from
Oct 14, 2020
Merged

Call PlatformView.dispose when removing hybrid composition platform views #21790

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 32 additions & 23 deletions shell/platform/android/io/flutter/plugin/platform/PlatformViewsController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import android.util.SparseArray;
import android.view.MotionEvent;
import android.view.View;
import android.view.ViewGroup;
import android.widget.FrameLayout;
import androidx.annotation.NonNull;
import androidx.annotation.Nullable;
Expand Down Expand Up @@ -83,7 +84,7 @@ public class PlatformViewsController implements PlatformViewsAccessibilityDelega
// The views returned by `PlatformView#getView()`.
//
// This only applies to hybrid composition.
private final SparseArray<View> platformViews;
private final SparseArray<PlatformView> platformViews;

// The platform view parents that are appended to `FlutterView`.
// If an entry in `platformViews` doesn't have an entry in this array, the platform view isn't
Expand Down Expand Up @@ -143,32 +144,24 @@ public void createAndroidViewForPlatformView(
}

final PlatformView platformView = factory.create(context, request.viewId, createParams);
final View view = platformView.getView();
if (view == null) {
throw new IllegalStateException(
"PlatformView#getView() returned null, but an Android view reference was expected.");
}
if (view.getParent() != null) {
throw new IllegalStateException(
"The Android view returned from PlatformView#getView() was already added to a parent view.");
}
platformViews.put(request.viewId, view);
Copy link

Choose a reason for hiding this comment

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

nit: what about moving the checks if (view == null) and if(view.getParent() != null) to initializePlatformViewIfNeeded ?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

platformViews.put(request.viewId, platformView);
}

@Override
public void disposeAndroidViewForPlatformView(int viewId) {
// Hybrid view.
final View platformView = platformViews.get(viewId);
final PlatformView platformView = platformViews.get(viewId);
final FlutterMutatorView parentView = platformViewParent.get(viewId);
if (platformView != null) {
if (parentView != null) {
parentView.removeView(platformView);
parentView.removeView(platformView.getView());
}
platformViews.remove(viewId);
platformView.dispose();
}

if (parentView != null) {
((FlutterView) flutterView).removeView(parentView);
Copy link

Choose a reason for hiding this comment

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

this should still work, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

flutterView may be null at this point, but calling getParent will always work

((ViewGroup) parentView.getParent()).removeView(parentView);
platformViewParent.remove(viewId);
}
}
Expand Down Expand Up @@ -311,8 +304,10 @@ public void onTouch(@NonNull PlatformViewsChannel.PlatformViewTouch touch) {
vdControllers.get(touch.viewId).dispatchTouchEvent(event);
} else if (platformViews.get(viewId) != null) {
final MotionEvent event = toMotionEvent(density, touch, /*usingVirtualDiplays=*/ false);
View view = platformViews.get(touch.viewId);
view.dispatchTouchEvent(event);
View view = platformViews.get(touch.viewId).getView();
Copy link

Choose a reason for hiding this comment

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

nit: should it check for null as well? Our API doesn't explicitly indicate that getView() is non-nullable.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

if (view != null) {
view.dispatchTouchEvent(event);
}
} else {
throw new IllegalStateException("Sending touch to an unknown view with id: " + viewId);
}
Expand Down Expand Up @@ -580,7 +575,7 @@ public void onPreEngineRestart() {
public View getPlatformViewById(Integer id) {
// Hybrid composition.
if (platformViews.get(id) != null) {
return platformViews.get(id);
return platformViews.get(id).getView();
}
VirtualDisplayController controller = vdControllers.get(id);
if (controller == null) {
Expand Down Expand Up @@ -690,6 +685,10 @@ private void flushAllViews() {
controller.dispose();
}
vdControllers.clear();

while (platformViews.size() > 0) {
channelHandler.disposeAndroidViewForPlatformView(platformViews.keyAt(0));
}
}

private void initializeRootImageViewIfNeeded() {
Expand All @@ -701,19 +700,27 @@ private void initializeRootImageViewIfNeeded() {

@VisibleForTesting
void initializePlatformViewIfNeeded(int viewId) {
final View view = platformViews.get(viewId);
if (view == null) {
final PlatformView platformView = platformViews.get(viewId);
if (platformView == null) {
throw new IllegalStateException(
"Platform view hasn't been initialized from the platform view channel.");
}
if (platformViewParent.get(viewId) != null) {
return;
}
if (platformView.getView() == null) {
throw new IllegalStateException(
"PlatformView#getView() returned null, but an Android view reference was expected.");
}
if (platformView.getView().getParent() != null) {
throw new IllegalStateException(
"The Android view returned from PlatformView#getView() was already added to a parent view.");
}
final FlutterMutatorView parentView =
new FlutterMutatorView(
context, context.getResources().getDisplayMetrics().density, androidTouchProcessor);
platformViewParent.put(viewId, parentView);
parentView.addView(view);
parentView.addView(platformView.getView());
((FlutterView) flutterView).addView(parentView);
}

Expand All @@ -740,9 +747,11 @@ public void onDisplayPlatformView(

final FrameLayout.LayoutParams layoutParams =
new FrameLayout.LayoutParams(viewWidth, viewHeight);
final View platformView = platformViews.get(viewId);
platformView.setLayoutParams(layoutParams);
platformView.bringToFront();
final View view = platformViews.get(viewId).getView();
if (view != null) {
view.setLayoutParams(layoutParams);
view.bringToFront();
}
currentFrameUsedPlatformViewIds.add(viewId);
}

Expand Down
45 changes: 12 additions & 33 deletions shell/platform/android/test/io/flutter/plugin/platform/PlatformViewsControllerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,7 @@
import io.flutter.embedding.engine.systemchannels.MouseCursorChannel;
import io.flutter.embedding.engine.systemchannels.SettingsChannel;
import io.flutter.embedding.engine.systemchannels.TextInputChannel;
import io.flutter.plugin.common.FlutterException;
import io.flutter.plugin.common.MethodCall;
import io.flutter.plugin.common.StandardMessageCodec;
import io.flutter.plugin.common.StandardMethodCodec;
import io.flutter.plugin.localization.LocalizationPlugin;
import java.nio.ByteBuffer;
Expand Down Expand Up @@ -262,7 +260,7 @@ public void createPlatformViewMessage__initializesAndroidView() {

// Simulate create call from the framework.
createPlatformView(jni, platformViewsController, platformViewId, "testType");
verify(platformView, times(1)).getView();
verify(viewFactory, times(1)).create(any(), eq(platformViewId), any());
}

@Test
Expand All @@ -286,21 +284,11 @@ public void createPlatformViewMessage__throwsIfViewIsNull() {
createPlatformView(jni, platformViewsController, platformViewId, "testType");
assertEquals(ShadowFlutterJNI.getResponses().size(), 1);

final ByteBuffer responseBuffer = ShadowFlutterJNI.getResponses().get(0);
responseBuffer.rewind();

StandardMethodCodec methodCodec = new StandardMethodCodec(new StandardMessageCodec());
try {
methodCodec.decodeEnvelope(responseBuffer);
} catch (FlutterException exception) {
assertTrue(
exception
.getMessage()
.contains(
"PlatformView#getView() returned null, but an Android view reference was expected."));
return;
}
assertFalse(true);
assertThrows(
IllegalStateException.class,
() -> {
platformViewsController.initializePlatformViewIfNeeded(platformViewId);
});
}

@Test
Expand All @@ -326,21 +314,11 @@ public void createPlatformViewMessage__throwsIfViewHasParent() {
createPlatformView(jni, platformViewsController, platformViewId, "testType");
assertEquals(ShadowFlutterJNI.getResponses().size(), 1);

final ByteBuffer responseBuffer = ShadowFlutterJNI.getResponses().get(0);
responseBuffer.rewind();

StandardMethodCodec methodCodec = new StandardMethodCodec(new StandardMessageCodec());
try {
methodCodec.decodeEnvelope(responseBuffer);
} catch (FlutterException exception) {
assertTrue(
exception
.getMessage()
.contains(
"The Android view returned from PlatformView#getView() was already added to a parent view."));
return;
}
assertFalse(true);
assertThrows(
IllegalStateException.class,
() -> {
platformViewsController.initializePlatformViewIfNeeded(platformViewId);
});
}

@Test
Expand Down Expand Up @@ -381,6 +359,7 @@ public void disposeAndroidView__hybridComposition() {

assertNotNull(androidView.getParent());
assertTrue(androidView.getParent() instanceof FlutterMutatorView);
verify(platformView, times(1)).dispose();
}

@Test
Expand Down