From 967c9dc7b1fd5333a4abddf036ed3edff6c061c2 Mon Sep 17 00:00:00 2001 From: Valentin Shergin Date: Thu, 13 Feb 2020 21:05:36 -0800 Subject: [PATCH] Fabric: `ConcreteShadowNode::getProps()` was renamed to `getConcreteProps()` and got new return type. Summary: Having the overridden function that returns a different type is apparently not a good idea (and might cause bugs and unexpected behavior), so it was renamed. The function also got a new return type (`const &` instead of `std::shared_ptr`) for simplicity, better performance, and smaller code size. Changelog: [Internal] Fabric-specific internal change. Reviewed By: JoshuaGross Differential Revision: D19837694 fbshipit-source-id: b7a96424bd040409371724907b3fb3931cd8a2e8 --- .../components/image/ImageShadowNode.cpp | 2 +- .../fabric/components/root/RootShadowNode.cpp | 4 ++-- .../components/slider/SliderShadowNode.cpp | 8 ++++---- .../text/basetext/BaseTextShadowNode.cpp | 5 +++-- .../text/paragraph/ParagraphShadowNode.cpp | 9 +++++---- .../AndroidTextInputShadowNode.cpp | 20 +++++++++---------- .../iostextinput/TextInputShadowNode.cpp | 12 +++++------ .../components/view/ConcreteViewShadowNode.h | 2 +- .../core/shadownode/ConcreteShadowNode.h | 8 ++++++-- 9 files changed, 38 insertions(+), 32 deletions(-) diff --git a/ReactCommon/fabric/components/image/ImageShadowNode.cpp b/ReactCommon/fabric/components/image/ImageShadowNode.cpp index 91ae574856c..5c34ca0ad34 100644 --- a/ReactCommon/fabric/components/image/ImageShadowNode.cpp +++ b/ReactCommon/fabric/components/image/ImageShadowNode.cpp @@ -38,7 +38,7 @@ void ImageShadowNode::updateStateIfNeeded() { } ImageSource ImageShadowNode::getImageSource() const { - auto sources = getProps()->sources; + auto sources = getConcreteProps().sources; if (sources.size() == 0) { return { diff --git a/ReactCommon/fabric/components/root/RootShadowNode.cpp b/ReactCommon/fabric/components/root/RootShadowNode.cpp index 74a93a2f536..e7736ef69c2 100644 --- a/ReactCommon/fabric/components/root/RootShadowNode.cpp +++ b/ReactCommon/fabric/components/root/RootShadowNode.cpp @@ -25,7 +25,7 @@ bool RootShadowNode::layoutIfNeeded( ensureUnsealed(); - auto layoutContext = getProps()->layoutContext; + auto layoutContext = getConcreteProps().layoutContext; layoutContext.affectedNodes = affectedNodes; layout(layoutContext); @@ -44,7 +44,7 @@ RootShadowNode::Unshared RootShadowNode::clone( LayoutConstraints const &layoutConstraints, LayoutContext const &layoutContext) const { auto props = std::make_shared( - *getProps(), layoutConstraints, layoutContext); + getConcreteProps(), layoutConstraints, layoutContext); auto newRootShadowNode = std::make_shared( *this, ShadowNodeFragment{ diff --git a/ReactCommon/fabric/components/slider/SliderShadowNode.cpp b/ReactCommon/fabric/components/slider/SliderShadowNode.cpp index be3e761f308..488fe566783 100644 --- a/ReactCommon/fabric/components/slider/SliderShadowNode.cpp +++ b/ReactCommon/fabric/components/slider/SliderShadowNode.cpp @@ -66,19 +66,19 @@ void SliderShadowNode::updateStateIfNeeded() { } ImageSource SliderShadowNode::getTrackImageSource() const { - return getProps()->trackImage; + return getConcreteProps().trackImage; } ImageSource SliderShadowNode::getMinimumTrackImageSource() const { - return getProps()->minimumTrackImage; + return getConcreteProps().minimumTrackImage; } ImageSource SliderShadowNode::getMaximumTrackImageSource() const { - return getProps()->maximumTrackImage; + return getConcreteProps().maximumTrackImage; } ImageSource SliderShadowNode::getThumbImageSource() const { - return getProps()->thumbImage; + return getConcreteProps().thumbImage; } #pragma mark - LayoutableShadowNode diff --git a/ReactCommon/fabric/components/text/basetext/BaseTextShadowNode.cpp b/ReactCommon/fabric/components/text/basetext/BaseTextShadowNode.cpp index d873d6025a6..4a109fac70a 100644 --- a/ReactCommon/fabric/components/text/basetext/BaseTextShadowNode.cpp +++ b/ReactCommon/fabric/components/text/basetext/BaseTextShadowNode.cpp @@ -28,7 +28,7 @@ AttributedString BaseTextShadowNode::getAttributedString( std::dynamic_pointer_cast(childNode); if (rawTextShadowNode) { auto fragment = AttributedString::Fragment{}; - fragment.string = rawTextShadowNode->getProps()->text; + fragment.string = rawTextShadowNode->getConcreteProps().text; fragment.textAttributes = textAttributes; // Storing a retaining pointer to `ParagraphShadowNode` inside @@ -45,7 +45,8 @@ AttributedString BaseTextShadowNode::getAttributedString( std::dynamic_pointer_cast(childNode); if (textShadowNode) { auto localTextAttributes = textAttributes; - localTextAttributes.apply(textShadowNode->getProps()->textAttributes); + localTextAttributes.apply( + textShadowNode->getConcreteProps().textAttributes); attributedString.appendAttributedString( textShadowNode->getAttributedString( localTextAttributes, *textShadowNode)); diff --git a/ReactCommon/fabric/components/text/paragraph/ParagraphShadowNode.cpp b/ReactCommon/fabric/components/text/paragraph/ParagraphShadowNode.cpp index a0ebadf1516..ad7570c97b6 100644 --- a/ReactCommon/fabric/components/text/paragraph/ParagraphShadowNode.cpp +++ b/ReactCommon/fabric/components/text/paragraph/ParagraphShadowNode.cpp @@ -18,7 +18,7 @@ char const ParagraphComponentName[] = "Paragraph"; AttributedString ParagraphShadowNode::getAttributedString() const { if (!cachedAttributedString_.has_value()) { auto textAttributes = TextAttributes::defaultTextAttributes(); - textAttributes.apply(getProps()->textAttributes); + textAttributes.apply(getConcreteProps().textAttributes); cachedAttributedString_ = BaseTextShadowNode::getAttributedString(textAttributes, *this); @@ -49,8 +49,9 @@ void ParagraphShadowNode::updateStateIfNeeded() { return; } - setStateData(ParagraphState{ - attributedString, getProps()->paragraphAttributes, textLayoutManager_}); + setStateData(ParagraphState{attributedString, + getConcreteProps().paragraphAttributes, + textLayoutManager_}); } #pragma mark - LayoutableShadowNode @@ -64,7 +65,7 @@ Size ParagraphShadowNode::measure(LayoutConstraints layoutConstraints) const { return textLayoutManager_->measure( AttributedStringBox{attributedString}, - getProps()->paragraphAttributes, + getConcreteProps().paragraphAttributes, layoutConstraints); } diff --git a/ReactCommon/fabric/components/textinput/androidtextinput/AndroidTextInputShadowNode.cpp b/ReactCommon/fabric/components/textinput/androidtextinput/AndroidTextInputShadowNode.cpp index 1db699b5925..b655da3cad9 100644 --- a/ReactCommon/fabric/components/textinput/androidtextinput/AndroidTextInputShadowNode.cpp +++ b/ReactCommon/fabric/components/textinput/androidtextinput/AndroidTextInputShadowNode.cpp @@ -32,17 +32,17 @@ void AndroidTextInputShadowNode::setContextContainer( AttributedString AndroidTextInputShadowNode::getAttributedString() const { // Use BaseTextShadowNode to get attributed string from children auto childTextAttributes = TextAttributes::defaultTextAttributes(); - childTextAttributes.apply(getProps()->textAttributes); + childTextAttributes.apply(getConcreteProps().textAttributes); auto attributedString = BaseTextShadowNode::getAttributedString(childTextAttributes, *this); // BaseTextShadowNode only gets children. We must detect and prepend text // value attributes manually. - if (!getProps()->text.empty()) { + if (!getConcreteProps().text.empty()) { auto textAttributes = TextAttributes::defaultTextAttributes(); - textAttributes.apply(getProps()->textAttributes); + textAttributes.apply(getConcreteProps().textAttributes); auto fragment = AttributedString::Fragment{}; - fragment.string = getProps()->text; + fragment.string = getConcreteProps().text; fragment.textAttributes = textAttributes; // If the TextInput opacity is 0 < n < 1, the opacity of the TextInput and // text value's background will stack. This is a hack/workaround to prevent @@ -64,14 +64,14 @@ AttributedString AndroidTextInputShadowNode::getPlaceholderAttributedString() // Return placeholder text, since text and children are empty. auto textAttributedString = AttributedString{}; auto fragment = AttributedString::Fragment{}; - fragment.string = getProps()->placeholder; + fragment.string = getConcreteProps().placeholder; if (fragment.string.empty()) { fragment.string = " "; } auto textAttributes = TextAttributes::defaultTextAttributes(); - textAttributes.apply(getProps()->textAttributes); + textAttributes.apply(getConcreteProps().textAttributes); // If there's no text, it's possible that this Fragment isn't actually // appended to the AttributedString (see implementation of appendFragment) @@ -125,12 +125,12 @@ void AndroidTextInputShadowNode::updateStateIfNeeded() { // in the AttributedString, and when State is updated, it needs some way to // reconstruct a Fragment with default TextAttributes. auto defaultTextAttributes = TextAttributes::defaultTextAttributes(); - defaultTextAttributes.apply(getProps()->textAttributes); + defaultTextAttributes.apply(getConcreteProps().textAttributes); auto newEventCount = (state.reactTreeAttributedString == reactTreeAttributedString ? 0 - : getProps()->mostRecentEventCount); + : getConcreteProps().mostRecentEventCount); auto newAttributedString = getMostRecentAttributedString(); // Even if we're here and updating state, it may be only to update the layout @@ -141,7 +141,7 @@ void AndroidTextInputShadowNode::updateStateIfNeeded() { setStateData(AndroidTextInputState{newEventCount, newAttributedString, reactTreeAttributedString, - getProps()->paragraphAttributes, + getConcreteProps().paragraphAttributes, defaultTextAttributes, ShadowView(*this), textLayoutManager_}); @@ -168,7 +168,7 @@ Size AndroidTextInputShadowNode::measure( return textLayoutManager_->measure( AttributedStringBox{attributedString}, - getProps()->paragraphAttributes, + getConcreteProps().paragraphAttributes, layoutConstraints); } diff --git a/ReactCommon/fabric/components/textinput/iostextinput/TextInputShadowNode.cpp b/ReactCommon/fabric/components/textinput/iostextinput/TextInputShadowNode.cpp index 6cfbc665176..b0d32e406ed 100644 --- a/ReactCommon/fabric/components/textinput/iostextinput/TextInputShadowNode.cpp +++ b/ReactCommon/fabric/components/textinput/iostextinput/TextInputShadowNode.cpp @@ -34,11 +34,11 @@ AttributedStringBox TextInputShadowNode::attributedStringBoxToMeasure() const { hasMeaningfulState ? AttributedString{} : getAttributedString(); if (attributedString.isEmpty()) { - auto placeholder = getProps()->placeholder; + auto placeholder = getConcreteProps().placeholder; // Note: `zero-width space` is insufficient in some cases (e.g. when we need // to measure the "hight" of the font). auto string = !placeholder.empty() ? placeholder : "I"; - auto textAttributes = getProps()->getEffectiveTextAttributes(); + auto textAttributes = getConcreteProps().getEffectiveTextAttributes(); attributedString.appendFragment({string, textAttributes, {}}); } @@ -46,11 +46,11 @@ AttributedStringBox TextInputShadowNode::attributedStringBoxToMeasure() const { } AttributedString TextInputShadowNode::getAttributedString() const { - auto textAttributes = getProps()->getEffectiveTextAttributes(); + auto textAttributes = getConcreteProps().getEffectiveTextAttributes(); auto attributedString = AttributedString{}; attributedString.appendFragment( - AttributedString::Fragment{getProps()->text, textAttributes}); + AttributedString::Fragment{getConcreteProps().text, textAttributes}); attributedString.appendAttributedString( BaseTextShadowNode::getAttributedString(textAttributes, *this)); @@ -69,7 +69,7 @@ void TextInputShadowNode::updateStateIfNeeded() { if (!getState() || getState()->getRevision() == 0) { auto state = TextInputState{}; state.attributedStringBox = AttributedStringBox{getAttributedString()}; - state.paragraphAttributes = getProps()->paragraphAttributes; + state.paragraphAttributes = getConcreteProps().paragraphAttributes; state.layoutManager = textLayoutManager_; setStateData(std::move(state)); } @@ -80,7 +80,7 @@ void TextInputShadowNode::updateStateIfNeeded() { Size TextInputShadowNode::measure(LayoutConstraints layoutConstraints) const { return textLayoutManager_->measure( attributedStringBoxToMeasure(), - getProps()->getEffectiveParagraphAttributes(), + getConcreteProps().getEffectiveParagraphAttributes(), layoutConstraints); } diff --git a/ReactCommon/fabric/components/view/ConcreteViewShadowNode.h b/ReactCommon/fabric/components/view/ConcreteViewShadowNode.h index 616770cf686..c76eec7521e 100644 --- a/ReactCommon/fabric/components/view/ConcreteViewShadowNode.h +++ b/ReactCommon/fabric/components/view/ConcreteViewShadowNode.h @@ -111,7 +111,7 @@ class ConcreteViewShadowNode : public ConcreteShadowNode< } Transform getTransform() const override { - return BaseShadowNode::getProps()->transform; + return BaseShadowNode::getConcreteProps().transform; } #pragma mark - DebugStringConvertible diff --git a/ReactCommon/fabric/core/shadownode/ConcreteShadowNode.h b/ReactCommon/fabric/core/shadownode/ConcreteShadowNode.h index 59a606b211b..80ef750777f 100644 --- a/ReactCommon/fabric/core/shadownode/ConcreteShadowNode.h +++ b/ReactCommon/fabric/core/shadownode/ConcreteShadowNode.h @@ -80,12 +80,16 @@ class ConcreteShadowNode : public ShadowNode { return {}; } - const SharedConcreteProps getProps() const { + /* + * Returns a concrete props object associated with the node. + * Thread-safe after the node is sealed. + */ + ConcreteProps const &getConcreteProps() const { assert(props_ && "Props must not be `nullptr`."); assert( std::dynamic_pointer_cast(props_) && "Props must be an instance of ConcreteProps class."); - return std::static_pointer_cast(props_); + return *static_cast(props_.get()); } /*