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: Android User Location / Camera issues #3363

Merged
merged 3 commits into from Feb 13, 2024
Merged

Fix: Android User Location / Camera issues #3363

merged 3 commits into from Feb 13, 2024

Conversation

mysport12
Copy link
Contributor

Description

  • Fixed puck images not being fetched after initial addToMap
  • Fixed puck images not being correctly applied when changed on the fly
  • Fixed camera follow props not properly accepting null/undefined values
  • Fixed crash in ExpressionParser by wrapping in a try/catch

Checklist

  • I've read CONTRIBUTING.md
  • I updated the doc/other generated code with running yarn generate in the root folder
  • I have tested the new feature on /example app.
    • In V11 mode/ios
    • In New Architecture mode/ios
    • In V11 mode/android
    • In New Architecture mode/android
  • I added/updated a sample - if a new feature was implemented (/example)

@mysport12
Android User Location/ Camera Fixes
d199e11 
- Fixed puck images not being fetched after initial addToMap
- Fixed puck images not being correctly applied when changed on the fly
- Fixed camera follow props not properly accepting null/undefined values
- Fixed crash in ExpressionParser by wrapping in a try/catch
@mysport12 mysport12 temporarily deployed to CI with Mapbox Tokens February 7, 2024 16:12 — with GitHub Actions Inactive
@mysport12 mysport12 temporarily deployed to CI with Mapbox Tokens February 7, 2024 16:12 — with GitHub Actions Inactive
@mysport12 mysport12 temporarily deployed to CI with Mapbox Tokens February 7, 2024 16:12 — with GitHub Actions Inactive
@mysport12 mysport12 temporarily deployed to CI with Mapbox Tokens February 7, 2024 16:12 — with GitHub Actions Inactive
@mysport12 mysport12 temporarily deployed to CI with Mapbox Tokens February 7, 2024 16:12 — with GitHub Actions Inactive
mysport12
mysport12 commented Feb 7, 2024
View reviewed changes
android/src/main/java/com/rnmapbox/rnmbx/components/location/RNMBXNativeUserLocation.kt
removeSubscriptions()
val imageManager = map.imageManager
this.imageManager = imageManager
_apply()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mfazekas There is an explicit _apply() call here because unlike on iOS when the new subscriptions are set up below this line, an initial call is not made to apply the changes

Copy link
Contributor

Choose a reason for hiding this comment

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

@mysport12 is there a reason we can't keep the swift and Kotlin in sync?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The implementations for this component are in sync between Swift and Kotlin, minus this one line. I didn't go down the rabbit hole to understand the differences between the iOS and Android image managers and why iOS calls the handler on init and Android doesn't.

@mysport12 mysport12 temporarily deployed to CI with Mapbox Tokens February 7, 2024 21:46 — with GitHub Actions Inactive
@mysport12 mysport12 temporarily deployed to CI with Mapbox Tokens February 7, 2024 21:46 — with GitHub Actions Inactive
@mysport12 mysport12 temporarily deployed to CI with Mapbox Tokens February 7, 2024 21:46 — with GitHub Actions Inactive
@mysport12 mysport12 temporarily deployed to CI with Mapbox Tokens February 7, 2024 21:46 — with GitHub Actions Inactive
@mysport12 mysport12 temporarily deployed to CI with Mapbox Tokens February 7, 2024 21:46 — with GitHub Actions Inactive
@mysport12 mysport12 temporarily deployed to CI with Mapbox Tokens February 8, 2024 15:48 — with GitHub Actions Inactive
@mysport12 mysport12 temporarily deployed to CI with Mapbox Tokens February 8, 2024 15:48 — with GitHub Actions Inactive
@mysport12 mysport12 temporarily deployed to CI with Mapbox Tokens February 8, 2024 15:48 — with GitHub Actions Inactive
@mysport12 mysport12 temporarily deployed to CI with Mapbox Tokens February 8, 2024 15:48 — with GitHub Actions Inactive
@mysport12 mysport12 temporarily deployed to CI with Mapbox Tokens February 8, 2024 15:48 — with GitHub Actions Inactive
@mysport12 mysport12 temporarily deployed to CI with Mapbox Tokens February 8, 2024 15:48 — with GitHub Actions Inactive
mfazekas
mfazekas reviewed Feb 9, 2024
View reviewed changes
android/src/main/java/com/rnmapbox/rnmbx/utils/ExpressionParser.java Outdated
String jsonString = new Gson().toJson(array);
return Expression.fromRaw(jsonString);
} catch (Exception e) {
Log.e(LOG_TAG, "An error occurred while attempting to parse the expression", e);
Copy link
Contributor

Choose a reason for hiding this comment

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

Pls use com.rnmapbox.rnmbx.utils.Logger, as android logger only goes to console and not visible on the ReactNative side

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 went that route originally but it threw an error saying I couldn't use a non-static method in a static one. If you are able to offer some assistance I would appreciate it. I can jump back into this next week otherwise

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Think I may have figured it out. Please review.

@mysport12 mysport12 temporarily deployed to CI with Mapbox Tokens February 9, 2024 20:42 — with GitHub Actions Inactive
@mysport12 mysport12 temporarily deployed to CI with Mapbox Tokens February 9, 2024 20:42 — with GitHub Actions Inactive
@mysport12 mysport12 temporarily deployed to CI with Mapbox Tokens February 9, 2024 20:42 — with GitHub Actions Inactive
@mysport12 mysport12 temporarily deployed to CI with Mapbox Tokens February 9, 2024 20:42 — with GitHub Actions Inactive
@mysport12 mysport12 temporarily deployed to CI with Mapbox Tokens February 9, 2024 20:42 — with GitHub Actions Inactive
@mysport12 mysport12 temporarily deployed to CI with Mapbox Tokens February 11, 2024 14:11 — with GitHub Actions Inactive
@mysport12 mysport12 temporarily deployed to CI with Mapbox Tokens February 11, 2024 14:11 — with GitHub Actions Inactive
@mysport12 mysport12 temporarily deployed to CI with Mapbox Tokens February 11, 2024 14:11 — with GitHub Actions Inactive
@mysport12 mysport12 temporarily deployed to CI with Mapbox Tokens February 11, 2024 14:11 — with GitHub Actions Inactive
@mysport12 mysport12 temporarily deployed to CI with Mapbox Tokens February 11, 2024 14:11 — with GitHub Actions Inactive
@mysport12 mysport12 temporarily deployed to CI with Mapbox Tokens February 12, 2024 13:38 — with GitHub Actions Inactive
@mysport12 mysport12 temporarily deployed to CI with Mapbox Tokens February 12, 2024 13:38 — with GitHub Actions Inactive
@mysport12 mysport12 temporarily deployed to CI with Mapbox Tokens February 12, 2024 13:38 — with GitHub Actions Inactive
@mysport12 mysport12 temporarily deployed to CI with Mapbox Tokens February 12, 2024 13:38 — with GitHub Actions Inactive
@mysport12 mysport12 temporarily deployed to CI with Mapbox Tokens February 12, 2024 13:38 — with GitHub Actions Inactive
@mysport12 mysport12 temporarily deployed to CI with Mapbox Tokens February 12, 2024 13:38 — with GitHub Actions Inactive
@mfazekas mfazekas merged commit 1ea117a into rnmapbox:main Feb 13, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mfazekas mfazekas mfazekas approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@mysport12 @mfazekas