From aff5a75d8ee1e1ffe12406c1c0d741832455a6a3 Mon Sep 17 00:00:00 2001 From: David Vacca Date: Sun, 1 Apr 2018 18:27:04 -0700 Subject: [PATCH] Refactor cloning of YogaNode Reviewed By: priteshrnandgaonkar Differential Revision: D7339832 fbshipit-source-id: 2de6f47ae7601ac083d3b9fbe10ffaf6307ae760 --- .../react/uimanager/ReactShadowNodeImpl.java | 25 +++++---- .../java/com/facebook/yoga/YogaConfig.java | 15 +++--- .../main/java/com/facebook/yoga/YogaNode.java | 14 +++++ ...nction.java => YogaNodeCloneFunction.java} | 4 +- .../jni/first-party/yogajni/jni/YGJNI.cpp | 54 +++++++++++-------- .../view/yoga/YogaLayoutableShadowNode.cpp | 2 +- ReactCommon/yoga/yoga/YGNode.cpp | 13 +++-- ReactCommon/yoga/yoga/Yoga-internal.h | 2 +- ReactCommon/yoga/yoga/Yoga.cpp | 16 +++--- ReactCommon/yoga/yoga/Yoga.h | 7 ++- 10 files changed, 94 insertions(+), 58 deletions(-) rename ReactAndroid/src/main/java/com/facebook/yoga/{YogaNodeClonedFunction.java => YogaNodeCloneFunction.java} (69%) diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/ReactShadowNodeImpl.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/ReactShadowNodeImpl.java index e6b4a90122..ebf8e6ae63 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/ReactShadowNodeImpl.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/ReactShadowNodeImpl.java @@ -10,6 +10,7 @@ import static java.lang.System.arraycopy; import com.facebook.infer.annotation.Assertions; import com.facebook.react.uimanager.annotations.ReactPropertyHolder; +import com.facebook.yoga.YogaNodeCloneFunction; import com.facebook.yoga.YogaAlign; import com.facebook.yoga.YogaBaselineFunction; import com.facebook.yoga.YogaConfig; @@ -21,7 +22,6 @@ import com.facebook.yoga.YogaFlexDirection; import com.facebook.yoga.YogaJustify; import com.facebook.yoga.YogaMeasureFunction; import com.facebook.yoga.YogaNode; -import com.facebook.yoga.YogaNodeClonedFunction; import com.facebook.yoga.YogaOverflow; import com.facebook.yoga.YogaPositionType; import com.facebook.yoga.YogaValue; @@ -61,17 +61,19 @@ public class ReactShadowNodeImpl implements ReactShadowNode private static final YogaConfig sYogaConfig; static { sYogaConfig = ReactYogaConfigProvider.get(); - sYogaConfig.setOnNodeCloned(new YogaNodeClonedFunction() { + sYogaConfig.setOnCloneNode(new YogaNodeCloneFunction() { @Override - public void onNodeCloned(YogaNode oldYogaNode, - YogaNode newYogaNode, + public YogaNode cloneNode(YogaNode oldYogaNode, YogaNode parent, int childIndex) { - ReactShadowNode parentReactShadowNode = (ReactShadowNode) parent.getData(); + ReactShadowNodeImpl parentReactShadowNode = (ReactShadowNodeImpl) parent.getData(); Assertions.assertNotNull(parentReactShadowNode); - - ReactShadowNode newReactShadowNode = (ReactShadowNode) newYogaNode.getData(); + ReactShadowNodeImpl newReactShadowNode = (ReactShadowNodeImpl) oldYogaNode.getData(); Assertions.assertNotNull(newReactShadowNode); + + ReactShadowNodeImpl newNode = newReactShadowNode.mutableCopy(); + parentReactShadowNode.replaceChild(newNode, childIndex); + return newNode.mYogaNode; } }); } @@ -133,6 +135,11 @@ public class ReactShadowNodeImpl implements ReactShadowNode mOriginalReactShadowNode = original; } + private void replaceChild(ReactShadowNodeImpl newNode, int childIndex) { + mChildren.remove(childIndex); + mChildren.add(childIndex, newNode); + } + /** * @return a copy of this object (no including copy of its children or the underlying yogaNode). */ @@ -145,7 +152,7 @@ public class ReactShadowNodeImpl implements ReactShadowNode ReactShadowNodeImpl copy = copy(); copy.mYogaNode = mYogaNode; // TODO: T26729293 clone YogaNode instead of reusing the same instance - //mYogaNode = original.mYogaNode.clone(); + //copy.mYogaNode = mYogaNode.clone(); copy.mNativeChildren = mNativeChildren == null ? null : new ArrayList<>(mNativeChildren); copy.mTotalNativeChildren = mTotalNativeChildren; copy.mChildren = mChildren == null ? null : new ArrayList<>(mChildren); @@ -165,7 +172,7 @@ public class ReactShadowNodeImpl implements ReactShadowNode ReactShadowNodeImpl copy = copy(); copy.mYogaNode = mYogaNode; // TODO: T26729293 clone YogaNode instead of reusing the same instance - //mYogaNode = original.mYogaNode.cloneWithNewChildren(); + //copy.mYogaNode = mYogaNode.clone(); copy.mNativeChildren = null; copy.mChildren = null; copy.mTotalNativeChildren = 0; diff --git a/ReactAndroid/src/main/java/com/facebook/yoga/YogaConfig.java b/ReactAndroid/src/main/java/com/facebook/yoga/YogaConfig.java index 2d90eec9de..ebb64aaff5 100644 --- a/ReactAndroid/src/main/java/com/facebook/yoga/YogaConfig.java +++ b/ReactAndroid/src/main/java/com/facebook/yoga/YogaConfig.java @@ -25,7 +25,7 @@ public class YogaConfig { long mNativePointer; private YogaLogger mLogger; - private YogaNodeClonedFunction mNodeClonedFunction; + private YogaNodeCloneFunction mYogaNodeCloneFunction; private native long jni_YGConfigNew(); public YogaConfig() { @@ -97,16 +97,15 @@ public class YogaConfig { return mLogger; } - private native void jni_YGConfigSetHasNodeClonedFunc(long nativePointer, boolean hasClonedFunc); + private native void jni_YGConfigSetHasCloneNodeFunc(long nativePointer, boolean hasClonedFunc); - public void setOnNodeCloned(YogaNodeClonedFunction nodeClonedFunction) { - mNodeClonedFunction = nodeClonedFunction; - jni_YGConfigSetHasNodeClonedFunc(mNativePointer, nodeClonedFunction != null); + public void setOnCloneNode(YogaNodeCloneFunction cloneYogaNodeFunction) { + mYogaNodeCloneFunction = cloneYogaNodeFunction; + jni_YGConfigSetHasCloneNodeFunc(mNativePointer, cloneYogaNodeFunction != null); } @DoNotStrip - public final void onNodeCloned( - YogaNode oldNode, YogaNode newNode, YogaNode parent, int childIndex) { - mNodeClonedFunction.onNodeCloned(oldNode, newNode, parent, childIndex); + private final YogaNode cloneNode(YogaNode oldNode, YogaNode parent, int childIndex) { + return mYogaNodeCloneFunction.cloneNode(oldNode, parent, childIndex); } } diff --git a/ReactAndroid/src/main/java/com/facebook/yoga/YogaNode.java b/ReactAndroid/src/main/java/com/facebook/yoga/YogaNode.java index 1c6e67a55e..577ee3fb05 100644 --- a/ReactAndroid/src/main/java/com/facebook/yoga/YogaNode.java +++ b/ReactAndroid/src/main/java/com/facebook/yoga/YogaNode.java @@ -697,4 +697,18 @@ public class YogaNode implements Cloneable { public void print() { jni_YGNodePrint(mNativePointer); } + + /** + * This method replaces the child at childIndex position with the newNode received by parameter. + * This is different than calling removeChildAt and addChildAt because this method ONLY replaces + * the child in the mChildren datastructure. @DoNotStrip: called from JNI + * + * @return the nativePointer of the newNode {@linl YogaNode} + */ + @DoNotStrip + private final long replaceChild(YogaNode newNode, int childIndex) { + mChildren.remove(childIndex); + mChildren.add(childIndex, newNode); + return newNode.mNativePointer; + } } diff --git a/ReactAndroid/src/main/java/com/facebook/yoga/YogaNodeClonedFunction.java b/ReactAndroid/src/main/java/com/facebook/yoga/YogaNodeCloneFunction.java similarity index 69% rename from ReactAndroid/src/main/java/com/facebook/yoga/YogaNodeClonedFunction.java rename to ReactAndroid/src/main/java/com/facebook/yoga/YogaNodeCloneFunction.java index f469546495..0733776a7e 100644 --- a/ReactAndroid/src/main/java/com/facebook/yoga/YogaNodeClonedFunction.java +++ b/ReactAndroid/src/main/java/com/facebook/yoga/YogaNodeCloneFunction.java @@ -10,8 +10,8 @@ package com.facebook.yoga; import com.facebook.proguard.annotations.DoNotStrip; @DoNotStrip -public interface YogaNodeClonedFunction { +public interface YogaNodeCloneFunction { @DoNotStrip - void onNodeCloned(YogaNode oldNode, YogaNode newNode, YogaNode parent, int childIndex); + YogaNode cloneNode(YogaNode oldNode, YogaNode parent, int childIndex); } diff --git a/ReactAndroid/src/main/jni/first-party/yogajni/jni/YGJNI.cpp b/ReactAndroid/src/main/jni/first-party/yogajni/jni/YGJNI.cpp index cc75df96a1..7f92ba8e06 100644 --- a/ReactAndroid/src/main/jni/first-party/yogajni/jni/YGJNI.cpp +++ b/ReactAndroid/src/main/jni/first-party/yogajni/jni/YGJNI.cpp @@ -142,31 +142,49 @@ static float YGJNIBaselineFunc(YGNodeRef node, float width, float height) { } } -static void YGJNIOnNodeClonedFunc( +static inline YGNodeRef _jlong2YGNodeRef(jlong addr) { + return reinterpret_cast(static_cast(addr)); +} + +static inline YGConfigRef _jlong2YGConfigRef(jlong addr) { + return reinterpret_cast(static_cast(addr)); +} + +static YGNodeRef YGJNIOnNodeClonedFunc( YGNodeRef oldNode, - YGNodeRef newNode, YGNodeRef parent, int childIndex) { auto config = oldNode->getConfig(); if (!config) { - return; + return nullptr; } + static auto onNodeClonedFunc = findClassStatic("com/facebook/yoga/YogaConfig") - ->getMethodgetMethod( local_ref, local_ref, - local_ref, - jint)>("onNodeCloned"); + jint)>("cloneNode"); auto context = reinterpret_cast(YGConfigGetContext(config)); auto javaConfig = context->config; - onNodeClonedFunc( + auto newNode = onNodeClonedFunc( javaConfig->get(), YGNodeJobject(oldNode)->lockLocal(), - YGNodeJobject(newNode)->lockLocal(), YGNodeJobject(parent)->lockLocal(), childIndex); + + static auto replaceChild = findClassStatic("com/facebook/yoga/YogaNode") + ->getMethod, + jint)>("replaceChild"); + + jlong newNodeNativePointer = replaceChild( + YGNodeJobject(parent)->lockLocal(), + newNode, + childIndex); + + return _jlong2YGNodeRef(newNodeNativePointer); } static YGSize YGJNIMeasureFunc( @@ -234,14 +252,6 @@ static int YGJNILogFunc(const YGConfigRef config, return result; } -static inline YGNodeRef _jlong2YGNodeRef(jlong addr) { - return reinterpret_cast(static_cast(addr)); -} - -static inline YGConfigRef _jlong2YGConfigRef(jlong addr) { - return reinterpret_cast(static_cast(addr)); -} - jlong jni_YGNodeNew(alias_ref thiz) { const YGNodeRef node = YGNodeNew(); node->setContext(new weak_ref(make_weak(thiz))); @@ -506,10 +516,10 @@ void jni_YGConfigSetUseLegacyStretchBehaviour(alias_ref, YGConfigSetUseLegacyStretchBehaviour(config, useLegacyStretchBehaviour); } -void jni_YGConfigSetHasNodeClonedFunc( +void jni_YGConfigSetHasCloneNodeFunc( alias_ref thiz, jlong nativePointer, - jboolean hasNodeClonedFunc) { + jboolean hasCloneNodeFunc) { const YGConfigRef config = _jlong2YGConfigRef(nativePointer); auto context = reinterpret_cast(YGConfigGetContext(config)); if (context && context->config) { @@ -517,15 +527,15 @@ void jni_YGConfigSetHasNodeClonedFunc( context->config = nullptr; } - if (hasNodeClonedFunc) { + if (hasCloneNodeFunc) { if (!context) { context = new YGConfigContext(); YGConfigSetContext(config, context); } context->config = new global_ref(make_global(thiz)); - YGConfigSetNodeClonedFunc(config, YGJNIOnNodeClonedFunc); + YGConfigSetCloneNodeFunc(config, YGJNIOnNodeClonedFunc); } else { - YGConfigSetNodeClonedFunc(config, nullptr); + YGConfigSetCloneNodeFunc(config, nullptr); } } @@ -652,7 +662,7 @@ jint JNI_OnLoad(JavaVM *vm, void *) { YGMakeNativeMethod(jni_YGConfigSetPointScaleFactor), YGMakeNativeMethod(jni_YGConfigSetUseLegacyStretchBehaviour), YGMakeNativeMethod(jni_YGConfigSetLogger), - YGMakeNativeMethod(jni_YGConfigSetHasNodeClonedFunc), + YGMakeNativeMethod(jni_YGConfigSetHasCloneNodeFunc), YGMakeNativeMethod( jni_YGConfigSetShouldDiffLayoutWithoutLegacyStretchBehaviour), }); diff --git a/ReactCommon/fabric/view/yoga/YogaLayoutableShadowNode.cpp b/ReactCommon/fabric/view/yoga/YogaLayoutableShadowNode.cpp index d7974ae0fe..60586c9249 100644 --- a/ReactCommon/fabric/view/yoga/YogaLayoutableShadowNode.cpp +++ b/ReactCommon/fabric/view/yoga/YogaLayoutableShadowNode.cpp @@ -24,7 +24,7 @@ SharedYogaConfig YogaLayoutableShadowNode::suitableYogaConfig() { if (!sharedYogaConfig) { sharedYogaConfig = std::make_shared(YGConfig({ - .cloneNodeCallback = YogaLayoutableShadowNode::yogaNodeCloneCallbackConnector + // .cloneNodeCallback = YogaLayoutableShadowNode::yogaNodeCloneCallbackConnector })); } diff --git a/ReactCommon/yoga/yoga/YGNode.cpp b/ReactCommon/yoga/yoga/YGNode.cpp index 8fcb44c39d..f15105f26f 100644 --- a/ReactCommon/yoga/yoga/YGNode.cpp +++ b/ReactCommon/yoga/yoga/YGNode.cpp @@ -558,15 +558,18 @@ void YGNode::cloneChildrenIfNeeded() { return; } - const YGNodeClonedFunc cloneNodeCallback = config_->cloneNodeCallback; + const YGCloneNodeFunc cloneNodeCallback = config_->cloneNodeCallback; for (uint32_t i = 0; i < childCount; ++i) { const YGNodeRef oldChild = children_[i]; - const YGNodeRef newChild = YGNodeClone(oldChild); + YGNodeRef newChild = nullptr; + if (cloneNodeCallback) { + newChild = cloneNodeCallback(oldChild, this, i); + } + if (newChild == nullptr) { + newChild = YGNodeClone(oldChild); + } replaceChild(newChild, i); newChild->setParent(this); - if (cloneNodeCallback) { - cloneNodeCallback(oldChild, newChild, this, i); - } } } diff --git a/ReactCommon/yoga/yoga/Yoga-internal.h b/ReactCommon/yoga/yoga/Yoga-internal.h index 56078d6c3f..d11f8c29d9 100644 --- a/ReactCommon/yoga/yoga/Yoga-internal.h +++ b/ReactCommon/yoga/yoga/Yoga-internal.h @@ -94,7 +94,7 @@ struct YGConfig { bool shouldDiffLayoutWithoutLegacyStretchBehaviour; float pointScaleFactor; YGLogger logger; - YGNodeClonedFunc cloneNodeCallback; + YGCloneNodeFunc cloneNodeCallback; void* context; }; diff --git a/ReactCommon/yoga/yoga/Yoga.cpp b/ReactCommon/yoga/yoga/Yoga.cpp index 1600b1b05f..b597727bc4 100644 --- a/ReactCommon/yoga/yoga/Yoga.cpp +++ b/ReactCommon/yoga/yoga/Yoga.cpp @@ -428,7 +428,7 @@ void YGNodeRemoveChild(const YGNodeRef parent, const YGNodeRef excludedChild) { // Otherwise we have to clone the node list except for the child we're trying to delete. // We don't want to simply clone all children, because then the host will need to free // the clone of the child that was just deleted. - const YGNodeClonedFunc cloneNodeCallback = + const YGCloneNodeFunc cloneNodeCallback = parent->getConfig()->cloneNodeCallback; uint32_t nextInsertIndex = 0; for (uint32_t i = 0; i < childCount; i++) { @@ -440,12 +440,16 @@ void YGNodeRemoveChild(const YGNodeRef parent, const YGNodeRef excludedChild) { parent->markDirtyAndPropogate(); continue; } - const YGNodeRef newChild = YGNodeClone(oldChild); + YGNodeRef newChild = nullptr; + if (cloneNodeCallback) { + newChild = cloneNodeCallback(oldChild, parent, nextInsertIndex); + } + if (newChild == nullptr) { + newChild = YGNodeClone(oldChild); + } parent->replaceChild(newChild, nextInsertIndex); newChild->setParent(parent); - if (cloneNodeCallback) { - cloneNodeCallback(oldChild, newChild, parent, nextInsertIndex); - } + nextInsertIndex++; } while (nextInsertIndex < childCount) { @@ -3964,7 +3968,7 @@ void *YGConfigGetContext(const YGConfigRef config) { return config->context; } -void YGConfigSetNodeClonedFunc(const YGConfigRef config, const YGNodeClonedFunc callback) { +void YGConfigSetCloneNodeFunc(const YGConfigRef config, const YGCloneNodeFunc callback) { config->cloneNodeCallback = callback; } diff --git a/ReactCommon/yoga/yoga/Yoga.h b/ReactCommon/yoga/yoga/Yoga.h index 1a985fa563..73e7c7fdb1 100644 --- a/ReactCommon/yoga/yoga/Yoga.h +++ b/ReactCommon/yoga/yoga/Yoga.h @@ -62,8 +62,7 @@ typedef int (*YGLogger)(const YGConfigRef config, YGLogLevel level, const char *format, va_list args); -typedef void (*YGNodeClonedFunc)(YGNodeRef oldNode, - YGNodeRef newNode, +typedef YGNodeRef (*YGCloneNodeFunc)(YGNodeRef oldNode, YGNodeRef parent, int childIndex); @@ -283,8 +282,8 @@ WIN_EXPORT bool YGConfigIsExperimentalFeatureEnabled(const YGConfigRef config, WIN_EXPORT void YGConfigSetUseWebDefaults(const YGConfigRef config, const bool enabled); WIN_EXPORT bool YGConfigGetUseWebDefaults(const YGConfigRef config); -WIN_EXPORT void YGConfigSetNodeClonedFunc(const YGConfigRef config, - const YGNodeClonedFunc callback); +WIN_EXPORT void YGConfigSetCloneNodeFunc(const YGConfigRef config, + const YGCloneNodeFunc callback); // Export only for C# WIN_EXPORT YGConfigRef YGConfigGetDefault(void);