Conversation
cg021
force-pushed
the
jni-58288
branch
3 times, most recently
from
f458492 to
0af2713
Compare
| } | ||
| } | ||
|
|
||
| public void onDisplayPlatformView(int viewId, int x, int y, int width, int height) {} |
shell/platform/android/io/flutter/plugin/platform/PlatformViewsController.java
Outdated
Show resolved
Hide resolved
|
|
||
| @SuppressWarnings("unused") | ||
| @UiThread | ||
| private void onDisplayPlatformView(int viewId, int x, int y, int width, int height) { |
There was a problem hiding this comment.
move this method below // ----- End Engine Lifecycle Support ---- since onDisplayPlatformView isn't related to the engine lifecycle.
Co-authored-by: Emmanuel Garcia <[email protected]>
| @Nullable private Long nativePlatformViewId; | ||
| @Nullable private AccessibilityDelegate accessibilityDelegate; | ||
| @Nullable private PlatformMessageHandler platformMessageHandler; | ||
| @Nullable private PlatformViewsController platformViewsController; |
There was a problem hiding this comment.
we need to add a setter for this in FlutterJNI.
| } | ||
|
|
||
| @Override | ||
| public void onDisplayPlatformView(int viewId, int x, int y, int width, int height) { |
There was a problem hiding this comment.
Actually, we won't need this method here. Instead call flutterJni.setPlatformViewsController(platformViewsController) below flutterJNI.addEngineLifecycleListener(engineLifecycleListener);.
| /** Lifecycle callback invoked before a hot restart of the Flutter engine. */ | ||
| void onPreEngineRestart(); | ||
|
|
||
| void onDisplayPlatformView(int viewId, int x, int y, int width, int height); |
| flutterLoader.ensureInitializationComplete(context, dartVmArgs); | ||
|
|
||
| flutterJNI.addEngineLifecycleListener(engineLifecycleListener); | ||
| flutterJni.setPlatformViewsController(platformViewsController); |
There was a problem hiding this comment.
my bad, I should have caught that- should be fixed now
|
One last thing, let's add a test to https://github.com/flutter/engine/blob/077918dcfdd9a1c34d235f23f45d1c89514203ce/shell/platform/android/test/io/flutter/embedding/engine/FlutterJNITest.java. The test case can ensure that Later on, we can continue extending the cases as we progress through the implementation. |
shell/platform/android/io/flutter/embedding/engine/FlutterEngine.java
Outdated
Show resolved
Hide resolved
| assertEquals(1, callbackInvocationCount.get()); | ||
|
|
||
| // --- Execute Test --- | ||
| flutterJNI.onDisplayPlatformView(0, 0, 0, 0, 0); |
There was a problem hiding this comment.
hmm. I think this needs to go in a separate @Test. Something like:
@Test
public void onDisplayPlatformView__callsPlatformViewsController() {
FlutterJNI flutterJNI = new FlutterJNI();
flutterJNI.setPlatformViewsController(new PlatformViewsController() {
// Override onDisplayPlatformView and increase counter.
});
flutterJNI.onDisplayPlatformView(/*viewId=*/ 1, /*x=*/ 10, /*y=*/ 20, /*width=*/ 100, /*height=*/ 200);
// assert that PlatformViewsController#onDisplayPlatformView was called with the same arguments passed above.
}There was a problem hiding this comment.
Typically unit tests are testing a single "unit" of functionality. Here the "unit" is that PlatformViewsController#onDisplayPlatformView is called.
There was a problem hiding this comment.
Also, bonus point if we check that it's called with the right arguments. e.g. the id, x, y, width, height should match the ones you passed, so I would actually give some real values to the call:
flutterJNI.onDisplayPlatformView(/*viewId=*/ 1, /*x=*/ 10, /*y=*/ 20, /*width=*/ 100, /*height=*/ 200);Then, collect this values in the overridden method and assert that they are the same as the one you passed.
There was a problem hiding this comment.
You can use Mockito to achieve this. See:
| int counter = 0; | ||
|
|
||
| FlutterJNI flutterJNI = new FlutterJNI(); | ||
| flutterJNI.setPlatformViewsController(new PlatformViewsController() { |
There was a problem hiding this comment.
If you are using Mockito, then this can be simplified a bit. e.g.
import static org.mockito.Mockito.verify;@Test
public void onDisplayPlatformView__callsPlatformViewsController() {
PlatformViewsController platformViewsController = mock(PlatformViewsController.class);
FlutterJNI flutterJNI = new FlutterJNI();
flutterJNI.setPlatformViewsController(platformViewsController);
// --- Execute Test ---
flutterJNI.onDisplayPlatformView(/*viewId=*/ 1, /*x=*/ 10, /*y=*/ 20, /*width=*/ 100, /*height=*/ 200);
// --- Verify Results ---
verify(platformViewsController, times(1)).onDisplayPlatformView(/*viewId=*/ 1, /*x=*/ 10, /*y=*/ 20, /*width=*/ 100, /*height=*/ 200);
}
|
The test is missing imports. I'm going to revert this change as it will make the tree red. https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket.appspot.com/8878407328975964992/+/steps/Host_Tests_for_android_jit_release_x86/0/stdout?format=raw |
3 participants
Description
Add JNI method
onDisplayPlatformViewfor hybrid composition in the engine.Related Issues
flutter/flutter#58288
Tests
I added the following tests:
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.Breaking Change
Did any tests fail when you ran them? Please read handling breaking changes.