Repository navigation
fix(android): stop unnecessary hidden location puck rendering - #4432
ajtrasatti wants to merge 4 commits into
Conversation
mfazekas
left a comment
There was a problem hiding this comment.
Thanks, this does what it says. I checked it on an Android 14 emulator (Release, arm64) with a probe close to yours: blank style, fixed camera, custom provider at 1 Hz. Frames counted over ~4 location updates per stage:
- hidden, moving: main 91, PR 0
- hidden, stationary + pulse: main 74, PR 0
- hidden, moving + pulse: main 85, PR 0
- hidden, after following stops: main 64, PR 0
Hidden normal/compass/course following still tracks the same way as on main. UserLocation onUpdate keeps firing while the hidden puck is idle. Default, custom image, pulsing and an asset:// 3D model all come back after hide/show. Another bug this fixes, worth adding to the description: on main a hidden puck with pulsing still draws the pulse ring. 140 rapid hide/show cycles with the device provider don't add location threads (the #4225 leak), so turning the component off on hide looks safe. The JVM tests pass, and 7 of the 10 fail with the visibility changes reverted.
A few things I'd like simplified before merging:
-
Pulsing extraction. The behavior change is only "no pulse while hidden", but the diff moves the whole pulsing block into a new top-level function, and its
LOG_TAGuses become string literals. Please keep the block where it was and addif (!visible) location2.pulsingEnabled = falseafter it. That gives the same final state.RNMBXNativeUserLocationTestexists only because of the extraction. I'd drop it, or keep a single hide/show case if you can test it without the move. -
The comment in
_applydescribes the manager'senabled = showUserLocation || followUserLocationrule, not this call. The call is what turns the component off when hidden. Please remove it. If a note is needed, a one-liner onState.enabledis the right place. -
addToMapnow callsshowNativeUserLocationtwice: directly, then again through_apply(). One is enough. -
@JvmOverloadsonshowNativeUserLocationand on the constructor isn't needed. All callers are Kotlin in this module, and it isn't public API we keep stable. Please add thevisibleparameter and update the three call sites. Injecting theLocationManagerfor tests is fine, but name the parameterlocationManagerrather thanmLocationManager. -
Tests. Please trim
LocationComponentManagerTestto the cases that pin the new behavior: hidden stays disabled until following starts, hiding during follow keeps the component enabled, and hide/show keeps subscriptions balanced. The "native owned configuration" and "shutdown" cases cover existing behavior. The first still passes with the fix reverted. ThemockStatic/mockConstructionsetup matchesViewTagResolverTest, so that part is fine.
The CI change is welcome: the existing Android JVM tests don't run in CI today. Please mention it in the description, since it is independent of the fix.
I'm not asking for an Android Detox test here, because the repo has no Android Detox setup. If that gets added later, the LocationPuckLifecycle fixture from #4431 can be reused.
|
Thanks for the review and suggestions. I’ve updated the PR accordingly:
I also added the visible pulse-ring bug to the description. CI enablement is included and described separately: the existing Android build now runs the JVM suite, with no new job, dependency, or emulator setup. Locally, all 17 JVM tests and 194 Jest tests pass. Reverting visibility handling fails two of the three focused tests; restoring it passes. I rebuilt Release and reran the native probe after moving the pulse override: hidden fixed-camera states settle to idle with pulse on/off, and following, location callbacks, and configuration restoration still work. |
Problem and fix
With
LocationPuck visible={false}, the Android wrapper installs transparent images but still tells its location-component manager that the puck is shown. Moving locations continue interpolating the invisible puck, and requested pulsing can keep rendering a stationary hidden puck with a fixed camera. The pulse ring can also remain visible even though the puck is hidden.Pass the actual visibility to the existing manager. Enable the location component when the puck is visible or the camera is following; otherwise disable it. Keep native ownership of the puck configuration while mounted, and suppress pulsing while hidden without changing the caller's requested settings.
Android following consumes the location component's indicator updates, so hidden following retains the enabled component and transparent puck. Showing the puck reapplies its configured images/model/pulse. No JS API changes.
Regression coverage
Adds three tests using the existing JUnit 5/Mockito setup:
The manager accepts an injected
locationManagerfor testing, with its existing instance as the default. All 17 Android JVM tests pass. Reverting the visibility change causes two of the three new tests to fail; restoring it passes.Independent CI improvement: the existing Android build also runs the JVM tests with
./gradlew assemble :rnmapbox_maps:testDebugUnitTest. This adds no new dependency, job or emulator runner. There are no screenshots, CPU thresholds or timing assertions in the JVM tests.Native validation
Validated in the React Native example, Release ARM64, Android 14 emulator, Mapbox 11.33.0 and RN 0.84.1, using a blank style and synthetic location/heading updates:
UserLocation.onUpdatewhile the puck is hidden and rendering is idle.asset://3D model restore after hide/show.Additional two-map lifecycle validation passes: 20 rapid hide/show cycles return both maps to idle; style reloads work with following off/on; switching which mounted map follows leaves the inactive map idle; remounting both maps preserves following; actual background/foreground resumes hidden following and returns fixed-camera hidden maps to idle.
Release build,
yarn generate, lint + 194 Jest tests, TypeScript andgit diff --checkpass. Jest mocks native modules; the JVM tests check settings/transitions and the emulator probe validates actual rendering. Physical-device energy and long-soak measurements remain outside this validation.Related work
Android wrapper follow-up to merged #4431 and the maintainer's Android follow-up comment.
mapbox/mapbox-maps-android#2739 handles pulsing during actual native-layer hidden/stopped states. Transparent images leave that layer marked visible, so this wrapper change additionally honors the React Native
visible={false}contract. Camera movement while following still renders intentionally.Reproducer in the existing example app
Use the example's normal access-token setup. Replace
example/src/examples/BugReportExample.jswith the following and open it directly (or temporarily import it fromexample/index.js). Grant location permission, relaunch once, then press Start probe. Each stage runs for eight seconds; inspect the last three location updates after settling. Only the fully-rendered callback is counted. The custom map provider andUserLocationdevice provider are independent; feed a device location to checkonUpdate.