Skip to content

feat(ios): forward mapViewImpl prop so RNMBXMapViewFactory is usable on iOS - #4289

Open
SamuelBrucksch wants to merge 1 commit into
rnmapbox:mainfrom
SamuelBrucksch:feat/ios-map-view-impl-prop
Open

feat(ios): forward mapViewImpl prop so RNMBXMapViewFactory is usable on iOS#4289
SamuelBrucksch wants to merge 1 commit into
rnmapbox:mainfrom
SamuelBrucksch:feat/ios-map-view-impl-prop

Conversation

@SamuelBrucksch

Copy link
Copy Markdown
Contributor

Description

Makes the existing RNMBXMapViewFactory hook reachable on iOS.

RNMBXMapViewFactory.register(_:factory:) is public API, and RNMBXMapView.createMapView() already prefers a factory-built MapView when mapViewImpl is set:

func createMapView() -> MapView {
  if let mapViewImpl = mapViewImpl, let mapViewInstance = createAndAddMapViewImpl(mapViewImpl, self) {
    _mapView = mapViewInstance
  } else {
    _mapView = MapView(frame: self.bounds, mapInitOptions: MapInitOptions())
    ...

But nothing ever assigned RNMBXMapView.mapViewImpl on iOS, so that branch was unreachable from JS and every mapViewImpl value fell through to the default MapView. The prop is declared in src/specs/RNMBXMapViewNativeComponent.ts and on the JS component, and it is forwarded on Android (RNMBXMapViewManager.setMapViewImpl) — this only brings iOS in line, so it's a missing-functionality fix rather than a behaviour change.

The assignment is placed before [_view didSetProps:@[]], since that is where createMapView() runs.

Why this is useful

The factory is the only way to control MapInitOptions from an app, which matters for things the props don't cover — in our case MapOptions.pixelRatio. MapboxMaps defaults it to UIScreen.main.nativeScale and sizes the Metal drawable as bounds * pixelRatio, so a CarPlay map renders at the phone's scale (3.0) on a head unit that only has ~1.88 px/pt — ~2.5x the pixels per frame, discarded by the compositor. With this prop forwarded, the app can register a factory that builds the MapView with the car screen's pixel ratio; no library change beyond this is needed.

Checklist

  • I've read CONTRIBUTING.md
  • I updated the doc/other generated code with running yarn generate in the root folder
    • no generated output is affected: the prop already exists in the specs and is marked @private in MapView.tsx, so it is not part of the generated docs
  • 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)

How it was verified

Not via /example — verified in a production app (RN 0.86, New Architecture, iOS, @rnmapbox/maps 10.3.2 with this change applied through patch-package):

  • before: a MapView with mapViewImpl="…" silently used the default MapView; the registered factory closure was never invoked (and createAndAddMapViewImpl's "No mapview factory registered" error never fired either, since mapViewImpl was nil at that point).

  • after: the factory closure runs for every CarPlay surface and the returned MapView is the one used. Log line from the factory, on a real head unit connection:

    CarMirrorMap | impl=abrpCarMirror:headUnit:3 pixelRatio=1.8836 screenSource=carPlayScene
    carScreen=(scale: 3.0, nativeScale: 3.0, bounds: (1019.3, 573.3), nativeBounds: (1920.0, 1080.0))
    

    and the Metal drawable then matches the head unit's resolution instead of ~1.6x it.

Happy to add an /example scene for it if you'd like — it needs a factory registration in the example app's AppDelegate since the hook is native-only, so I left that out of this PR unless you want it.


🤖 Prepared by Devin on behalf of @SamuelBrucksch.

…on iOS

RNMBXMapViewFactory.register is public and RNMBXMapView.createMapView()
already prefers a factory-built MapView, but nothing assigned
RNMBXMapView.mapViewImpl on iOS, so the hook could never be reached from
JS. The prop is declared in the specs and forwarded on Android
(RNMBXMapViewManager.setMapViewImpl), so this only brings iOS in line.

Assigned before -didSetProps:, which is where createMapView() runs.
@SamuelBrucksch

Copy link
Copy Markdown
Contributor Author

Some history I dug up while checking for duplicates — this is closer to a regression than a gap that was never filled:

ios/RNMBX/RNMBXMapViewManager.m no longer exists on main (paper view managers went away with the old-architecture support), and RNMBXMapViewComponentView.mm never carried the equivalent line — so the iOS side of the feature has been dead since then, while Android kept working through RNMBXMapViewManager.setMapViewImpl. This PR just restores what #3317 did, in the Fabric component view.

Possibly related: #2728 ("Map aspect ratio broken inside CarPlay on iOS", also reported for an external monitor) was closed as not-planned for lack of resources. That class of problem comes from the map being initialised with the main screen's scale — MapboxMaps defaults MapOptions.pixelRatio to UIScreen.main.nativeScale and hard-wires the Metal drawable to it — so with this prop forwarded, apps rendering onto a secondary screen can correct it themselves through a factory without any further library change. Not claiming it as a fix for that issue, just noting the connection.

For completeness: no open issue tracks this. I searched the repo for mapViewImpl, MapViewFactory, MapInitOptions, pixelRatio and CarPlay; the only other hits are the two merged PRs above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant