From bb5622dd89a7e8562fd9c0f101bd6cabf891c877 Mon Sep 17 00:00:00 2001 From: Joshua Gross Date: Wed, 18 Dec 2019 14:59:24 -0800 Subject: [PATCH] Prop parsing: always require explicit default argument to `convertRawProp` Summary: Having automatic defaults/an optional arg for `convertRawProp` has caused way more problems than it is worth. Remove the default argument. Changelog: [Internal] Reviewed By: shergin Differential Revision: D19151594 fbshipit-source-id: 839ec8d138b2c3c083f221a2871582454004648c --- .../fabric/attributedstring/conversions.h | 17 ++- .../fabric/components/image/ImageProps.cpp | 13 +- .../components/scrollview/ScrollViewProps.cpp | 54 ++++++--- .../text/paragraph/ParagraphProps.cpp | 9 +- .../components/text/rawtext/RawTextProps.cpp | 2 +- .../AndroidTextInputProps.cpp | 2 +- .../fabric/components/view/ViewProps.cpp | 60 ++++++---- .../view/accessibility/AccessibilityProps.cpp | 42 ++++--- .../fabric/components/view/propsConversions.h | 111 +++++++++++++----- ReactCommon/fabric/core/propsConversions.h | 4 +- ReactCommon/fabric/core/shadownode/Props.cpp | 2 +- 11 files changed, 220 insertions(+), 96 deletions(-) diff --git a/ReactCommon/fabric/attributedstring/conversions.h b/ReactCommon/fabric/attributedstring/conversions.h index a5d3b73a07b..9336249b2a5 100644 --- a/ReactCommon/fabric/attributedstring/conversions.h +++ b/ReactCommon/fabric/attributedstring/conversions.h @@ -414,33 +414,40 @@ inline std::string toString( inline ParagraphAttributes convertRawProp( RawProps const &rawProps, + ParagraphAttributes const &sourceParagraphAttributes, ParagraphAttributes const &defaultParagraphAttributes) { auto paragraphAttributes = ParagraphAttributes{}; paragraphAttributes.maximumNumberOfLines = convertRawProp( rawProps, "numberOfLines", + sourceParagraphAttributes.maximumNumberOfLines, defaultParagraphAttributes.maximumNumberOfLines); paragraphAttributes.ellipsizeMode = convertRawProp( - rawProps, "ellipsizeMode", defaultParagraphAttributes.ellipsizeMode); + rawProps, + "ellipsizeMode", + sourceParagraphAttributes.ellipsizeMode, + defaultParagraphAttributes.ellipsizeMode); paragraphAttributes.textBreakStrategy = convertRawProp( rawProps, "textBreakStrategy", + sourceParagraphAttributes.textBreakStrategy, defaultParagraphAttributes.textBreakStrategy); paragraphAttributes.adjustsFontSizeToFit = convertRawProp( rawProps, "adjustsFontSizeToFit", + sourceParagraphAttributes.adjustsFontSizeToFit, defaultParagraphAttributes.adjustsFontSizeToFit); paragraphAttributes.minimumFontSize = convertRawProp( rawProps, "minimumFontSize", - defaultParagraphAttributes.minimumFontSize, - std::numeric_limits::quiet_NaN()); + sourceParagraphAttributes.minimumFontSize, + defaultParagraphAttributes.minimumFontSize); paragraphAttributes.maximumFontSize = convertRawProp( rawProps, "maximumFontSize", - defaultParagraphAttributes.maximumFontSize, - std::numeric_limits::quiet_NaN()); + sourceParagraphAttributes.maximumFontSize, + defaultParagraphAttributes.maximumFontSize); return paragraphAttributes; } diff --git a/ReactCommon/fabric/components/image/ImageProps.cpp b/ReactCommon/fabric/components/image/ImageProps.cpp index 7608baaaf2b..109ac9e8c07 100644 --- a/ReactCommon/fabric/components/image/ImageProps.cpp +++ b/ReactCommon/fabric/components/image/ImageProps.cpp @@ -14,20 +14,23 @@ namespace react { ImageProps::ImageProps(const ImageProps &sourceProps, const RawProps &rawProps) : ViewProps(sourceProps, rawProps), - sources(convertRawProp(rawProps, "source", sourceProps.sources)), + sources(convertRawProp(rawProps, "source", sourceProps.sources, {})), defaultSources(convertRawProp( rawProps, "defaultSource", - sourceProps.defaultSources)), + sourceProps.defaultSources, + {})), resizeMode(convertRawProp( rawProps, "resizeMode", sourceProps.resizeMode, ImageResizeMode::Stretch)), blurRadius( - convertRawProp(rawProps, "blurRadius", sourceProps.blurRadius)), - capInsets(convertRawProp(rawProps, "capInsets", sourceProps.capInsets)), - tintColor(convertRawProp(rawProps, "tintColor", sourceProps.tintColor)) {} + convertRawProp(rawProps, "blurRadius", sourceProps.blurRadius, {})), + capInsets( + convertRawProp(rawProps, "capInsets", sourceProps.capInsets, {})), + tintColor( + convertRawProp(rawProps, "tintColor", sourceProps.tintColor, {})) {} } // namespace react } // namespace facebook diff --git a/ReactCommon/fabric/components/scrollview/ScrollViewProps.cpp b/ReactCommon/fabric/components/scrollview/ScrollViewProps.cpp index 88209c35a26..fdc3c113e20 100644 --- a/ReactCommon/fabric/components/scrollview/ScrollViewProps.cpp +++ b/ReactCommon/fabric/components/scrollview/ScrollViewProps.cpp @@ -23,11 +23,13 @@ ScrollViewProps::ScrollViewProps( alwaysBounceHorizontal(convertRawProp( rawProps, "alwaysBounceHorizontal", - sourceProps.alwaysBounceHorizontal)), + sourceProps.alwaysBounceHorizontal, + {})), alwaysBounceVertical(convertRawProp( rawProps, "alwaysBounceVertical", - sourceProps.alwaysBounceVertical)), + sourceProps.alwaysBounceVertical, + {})), bounces(convertRawProp(rawProps, "bounces", sourceProps.bounces, true)), bouncesZoom(convertRawProp( rawProps, @@ -37,13 +39,18 @@ ScrollViewProps::ScrollViewProps( canCancelContentTouches(convertRawProp( rawProps, "canCancelContentTouches", - sourceProps.canCancelContentTouches)), - centerContent( - convertRawProp(rawProps, "centerContent", sourceProps.centerContent)), + sourceProps.canCancelContentTouches, + true)), + centerContent(convertRawProp( + rawProps, + "centerContent", + sourceProps.centerContent, + {})), automaticallyAdjustContentInsets(convertRawProp( rawProps, "automaticallyAdjustContentInsets", - sourceProps.automaticallyAdjustContentInsets)), + sourceProps.automaticallyAdjustContentInsets, + {})), decelerationRate(convertRawProp( rawProps, "decelerationRate", @@ -52,15 +59,18 @@ ScrollViewProps::ScrollViewProps( directionalLockEnabled(convertRawProp( rawProps, "directionalLockEnabled", - sourceProps.directionalLockEnabled)), + sourceProps.directionalLockEnabled, + {})), indicatorStyle(convertRawProp( rawProps, "indicatorStyle", - sourceProps.indicatorStyle)), + sourceProps.indicatorStyle, + {})), keyboardDismissMode(convertRawProp( rawProps, "keyboardDismissMode", - sourceProps.keyboardDismissMode)), + sourceProps.keyboardDismissMode, + {})), maximumZoomScale(convertRawProp( rawProps, "maximumZoomScale", @@ -76,8 +86,11 @@ ScrollViewProps::ScrollViewProps( "scrollEnabled", sourceProps.scrollEnabled, true)), - pagingEnabled( - convertRawProp(rawProps, "pagingEnabled", sourceProps.pagingEnabled)), + pagingEnabled(convertRawProp( + rawProps, + "pagingEnabled", + sourceProps.pagingEnabled, + {})), pinchGestureEnabled(convertRawProp( rawProps, "pinchGestureEnabled", @@ -101,26 +114,33 @@ ScrollViewProps::ScrollViewProps( scrollEventThrottle(convertRawProp( rawProps, "scrollEventThrottle", - sourceProps.scrollEventThrottle)), + sourceProps.scrollEventThrottle, + {})), zoomScale(convertRawProp( rawProps, "zoomScale", sourceProps.zoomScale, (Float)1.0)), - contentInset( - convertRawProp(rawProps, "contentInset", sourceProps.contentInset)), + contentInset(convertRawProp( + rawProps, + "contentInset", + sourceProps.contentInset, + {})), scrollIndicatorInsets(convertRawProp( rawProps, "scrollIndicatorInsets", - sourceProps.scrollIndicatorInsets)), + sourceProps.scrollIndicatorInsets, + {})), snapToInterval(convertRawProp( rawProps, "snapToInterval", - sourceProps.snapToInterval)), + sourceProps.snapToInterval, + {})), snapToAlignment(convertRawProp( rawProps, "snapToAlignment", - sourceProps.snapToAlignment)) {} + sourceProps.snapToAlignment, + {})) {} #pragma mark - DebugStringConvertible diff --git a/ReactCommon/fabric/components/text/paragraph/ParagraphProps.cpp b/ReactCommon/fabric/components/text/paragraph/ParagraphProps.cpp index 2b9e8926d0e..47f32321012 100644 --- a/ReactCommon/fabric/components/text/paragraph/ParagraphProps.cpp +++ b/ReactCommon/fabric/components/text/paragraph/ParagraphProps.cpp @@ -23,9 +23,12 @@ ParagraphProps::ParagraphProps( : ViewProps(sourceProps, rawProps), BaseTextProps(sourceProps, rawProps), paragraphAttributes( - convertRawProp(rawProps, sourceProps.paragraphAttributes)), - isSelectable( - convertRawProp(rawProps, "selectable", sourceProps.isSelectable)){}; + convertRawProp(rawProps, sourceProps.paragraphAttributes, {})), + isSelectable(convertRawProp( + rawProps, + "selectable", + sourceProps.isSelectable, + {})){}; #pragma mark - DebugStringConvertible diff --git a/ReactCommon/fabric/components/text/rawtext/RawTextProps.cpp b/ReactCommon/fabric/components/text/rawtext/RawTextProps.cpp index 0ea008fdc7a..2823ca7dbd4 100644 --- a/ReactCommon/fabric/components/text/rawtext/RawTextProps.cpp +++ b/ReactCommon/fabric/components/text/rawtext/RawTextProps.cpp @@ -17,7 +17,7 @@ RawTextProps::RawTextProps( const RawTextProps &sourceProps, const RawProps &rawProps) : Props(sourceProps, rawProps), - text(convertRawProp(rawProps, "text", sourceProps.text)){}; + text(convertRawProp(rawProps, "text", sourceProps.text, {})){}; #pragma mark - DebugStringConvertible diff --git a/ReactCommon/fabric/components/textinput/androidtextinput/AndroidTextInputProps.cpp b/ReactCommon/fabric/components/textinput/androidtextinput/AndroidTextInputProps.cpp index ba4bfe633cb..5f25f17da9e 100644 --- a/ReactCommon/fabric/components/textinput/androidtextinput/AndroidTextInputProps.cpp +++ b/ReactCommon/fabric/components/textinput/androidtextinput/AndroidTextInputProps.cpp @@ -222,7 +222,7 @@ AndroidTextInputProps::AndroidTextInputProps( {0})), text(convertRawProp(rawProps, "text", sourceProps.text, {})), paragraphAttributes( - convertRawProp(rawProps, sourceProps.paragraphAttributes)) {} + convertRawProp(rawProps, sourceProps.paragraphAttributes, {})) {} // TODO T53300085: support this in codegen; this was hand-written folly::dynamic AndroidTextInputProps::getDynamic() const { diff --git a/ReactCommon/fabric/components/view/ViewProps.cpp b/ReactCommon/fabric/components/view/ViewProps.cpp index a4a21338209..0c0ac487e8e 100644 --- a/ReactCommon/fabric/components/view/ViewProps.cpp +++ b/ReactCommon/fabric/components/view/ViewProps.cpp @@ -29,48 +29,68 @@ ViewProps::ViewProps(ViewProps const &sourceProps, RawProps const &rawProps) foregroundColor(convertRawProp( rawProps, "foregroundColor", - sourceProps.foregroundColor)), + sourceProps.foregroundColor, + {})), backgroundColor(convertRawProp( rawProps, "backgroundColor", - sourceProps.backgroundColor)), + sourceProps.backgroundColor, + {})), borderRadii(convertRawProp( rawProps, "border", "Radius", - sourceProps.borderRadii)), + sourceProps.borderRadii, + {})), borderColors(convertRawProp( rawProps, "border", "Color", - sourceProps.borderColors)), + sourceProps.borderColors, + {})), borderStyles(convertRawProp( rawProps, "border", "Style", - sourceProps.borderStyles)), + sourceProps.borderStyles, + {})), shadowColor( - convertRawProp(rawProps, "shadowColor", sourceProps.shadowColor)), - shadowOffset( - convertRawProp(rawProps, "shadowOffset", sourceProps.shadowOffset)), - shadowOpacity( - convertRawProp(rawProps, "shadowOpacity", sourceProps.shadowOpacity)), - shadowRadius( - convertRawProp(rawProps, "shadowRadius", sourceProps.shadowRadius)), - transform(convertRawProp(rawProps, "transform", sourceProps.transform)), + convertRawProp(rawProps, "shadowColor", sourceProps.shadowColor, {})), + shadowOffset(convertRawProp( + rawProps, + "shadowOffset", + sourceProps.shadowOffset, + {})), + shadowOpacity(convertRawProp( + rawProps, + "shadowOpacity", + sourceProps.shadowOpacity, + {})), + shadowRadius(convertRawProp( + rawProps, + "shadowRadius", + sourceProps.shadowRadius, + {})), + transform( + convertRawProp(rawProps, "transform", sourceProps.transform, {})), backfaceVisibility(convertRawProp( rawProps, "backfaceVisibility", - sourceProps.backfaceVisibility)), + sourceProps.backfaceVisibility, + {})), shouldRasterize(convertRawProp( rawProps, "shouldRasterize", - sourceProps.shouldRasterize)), - zIndex(convertRawProp(rawProps, "zIndex", sourceProps.zIndex)), - pointerEvents( - convertRawProp(rawProps, "pointerEvents", sourceProps.pointerEvents)), - hitSlop(convertRawProp(rawProps, "hitSlop", sourceProps.hitSlop)), - onLayout(convertRawProp(rawProps, "onLayout", sourceProps.onLayout)), + sourceProps.shouldRasterize, + {})), + zIndex(convertRawProp(rawProps, "zIndex", sourceProps.zIndex, {})), + pointerEvents(convertRawProp( + rawProps, + "pointerEvents", + sourceProps.pointerEvents, + {})), + hitSlop(convertRawProp(rawProps, "hitSlop", sourceProps.hitSlop, {})), + onLayout(convertRawProp(rawProps, "onLayout", sourceProps.onLayout, {})), collapsable(convertRawProp( rawProps, "collapsable", diff --git a/ReactCommon/fabric/components/view/accessibility/AccessibilityProps.cpp b/ReactCommon/fabric/components/view/accessibility/AccessibilityProps.cpp index c7897a3683d..b7f9bd7bfde 100644 --- a/ReactCommon/fabric/components/view/accessibility/AccessibilityProps.cpp +++ b/ReactCommon/fabric/components/view/accessibility/AccessibilityProps.cpp @@ -18,53 +18,67 @@ namespace react { AccessibilityProps::AccessibilityProps( AccessibilityProps const &sourceProps, RawProps const &rawProps) - : accessible( - convertRawProp(rawProps, "accessible", sourceProps.accessible)), + : accessible(convertRawProp( + rawProps, + "accessible", + sourceProps.accessible, + false)), accessibilityTraits(convertRawProp( rawProps, "accessibilityRole", - sourceProps.accessibilityTraits)), + sourceProps.accessibilityTraits, + AccessibilityTraits::None)), accessibilityLabel(convertRawProp( rawProps, "accessibilityLabel", - sourceProps.accessibilityLabel)), + sourceProps.accessibilityLabel, + "")), accessibilityHint(convertRawProp( rawProps, "accessibilityHint", - sourceProps.accessibilityHint)), + sourceProps.accessibilityHint, + "")), accessibilityActions(convertRawProp( rawProps, "accessibilityActions", - sourceProps.accessibilityActions)), + sourceProps.accessibilityActions, + {})), accessibilityViewIsModal(convertRawProp( rawProps, "accessibilityViewIsModal", - sourceProps.accessibilityViewIsModal)), + sourceProps.accessibilityViewIsModal, + false)), accessibilityElementsHidden(convertRawProp( rawProps, "accessibilityElementsHidden", - sourceProps.accessibilityElementsHidden)), + sourceProps.accessibilityElementsHidden, + false)), accessibilityIgnoresInvertColors(convertRawProp( rawProps, "accessibilityIgnoresInvertColors", - sourceProps.accessibilityIgnoresInvertColors)), + sourceProps.accessibilityIgnoresInvertColors, + false)), onAccessibilityTap(convertRawProp( rawProps, "onAccessibilityTap", - sourceProps.onAccessibilityTap)), + sourceProps.onAccessibilityTap, + {})), onAccessibilityMagicTap(convertRawProp( rawProps, "onAccessibilityMagicTap", - sourceProps.onAccessibilityMagicTap)), + sourceProps.onAccessibilityMagicTap, + {})), onAccessibilityEscape(convertRawProp( rawProps, "onAccessibilityEscape", - sourceProps.onAccessibilityEscape)), + sourceProps.onAccessibilityEscape, + {})), onAccessibilityAction(convertRawProp( rawProps, "onAccessibilityAction", - sourceProps.onAccessibilityAction)), - testId(convertRawProp(rawProps, "testId", sourceProps.testId)) {} + sourceProps.onAccessibilityAction, + {})), + testId(convertRawProp(rawProps, "testId", sourceProps.testId, "")) {} #pragma mark - DebugStringConvertible diff --git a/ReactCommon/fabric/components/view/propsConversions.h b/ReactCommon/fabric/components/view/propsConversions.h index ac450f83a2f..a21e3a95937 100644 --- a/ReactCommon/fabric/components/view/propsConversions.h +++ b/ReactCommon/fabric/components/view/propsConversions.h @@ -212,29 +212,70 @@ static inline CascadedRectangleCorners convertRawProp( RawProps const &rawProps, char const *prefix, char const *suffix, - CascadedRectangleCorners const &sourceValue) { + CascadedRectangleCorners const &sourceValue, + CascadedRectangleCorners const &defaultValue) { CascadedRectangleCorners result; result.topLeft = convertRawProp( - rawProps, "TopLeft", sourceValue.topLeft, {}, prefix, suffix); + rawProps, + "TopLeft", + sourceValue.topLeft, + defaultValue.topLeft, + prefix, + suffix); result.topRight = convertRawProp( - rawProps, "TopRight", sourceValue.topRight, {}, prefix, suffix); + rawProps, + "TopRight", + sourceValue.topRight, + defaultValue.topRight, + prefix, + suffix); result.bottomLeft = convertRawProp( - rawProps, "BottomLeft", sourceValue.bottomLeft, {}, prefix, suffix); + rawProps, + "BottomLeft", + sourceValue.bottomLeft, + defaultValue.bottomLeft, + prefix, + suffix); result.bottomRight = convertRawProp( - rawProps, "BottomRight", sourceValue.bottomRight, {}, prefix, suffix); + rawProps, + "BottomRight", + sourceValue.bottomRight, + defaultValue.bottomRight, + prefix, + suffix); result.topStart = convertRawProp( - rawProps, "TopStart", sourceValue.topStart, {}, prefix, suffix); + rawProps, + "TopStart", + sourceValue.topStart, + defaultValue.topStart, + prefix, + suffix); result.topEnd = convertRawProp( - rawProps, "TopEnd", sourceValue.topEnd, {}, prefix, suffix); + rawProps, + "TopEnd", + sourceValue.topEnd, + defaultValue.topEnd, + prefix, + suffix); result.bottomStart = convertRawProp( - rawProps, "BottomStart", sourceValue.bottomStart, {}, prefix, suffix); + rawProps, + "BottomStart", + sourceValue.bottomStart, + defaultValue.bottomStart, + prefix, + suffix); result.bottomEnd = convertRawProp( - rawProps, "BottomEnd", sourceValue.bottomEnd, {}, prefix, suffix); + rawProps, + "BottomEnd", + sourceValue.bottomEnd, + defaultValue.bottomEnd, + prefix, + suffix); - result.all = - convertRawProp(rawProps, "", sourceValue.all, {}, prefix, suffix); + result.all = convertRawProp( + rawProps, "", sourceValue.all, defaultValue.all, prefix, suffix); return result; } @@ -244,29 +285,45 @@ static inline CascadedRectangleEdges convertRawProp( RawProps const &rawProps, char const *prefix, char const *suffix, - CascadedRectangleEdges const &sourceValue) { + CascadedRectangleEdges const &sourceValue, + CascadedRectangleEdges const &defaultValue) { CascadedRectangleEdges result; - result.left = - convertRawProp(rawProps, "Left", sourceValue.left, {}, prefix, suffix); - result.right = - convertRawProp(rawProps, "Right", sourceValue.right, {}, prefix, suffix); - result.top = - convertRawProp(rawProps, "Top", sourceValue.top, {}, prefix, suffix); + result.left = convertRawProp( + rawProps, "Left", sourceValue.left, defaultValue.left, prefix, suffix); + result.right = convertRawProp( + rawProps, "Right", sourceValue.right, defaultValue.right, prefix, suffix); + result.top = convertRawProp( + rawProps, "Top", sourceValue.top, defaultValue.top, prefix, suffix); result.bottom = convertRawProp( - rawProps, "Bottom", sourceValue.bottom, {}, prefix, suffix); + rawProps, + "Bottom", + sourceValue.bottom, + defaultValue.bottom, + prefix, + suffix); - result.start = - convertRawProp(rawProps, "Start", sourceValue.start, {}, prefix, suffix); - result.end = - convertRawProp(rawProps, "End", sourceValue.end, {}, prefix, suffix); + result.start = convertRawProp( + rawProps, "Start", sourceValue.start, defaultValue.start, prefix, suffix); + result.end = convertRawProp( + rawProps, "End", sourceValue.end, defaultValue.end, prefix, suffix); result.horizontal = convertRawProp( - rawProps, "Horizontal", sourceValue.horizontal, {}, prefix, suffix); + rawProps, + "Horizontal", + sourceValue.horizontal, + defaultValue.horizontal, + prefix, + suffix); result.vertical = convertRawProp( - rawProps, "Vertical", sourceValue.vertical, {}, prefix, suffix); + rawProps, + "Vertical", + sourceValue.vertical, + defaultValue.vertical, + prefix, + suffix); - result.all = - convertRawProp(rawProps, "", sourceValue.all, {}, prefix, suffix); + result.all = convertRawProp( + rawProps, "", sourceValue.all, defaultValue.all, prefix, suffix); return result; } diff --git a/ReactCommon/fabric/core/propsConversions.h b/ReactCommon/fabric/core/propsConversions.h index 5e6be1373c7..468c83a6ded 100644 --- a/ReactCommon/fabric/core/propsConversions.h +++ b/ReactCommon/fabric/core/propsConversions.h @@ -76,7 +76,7 @@ T convertRawProp( RawProps const &rawProps, char const *name, T const &sourceValue, - U const &defaultValue = U(), + U const &defaultValue, char const *namePrefix = nullptr, char const *nameSuffix = nullptr) { const auto *rawValue = rawProps.at(name, namePrefix, nameSuffix); @@ -101,7 +101,7 @@ static better::optional convertRawProp( RawProps const &rawProps, char const *name, better::optional const &sourceValue, - better::optional const &defaultValue = {}, + better::optional const &defaultValue, char const *namePrefix = nullptr, char const *nameSuffix = nullptr) { const auto *rawValue = rawProps.at(name, namePrefix, nameSuffix); diff --git a/ReactCommon/fabric/core/shadownode/Props.cpp b/ReactCommon/fabric/core/shadownode/Props.cpp index 0a903820dfa..bb4a057b628 100644 --- a/ReactCommon/fabric/core/shadownode/Props.cpp +++ b/ReactCommon/fabric/core/shadownode/Props.cpp @@ -14,7 +14,7 @@ namespace facebook { namespace react { Props::Props(const Props &sourceProps, const RawProps &rawProps) - : nativeId(convertRawProp(rawProps, "nativeID", sourceProps.nativeId)), + : nativeId(convertRawProp(rawProps, "nativeID", sourceProps.nativeId, {})), revision(sourceProps.revision + 1) #ifdef ANDROID ,