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.

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

Jump to bottom

Fix Native Rotation Android #18872

Closed
wants to merge 3 commits into from
Closed

Fix Native Rotation Android #18872

wants to merge 3 commits into from

Conversation

scisci
Copy link
Contributor

@scisci scisci commented Apr 16, 2018 •
edited
Loading

Fixes #14161
Android crashes in some cases if an animated transform config contains a string value, like a rotation.
This PR fixes that by ensuring all values sent to the native side are doubles. It adds __transformDataType to AnimatedTransform.js.

Test Plan

Added integration test ReactAndroid/src/androidText/js/AnimatedTransformTestModule.js This test fails with the following error INSTRUMENTATION_RESULT: longMsg=java.lang.ClassCastException: java.lang.String cannot be cast to java.lang.Double, if the changes to AnimatedTransform.js are reverted.

changelog

[Android] [Fixed] - Fixes Android crash on animated style with string rotation

@scisci scisci changed the title adds __transformDataType to AnimatedTransform Fix Native Rotation Android Apr 16, 2018
@scisci scisci mentioned this pull request Apr 16, 2018
Closed
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 16, 2018
@facebook-github-bot
Copy link
Contributor

@scisci I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

@scisci
Copy link
Contributor Author

scisci commented May 17, 2018 •
edited
Loading

@janicduplessis

@henrikra
Copy link

henrikra commented Jun 4, 2018

I am waiting for this PR :)

@facebook-github-bot
Copy link
Contributor

@scisci I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

janicduplessis
janicduplessis suggested changes Jan 29, 2019
View reviewed changes
Copy link
Contributor

@janicduplessis janicduplessis left a comment

Choose a reason for hiding this comment

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

Awesome work, sorry this got lost in my github notifications.

Just one small change to avoid some duplication of the deg parsing logic.

Libraries/Animated/src/nodes/AnimatedTransform.js Outdated Show resolved Hide resolved
@cpojer
Copy link
Contributor

cpojer commented Jan 30, 2019

@scisci it seems there are some problems with your PR. Would you mind rebasing it on master and making sure the JS tests pass?

@scisci scisci force-pushed the fix-native-rotation-android branch from 1ceaf73 to 243ca50 Compare January 30, 2019 21:41
analysis-bot
analysis-bot suggested changes Jan 30, 2019
View reviewed changes
Copy link

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

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

Code analysis results:

  • eslint found some issues.

Libraries/Animated/src/NativeAnimatedHelper.js Outdated Show resolved Hide resolved
ReactAndroid/src/androidTest/js/AnimatedTransformTestModule.js Outdated Show resolved Hide resolved
ReactAndroid/src/androidTest/js/AnimatedTransformTestModule.js Outdated Show resolved Hide resolved
ReactAndroid/src/androidTest/js/AnimatedTransformTestModule.js Outdated Show resolved Hide resolved
ReactAndroid/src/androidTest/js/AnimatedTransformTestModule.js Outdated Show resolved Hide resolved
ReactAndroid/src/androidTest/js/AnimatedTransformTestModule.js Outdated Show resolved Hide resolved
ReactAndroid/src/androidTest/js/AnimatedTransformTestModule.js Outdated Show resolved Hide resolved
ReactAndroid/src/androidTest/js/AnimatedTransformTestModule.js Outdated Show resolved Hide resolved
@scisci
Copy link
Contributor Author

scisci commented Jan 30, 2019

@cpojer, I rebased and tests are now passing

@janicduplessis
Copy link
Contributor

@scisci Could you just address the lint issues that the bot pointed out? Especially the one about prettier since it might block landing this.

@scisci
Copy link
Contributor Author

scisci commented Jan 31, 2019

@janicduplessis I've fixed the eslint errors.

cpojer
cpojer approved these changes Jan 31, 2019
View reviewed changes
Copy link
Contributor

@cpojer cpojer left a comment

Choose a reason for hiding this comment

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

Thanks so much for this fix, and bonus points to you for writing a test!

@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Jan 31, 2019
facebook-github-bot
facebook-github-bot approved these changes Jan 31, 2019
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Collaborator

@scisci merged commit e405e84 into facebook:master.

@facebook facebook locked as resolved and limited conversation to collaborators Jan 31, 2019
@react-native-bot react-native-bot added the Merged This PR has been merged. label Jan 31, 2019
grabbou pushed a commit that referenced this pull request Feb 4, 2019
@scisci @grabbou
Fix Native Rotation Android (#18872)
c3bd341 
Summary:
Fixes #14161
Android crashes in some cases if an animated transform config contains a string value, like a rotation.
This PR fixes that by ensuring all values sent to the native side are doubles. It adds `__transformDataType` to AnimatedTransform.js.

Added integration test `ReactAndroid/src/androidText/js/AnimatedTransformTestModule.js` This test fails with the following error `INSTRUMENTATION_RESULT: longMsg=java.lang.ClassCastException: java.lang.String cannot be cast to java.lang.Double`, if the changes to AnimatedTransform.js are reverted.

[Android] [Fixed] - Fixes Android crash on animated style with string rotation
Pull Request resolved: #18872

Differential Revision: D13894676

Pulled By: cpojer

fbshipit-source-id: 297e8132563460802e53f3ac551c3ba9ed943736
@hramos hramos removed the Import Started This pull request has been imported. This does not imply the PR has been approved. label Feb 6, 2019
@hramos hramos removed the Import Started This pull request has been imported. This does not imply the PR has been approved. label Feb 6, 2019
matt-oakes pushed a commit to matt-oakes/react-native that referenced this pull request Feb 7, 2019
@scisci @facebook-github-bot
Fix Native Rotation Android (facebook#18872)
be0a159 
Summary:
Fixes facebook#14161
Android crashes in some cases if an animated transform config contains a string value, like a rotation.
This PR fixes that by ensuring all values sent to the native side are doubles. It adds `__transformDataType` to AnimatedTransform.js.

Added integration test `ReactAndroid/src/androidText/js/AnimatedTransformTestModule.js` This test fails with the following error `INSTRUMENTATION_RESULT: longMsg=java.lang.ClassCastException: java.lang.String cannot be cast to java.lang.Double`, if the changes to AnimatedTransform.js are reverted.

[Android] [Fixed] - Fixes Android crash on animated style with string rotation
Pull Request resolved: facebook#18872

Differential Revision: D13894676

Pulled By: cpojer

fbshipit-source-id: 297e8132563460802e53f3ac551c3ba9ed943736
t-nanava pushed a commit to microsoft/react-native-macos that referenced this pull request Jun 17, 2019
@scisci @grabbou
Fix Native Rotation Android (facebook#18872)
193cb1f 
Summary:
Fixes facebook#14161
Android crashes in some cases if an animated transform config contains a string value, like a rotation.
This PR fixes that by ensuring all values sent to the native side are doubles. It adds `__transformDataType` to AnimatedTransform.js.

Added integration test `ReactAndroid/src/androidText/js/AnimatedTransformTestModule.js` This test fails with the following error `INSTRUMENTATION_RESULT: longMsg=java.lang.ClassCastException: java.lang.String cannot be cast to java.lang.Double`, if the changes to AnimatedTransform.js are reverted.

[Android] [Fixed] - Fixes Android crash on animated style with string rotation
Pull Request resolved: facebook#18872

Differential Revision: D13894676

Pulled By: cpojer

fbshipit-source-id: 297e8132563460802e53f3ac551c3ba9ed943736
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@analysis-bot analysis-bot analysis-bot requested changes

@cpojer cpojer cpojer approved these changes

@janicduplessis janicduplessis janicduplessis approved these changes

@facebook-github-bot facebook-github-bot facebook-github-bot approved these changes

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Platform: Android Android applications.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Error when using rotation transform and "useNativeDriver: true" in Android
8 participants
@scisci @facebook-github-bot @henrikra @cpojer @janicduplessis @react-native-bot @analysis-bot @hramos