-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Consider pixel density in coordinate<->point conversion (Android) #2390
Conversation
@@ -142,12 +142,14 @@ public void onSnapshotReady(@Nullable Bitmap snapshot) { | |||
|
|||
@ReactMethod | |||
public void pointForCoordinate(final int tag, ReadableMap coordinate, final Promise promise) { | |||
final ReactApplicationContext context = getReactApplicationContext(); | |||
double density = (double) context.getResources().getDisplayMetrics().density; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I applied these changes to my local node module and was unable to build for android.
I believe this line needs read final double density = (double) [context.getResources().getDisplayMetrics().density;
Once I made this change to the local file, I was able to build. I can also confirm that the PR's code does fix the issue described for android.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 146 needs to be declared with "final" otherwise the build fails with a scope error.
Thanks, fixed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can validate that this PR solves the inaccuracy problem on Android, and does not affect IOS. Everything works as expected. We are using this updated coordinateForPoint in production now. (we have pinned a master version and added critical bug fixes like this).
There are no releases since April, seems like the guys are busy :-) |
LGTM @alvelig 🐽 |
LGTM |
Does any other open PR do the same thing?
Nope
What issue is this PR fixing?
pointForCoordinate
andcoordinateForPoint
use raw pixel values, which is inconsistent with other functions and with iOS implementation.How did you test this PR?
This affects Android code only, and was tested locally in a sample app.