react-native-maps icon indicating copy to clipboard operation
react-native-maps copied to clipboard
verified publisher icon react-native-maps

feat(android): use new Advanced Markers instead of deprecated ones

Open mateki0 opened this issue 1 year ago • 2 comments

Does any other open PR do the same thing?

No

What issue is this PR fixing?

https://github.com/react-native-maps/react-native-maps/discussions/4481 There are few problems with markers currently: poor performance, blinking, etc.

How did you test this PR?

  • It affects only Android Google Maps.
  • I have tested it on Samsung M23.

I have added new example screen MarkersCollisionBehavior in which I've used one of new features available only in Advanced Markers. Also I have fixed AnimatedMarkerPosition screen on Android in which you can test animating to coordinates.

AdvancedMarkers will be used by default if we pass googleMapId and if it's supported by device.

Some devices might not support the new map renderer and therefore cannot display advanced markers and in this case we we will use old Markers.

mateki0 avatar Apr 23 '24 08:04 mateki0

@mateki0 awesome work, but I wouldn't rush in merging this because it's rather a critical change and needs some thoughtful considerations.

what comes to mind, and should make the implementation better:

  1. JS should know about marker type, I suggest we return onMapReady callback the capabilities of googleMaps.
  2. we should have AdvancedMarker as a type in JS as well, and automatically select it for Marker implementation on Android if it's available, but the user can always switch to classical (can be as simple as prop) classicalGoogleMarker={true/ false}
  3. use Inheritance on the java side where Classical has a different implementation, that way the code is more manageable.
  4. getMarkerOptions is strangely implemented, specifically collisionBehavior should be translated to the value google maps needs when set, ideally using initialProps as well, that way it looks a bit cleaner / more stateful.
  5. I'm pretty sure that googleMapId is available for iOS as well, google cloud maps are available for both platforms for a while now.
  6. advance markers are available for iOS so I think we have to finish this fully before merging.
  7. are you sure mapID is required? not just latestRenderer ?

salah-ghanim avatar May 04 '24 09:05 salah-ghanim

Nice suggestions :)

  1. I don't know why but onMapReady always returns undefined for me and with the same implementation onMapLoaded works fine, so I've put this information to onMapLoaded.
  2. Added but only with collisionBehavior for now.
  3. Done. However I did not implement every feature that AdvancedMarkers offers, only collisionBehavior for now because we offer using Marker with custom View inside. Should I add support for PinConfig?
  4. I didn't found a way to pass collisionBehavior as initialProps, but I refactored it a bit anyway. Did you mean initialProps that we have in MapManager.java?
  5. Changed googleMapId description.
  6. Can we add support for iOS in separate PR? I think that it will be safer and easier to merge if we split it.
  7. Docs says that google map id is required. There is also information about latest renderer but it still won't work with latest renderer and without mapId https://developers.google.com/maps/documentation/android-sdk/advanced-markers/start#check_map_capabilities_required

mateki0 avatar May 07 '24 05:05 mateki0

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If the PR remains relevant, simply comment Still relevant and the PR will remain open. Thank you for your contributions.

github-actions[bot] avatar Aug 07 '24 01:08 github-actions[bot]

@salah-ghanim @mateki0 Could you please look into this once again? Advanced Markers could really fix the performance issues for Android: PinConfig could help replace the need for react-native views in marker, thus improving performance. iconView allows to use Android Views directly without converting them to bitmap:

mr-tm avatar Dec 05 '24 16:12 mr-tm

If I get a re-review I can address it or merge depending on the result, but that's something I don't have an influence on. I will reopen this PR now.

About PinConfig and iconView. Is it something we can treat as an improvement to this PR and do it separately? I would prefer not to add any bigger changes here

Edit: There are some conflicts which I will resolve later today

mateki0 avatar Dec 09 '24 09:12 mateki0

Closing this one because I don't have access to the fork/don't want to push changes to the TWG fork code. I created a new PR from my own fork which does the same thing - https://github.com/react-native-maps/react-native-maps/pull/5281

mateki0 avatar Dec 12 '24 10:12 mateki0