Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion DEPS
Original file line number Diff line number Diff line change
Expand Up @@ -483,7 +483,7 @@ deps = {
'packages': [
{
'package': 'flutter/android/robolectric_bundle',
'version': 'last_updated:2019-08-02T16:01:27-0700'
'version': 'last_updated:2019-09-09T16:47:38-0700'
}
],
'condition': 'download_android_deps',
Expand Down
1 change: 1 addition & 0 deletions shell/platform/android/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -416,6 +416,7 @@ action("robolectric_tests") {
"test/io/flutter/embedding/engine/RenderingComponentTest.java",
"test/io/flutter/embedding/engine/renderer/FlutterRendererTest.java",
"test/io/flutter/embedding/engine/systemchannels/PlatformChannelTest.java",
"test/io/flutter/plugin/platform/SingleViewPresentationTest.java",
"test/io/flutter/util/PreconditionsTest.java",
]

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,24 @@
import android.os.Build;
import android.os.Bundle;
import android.support.annotation.Keep;
import android.support.annotation.NonNull;
import android.support.annotation.Nullable;
import android.util.Log;
import android.view.*;
import android.view.Display;
import android.view.Gravity;
import android.view.View;
import android.view.ViewGroup;
import android.view.WindowManager;
import android.view.accessibility.AccessibilityEvent;
import android.view.inputmethod.InputMethodManager;
import android.widget.FrameLayout;

import java.lang.reflect.*;
import java.lang.reflect.InvocationHandler;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.lang.reflect.Proxy;

import static android.content.Context.INPUT_METHOD_SERVICE;
import static android.content.Context.WINDOW_SERVICE;
import static android.view.View.OnFocusChangeListener;

Expand Down Expand Up @@ -99,7 +110,7 @@ public SingleViewPresentation(
Object createParams,
OnFocusChangeListener focusChangeListener
) {
super(outerContext, display);
super(new ImmContext(outerContext), display);
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens to SingleViewPresentation when its associated FlutterEngine is detached from the UI? Does it get destroyed? If so, should we be explicitly cleaning up this ImmContext in any way? Or even more generally, is SingleViewPresentation currently implemented in a way that respects FlutterEngine being detached from the UI?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how this object behaves in an add to app situation. It looks like SingleViewPresentation is instantiated through VirtualDisplayController#create, when createPlatformView is called via the Flutter widget. I'm not familiar enough with the entire flow to know how that would behave if FlutterEngine was attached and detached. @amirh, WDYT?

As far as I can tell mContext itself should be consistent for the SVP, though.

this.viewFactory = viewFactory;
this.accessibilityEventsDelegate = accessibilityEventsDelegate;
this.viewId = viewId;
Expand Down Expand Up @@ -128,7 +139,7 @@ public SingleViewPresentation(
OnFocusChangeListener focusChangeListener,
boolean startFocused
) {
super(outerContext, display);
super(new ImmContext(outerContext), display);
this.accessibilityEventsDelegate = accessibilityEventsDelegate;
viewFactory = null;
this.state = state;
Expand All @@ -154,7 +165,10 @@ protected void onCreate(Bundle savedInstanceState) {
}

container = new FrameLayout(getContext());
PresentationContext context = new PresentationContext(getContext(), state.windowManagerHandler);

// Our base mContext has already been wrapped with an IMM cache at instantiation time, but
// we want to wrap it again here to also return state.windowManagerHandler.
Context context = new PresentationContext(getContext(), state.windowManagerHandler);

if (state.platformView == null) {
state.platformView = viewFactory.create(context, viewId, createParams);
Expand Down Expand Up @@ -235,14 +249,51 @@ private static int atMost(int measureSpec) {
}
}

/**
* Proxies a Context replacing the WindowManager with our custom instance.
*/
static class PresentationContext extends ContextWrapper {
private WindowManager windowManager;
private final WindowManagerHandler windowManagerHandler;
/** Answers calls for {@link InputMethodManager} with an instance cached at creation time. */
// TODO(mklim): This caches the IMM at construction time and won't pick up any changes. In rare
// cases where the FlutterView changes windows this will return an outdated instance. This
// should be fixed to instead defer returning the IMM to something that know's FlutterView's
// true Context.
private static class ImmContext extends ContextWrapper {
private @NonNull
final InputMethodManager inputMethodManager;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it safe to cache this here? is it possible that the IMM for FlutterView is changed after we cached it? e.g FlutterView was moved to a different window (I actually won't be surprised if it's common for code that was written prior to Q to assume the IMM doesn't change, so not sure if there's any fail safe that mitigates it already).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, this would break if FlutterView moved to a different window after we cached it. FlutterView would have a new IMM then and this would still be getting the one from the previous window.

This is tricky. By definition we can't do anything around detecting what window we're "really" in and if it changes here since the whole point is to return the IMM from the "wrong" window. We also couldn't give the responsibility to FlutterView/etc to update this, for many reasons.

The only thing I'm thinking of right now is that we pipe a method reference all the way from FlutterView to here and delegate returning the IMM from that instead of caching anything. I'm not sure if that's worth it though, WDYT? I don't know if changing FlutterView's window at runtime is really supported as-is.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we could extend presentation without calling it's constructor (which uses createPresentationContext).

Copy link
Contributor

Choose a reason for hiding this comment

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

For the new embedding the plan is to have the PlatformViewsController aware of the current FlutterView (@matthew-carroll is doing this I believe). When that is done it should be easy to use the current FlutterView's IMM. As this thing should land soon let's leave a TODO here for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, added a TODO for now.


ImmContext(Context base) {
this(base, /*inputMethodManager=*/null);
}

private ImmContext(Context base, @Nullable InputMethodManager inputMethodManager) {
super(base);
this.inputMethodManager = inputMethodManager != null ? inputMethodManager : (InputMethodManager) base.getSystemService(INPUT_METHOD_SERVICE);
}

@Override
public Object getSystemService(String name) {
if (INPUT_METHOD_SERVICE.equals(name)) {
return inputMethodManager;
}
return super.getSystemService(name);
}

@Override
public Context createDisplayContext(Display display) {
Context displayContext = super.createDisplayContext(display);
return new ImmContext(displayContext, inputMethodManager);
}
}

PresentationContext(Context base, WindowManagerHandler windowManagerHandler) {
/** Proxies a Context replacing the WindowManager with our custom instance. */
// TODO(mklim): This caches the IMM at construction time and won't pick up any changes. In rare
// cases where the FlutterView changes windows this will return an outdated instance. This
// should be fixed to instead defer returning the IMM to something that know's FlutterView's
// true Context.
private static class PresentationContext extends ContextWrapper {
private @NonNull
final WindowManagerHandler windowManagerHandler;
private @Nullable
WindowManager windowManager;

PresentationContext(Context base, @NonNull WindowManagerHandler windowManagerHandler) {
super(base);
this.windowManagerHandler = windowManagerHandler;
}
Expand Down
4 changes: 2 additions & 2 deletions shell/platform/android/test/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -64,13 +64,13 @@ Once you've uploaded the new version, also make sure to tag it with the updated
timestamp and robolectric version (most likely still 3.8, unless you've migrated
all the packages to 4+).

$ cipd set-tag flutter/android/robolectric --version=<new_version_hash> -tag=last_updated:<timestamp>
$ cipd set-tag flutter/android/robolectric_bundle --version=<new_version_hash> -tag=last_updated:<timestamp>

Example of a last-updated timestamp: 2019-07-29T15:27:42-0700

You can generate the same date format with `date +%Y-%m-%dT%T%z`.

$ cipd set-tag flutter/android/robolectric --version=<new_version_hash> -tag=robolectric_version:<robolectric_version>
$ cipd set-tag flutter/android/robolectric_bundle --version=<new_version_hash> -tag=robolectric_version:<robolectric_version>

You can run `cipd describe flutter/android/robolectric_bundle
--version=<new_version_hash>` to verify. You should see:
Expand Down
21 changes: 11 additions & 10 deletions shell/platform/android/test/io/flutter/FlutterTestSuite.java
Original file line number Diff line number Diff line change
Expand Up @@ -8,28 +8,29 @@
import org.junit.runners.Suite;
import org.junit.runners.Suite.SuiteClasses;

import io.flutter.embedding.android.FlutterActivityAndFragmentDelegateTest;
import io.flutter.embedding.android.FlutterActivityTest;
import io.flutter.embedding.android.FlutterFragmentTest;
import io.flutter.embedding.engine.FlutterEngineCacheTest;
import io.flutter.embedding.engine.systemchannels.PlatformChannelTest;
import io.flutter.embedding.engine.FlutterJNITest;
import io.flutter.embedding.engine.RenderingComponentTest;
import io.flutter.embedding.engine.renderer.FlutterRendererTest;
import io.flutter.embedding.engine.systemchannels.PlatformChannelTest;
import io.flutter.plugin.platform.SingleViewPresentationTest;
import io.flutter.util.PreconditionsTest;
import io.flutter.embedding.engine.FlutterJNITest;

@RunWith(Suite.class)
@SuiteClasses({
PreconditionsTest.class,
SmokeTest.class,
FlutterActivityTest.class,
FlutterFragmentTest.class,
// FlutterActivityAndFragmentDelegateTest.class, TODO(mklim): Fix and re-enable this
FlutterActivityTest.class,
FlutterEngineCacheTest.class,
FlutterFragmentTest.class,
FlutterJNITest.class,
RenderingComponentTest.class,
FlutterRendererTest.class,
PlatformChannelTest.class
PlatformChannelTest.class,
PreconditionsTest.class,
RenderingComponentTest.class,
SingleViewPresentationTest.class,
SmokeTest.class,
})
/** Runs all of the unit tests listed in the {@code @SuiteClasses} annotation. */
public class FlutterTestSuite {}
public class FlutterTestSuite { }
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
package io.flutter.plugin.platform;

import android.annotation.TargetApi;
import android.content.Context;
import android.hardware.display.DisplayManager;
import android.view.Display;
import android.view.inputmethod.InputMethodManager;

import org.junit.Test;
import org.junit.runner.RunWith;
import org.robolectric.RobolectricTestRunner;
import org.robolectric.RuntimeEnvironment;
import org.robolectric.annotation.Config;
import org.robolectric.shadows.ShadowDisplay;
import org.robolectric.shadows.ShadowDisplayManager;
import org.robolectric.shadows.ShadowInputMethodManager;

import static junit.framework.Assert.assertEquals;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.when;

@Config(manifest = Config.NONE, shadows = {ShadowInputMethodManager.class, ShadowDisplayManager.class, ShadowDisplay.class}, sdk = 27)
@RunWith(RobolectricTestRunner.class)
@TargetApi(27)
public class SingleViewPresentationTest {
@Test
public void returnsOuterContextInputMethodManager() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we please name all unit tests in a similar fashion to the others in the embedding, it[DoesSomething[WhenSomethingHappens]]? I'd prefer that all unit test names are framed with respect to the object under test so that we do not confuse unit tests with component, UI, and end-to-end tests. I'm working on a doc to formalize some testing practices in the embedding which I will share for comment soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm fine with that naming convention, but I think if we want to enforce any kind of subjective stylistic decisions at that level of granularity it should be a larger style guide discussion we have and commit to first, then point PR authors to the existing style guide. This particular test is internally consistent, which I think is a reasonable bar for something this minor. I'd like to avoid situations where PRs are reviewed based on how reviewers would personally have written code vs if the code is a net improvement to the code health. Basically, "Don’t block CLs from being submitted based only on personal style preferences.".

In general I'm wary of requiring something as specific and subjective as a specific unit test name style in our code requirements, beyond nitpicking that adjacent tests preferably be consistent. There is no overwhelming benefit or right answer to any particular naming convention, and enforcing any one would leads to PRs being temporarily blocked for no measurable benefit. It's also the kind of requirement we can't really expect PR authors to lint for or predict ahead of time. When the tests are run the names aren't even printed to the console unless they're failing.

Happy to discuss further in a doc. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. I do think there is a meaningful role for this particular naming convention, but we can discuss further in the doc.

// There's a bug in Android Q caused by the IMM being instanced per display.
// https://github.com/flutter/flutter/issues/38375. We need the context returned by
// SingleViewPresentation to be consistent from its instantiation instead of defaulting to
// what the system would have returned at call time.

// It's not possible to set up the exact same conditions as the unit test in the bug here,
// but we can make sure that we're wrapping the Context passed in at instantiation time and
// returning the same InputMethodManager from it. This test passes in a Spy context instance
// that initially returns a mock. Without the bugfix this test falls back to Robolectric's
// system service instead of the spy's and fails.

// Create an SVP under test with a Context that returns a local IMM mock.
Context context = spy(RuntimeEnvironment.application);
InputMethodManager expected = mock(InputMethodManager.class);
when(context.getSystemService(Context.INPUT_METHOD_SERVICE)).thenReturn(expected);
DisplayManager dm = (DisplayManager) context.getSystemService(Context.DISPLAY_SERVICE);
SingleViewPresentation svp = new SingleViewPresentation(context, dm.getDisplay(0), null, null, null, false);

// Get the IMM from the SVP's context.
InputMethodManager actual = (InputMethodManager) svp.getContext().getSystemService(Context.INPUT_METHOD_SERVICE);

// This should be the mocked instance from construction, not the IMM from the greater
// Android OS (or Robolectric's shadow, in this case).
assertEquals(expected, actual);
}

@Test
public void returnsOuterContextInputMethodManager_createDisplayContext() {
// The IMM should also persist across display contexts created from the base context.

// Create an SVP under test with a Context that returns a local IMM mock.
Context context = spy(RuntimeEnvironment.application);
InputMethodManager expected = mock(InputMethodManager.class);
when(context.getSystemService(Context.INPUT_METHOD_SERVICE)).thenReturn(expected);
Display display = ((DisplayManager) context.getSystemService(Context.DISPLAY_SERVICE)).getDisplay(0);
SingleViewPresentation svp = new SingleViewPresentation(context, display, null, null, null, false);

// Get the IMM from the SVP's context.
InputMethodManager actual = (InputMethodManager) svp.getContext().createDisplayContext(display).getSystemService(Context.INPUT_METHOD_SERVICE);

// This should be the mocked instance from construction, not the IMM from the greater
// Android OS (or Robolectric's shadow, in this case).
assertEquals(expected, actual);
}
}