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
ContributorAuthor

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 freeto 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

@SamuelBrucksch
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
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
ContributorAuthor

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 freeto 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

@SamuelBrucksch
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
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
ContributorAuthor

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 freeto 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

@SamuelBrucksch
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
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
ContributorAuthor

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 freeto 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

@SamuelBrucksch
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
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
ContributorAuthor

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 freeto 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

@SamuelBrucksch
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
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
ContributorAuthor

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 freeto 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

@SamuelBrucksch
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
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
ContributorAuthor

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 freeto 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

@SamuelBrucksch
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
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
ContributorAuthor

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 freeto 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

@SamuelBrucksch