Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

BREAKING: Android: Correct value of Dimensions.get('screen').fontScale #11008

Closed
wants to merge 8 commits into from
Closed

BREAKING: Android: Correct value of Dimensions.get('screen').fontScale #11008

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f27182f
Android: Add UIManager.getContentSizeMultiplier
Nov 18, 2016
2f896a7
Revert "Android: Add UIManager.getContentSizeMultiplier"
Feb 7, 2017
0f5ea42
Correctly export fontScale to JS
Feb 7, 2017
340f7ca
Begin fixing buck errors
Feb 8, 2017
3ce2b47
Don't set mFontScale in emitUpdateDimensionsEvent
Feb 9, 2017
e8fc329
Share construction of the dimensions object
Feb 9, 2017
55a9467
Add 'change' event for subscribing to Dimensions updates with a cross…
Feb 13, 2017
f5c2fff
Tweak formatting
Feb 13, 2017
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
45 changes: 45 additions & 0 deletions Libraries/Utilities/Dimensions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,15 @@
*/
'use strict';

var EventEmitter = require('EventEmitter');
var Platform = require('Platform');
var UIManager = require('UIManager');
var RCTDeviceEventEmitter = require('RCTDeviceEventEmitter');

var invariant = require('fbjs/lib/invariant');

var eventEmitter = new EventEmitter();
var dimensionsInitialized = false;
var dimensions = {};
class Dimensions {
/**
Expand Down Expand Up @@ -60,6 +63,15 @@ class Dimensions {
}

Object.assign(dimensions, dims);
if (dimensionsInitialized) {
// Don't fire 'change' the first time the dimensions are set.
eventEmitter.emit('change', {
window: dimensions.window,
screen: dimensions.screen
});
} else {
dimensionsInitialized = true;
}
}

/**
Expand All @@ -81,6 +93,39 @@ class Dimensions {
invariant(dimensions[dim], 'No dimension set for key ' + dim);
return dimensions[dim];
}

/**
* Add an event handler. Supported events:
*
* - `change`: Fires when a property within the `Dimensions` object changes. The argument
* to the event handler is an object with `window` and `screen` properties whose values
* are the same as the return values of `Dimensions.get('window')` and
* `Dimensions.get('screen')`, respectively.
*/
static addEventListener(
type: string,
handler: Function
) {
invariant(
'change' === type,
'Trying to subscribe to unknown event: "%s"', type
);
eventEmitter.addListener(type, handler);
}

/**
* Remove an event handler.
*/
static removeEventListener(
type: string,
handler: Function
) {
invariant(
'change' === type,
'Trying to remove listener for unknown event: "%s"', type
);
eventEmitter.removeListener(type, handler);
}
}

Dimensions.set(UIManager.Dimensions);
Expand Down
25 changes: 4 additions & 21 deletions ReactAndroid/src/main/java/com/facebook/react/ReactRootView.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,27 +394,10 @@ private void emitOrientationChanged(final int newRotation) {
}

private void emitUpdateDimensionsEvent() {
DisplayMetrics windowDisplayMetrics = DisplayMetricsHolder.getWindowDisplayMetrics();
DisplayMetrics screenDisplayMetrics = DisplayMetricsHolder.getScreenDisplayMetrics();

WritableMap windowDisplayMetricsMap = Arguments.createMap();
windowDisplayMetricsMap.putInt("width", windowDisplayMetrics.widthPixels);
windowDisplayMetricsMap.putInt("height", windowDisplayMetrics.heightPixels);
windowDisplayMetricsMap.putDouble("scale", windowDisplayMetrics.density);
windowDisplayMetricsMap.putDouble("fontScale", windowDisplayMetrics.scaledDensity);
windowDisplayMetricsMap.putDouble("densityDpi", windowDisplayMetrics.densityDpi);

WritableMap screenDisplayMetricsMap = Arguments.createMap();
screenDisplayMetricsMap.putInt("width", screenDisplayMetrics.widthPixels);
screenDisplayMetricsMap.putInt("height", screenDisplayMetrics.heightPixels);
screenDisplayMetricsMap.putDouble("scale", screenDisplayMetrics.density);
screenDisplayMetricsMap.putDouble("fontScale", screenDisplayMetrics.scaledDensity);
screenDisplayMetricsMap.putDouble("densityDpi", screenDisplayMetrics.densityDpi);

WritableMap dimensionsMap = Arguments.createMap();
dimensionsMap.putMap("windowPhysicalPixels", windowDisplayMetricsMap);
dimensionsMap.putMap("screenPhysicalPixels", screenDisplayMetricsMap);
sendEvent("didUpdateDimensions", dimensionsMap);
mReactInstanceManager
.getCurrentReactContext()
.getNativeModule(UIManagerModule.class)
.emitUpdateDimensionsEvent();
}

private void sendEvent(String eventName, @Nullable WritableMap params) {
Expand Down
28 changes: 25 additions & 3 deletions ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,10 @@
import com.facebook.react.bridge.ReactMethod;
import com.facebook.react.bridge.ReadableArray;
import com.facebook.react.bridge.ReadableMap;
import com.facebook.react.bridge.WritableMap;
import com.facebook.react.common.ReactConstants;
import com.facebook.react.module.annotations.ReactModule;
import com.facebook.react.modules.core.DeviceEventManagerModule;
import com.facebook.react.uimanager.debug.NotThreadSafeViewHierarchyUpdateDebugListener;
import com.facebook.react.uimanager.events.EventDispatcher;
import com.facebook.systrace.Systrace;
Expand Down Expand Up @@ -88,6 +90,7 @@ public class UIManagerModule extends ReactContextBaseJavaModule implements
private final Map<String, Object> mModuleConstants;
private final UIImplementation mUIImplementation;
private final MemoryTrimCallback mMemoryTrimCallback = new MemoryTrimCallback();
private float mFontScale;

private int mNextRootViewTag = 1;
private int mBatchId = 0;
Expand All @@ -100,7 +103,8 @@ public UIManagerModule(
super(reactContext);
DisplayMetricsHolder.initDisplayMetricsIfNotInitialized(reactContext);
mEventDispatcher = new EventDispatcher(reactContext);
mModuleConstants = createConstants(viewManagerList, lazyViewManagersEnabled);
mFontScale = getReactApplicationContext().getResources().getConfiguration().fontScale;
mModuleConstants = createConstants(viewManagerList, lazyViewManagersEnabled, mFontScale);
mUIImplementation = uiImplementationProvider
.createUIImplementation(reactContext, viewManagerList, mEventDispatcher);

Expand Down Expand Up @@ -133,6 +137,12 @@ public void initialize() {
@Override
public void onHostResume() {
mUIImplementation.onHostResume();

float fontScale = getReactApplicationContext().getResources().getConfiguration().fontScale;
if (mFontScale != fontScale) {
mFontScale = fontScale;
emitUpdateDimensionsEvent();
}
}

@Override
Expand All @@ -156,13 +166,15 @@ public void onCatalystInstanceDestroy() {

private static Map<String, Object> createConstants(
List<ViewManager> viewManagerList,
boolean lazyViewManagersEnabled) {
boolean lazyViewManagersEnabled,
float fontScale) {
ReactMarker.logMarker(CREATE_UI_MANAGER_MODULE_CONSTANTS_START);
Systrace.beginSection(Systrace.TRACE_TAG_REACT_JAVA_BRIDGE, "CreateUIManagerConstants");
try {
return UIManagerModuleConstantsHelper.createConstants(
viewManagerList,
lazyViewManagersEnabled);
lazyViewManagersEnabled,
fontScale);
} finally {
Systrace.endSection(Systrace.TRACE_TAG_REACT_JAVA_BRIDGE);
ReactMarker.logMarker(CREATE_UI_MANAGER_MODULE_CONSTANTS_END);
Expand Down Expand Up @@ -541,6 +553,16 @@ public void sendAccessibilityEvent(int tag, int eventType) {
mUIImplementation.sendAccessibilityEvent(tag, eventType);
}

public void emitUpdateDimensionsEvent() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I know you're not adding this, but seems like we should be sharing code with where we create this map when exposing the initial constants.

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 fixed this now. One interesting detail is that the dimensions part of constants is now a WritableMap instead of a HashMap. I wasn't sure what the impact of this would be. It built and ran fine.

sendEvent("didUpdateDimensions", UIManagerModuleConstants.getDimensionsConstants(mFontScale));
}

private void sendEvent(String eventName, @Nullable WritableMap params) {
getReactApplicationContext()
.getJSModule(DeviceEventManagerModule.RCTDeviceEventEmitter.class)
.emit(eventName, params);
}

/**
* Schedule a block to be executed on the UI thread. Useful if you need to execute
* view logic after all currently queued view updates have completed.
Expand Down
57 changes: 29 additions & 28 deletions ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerModuleConstants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
import android.view.accessibility.AccessibilityEvent;
import android.widget.ImageView;

import com.facebook.react.bridge.Arguments;
import com.facebook.react.bridge.WritableMap;
import com.facebook.react.common.MapBuilder;
import com.facebook.react.uimanager.events.TouchEventType;

Expand Down Expand Up @@ -81,7 +83,7 @@
.build();
}

public static Map<String, Object> getConstants() {
public static Map<String, Object> getConstants(float fontScale) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't we decide we can get this off of displayMetrics as scaledDensity / density? At the least, I'd rather just pass in the Configuration object if you're worried about loss of precision

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this earlier comment you mentioned that we should get the font scale from getResources().getConfiguration().fontScale rather than calculating it from scaledDensity / density.

The reason I pass fontScale around rather than the Configuration object is that I wanted it to be very obvious that we are passing mFontScale to JavaScript. I was concerned that there might be some race where font scale is extracted from the Configuration object but it has a different value than mFontScale.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, this sounds reasonable

HashMap<String, Object> constants = new HashMap<String, Object>();
constants.put(
"UIView",
Expand All @@ -95,35 +97,9 @@ public static Map<String, Object> getConstants() {
"ScaleAspectCenter",
ImageView.ScaleType.CENTER_INSIDE.ordinal())));

DisplayMetrics displayMetrics = DisplayMetricsHolder.getWindowDisplayMetrics();
DisplayMetrics screenDisplayMetrics = DisplayMetricsHolder.getScreenDisplayMetrics();
constants.put(
"Dimensions",
MapBuilder.of(
"windowPhysicalPixels",
MapBuilder.of(
"width",
displayMetrics.widthPixels,
"height",
displayMetrics.heightPixels,
"scale",
displayMetrics.density,
"fontScale",
displayMetrics.scaledDensity,
"densityDpi",
displayMetrics.densityDpi),
"screenPhysicalPixels",
MapBuilder.of(
"width",
screenDisplayMetrics.widthPixels,
"height",
screenDisplayMetrics.heightPixels,
"scale",
screenDisplayMetrics.density,
"fontScale",
screenDisplayMetrics.scaledDensity,
"densityDpi",
screenDisplayMetrics.densityDpi)));
getDimensionsConstants(fontScale));

constants.put(
"StyleConstants",
Expand Down Expand Up @@ -157,4 +133,29 @@ public static Map<String, Object> getConstants() {

return constants;
}

public static WritableMap getDimensionsConstants(float fontScale) {
DisplayMetrics windowDisplayMetrics = DisplayMetricsHolder.getWindowDisplayMetrics();
DisplayMetrics screenDisplayMetrics = DisplayMetricsHolder.getScreenDisplayMetrics();

WritableMap windowDisplayMetricsMap = Arguments.createMap();
windowDisplayMetricsMap.putInt("width", windowDisplayMetrics.widthPixels);
windowDisplayMetricsMap.putInt("height", windowDisplayMetrics.heightPixels);
windowDisplayMetricsMap.putDouble("scale", windowDisplayMetrics.density);
windowDisplayMetricsMap.putDouble("fontScale", fontScale);
windowDisplayMetricsMap.putDouble("densityDpi", windowDisplayMetrics.densityDpi);

WritableMap screenDisplayMetricsMap = Arguments.createMap();
screenDisplayMetricsMap.putInt("width", screenDisplayMetrics.widthPixels);
screenDisplayMetricsMap.putInt("height", screenDisplayMetrics.heightPixels);
screenDisplayMetricsMap.putDouble("scale", screenDisplayMetrics.density);
screenDisplayMetricsMap.putDouble("fontScale", fontScale);
screenDisplayMetricsMap.putDouble("densityDpi", screenDisplayMetrics.densityDpi);

WritableMap dimensionsMap = Arguments.createMap();
dimensionsMap.putMap("windowPhysicalPixels", windowDisplayMetricsMap);
dimensionsMap.putMap("screenPhysicalPixels", screenDisplayMetricsMap);

return dimensionsMap;
}
}
6 changes: 4 additions & 2 deletions ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerModuleConstantsHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import java.util.List;
import java.util.Map;

import com.facebook.react.bridge.ReactApplicationContext;
import com.facebook.react.common.MapBuilder;
import com.facebook.systrace.Systrace;
import com.facebook.systrace.SystraceMessage;
Expand Down Expand Up @@ -42,8 +43,9 @@
*/
/* package */ static Map<String, Object> createConstants(
List<ViewManager> viewManagers,
boolean lazyViewManagersEnabled) {
Map<String, Object> constants = UIManagerModuleConstants.getConstants();
boolean lazyViewManagersEnabled,
float fontScale) {
Map<String, Object> constants = UIManagerModuleConstants.getConstants(fontScale);
Map bubblingEventTypesConstants = UIManagerModuleConstants.getBubblingEventTypeConstants();
Map directEventTypesConstants = UIManagerModuleConstants.getDirectEventTypeConstants();

Expand Down