Skip to content

Do not serialize text attributes that are not used later #42703

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

Closed
wants to merge 1 commit into from
Closed

Do not serialize text attributes that are not used later #42703

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 0 additions & 7 deletions packages/react-native/ReactAndroid/api/ReactAndroid.api
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7035,27 +7035,20 @@ public class com/facebook/react/views/text/ReactVirtualTextViewManager$$PropsSet

public class com/facebook/react/views/text/TextAttributeProps : com/facebook/react/views/text/EffectiveTextAttributeProvider {
public static final field TA_KEY_ACCESSIBILITY_ROLE S
public static final field TA_KEY_ALIGNMENT S
public static final field TA_KEY_ALLOW_FONT_SCALING S
public static final field TA_KEY_BACKGROUND_COLOR S
public static final field TA_KEY_BEST_WRITING_DIRECTION S
public static final field TA_KEY_FONT_FAMILY S
public static final field TA_KEY_FONT_SIZE S
public static final field TA_KEY_FONT_SIZE_MULTIPLIER S
public static final field TA_KEY_FONT_STYLE S
public static final field TA_KEY_FONT_VARIANT S
public static final field TA_KEY_FONT_WEIGHT S
public static final field TA_KEY_FOREGROUND_COLOR S
public static final field TA_KEY_IS_HIGHLIGHTED S
public static final field TA_KEY_LAYOUT_DIRECTION S
public static final field TA_KEY_LETTER_SPACING S
public static final field TA_KEY_LINE_BREAK_STRATEGY S
public static final field TA_KEY_LINE_HEIGHT S
public static final field TA_KEY_OPACITY S
public static final field TA_KEY_ROLE S
public static final field TA_KEY_TEXT_DECORATION_COLOR S
public static final field TA_KEY_TEXT_DECORATION_LINE S
public static final field TA_KEY_TEXT_DECORATION_STYLE S
public static final field TA_KEY_TEXT_SHADOW_COLOR S
public static final field TA_KEY_TEXT_SHADOW_OFFSET_DX S
public static final field TA_KEY_TEXT_SHADOW_OFFSET_DY S
Expand Down
22 changes: 0 additions & 22 deletions ...t-native/ReactAndroid/src/main/java/com/facebook/react/views/text/TextAttributeProps.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,26 +36,19 @@ public class TextAttributeProps implements EffectiveTextAttributeProvider {
// constants for Text Attributes serialization
public static final short TA_KEY_FOREGROUND_COLOR = 0;
public static final short TA_KEY_BACKGROUND_COLOR = 1;
public static final short TA_KEY_OPACITY = 2;
public static final short TA_KEY_FONT_FAMILY = 3;
public static final short TA_KEY_FONT_SIZE = 4;
public static final short TA_KEY_FONT_SIZE_MULTIPLIER = 5;
public static final short TA_KEY_FONT_WEIGHT = 6;
public static final short TA_KEY_FONT_STYLE = 7;
public static final short TA_KEY_FONT_VARIANT = 8;
public static final short TA_KEY_ALLOW_FONT_SCALING = 9;
public static final short TA_KEY_LETTER_SPACING = 10;
public static final short TA_KEY_LINE_HEIGHT = 11;
public static final short TA_KEY_ALIGNMENT = 12;
public static final short TA_KEY_BEST_WRITING_DIRECTION = 13;
public static final short TA_KEY_TEXT_DECORATION_COLOR = 14;
public static final short TA_KEY_TEXT_DECORATION_LINE = 15;
public static final short TA_KEY_TEXT_DECORATION_STYLE = 16;
public static final short TA_KEY_TEXT_SHADOW_RADIUS = 18;
public static final short TA_KEY_TEXT_SHADOW_COLOR = 19;
public static final short TA_KEY_TEXT_SHADOW_OFFSET_DX = 20;
public static final short TA_KEY_TEXT_SHADOW_OFFSET_DY = 21;
public static final short TA_KEY_IS_HIGHLIGHTED = 22;
public static final short TA_KEY_LAYOUT_DIRECTION = 23;
public static final short TA_KEY_ACCESSIBILITY_ROLE = 24;
public static final short TA_KEY_LINE_BREAK_STRATEGY = 25;
Expand Down Expand Up @@ -144,7 +137,6 @@ private TextAttributeProps() {}
public static TextAttributeProps fromMapBuffer(MapBuffer props) {
TextAttributeProps result = new TextAttributeProps();

// TODO T83483191: Review constants that are not being set!
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're deserializing text attributes of a fragment. Some attributes are not handled on the fragment level but apply to the whole Text element. I believe there's not much more to investigate here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Relevant:

react-native/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/TextAttributeProps.java

Lines 31 to 33 in 27f3810

// TODO: T63643819 refactor naming of TextAttributeProps to make explicit that this represents
// TextAttributes and not TextProps. As part of this refactor extract methods that don't belong to
// TextAttributeProps (e.g. TextAlign)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The fact that we currently don't do what this comment describes is blocking me.

I'd like to...

  • Split TextAttributes (C++) to TextAttributes (<Text>-level properties) and FragmentAttributes (fragment-level properties)
  • Refactor TextAttributeProps (Java) to FragmentAttributes

Currently, one is a subset of another; not modeling them separately is at most a code smell.

I'd like them to be two slightly different sets (sharing a great common subset), so not modeling them separately is a problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, this doesn't seem correct, thanks for cleaning up

Iterator<MapBuffer.Entry> iterator = props.iterator();
while (iterator.hasNext()) {
MapBuffer.Entry entry = iterator.next();
Expand All @@ -155,16 +147,12 @@ public static TextAttributeProps fromMapBuffer(MapBuffer props) {
case TA_KEY_BACKGROUND_COLOR:
result.setBackgroundColor(entry.getIntValue());
break;
case TA_KEY_OPACITY:
break;
case TA_KEY_FONT_FAMILY:
result.setFontFamily(entry.getStringValue());
break;
case TA_KEY_FONT_SIZE:
result.setFontSize((float) entry.getDoubleValue());
break;
case TA_KEY_FONT_SIZE_MULTIPLIER:
break;
case TA_KEY_FONT_WEIGHT:
result.setFontWeight(entry.getStringValue());
break;
Expand All @@ -183,17 +171,9 @@ public static TextAttributeProps fromMapBuffer(MapBuffer props) {
case TA_KEY_LINE_HEIGHT:
result.setLineHeight((float) entry.getDoubleValue());
break;
case TA_KEY_ALIGNMENT:
break;
case TA_KEY_BEST_WRITING_DIRECTION:
break;
case TA_KEY_TEXT_DECORATION_COLOR:
break;
case TA_KEY_TEXT_DECORATION_LINE:
result.setTextDecorationLine(entry.getStringValue());
break;
case TA_KEY_TEXT_DECORATION_STYLE:
break;
case TA_KEY_TEXT_SHADOW_RADIUS:
result.setTextShadowRadius((float) entry.getDoubleValue());
break;
Expand All @@ -206,8 +186,6 @@ public static TextAttributeProps fromMapBuffer(MapBuffer props) {
case TA_KEY_TEXT_SHADOW_OFFSET_DY:
result.setTextShadowOffsetDy((float) entry.getDoubleValue());
break;
case TA_KEY_IS_HIGHLIGHTED:
break;
case TA_KEY_LAYOUT_DIRECTION:
result.setLayoutDirection(entry.getStringValue());
break;
Expand Down
60 changes: 0 additions & 60 deletions packages/react-native/ReactCommon/react/renderer/attributedstring/conversions.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -813,18 +813,12 @@ inline folly::dynamic toDynamic(const TextAttributes& textAttributes) {
_textAttributes(
"backgroundColor", toAndroidRepr(textAttributes.backgroundColor));
}
if (!std::isnan(textAttributes.opacity)) {
_textAttributes("opacity", textAttributes.opacity);
}
if (!textAttributes.fontFamily.empty()) {
_textAttributes("fontFamily", textAttributes.fontFamily);
}
if (!std::isnan(textAttributes.fontSize)) {
_textAttributes("fontSize", textAttributes.fontSize);
}
if (!std::isnan(textAttributes.fontSizeMultiplier)) {
_textAttributes("fontSizeMultiplier", textAttributes.fontSizeMultiplier);
}
if (textAttributes.fontWeight.has_value()) {
_textAttributes("fontWeight", toString(*textAttributes.fontWeight));
}
Expand All @@ -846,31 +840,15 @@ inline folly::dynamic toDynamic(const TextAttributes& textAttributes) {
if (!std::isnan(textAttributes.lineHeight)) {
_textAttributes("lineHeight", textAttributes.lineHeight);
}
if (textAttributes.alignment.has_value()) {
_textAttributes("alignment", toString(*textAttributes.alignment));
}
if (textAttributes.baseWritingDirection.has_value()) {
_textAttributes(
"baseWritingDirection", toString(*textAttributes.baseWritingDirection));
}
if (textAttributes.lineBreakStrategy.has_value()) {
_textAttributes(
"lineBreakStrategyIOS", toString(*textAttributes.lineBreakStrategy));
}
// Decoration
if (textAttributes.textDecorationColor) {
_textAttributes(
"textDecorationColor",
toAndroidRepr(textAttributes.textDecorationColor));
}
if (textAttributes.textDecorationLineType.has_value()) {
_textAttributes(
"textDecorationLine", toString(*textAttributes.textDecorationLineType));
}
if (textAttributes.textDecorationStyle.has_value()) {
_textAttributes(
"textDecorationStyle", toString(*textAttributes.textDecorationStyle));
}
// Shadow
// textShadowOffset = textAttributes.textShadowOffset.has_value() ?
// textAttributes.textShadowOffset.value() : textShadowOffset;
Expand All @@ -882,9 +860,6 @@ inline folly::dynamic toDynamic(const TextAttributes& textAttributes) {
"textShadowColor", toAndroidRepr(textAttributes.textShadowColor));
}
// Special
if (textAttributes.isHighlighted.has_value()) {
_textAttributes("isHighlighted", *textAttributes.isHighlighted);
}
if (textAttributes.layoutDirection.has_value()) {
_textAttributes(
"layoutDirection", toString(*textAttributes.layoutDirection));
Expand Down Expand Up @@ -950,26 +925,19 @@ constexpr static MapBuffer::Key FR_KEY_TEXT_ATTRIBUTES = 5;
// constants for Text Attributes serialization
constexpr static MapBuffer::Key TA_KEY_FOREGROUND_COLOR = 0;
constexpr static MapBuffer::Key TA_KEY_BACKGROUND_COLOR = 1;
constexpr static MapBuffer::Key TA_KEY_OPACITY = 2;
constexpr static MapBuffer::Key TA_KEY_FONT_FAMILY = 3;
constexpr static MapBuffer::Key TA_KEY_FONT_SIZE = 4;
constexpr static MapBuffer::Key TA_KEY_FONT_SIZE_MULTIPLIER = 5;
constexpr static MapBuffer::Key TA_KEY_FONT_WEIGHT = 6;
constexpr static MapBuffer::Key TA_KEY_FONT_STYLE = 7;
constexpr static MapBuffer::Key TA_KEY_FONT_VARIANT = 8;
constexpr static MapBuffer::Key TA_KEY_ALLOW_FONT_SCALING = 9;
constexpr static MapBuffer::Key TA_KEY_LETTER_SPACING = 10;
constexpr static MapBuffer::Key TA_KEY_LINE_HEIGHT = 11;
constexpr static MapBuffer::Key TA_KEY_ALIGNMENT = 12;
constexpr static MapBuffer::Key TA_KEY_BEST_WRITING_DIRECTION = 13;
constexpr static MapBuffer::Key TA_KEY_TEXT_DECORATION_COLOR = 14;
constexpr static MapBuffer::Key TA_KEY_TEXT_DECORATION_LINE = 15;
constexpr static MapBuffer::Key TA_KEY_TEXT_DECORATION_STYLE = 16;
constexpr static MapBuffer::Key TA_KEY_TEXT_SHADOW_RADIUS = 18;
constexpr static MapBuffer::Key TA_KEY_TEXT_SHADOW_COLOR = 19;
constexpr static MapBuffer::Key TA_KEY_TEXT_SHADOW_OFFSET_DX = 20;
constexpr static MapBuffer::Key TA_KEY_TEXT_SHADOW_OFFSET_DY = 21;
constexpr static MapBuffer::Key TA_KEY_IS_HIGHLIGHTED = 22;
constexpr static MapBuffer::Key TA_KEY_LAYOUT_DIRECTION = 23;
constexpr static MapBuffer::Key TA_KEY_ACCESSIBILITY_ROLE = 24;
constexpr static MapBuffer::Key TA_KEY_LINE_BREAK_STRATEGY = 25;
Expand Down Expand Up @@ -1036,19 +1004,12 @@ inline MapBuffer toMapBuffer(const TextAttributes& textAttributes) {
builder.putInt(
TA_KEY_BACKGROUND_COLOR, toAndroidRepr(textAttributes.backgroundColor));
}
if (!std::isnan(textAttributes.opacity)) {
builder.putDouble(TA_KEY_OPACITY, textAttributes.opacity);
}
if (!textAttributes.fontFamily.empty()) {
builder.putString(TA_KEY_FONT_FAMILY, textAttributes.fontFamily);
}
if (!std::isnan(textAttributes.fontSize)) {
builder.putDouble(TA_KEY_FONT_SIZE, textAttributes.fontSize);
}
if (!std::isnan(textAttributes.fontSizeMultiplier)) {
builder.putDouble(
TA_KEY_FONT_SIZE_MULTIPLIER, textAttributes.fontSizeMultiplier);
}
if (textAttributes.fontWeight.has_value()) {
builder.putString(TA_KEY_FONT_WEIGHT, toString(*textAttributes.fontWeight));
}
Expand All @@ -1069,14 +1030,6 @@ inline MapBuffer toMapBuffer(const TextAttributes& textAttributes) {
if (!std::isnan(textAttributes.lineHeight)) {
builder.putDouble(TA_KEY_LINE_HEIGHT, textAttributes.lineHeight);
}
if (textAttributes.alignment.has_value()) {
builder.putString(TA_KEY_ALIGNMENT, toString(*textAttributes.alignment));
}
if (textAttributes.baseWritingDirection.has_value()) {
builder.putString(
TA_KEY_BEST_WRITING_DIRECTION,
toString(*textAttributes.baseWritingDirection));
}
if (textAttributes.lineBreakStrategy.has_value()) {
builder.putString(
TA_KEY_LINE_BREAK_STRATEGY,
Expand All @@ -1088,21 +1041,11 @@ inline MapBuffer toMapBuffer(const TextAttributes& textAttributes) {
}

// Decoration
if (textAttributes.textDecorationColor) {
builder.putInt(
TA_KEY_TEXT_DECORATION_COLOR,
toAndroidRepr(textAttributes.textDecorationColor));
}
if (textAttributes.textDecorationLineType.has_value()) {
builder.putString(
TA_KEY_TEXT_DECORATION_LINE,
toString(*textAttributes.textDecorationLineType));
}
if (textAttributes.textDecorationStyle.has_value()) {
builder.putString(
TA_KEY_TEXT_DECORATION_STYLE,
toString(*textAttributes.textDecorationStyle));
}

// Shadow
if (!std::isnan(textAttributes.textShadowRadius)) {
Expand All @@ -1121,9 +1064,6 @@ inline MapBuffer toMapBuffer(const TextAttributes& textAttributes) {
TA_KEY_TEXT_SHADOW_OFFSET_DY, textAttributes.textShadowOffset->height);
}
// Special
if (textAttributes.isHighlighted.has_value()) {
builder.putBool(TA_KEY_IS_HIGHLIGHTED, *textAttributes.isHighlighted);
}
if (textAttributes.layoutDirection.has_value()) {
builder.putString(
TA_KEY_LAYOUT_DIRECTION, toString(*textAttributes.layoutDirection));
Expand Down