Use getRealMetrics for display metrics. Closes #4934

[jaysoo]

Fixes #4934.

Since API level 17, there has a Display.getRealMetrics method. This allows us to get the actual sizes of the screen (including soft menu bar and other system decor elements).

See: http://developer.android.com/reference/android/view/Display.html#getRealMetrics(android.util.DisplayMetrics)

I'm not sure if there is a good way to write unit or integration tests for this. Please let me know if there are any suggestions or concerns.

[facebook-github-bot]

By analyzing the blame information on this pull request, we identified @mkonicek, @astreet and @andreicoman11 to be potential reviewers.

[satya164]

Why not display.getRealMetrics?

[jaysoo]

In case the method is missing (API < 17). As far as I can tell Rn supports 15+?

[satya164]

@jaysoo Yes, but you're already checking Build.VERSION.SDK_INT >= 17

[facebook-github-bot]

@jaysoo updated the pull request.

[jaysoo]

@satya164 Updated the PR to not silently catch exceptions.

Reflection exceptions are logged using FLog, and the invocation exception is rethrown as runtime exception.

[satya164]

Why use Reflection if you're already checking Build.VERSION.SDK_INT >= 17?

[facebook-github-bot]

@jaysoo updated the pull request.

[facebook-github-bot]

@jaysoo updated the pull request.

[jaysoo]

@satya164 Removed reflection usage. I was worried about compiling it on API 16. Tried it using emulator, and it seems like there's no problem.

This also removes the try-catch.

[mkonicek]

If any of this fails (getSystemService returns null, the result is not of type WindowManager etc.) we probably don't want to crash the whole UIManagerModule. Can you wrap all of this in a try-catch block and log the exception using FLog (check other call sites of FLog for examples).

[mkonicek]

Looks legit, thanks a lot for the PR! Note that almost the whole team is on vacation until the end of the year so we'll be very slow on code review until the beginning of January.

[jaysoo]

@mkonicek Thanks for the feedback. I'll make the changes tonight. :)

[facebook-github-bot]

@jaysoo updated the pull request.

[jaysoo]

@mkonicek Updated the PR:

1. Used reactContext.getSystemService instead of reactContext.getApplicationContext().getSystemService.
2. Wrapped the entire block with try-catch, with a FLog.e for the caught exception.

[facebook-github-bot]

@jaysoo updated the pull request.

[facebook-github-bot]

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/884116028375399/int_phab to review.

[satya164]

ping @bestander :)

[bestander]

bumped @astreet, he controls the ship

[mkonicek]

Looks like internal integration tests are failing but can't tell if it's related. Open sourcing them will help.

[bestander]

on it now

[bestander]

Hi @jaysoo, I figured out what the problem was.
We have quite a few internal tests uimanager class and this mGetRawW.invoke(display); throws an error in tests environment.

First I tried to put a fix in your PR to make the dimensions mockable but failed to find a quick way to do it.
As a matter of fact we are working on open sourcing a big bunch of tests to make contribution experience super productive and pleasant.
Tomorrow I am going to push to master all the tests related to uimanager.
Could you rebase on master after that and see an easy way to mock the Display dimensions for tests?

[jaysoo]

@bestander Sure, I'd be happy to. :)

Is there anything I should keep in mind for the tests?

[bestander]

@jaysoo, nothing too special.
The tests are creating UIManagerModule and run in JVM environment with Android SDK emulated by robolectric.

Here is a sample I am importing:

package com.facebook.react.uimanager;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;

import android.graphics.Color;
import android.view.Choreographer;
import android.view.View;
import android.view.ViewGroup;
import android.widget.TextView;

import com.facebook.react.ReactRootView;
import com.facebook.react.animation.Animation;
import com.facebook.react.animation.AnimationPropertyUpdater;
import com.facebook.react.bridge.Callback;
import com.facebook.react.bridge.Arguments;
import com.facebook.react.bridge.ReactApplicationContext;
import com.facebook.react.bridge.CatalystInstance;
import com.facebook.react.bridge.SimpleArray;
import com.facebook.react.bridge.SimpleMap;
import com.facebook.react.views.text.ReactRawTextManager;
import com.facebook.react.views.text.ReactTextShadowNode;
import com.facebook.react.views.text.ReactTextViewManager;
import com.facebook.react.views.view.ReactViewGroup;
import com.facebook.react.views.view.ReactViewManager;
import com.facebook.react.bridge.ReactTestHelper;

import org.powermock.core.classloader.annotations.PrepareForTest;
import org.powermock.core.classloader.annotations.PowerMockIgnore;
import org.powermock.modules.junit4.rule.PowerMockRule;
import org.junit.Rule;
import org.robolectric.RobolectricTestRunner;

import org.robolectric.RuntimeEnvironment;

import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.invocation.InvocationOnMock;
import org.mockito.stubbing.Answer;
import org.powermock.api.mockito.PowerMockito;
import org.powermock.core.classloader.annotations.PrepareForTest;
import org.robolectric.Robolectric;

import static org.fest.assertions.api.Assertions.assertThat;
import static org.mockito.Matchers.any;
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

/**
 * Tests for {@link UIManagerModule}.
 */
@PrepareForTest({Arguments.class, ReactChoreographer.class})
@RunWith(RobolectricTestRunner.class)
@PowerMockIgnore({"org.mockito.*", "org.robolectric.*", "android.*"})
public class UIManagerModuleTest {

  @Rule
  public PowerMockRule rule = new PowerMockRule();

  private ReactApplicationContext mReactContext;
  private CatalystInstance mCatalystInstanceMock;
  private ArrayList<Choreographer.FrameCallback> mPendingChoreographerCallbacks;

  @Before
  public void setUp() {
    PowerMockito.mockStatic(Arguments.class, ReactChoreographer.class);

    ReactChoreographer choreographerMock = mock(ReactChoreographer.class);
    PowerMockito.when(Arguments.createArray()).thenAnswer(new Answer<Object>() {
      @Override
      public Object answer(InvocationOnMock invocation) throws Throwable {
        return new SimpleArray();
      }
    });
    PowerMockito.when(Arguments.createMap()).thenAnswer(new Answer<Object>() {
      @Override
      public Object answer(InvocationOnMock invocation) throws Throwable {
        return new SimpleMap();
      }
    });
    PowerMockito.when(ReactChoreographer.getInstance()).thenReturn(choreographerMock);

    mPendingChoreographerCallbacks = new ArrayList<>();
    doAnswer(new Answer() {
      @Override
      public Object answer(InvocationOnMock invocation) throws Throwable {
        mPendingChoreographerCallbacks
            .add((Choreographer.FrameCallback) invocation.getArguments()[1]);
        return null;
      }
    }).when(choreographerMock).postFrameCallback(
        any(ReactChoreographer.CallbackType.class),
        any(Choreographer.FrameCallback.class));

    mCatalystInstanceMock = ReactTestHelper.createMockCatalystInstance();
    mReactContext = new ReactApplicationContext(RuntimeEnvironment.application);
    mReactContext.initializeWithInstance(mCatalystInstanceMock);

    UIManagerModule uiManagerModuleMock = mock(UIManagerModule.class);
    when(mCatalystInstanceMock.getNativeModule(UIManagerModule.class))
        .thenReturn(uiManagerModuleMock);
  }

  @Test
  public void testCreateSimpleHierarchy() {
    UIManagerModule uiManager = getUIManagerModule();

    ViewGroup rootView = createSimpleTextHierarchy(uiManager, "Some text");

    assertThat(rootView.getChildCount()).isEqualTo(1);

    View firstChild = rootView.getChildAt(0);
    assertThat(firstChild).isInstanceOf(TextView.class);
    assertThat(((TextView) firstChild).getText().toString()).isEqualTo("Some text");
  }

private void executePendingChoreographerCallbacks() {
    ArrayList<Choreographer.FrameCallback> callbacks =
        new ArrayList<>(mPendingChoreographerCallbacks);
    mPendingChoreographerCallbacks.clear();
    for (Choreographer.FrameCallback frameCallback : callbacks) {
      frameCallback.doFrame(0);
    }
  }

  private UIManagerModule getUIManagerModule() {
    List<ViewManager> viewManagers = Arrays.<ViewManager>asList(
        new ReactViewManager(),
        new ReactTextViewManager(),
        new ReactRawTextManager());
    UIManagerModule uiManagerModule =  new UIManagerModule(
        mReactContext,
        viewManagers,
        new UIImplementation(mReactContext, viewManagers));
    uiManagerModule.onHostResume();
    return uiManagerModule;
  }
}

[bestander]

@jaysoo the tests are in master branch, could you rebase?
To run tests locally:
gradlew :ReactAndroid:testDebugUnitTest

CI will report the tests properly in about an hour when #5582 lands

[facebook-github-bot]

@jaysoo updated the pull request.

[jaysoo]

@bestander taking a quick look now. Sorry for delay.

[jaysoo]

@bestander Ran gradlew :ReactAndroid:testDebugUnitTest and it completed successfully. Anything else I should do or check?

[bestander]

Now it feels strange 0_o

[bestander]

I was quite sure that tests would fail in circleCI but they passed just fine.
Must be something in the internal FB environment that runs the tests...
Let me try merging it again and maybe do the necessary changes against the internal CI.

[bestander]

@facebook-github-bot shipit

[facebook-github-bot]

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/884116028375399/int_phab to review.

[bestander]

@jaysoo just a heads up, I found the issue with our internal test runners.
We are running some tests with robolectric 2 which caused the problem.
It will be easy for me to migrate the remaining tests to OSS and robolectric 3.
Sorry for the delay, I'll deal with the rest and get this PR merged early next week

[bestander]

[jaysoo]

@bestander You sir are awesome! Thanks for the help! And everyone else on this issue. 💯

[ide]

The real metrics probably should be exposed via Dimensions.get('screen') instead of Dimensions.get('window') (on iPhone these two would be equal). The common case for apps is that the software nav buttons on Android are ever-present and Dimensions.get('window') should account for them. Otherwise this is going to break a lot of cross-platform apps that use Dimensions.

[Larney11]

Solution
Check out this StackOverflow solution.