Repository navigation
fix(UiControls): prevent 3D map UI controls from overlapping status bar - #87
LoyalAbbas wants to merge 2 commits into
Conversation
kikoso
left a comment
kikoso
left a comment
There was a problem hiding this comment.
Hi @LoyalAbbas ! Thanks for looking into this. My main concern is that we are reaching into the SDK's internal view hierarchy (any top-aligned LinearLayout inside Map3DView), which can break silently with any SDK release, and it is quite a lot of code for a samples repo. Have we raised this with the SDK team? Ideally Map3DView would respect insets or expose a padding API. If we keep the workaround for now, a // Workaround: comment with the bug link would help. Can you take a look at the comments? Thanks!
| explicitStatusBarTop: Int? = null, | ||
| ) { | ||
| if (explicitStatusBarTop != null && explicitStatusBarTop > 0) { | ||
| mapView.setTag(R.id.top_bar, explicitStatusBarTop) |
There was a problem hiding this comment.
We are reusing R.id.top_bar and R.id.map3dView as tag keys here (and in line 301), which is confusing to read since both are also real view IDs looked up in resolveStatusBarOverlapPx. Could we declare dedicated keys in an ids.xml instead (e.g. map3d_status_bar_top_tag, map3d_insets_listener_tag)?
| mapView: ViewGroup, | ||
| explicitStatusBarTop: Int? = null, | ||
| ) { | ||
| if (explicitStatusBarTop != null && explicitStatusBarTop > 0) { |
There was a problem hiding this comment.
Since we only cache the value when it is > 0, once a positive value is stored there is no way to bring it back to 0: passing an explicit 0 (as bindWindowInsetsAndMapHierarchy does) just falls back to the old tag. Should we clear the tag when 0 is passed, or do I miss anything?
| if (view.getTag(R.id.map3dView) == true) return | ||
| view.setTag(R.id.map3dView, true) | ||
|
|
||
| view.addOnLayoutChangeListener { _, _, _, _, _, _, _, _, _ -> |
There was a problem hiding this comment.
This listener ends up registered on every nested ViewGroup of the map, and each layout pass on any of them walks the whole Map3DView subtree again (possibly triggering another layout). Would it make sense to register it only on the root map view, or only on the containers we actually offset?
| } | ||
|
|
||
| if (view === rootMapView) { | ||
| rootMapView.setOnHierarchyChangeListener( |
There was a problem hiding this comment.
ViewGroup only supports one OnHierarchyChangeListener, so this silently replaces anything the SDK may have set on Map3DView internally. Being this an SDK-owned view, I am a bit wary of it. Could we rely on the layout listener alone?
| } | ||
| if (lp != null && isTopAligned) { | ||
| ensureLayoutListener(child, rootMapView) | ||
| val effectiveBase = if (child.paddingTop > 0) 0 else baseMarginPx |
There was a problem hiding this comment.
Just to understand it: why does a positive paddingTop mean we should drop the base margin? Also, the 16dp base assumes the SDK's internal default, so if that changes we would be forcing 16dp onto every top LinearLayout, even when there is no overlap.
| val overlapPx = if (view.isInEditMode) { | ||
| 0 | ||
| } else { | ||
| val mapView = Map3DSettings.findMap3DView(view.rootView) |
There was a problem hiding this comment.
This runs on every recomposition and walks the whole hierarchy from rootView. It also reads view geometry rather than state, so it will not recompose when the map gets laid out or the insets change. Could we move it into a remember/LaunchedEffect keyed on the insets?
| */ | ||
| @JvmOverloads | ||
| fun updateTopMarginForUiControls(uiControls: Map3DUiControls = getSettings().uiControls) { | ||
| applyStatusBarInsetsToSiblingMapView() |
There was a problem hiding this comment.
updateTopMarginForUiControls now also changes the map's internal containers, which is a surprising side effect for a public method with this name. Together with lines 60, 208 and 209, we apply it three or four times per settings change. What about removing it from here and calling it explicitly only where needed?
| ).apply { topMargin = baseMarginPx } | ||
| mapView.addView(delayedTrailingControls, trailingLp) | ||
|
|
||
| // Simulate explicit re-application (as triggered by onChildViewAdded / onMapReady) |
There was a problem hiding this comment.
With this explicit call we are not really testing the onChildViewAdded path: the test would pass even without the hierarchy listener. Could we remove it and just idle the looper? Also, since this tests common code, shouldn't it live in common/src/test next to Map3DSettingsControllerTest?
Thanks for the thorough review, @kikoso! Completely agree that Map3DView should handle window insets or expose a padding API natively—I will discuss this with the SDK team so we can support it properly in the SDK. For now, I've added a // Workaround: comment in Map3DSettings.kt and resolved all your inline comments in the latest commit. |
- Add dedicated view tag IDs in common `ids.xml` instead of reusing view layout IDs - Support clearing cached status bar offset when an explicit `0` is passed - Remove `OnHierarchyChangeListener` and scope `OnLayoutChangeListener` to root map view and offset control containers - Preserve each control container's original SDK `topMargin` instead of assuming a hardcoded 16dp base margin - Move Compose status bar overlap calculation into a `DisposableEffect` with layout listener state - Remove sibling map mutation side effect from `Map3DQuickSettingsButton.updateTopMarginForUiControls` - Move `Map3DStatusBarInsetsTest` to `:Maps3DSamples:ApiDemos:common` and test delayed child layout listener directly
- Add dedicated view tag IDs in common `ids.xml` instead of reusing view layout IDs - Support clearing cached status bar offset when an explicit `0` is passed - Remove `OnHierarchyChangeListener` and scope `OnLayoutChangeListener` to root map view and offset control containers - Preserve each control container's original SDK `topMargin` instead of assuming a hardcoded 16dp base margin - Move Compose status bar overlap calculation into a `DisposableEffect` with layout listener state - Remove sibling map mutation side effect from `Map3DQuickSettingsButton.updateTopMarginForUiControls` - Move `Map3DStatusBarInsetsTest` to `:Maps3DSamples:ApiDemos:common` and test delayed child layout listener directly
acdaba5 to
e75d5a4
Compare
2 participants
Summary
Fixes top-anchored Maps 3D SDK UI controls (
ControlPlacement.LEADINGandControlPlacement.TRAILING) and the Quick Settings FAB overlapping the system status bar and camera cutout on full-screen edge-to-edge map screens across Kotlin Views, Java Views, and Jetpack Compose.Problem & Root Cause
16dptop margin, placing them inside the status bar / display cutout region.setUiControlsPlacement(...)(such as ononResume()or when toggling controls) also reset the inner Play Services delegateLinearLayouttop margins back to16dp.Changes
Map3DSettings,Map3DQuickSettingsButton,Map3DQuickSettingsWidget):Map3DSettings.applyStatusBarInsetsToMapView(...)andfindMap3DView(...)(without reflection) to offset top leading and trailing SDK control containers bystatusBarOverlap + 16dpon full-screen maps while keeping0extra offset when the map sits below a non-overlay toolbar (top_bar).ViewGroups and top-alignedLinearLayouts so SDK margin resets onsetUiControlsPlacementare immediately corrected.resolveStatusBarHeightPxto readgetInsetsIgnoringVisibility(statusBars() or displayCutout())so offsets hold in sticky immersive mode.Map3DQuickSettingsButtonfor standalone activities (PathFollowingActivity,HelloMapActivity).uiControlsOverridesupport inMap3DQuickSettingsWidget, plus automatic inset protection inrememberMap3DSessionSettings.SampleBaseActivity,UiControlsActivity):SampleBaseActivitybeforeonQuickSettingsChangedcallbacks.setUiControlsPlacementcalls in Kotlin and JavaUiControlsActivity.UiControlsActivity):uiControlsOverridetoMap3DQuickSettingsWidgetso the Quick Settings FAB stays8dpbelow active trailing controls.Map3DStatusBarInsetsTest):topMargin.