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
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,28 @@ public class PropsNode extends Node implements FinalNode {
private final JavaOnlyMap mPropMap;
private final ReactStylesDiffMap mDiffMap;

private void addProp(WritableMap propMap, String key, Object value) {
wcandillon marked this conversation as resolved.
Show resolved Hide resolved
if (value == null) {
propMap.putNull(key);
} else if (value instanceof Double) {
propMap.putDouble(key, (Double) value);
} else if (value instanceof Integer) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that there's no case when it's Integer coz JS's number is serialized to Double, no?

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 can you check if the latest version of the patch is in order? We now look at the value type instead of its Java instance.

propMap.putInt(key, (Integer) value);
} else if (value instanceof Number) {
propMap.putDouble(key, ((Number) value).doubleValue());
} else if (value instanceof Boolean) {
propMap.putBoolean(key, (Boolean) value);
} else if (value instanceof String) {
propMap.putString(key, (String) value);
} else if (value instanceof WritableArray) {
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.

Also, it's never another map, no? Boolean, Number, Array and String are enough.

Can we reuse it here https://github.com/kmagiera/react-native-reanimated/pull/311/files#diff-4f72bb0bf303947e60ceb110eb069188R99? It appears to be a similar code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. Can it be Array though then? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are correct!

propMap.putArray(key, (WritableArray)value);
} else if (value instanceof WritableMap) {
propMap.putMap(key, (WritableMap)value);
} else {
throw new IllegalStateException("Unknown type of animated value");
}
}

public PropsNode(
int nodeID,
ReadableMap config,
Expand Down Expand Up @@ -89,12 +111,13 @@ protected Double evaluate() {
}
} else {
String key = entry.getKey();
Object value = 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 @@ -334,13 +334,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