Skip to content

[Android] store borderColor in a non lossy way #9380

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
Closed
manicakes wants to merge 2 commits into from
Closed

[Android] store borderColor in a non lossy way #9380

manicakes wants to merge 2 commits into from

Conversation

@manicakes
Copy link

@manicakes manicakes commented Aug 12, 2016 •
edited
Loading

Fix for issue #3652 -

Converting from int to float is lossy for very large numbers, so storing borderColor as a single Spacing object (which uses float) was not working for certain colors.

So, this pull request splits borderColor into alpha and RGB components, and stores each of these as their own respective Spacing objects.

Test Plan

Check out @cosmith sample code here that triggers the bug: https://rnplay.org/apps/l1bw2A

What currently looks like this:

screen shot 2016-08-13 at 6 22 28 pm

Should look like this (with my fix applied):

screen shot 2016-08-13 at 6 20 08 pm

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Aug 12, 2016

By analyzing the blame information on this pull request, we identified @andreicoman11 and @foghina to be potential reviewers.

@facebook-github-bot facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Aug 12, 2016
@manicakes manicakes changed the title Store Spacing values internally as doubles instead of floats, otherwi… [Android] store borderColor as double instead of long. Aug 12, 2016
@manicakes manicakes mentioned this pull request Aug 12, 2016
Closed
@ghost ghost 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 Aug 12, 2016
@ghost
Copy link

ghost commented Aug 12, 2016

@manicakes updated the pull request.

2 similar comments
@ghost
Copy link

ghost commented Aug 12, 2016

@manicakes updated the pull request.

@ghost
Copy link

ghost commented Aug 12, 2016

@manicakes updated the pull request.

@manicakes manicakes force-pushed the fix_android_bordercolor branch from eb51e7a to 73c8f7a Compare August 12, 2016 21:41
@ghost
Copy link

ghost commented Aug 12, 2016

@manicakes updated the pull request.

@manicakes
Copy link
Author

manicakes commented Aug 12, 2016

@andreicoman11 and @foghina looking forward to your feedback - thanks!

@ghost
Copy link

ghost commented Aug 12, 2016

@manicakes updated the pull request.

@ghost ghost 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 Aug 12, 2016
@manicakes manicakes force-pushed the fix_android_bordercolor branch from 4b8d3c6 to a7121db Compare August 12, 2016 22:43
@ghost
Copy link

ghost commented Aug 12, 2016

@manicakes updated the pull request.

1 similar comment
@ghost
Copy link

ghost commented Aug 12, 2016

@manicakes updated the pull request.

@ghost ghost 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 Aug 12, 2016
@manicakes manicakes changed the title [Android] store borderColor as double instead of long. [Android] store borderColor in a non lossy way Aug 13, 2016
@ghost ghost 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 Aug 13, 2016
foghina
foghina reviewed Aug 13, 2016
View reviewed changes
ReactAndroid/src/main/java/com/facebook/react/views/view/ReactViewBackgroundDrawable.java
*/
/* package */ class ReactViewBackgroundDrawable extends Drawable {

private static final int DEFAULT_BORDER_COLOR = Color.BLACK;
Copy link
Contributor

@foghina foghina Aug 13, 2016

Choose a reason for hiding this comment

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

Is this still used anywhere? Can we remove it?

Copy link
Author

@manicakes manicakes Aug 13, 2016

Choose a reason for hiding this comment

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

It's used in the subsequent lines, as I split out the rgb and alpha components. Hope that makes sense.

Copy link
Contributor

@foghina foghina Aug 15, 2016

Choose a reason for hiding this comment

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

Ah, I must be blind. 👓

@foghina foghina self-assigned this Aug 13, 2016
@foghina
Copy link
Contributor

foghina commented Aug 13, 2016

Sweet, thanks for picking this up! Can you also provide a test plan for your PR? E.g. a screenshot of the sample app from #3652 running correctly.

@ghost ghost 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 Aug 13, 2016
@manicakes
Copy link
Author

manicakes commented Aug 13, 2016

@foghina no prob! Will include test plan and update the PR shortly.

@ghost ghost 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 Aug 13, 2016
Storing borderColor in a single Spacing object was not working as…
9fbe19d 
… converting `int` to `float` is lossy.

So, we first split `borderColor` into RGB and Alpha values, and store each as its own `Spacing` object.
@manicakes manicakes force-pushed the fix_android_bordercolor branch from a7121db to 9fbe19d Compare August 13, 2016 22:18
@ghost
Copy link

ghost commented Aug 13, 2016

@manicakes updated the pull request.

@manicakes
Copy link
Author

manicakes commented Aug 13, 2016

@foghina updated - thanks for the feedback.

@ghost ghost 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 Aug 13, 2016
@foghina
Copy link
Contributor

foghina commented Aug 15, 2016

@facebook-github-bot shipit

awesomesauce

@ghost ghost 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 Aug 15, 2016
@ghost
Copy link

ghost commented Aug 15, 2016

@manicakes updated the pull request.

@ghost ghost 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 Aug 15, 2016
@foghina
Copy link
Contributor

foghina commented Aug 15, 2016

Hmm, something didn't work. Lemme try again.

@facebook-github-bot shipit pls

@ghost ghost 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 Aug 15, 2016
@manicakes
Copy link
Author

manicakes commented Aug 15, 2016

Hmmm @foghina maybe the shipit command has to be the only content in the comment for it to work? :shipit:

@foghina
Copy link
Contributor

foghina commented Aug 15, 2016

It usually works with other stuff in there as well, but yeah lemme try.

@foghina
Copy link
Contributor

foghina commented Aug 15, 2016

@facebook-github-bot shipit

@ghost ghost added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed labels Aug 15, 2016
@ghost
Copy link

ghost commented Aug 15, 2016

Thanks for importing.If you are an FB employee go to Phabricator to review internal test results.

@foghina
Copy link
Contributor

foghina commented Aug 15, 2016

Yay. We should make "shipit pls" a command though.

@ghost ghost 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 Aug 15, 2016
@ghost ghost closed this in 8095707 Aug 15, 2016
LearningDave pushed a commit to LearningDave/react-native that referenced this pull request Aug 16, 2016
@LearningDave
store borderColor in a non lossy way
ae4b870 
Summary:
Fix for issue facebook#3652 -

Converting from `int` to `float` is lossy for very large numbers, so storing `borderColor` as a single `Spacing` object (which uses `float`) was not working for certain colors.

So, this pull request splits `borderColor` into alpha and RGB components, and stores each of these as their own respective `Spacing` objects.

*Test Plan*

Check out cosmith sample code here that triggers the bug: https://rnplay.org/apps/l1bw2A

What currently looks like this:

<img width="548" alt="screen shot 2016-08-13 at 6 22 28 pm" src="https://cloud.githubusercontent.com/assets/1630466/17645965/9346f05e-6183-11e6-8d40-3e458b08fd9a.png">

Should look like this (with my fix applied):

<img width="543" alt="screen shot 2016-08-13 at 6 20 08 pm" src="https://cloud.githubusercontent.com/assets/1630466/17645968/9c26d1d0-6183-11e6-8759-75a5e99f498a.png">
Closes facebook#9380

Differential Revision: D3716707

Pulled By: foghina

fbshipit-source-id: 1164378112e2a58d43c8f5fc671c2efdb64b412b
mpretty-cyro pushed a commit to HomePass/react-native that referenced this pull request Aug 25, 2016
store borderColor in a non lossy way
353eb08 
Summary:
Fix for issue facebook#3652 -

Converting from `int` to `float` is lossy for very large numbers, so storing `borderColor` as a single `Spacing` object (which uses `float`) was not working for certain colors.

So, this pull request splits `borderColor` into alpha and RGB components, and stores each of these as their own respective `Spacing` objects.

*Test Plan*

Check out cosmith sample code here that triggers the bug: https://rnplay.org/apps/l1bw2A

What currently looks like this:

<img width="548" alt="screen shot 2016-08-13 at 6 22 28 pm" src="https://cloud.githubusercontent.com/assets/1630466/17645965/9346f05e-6183-11e6-8d40-3e458b08fd9a.png">

Should look like this (with my fix applied):

<img width="543" alt="screen shot 2016-08-13 at 6 20 08 pm" src="https://cloud.githubusercontent.com/assets/1630466/17645968/9c26d1d0-6183-11e6-8759-75a5e99f498a.png">
Closes facebook#9380

Differential Revision: D3716707

Pulled By: foghina

fbshipit-source-id: 1164378112e2a58d43c8f5fc671c2efdb64b412b
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@foghina foghina

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Import Started This pull request has been imported. This does not imply the PR has been approved.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@manicakes @facebook-github-bot @foghina