Skip to content
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.

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

fix(🐛): Fix Android bug when dealing with string animated values #311

Merged
merged 9 commits into from
Sep 26, 2019
37 changes: 21 additions & 16 deletions android/src/main/java/com/swmansion/reanimated/nodes/PropsNode.java
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,23 @@ public class PropsNode extends Node implements FinalNode {
private final JavaOnlyMap mPropMap;
private final ReactStylesDiffMap mDiffMap;

private void addProp(WritableMap propMap, String key, WritableMap value) {
ReadableType type = value.getType(key);
switch (type) {
case Number:
propMap.putDouble(key, value.getDouble(key));
break;
case String:
propMap.putString(key, value.getString(key));
break;
case Array:
propMap.putArray(key, (WritableArray) value.getArray(key));
break;
default:
throw new IllegalArgumentException("Unexpected type " + type);
}
}

public PropsNode(
int nodeID,
ReadableMap config,
Expand Down Expand Up @@ -72,29 +89,17 @@ protected Double evaluate() {
hasJSProps = true;
dest = jsProps;
}
ReadableType type = style.getType(key);
switch (type) {
case Number:
dest.putDouble(key, style.getDouble(key));
break;
case String:
dest.putString(key, style.getString(key));
break;
case Array:
dest.putArray(key, (WritableArray) style.getArray(key));
break;
default:
throw new IllegalArgumentException("Unexpected type " + type);
}
addProp(dest, key, style);
}
} else {
String key = entry.getKey();
WritableMap value = (WritableMap)node.value();
if (mNodesManager.uiProps.contains(key)) {
hasUIProps = true;
mPropMap.putDouble(key, node.doubleValue());
addProp(mPropMap, key, value);
} else {
hasNativeProps = true;
nativeProps.putDouble(key, node.doubleValue());
addProp(nativeProps, key, value);
}
}
}
Expand Down
8 changes: 1 addition & 7 deletions react-native-reanimated.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -312,13 +312,7 @@ declare module 'react-native-reanimated' {
): void

// configuration

// `addWhitelistedNativeProps` will likely be removed soon, and so is
// intentionally not exposed to TypeScript. If it is needed, it could be
// uncommented here, or just use
// `(Animated as any).addWhitelistedNativeProps({ myProp: true });`

// addWhitelistedNativeProps(props: { [key: string]: true }): void;
export function addWhitelistedNativeProps(props: { [key: string]: true }): void;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@msand I did not need to use this function in order to make the path animation work on Android. Is it a performance optimization to use it?

Copy link
Contributor

Choose a reason for hiding this comment

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

@wcandillon I think without this the change might go from native to js and back to native, rather than stay native. But, I'm not sure, haven't read the latest source, nor placed any debug breakpoints to test this hypothesis.

Copy link
Contributor

@osdnk osdnk Jun 23, 2019

Choose a reason for hiding this comment

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

So yes, animating string like SVG path includes JS trip, but it's ok, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or no, it doesn't. Hmmm, need to check!

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, yes it's animating with native drivers as well 😅

}

export default Animated;
Expand Down