Use `getRealMetrics` for display metrics. Closes #4934 #4935

jaysoo · 2015-12-23T02:30:40Z

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 · 2015-12-23T02:31:33Z

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

satya164 · 2015-12-23T13:11:06Z

ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerModule.java

Why not display.getRealMetrics?

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

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

facebook-github-bot · 2015-12-23T14:36:28Z

@jaysoo updated the pull request.

jaysoo · 2015-12-23T14:45:52Z

@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 · 2015-12-23T15:17:15Z

ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerModule.java

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

facebook-github-bot · 2015-12-23T19:34:37Z

@jaysoo updated the pull request.

facebook-github-bot · 2015-12-23T20:26:11Z

@jaysoo updated the pull request.

jaysoo · 2015-12-23T20:27:19Z

@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 · 2015-12-24T00:16:55Z

ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerModule.java

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 · 2015-12-24T00:22:04Z

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 · 2015-12-24T00:38:09Z

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

facebook-github-bot · 2015-12-24T01:35:27Z

@jaysoo updated the pull request.

jaysoo · 2015-12-24T01:40:04Z

@mkonicek Updated the PR:

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

facebook-github-bot · 2015-12-24T01:40:18Z

@jaysoo updated the pull request.

facebook-github-bot · 2016-01-07T11:00:35Z

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 · 2016-01-22T21:40:37Z

ping @bestander :)

bestander · 2016-01-25T12:59:59Z

bumped @astreet, he controls the ship

mkonicek · 2016-01-26T16:00:58Z

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

bestander · 2016-01-26T17:02:55Z

on it now

bestander · 2016-01-26T20:09:49Z

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 · 2016-01-26T22:45:15Z

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

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

bestander · 2016-01-27T10:39:40Z

@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 · 2016-01-27T15:59:43Z

@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 · 2016-01-28T18:34:15Z

@jaysoo updated the pull request.

jaysoo · 2016-01-28T18:35:04Z

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

jaysoo · 2016-01-28T18:44:18Z

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

bestander · 2016-01-28T18:55:04Z

Now it feels strange 0_o

bestander · 2016-01-28T18:58:43Z

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 · 2016-01-28T18:58:51Z

@facebook-github-bot shipit

facebook-github-bot · 2016-01-28T19:00:16Z

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 · 2016-01-29T22:07:58Z

@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 · 2016-02-01T17:15:35Z

jaysoo · 2016-02-01T17:18:54Z

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

ide · 2016-02-04T19:44:57Z

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.

Summary: public #4935 changed the window dimensions for android by replacing them with the actual screen dimensions. This changes the window dimensions back to their original values and adds `Dimensions.get('screen')` for the actual screen dimensions of the device. Reviewed By: astreet Differential Revision: D2921584 fb-gh-sync-id: 5d2677029c71d50691691dc651a11e9c8b115e8f shipit-source-id: 5d2677029c71d50691691dc651a11e9c8b115e8f

Summary: public facebook#4935 changed the window dimensions for android by replacing them with the actual screen dimensions. This changes the window dimensions back to their original values and adds `Dimensions.get('screen')` for the actual screen dimensions of the device. Reviewed By: astreet Differential Revision: D2921584 fb-gh-sync-id: 5d2677029c71d50691691dc651a11e9c8b115e8f shipit-source-id: 5d2677029c71d50691691dc651a11e9c8b115e8f

Larney11 · 2016-08-06T12:15:39Z

Solution
Check out this StackOverflow solution.

facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Dec 23, 2015

jaysoo closed this Dec 23, 2015

jaysoo reopened this Dec 23, 2015

satya164 reviewed Dec 23, 2015
View reviewed changes

jaysoo force-pushed the 4934-android-window-height branch from af5906c to 0c65ffa Compare December 23, 2015 14:35

satya164 reviewed Dec 23, 2015
View reviewed changes

jaysoo force-pushed the 4934-android-window-height branch from 0c65ffa to f86a428 Compare December 23, 2015 19:33

jaysoo force-pushed the 4934-android-window-height branch from f86a428 to 897c930 Compare December 23, 2015 20:25

mkonicek reviewed Dec 24, 2015
View reviewed changes

mkonicek added the GH Review: needs-revision label Dec 24, 2015

jaysoo force-pushed the 4934-android-window-height branch from 897c930 to 4e62be9 Compare December 24, 2015 01:34

jaysoo force-pushed the 4934-android-window-height branch from 4e62be9 to f664a4c Compare December 24, 2015 01:39

satya164 added Android and removed GH Review: needs-revision labels Dec 24, 2015

satya164 changed the title ~~Use getRealMetrics for display metrics~~ Use getRealMetrics for display metrics. Closes #4934 Dec 24, 2015

astreet added GH Review: accepted and removed GH Review: review-needed labels Jan 7, 2016

uses getRealMetrics on display to get real height metrics

e09f29e

jaysoo force-pushed the 4934-android-window-height branch from 50fd080 to e09f29e Compare January 28, 2016 18:33

ghost closed this in 8e60394 Feb 1, 2016

This pull request was closed.

Uh oh!

Use getRealMetrics for display metrics. Closes #4934 #4935

Use getRealMetrics for display metrics. Closes #4934 #4935

Uh oh!

Conversation

jaysoo commented Dec 23, 2015

Uh oh!

facebook-github-bot commented Dec 23, 2015

Uh oh!

satya164 Dec 23, 2015

Choose a reason for hiding this comment

Uh oh!

jaysoo Dec 23, 2015

Choose a reason for hiding this comment

Uh oh!

satya164 Dec 23, 2015

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Dec 23, 2015

Uh oh!

jaysoo commented Dec 23, 2015

Uh oh!

satya164 Dec 23, 2015

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Dec 23, 2015

Uh oh!

facebook-github-bot commented Dec 23, 2015

Uh oh!

jaysoo commented Dec 23, 2015

Uh oh!

mkonicek Dec 24, 2015

Choose a reason for hiding this comment

Uh oh!

mkonicek commented Dec 24, 2015

Uh oh!

jaysoo commented Dec 24, 2015

Uh oh!

facebook-github-bot commented Dec 24, 2015

Uh oh!

jaysoo commented Dec 24, 2015

Uh oh!

facebook-github-bot commented Dec 24, 2015

Uh oh!

facebook-github-bot commented Jan 7, 2016

Uh oh!

satya164 commented Jan 22, 2016

Uh oh!

bestander commented Jan 25, 2016

Uh oh!

mkonicek commented Jan 26, 2016

Uh oh!

bestander commented Jan 26, 2016

Uh oh!

bestander commented Jan 26, 2016

Uh oh!

jaysoo commented Jan 26, 2016

Uh oh!

bestander commented Jan 27, 2016

Uh oh!

bestander commented Jan 27, 2016

Uh oh!

facebook-github-bot commented Jan 28, 2016

Uh oh!

jaysoo commented Jan 28, 2016

Uh oh!

jaysoo commented Jan 28, 2016

Uh oh!

bestander commented Jan 28, 2016

Uh oh!

bestander commented Jan 28, 2016

Uh oh!

bestander commented Jan 28, 2016

Uh oh!

facebook-github-bot commented Jan 28, 2016

Uh oh!

bestander commented Jan 29, 2016

Uh oh!

bestander commented Feb 1, 2016

Uh oh!

Use `getRealMetrics` for display metrics. Closes #4934 #4935

Use `getRealMetrics` for display metrics. Closes #4934 #4935