fix(map): retain camera across tab navigation#6352
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughChangesMap camera persistence
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant MapView
participant MapViewModel
participant MapPrefs
participant DataStore
MapViewModel->>MapPrefs: load camera position
MapPrefs->>DataStore: read camera preferences
DataStore-->>MapPrefs: saved camera record or null
MapPrefs-->>MapViewModel: camera position
MapView->>MapViewModel: restore or fit node bounds
MapView->>MapViewModel: save camera changes
MapViewModel->>MapPrefs: persist camera position
MapPrefs->>DataStore: atomically write camera preferences
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
androidApp/src/google/kotlin/org/meshtastic/app/map/prefs/map/GoogleMapsPrefs.kt (2)
125-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the float→double migration rationale.
getCameraCoordinatesilently falls back to afloatPreferencesKeyof the same name onClassCastException. This is a legitimate workaround — AndroidX DataStore docs confirm "Using overlapping keys with different types can result in ClassCastException", which is exactly what happens when a previouslyfloatPreferencesKey-backed value is now read viadoublePreferencesKey. Without a comment, a future reader is likely to mistake this for dead defensive code and remove it, silently breaking migration for existing installs.📝 Suggested comment
+ // camera_target_lat/lng were previously stored as Float; read as Double here and fall back to the + // legacy Float key so existing installs migrate without losing their saved camera position. private fun Preferences.getCameraCoordinate(key: Preferences.Key<Double>): Double? = try { this[key] } catch (_: ClassCastException) { this[floatPreferencesKey(key.name)]?.toDouble() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@androidApp/src/google/kotlin/org/meshtastic/app/map/prefs/map/GoogleMapsPrefs.kt` around lines 125 - 129, Document the migration rationale directly above getCameraCoordinate: explain that the ClassCastException fallback reads the same preference name through floatPreferencesKey to migrate values written by the previous float-backed implementation, preserving compatibility with existing installs. Keep the existing fallback behavior unchanged.
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo different classes named
MapCameraPosition. This PR introducesorg.meshtastic.core.repository.MapCameraPosition(latitude/longitude/zoom) and a same-named but differently-shapedorg.meshtastic.app.map.prefs.map.MapCameraPosition(targetLat/targetLng/zoom/tilt/bearing) in the google flavor, which depends oncore:repository. Both are simultaneously visible in the google module's classpath, risking autocomplete/import confusion between the two.
androidApp/src/google/kotlin/org/meshtastic/app/map/prefs/map/GoogleMapsPrefs.kt#L53-59: rename this flavor-specific type (e.g.GoogleCameraPosition) to disambiguate from the sharedMapPrefscontract type.core/repository/src/commonMain/kotlin/org/meshtastic/core/repository/AppPreferences.kt#L282-283: no change needed here; kept as the anchor reference for the canonical sharedMapCameraPositionname that the google-flavor type collides with.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@androidApp/src/google/kotlin/org/meshtastic/app/map/prefs/map/GoogleMapsPrefs.kt` at line 1, Rename the google-flavor MapCameraPosition type in GoogleMapsPrefs.kt to a distinct name such as GoogleCameraPosition, and update all constructors, properties, imports, and references in that flavor implementation accordingly. Leave the canonical org.meshtastic.core.repository.MapCameraPosition contract unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@androidApp/src/fdroid/kotlin/org/meshtastic/app/map/MapView.kt`:
- Around line 668-683: Update the boxOverlayListener callbacks in MapView so
saveCameraPosition is persisted only after scrolling or zooming has finished,
using OSMDroid’s movement-state signal (equivalent to gating on isMoving) rather
than saving on every intermediate callback. Keep the existing overlay generation
behavior in onScroll unchanged.
---
Nitpick comments:
In
`@androidApp/src/google/kotlin/org/meshtastic/app/map/prefs/map/GoogleMapsPrefs.kt`:
- Around line 125-129: Document the migration rationale directly above
getCameraCoordinate: explain that the ClassCastException fallback reads the same
preference name through floatPreferencesKey to migrate values written by the
previous float-backed implementation, preserving compatibility with existing
installs. Keep the existing fallback behavior unchanged.
- Line 1: Rename the google-flavor MapCameraPosition type in GoogleMapsPrefs.kt
to a distinct name such as GoogleCameraPosition, and update all constructors,
properties, imports, and references in that flavor implementation accordingly.
Leave the canonical org.meshtastic.core.repository.MapCameraPosition contract
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9d261360-0a62-4e33-86ad-637cddb7cbe8
📒 Files selected for processing (11)
androidApp/src/fdroid/kotlin/org/meshtastic/app/map/MapView.ktandroidApp/src/fdroid/kotlin/org/meshtastic/app/map/MapViewModel.ktandroidApp/src/fdroid/kotlin/org/meshtastic/app/map/MapViewWithLifecycle.ktandroidApp/src/google/kotlin/org/meshtastic/app/map/MapView.ktandroidApp/src/google/kotlin/org/meshtastic/app/map/MapViewModel.ktandroidApp/src/google/kotlin/org/meshtastic/app/map/prefs/map/GoogleMapsPrefs.ktandroidApp/src/testGoogle/kotlin/org/meshtastic/app/map/prefs/map/GoogleMapsPrefsTest.ktcore/prefs/src/commonMain/kotlin/org/meshtastic/core/prefs/map/MapPrefsImpl.ktcore/prefs/src/commonTest/kotlin/org/meshtastic/core/prefs/map/MapPrefsImplTest.ktcore/repository/src/commonMain/kotlin/org/meshtastic/core/repository/AppPreferences.ktcore/testing/src/commonMain/kotlin/org/meshtastic/core/testing/FakeAppPreferences.kt
jamesarich
left a comment
jamesarich
left a comment
There was a problem hiding this comment.
Approving. Solid camera-persistence refactor across both flavors.
The Loading → FitNodes/Restored state machine with the != Restored → return save guard cleanly prevents the initial node-fit from clobbering a restored camera, and gating node-fitting on map-ready (onMapLoaded / map.post {}) avoids the newLatLngBounds layout crash. Nice touch preserving existing installs via the legacy Float→Double ClassCastException fallback, and good coverage on both prefs impls.
Two non-blocking notes:
- The fdroid
MapViewearly-returns whileInitialCameraState.Loading, so there is a blank frame until the camera loads from disk — fine in practice given DataStore is fast. - CI is still queued at review time; enabling auto-merge so it lands once checks pass.
2 participants
Switching away from the map recreated provider camera state, causing the map to reopen at a default or unrelated viewport. This retains the last camera across tab navigation while preserving the first-use behavior of fitting discovered nodes.
Testing Performed
./gradlew spotlessCheck detekt assembleDebug :core:prefs:jvmTest --tests org.meshtastic.core.prefs.map.MapPrefsImplTest :androidApp:testFdroidDebugUnitTest :androidApp:testGoogleDebugUnitTest --tests org.meshtastic.app.map.prefs.map.GoogleMapsPrefsTestspotlessApply spotlessCheck detekt assembleDebug test allTests --continueinvocation exceeded the 10-minute local command limit without reporting a map-related failure; the bounded cross-flavor matrix above completed successfully.Summary by CodeRabbit