Update layout props without crossing JS bridge #45

osdnk · 2018-08-01T10:05:37Z

Motivation

The problem occurs if we wish to update props which are not supported natively to be modifier via Aniamted.Value. It refers to props like top, width etc.

Changes

Initially wish to add handling for greater number of properties in RN core which came out to be quite difficult and I gave up with this idea.
The another approach I decided to try was to use flow of updating properties from JS, but call them from native code.
Before these changes the flow was:

Changing property not supported natively e.g. width:

Animated Value is being changes as result of animation

The bias of passing bridge seems to be extremely useless and therefore I managed to omit it.

now it looks more or less like:

…nto update-jsprops-without-bridge

kmagiera · 2018-08-02T11:33:43Z

src/ConfigHelper.js

+};
+
+function configureJSPropsHandledNatively() {
+  ReanimatedModule.configureJSPropsHandledNatively(


Lets move this to a common method that would take both lists at once

kmagiera · 2018-08-02T11:36:38Z

src/ConfigHelper.js

@@ -35,3 +35,74 @@ export function addWhitelistedNativeProps(props) {
 }

 configureNativeProps();
+
+const JS_PROPS_HANDLED_NATIVELY_WHITELIST = {


Add comment:

Whitelist of view props that can be updated in native thread via UIManagerModule

Change NATIVE_PROPS_WHITELIST to UI_THREAD_PROPS_WHITELIST and also change comment to tell that the props are updated directly on UI thread

Change JS_PROPS_HANDLED_ to NATIVE_THREAD_PROPS_WHITELIST

kmagiera · 2018-08-02T12:25:55Z

android/src/main/java/com/swmansion/reanimated/NodesManager.java

@@ -68,13 +69,15 @@
  public double currentFrameTimeMs;
  public final UpdateContext updateContext;
  public Set<String> nativeProps = Collections.emptySet();
+  public Set<String> jsPropsHandledNatively = Collections.emptySet();


Let's use uiProps for what used to be nativeProps and nativeProps for jsPropsHandledNatively

kmagiera · 2018-08-02T12:29:00Z

android/src/main/java/com/swmansion/reanimated/NodesManager.java

+              new GuardedRunnable(mContext) {
+                @Override
+                public void runGuarded() {
+                  mUIManager.onBatchComplete();


I think we shouldn't just call onBatchComplete like that. This may intercept batched calls being made at the same time from JS. Would be the best to detect if we are in the middle of a batch and if so we can skip calling onBatchComplete and rely on getting it dispatched from JS thread. Also May be worth enqueuing all the UIManager operations here such that we can call it from a single runnable without risking some other operations being enqueued in the meantime

kmagiera · 2018-08-06T09:30:16Z

android/src/main/java/com/swmansion/reanimated/NodesManager.java

+                @Override
+                public void runGuarded() {
+                  while (!mOperationsInBatch.isEmpty()) {
+                    NativeUpdateOperation op = mOperationsInBatch.remove();


sync this quuee

kmagiera · 2018-08-06T09:30:23Z

android/src/main/java/com/swmansion/reanimated/NodesManager.java

@@ -284,6 +318,10 @@ public void disconnectNodeFromView(int nodeID, int viewTag) {
    ((PropsNode) node).disconnectFromView(viewTag);
  }

+  public void setUpdateView(int viewTag, WritableMap nativeProps) {


kmagiera · 2018-08-06T12:41:57Z

android/src/main/java/com/swmansion/reanimated/NodesManager.java

@@ -137,6 +155,30 @@ private void onAnimationFrame(long frameTimeNanos) {
      Node.runUpdates(updateContext);
    }

+    if (!mOperationsInBatch.isEmpty()) {
+      final Queue<NativeUpdateOperation> copiedProps =  new LinkedList<>(mOperationsInBatch);


rename copiedProps

kmagiera · 2018-08-06T12:43:06Z

android/src/main/java/com/swmansion/reanimated/nodes/PropsNode.java

-        evt.putMap("props", jsProps);
-        mNodesManager.sendEvent("onReanimatedPropsChange", evt);
+      if (hasNativeProps) {
+        mNodesManager.updateView(mConnectedViewTag, nativeProps);


enqueueUpdateViewOnNativeThread

kmagiera · 2018-08-06T12:46:47Z

android/src/main/java/com/facebook/react/uimanager/UIManagerUtils.java

@@ -0,0 +1,8 @@
+package com.facebook.react.uimanager;
+
+// This class has been made in order to export package method of UIImplementation


Use javadoc format to document this class

kmagiera · 2018-08-06T12:47:01Z

ios/REAModule.m

@@ -107,13 +107,15 @@ - (void)setBridge:(RCTBridge *)bridge
  }];
 }

-RCT_EXPORT_METHOD(configureNativeProps:(nonnull NSArray<NSString *> *)nativeProps)
+RCT_EXPORT_METHOD(configureProps:(nonnull NSArray<NSString *> *)nativeProps
+                  nativeProps:(nonnull NSArray<NSString *> *)uiProps)


uiProps:(nonnull NSArray<NSString *> *)uiProps)

kmagiera · 2018-08-06T12:50:44Z

android/src/main/java/com/swmansion/reanimated/nodes/PropsNode.java

    boolean hasNativeProps = false;
-    boolean hasJSProps = false;


Lets keep the code responsible for updating props in JS. I believe there are still some props that we haven't whitelisted, we don't want to restrict usages that are curerntly possible

kmagiera · 2018-08-06T12:57:29Z

ios/REANodesManager.m

+    RCTExecuteOnUIManagerQueue(^{
+      BOOL shouldFinishBatch = ((NSHashTable<RCTShadowView *> *)[self.uiManager valueForKey:@"_shadowViewsWithUpdatedChildren"]).count == 0;
+      for (int i = 0; i < copiedNumberOfOperationsInBatch; i++) {
+        [self.uiManager updateView:copiedProps[i].reactTag viewName:copiedProps[i].viewName props:copiedProps[i].nativeProps];


copiedProps[i](self.uiManager);

kmagiera · 2018-08-06T12:58:20Z

ios/REANodesManager.m

+- (void)updateView:(nonnull NSNumber *)reactTag
+          viewName:(NSString *) viewName
+       nativeProps:(NSMutableDictionary *)nativeProps {
+  [_operationsInBatch addObject:[[REANativeUpdateOperation alloc] initWithReactTag:reactTag viewName:viewName nativeProps:nativeProps]];


[_operationsInBatch addObject:^(RCTUIManager *uiManager) { [uiManager updateView:reactTag viewName:viewName props:nativeProps]; }

kmagiera · 2018-08-06T13:03:15Z

ios/REANodesManager.m

+        [self.uiManager updateView:copiedProps[i].reactTag viewName:copiedProps[i].viewName props:copiedProps[i].nativeProps];
+      }
+      if (shouldFinishBatch) {
+        [self.uiManager batchDidComplete];


Maybe we can use https://github.com/facebook/react-native/blob/310a2850c55fef194d54fcb7ea4649ed0e800deb/React/Modules/RCTUIManager.m#L1132

kmagiera · 2018-08-06T14:23:53Z

android/src/main/java/com/swmansion/reanimated/NodesManager.java

@@ -137,6 +155,30 @@ private void onAnimationFrame(long frameTimeNanos) {
      Node.runUpdates(updateContext);
    }

+    if (!mOperationsInBatch.isEmpty()) {
+      final Queue<NativeUpdateOperation> copiedOperationsQueue =  new LinkedList<>(mOperationsInBatch);


final Queue<NativeUpdateOperation> copiedOperationsQueue = mOperationsInBatch; mOpertionsInBatch = new LinkedList<>();

(cherry picked from commit 255bb74)

osdnk · 2018-08-07T08:08:11Z

android/src/main/java/com/swmansion/reanimated/NodesManager.java

+                    }
+                  }
+                  if (shouldDispatchUpdates) {
+                    mUIImplementation.dispatchViewUpdates(-1); // no associated batchId


@kmagiera,
decided not to use onBatchCompleted because it sometimes makes unexpected behaviour.
While changing screen via navigator it sometimes "blink" with old layout (v easy to repro)
Indeed I figured out that batchId should not have any impact because it only matters in systrace.
https://github.com/facebook/react-native/search?q=batchId&unscoped_q=batchId&type=Code
But it makes this error 🤷‍♂️

kmagiera · 2018-08-14T08:47:00Z

ios/REANodesManager.m

 #import "Nodes/REAAlwaysNode.h"

+@interface RCTUIManager ()


add comment

This commit facebook/react-native@2558c3d added string to `transform` type and it broke Reanimated types.  ## Summary fixes [#45](#4548)

This commit facebook/react-native@2558c3d added string to `transform` type and it broke Reanimated types.  ## Summary fixes [software-mansion#45](software-mansion#4548)

osdnk added 3 commits August 1, 2018 00:53

Add handling od iOS and Android

1c71ea6

Fix style

7cc279d

Delete proder.sh

f138da9

osdnk changed the title ~~Add updating jsPriops withour using bridre~~ Add updating jsProps without using bridge Aug 1, 2018

osdnk added 13 commits August 1, 2018 13:09

Fix example

1505a0f

staph

35c588e

example

4922696

example

533605c

Improve iOS

efb63c0

Optimize android

f96bb16

Add whitelist ⚪ 📎 📊

d28778c

Update REAPropsNode.m

d77f5e6

Fix example

8601eb1

Merge remote-tracking branch 'origin/update-jsprops-without-bridge' i…

fdc0115

…nto update-jsprops-without-bridge

Merge branch 'master' into update-jsprops-without-bridge

d5b9d31

Update App.js

e3028cc

Update ConfigHelper.js

c6b0337

osdnk mentioned this pull request Aug 2, 2018

Can I animate the opacity of a shadow? #44

Closed

kmagiera changed the title ~~Add updating jsProps without using bridge~~ Update layout props without crossing JS bridge Aug 2, 2018

kmagiera requested changes Aug 2, 2018

View reviewed changes

osdnk added 2 commits August 3, 2018 20:36

Works

86f5eb0

Update REANodesManager.h

1d40510

kmagiera reviewed Aug 6, 2018

View reviewed changes

osdnk added 5 commits August 6, 2018 13:58

Add files

220058e

Mor pr issues

19f32d7

Merge branch 'master' into update-jsprops-without-bridge

fbc8df4

Merge branch 'master' into update-jsprops-without-bridge

72d8a98

Update Android things

255bb74

kmagiera requested changes Aug 6, 2018

View reviewed changes

kmagiera reviewed Aug 6, 2018

View reviewed changes

osdnk added 14 commits August 7, 2018 09:02

Pr stuff

24a5338

Update UIManagerReanimatedHelper.java

fcd454b

revert CAC

2319688

Update Android things

705b32a

(cherry picked from commit 255bb74)

Update REAPropsNode.m

584e2c6

Update ConfigHelper.js

db8cf93

Update NodesManager.java

53e2589

js props

d5e5a45

Update REANodesManager.m

7dee03c

simplify

08c54f4

dead

a5f9b3b

style

5adb8ba

style

bf380fd

style

e3fc789

osdnk commented Aug 7, 2018

View reviewed changes

osdnk and others added 2 commits August 14, 2018 09:42

Update Animated.js

ce8c2c2

Update UIManagerReanimatedHelper.java

e01e4e4

kmagiera reviewed Aug 14, 2018

View reviewed changes

ios/REANodesManager.m Outdated

#import "Nodes/REAAlwaysNode.h"

@interface RCTUIManager ()

Copy link

Member

kmagiera Aug 14, 2018

Choose a reason for hiding this comment

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

add comment

kmagiera approved these changes Aug 14, 2018

View reviewed changes

Update REANodesManager.m

0a5bec1

osdnk merged commit d54f70e into master Aug 14, 2018

osdnk deleted the update-jsprops-without-bridge branch December 19, 2019 16:31

hannojg mentioned this pull request Aug 2, 2021

SIGABRT #2098

Closed

3 tasks

gaurav-vij mentioned this pull request Sep 15, 2023

App Crashing intermittently with 3.5.1 #5091

Closed

Abhishek-Khanduri mentioned this pull request Apr 22, 2024

[android] app crash -> JNI DETECTED ERROR IN APPLICATION: java_object == null #5923

Closed

Rexogamer mentioned this pull request Aug 13, 2024

bug: crashing on Android debug build with RN 0.75 #6415

Open

tara-singh-danu mentioned this pull request Sep 15, 2024

Android crash on launch, Fatal signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0x30 in tid 24002 (mqt_js), pid 23922 #6509

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update layout props without crossing JS bridge #45

Update layout props without crossing JS bridge #45

osdnk commented Aug 1, 2018 •

edited

Loading

kmagiera Aug 2, 2018

kmagiera Aug 2, 2018

kmagiera Aug 2, 2018

kmagiera Aug 2, 2018

kmagiera Aug 6, 2018

kmagiera Aug 6, 2018

kmagiera Aug 6, 2018

kmagiera Aug 6, 2018

kmagiera Aug 6, 2018

kmagiera Aug 6, 2018

kmagiera Aug 6, 2018

kmagiera Aug 6, 2018

kmagiera Aug 6, 2018

kmagiera Aug 6, 2018

kmagiera Aug 6, 2018

osdnk Aug 7, 2018

kmagiera Aug 14, 2018

		@@ -0,0 +1,8 @@
		package com.facebook.react.uimanager;

		// This class has been made in order to export package method of UIImplementation

Update layout props without crossing JS bridge #45

Update layout props without crossing JS bridge #45

Conversation

osdnk commented Aug 1, 2018 • edited Loading

Motivation

Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

osdnk commented Aug 1, 2018 •

edited

Loading