Fix: Map position of Explore set correctly when navigating from Nearby #6544
Conversation
WalkthroughThe changes guard reading of prev_zoom/prev_latitude/prev_longitude with Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Nearby as Nearby Map
participant Explore as ExploreFragment
participant Map as ExploreMapFragment
User->>Nearby: Zoom to Z, tap "Show in Explore"
Nearby->>Explore: start with args (prev_zoom, prev_latitude, prev_longitude)
rect rgb(230,245,230)
note right of Explore: Argument handling
Explore->>Explore: check arguments?.containsKey("prev_zoom") etc.
Explore->>Map: create / pass guarded args
end
rect rgb(220,235,255)
note right of Map: Map-ready flow (deferred if not resumed)
Map->>Map: isResumed?
alt Not resumed
Map->>Map: set shouldPerformMapReadyActionsOnResume = true
else Resumed
Map->>Map: performMapReadyActions()
end
Map->>Map: onResume() checks flag → performMapReadyActions()
Map->>Map: safeZoom = clamp(prevZoom,1.0,22.0)
Map->>Map: moveCameraToPosition(lat, lon, safeZoom)
end
Map->>User: Display map at clamped zoom
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/src/main/java/fr/free/nrw/commons/explore/ExploreMapRootFragment.kt (1)
42-46: Risk: Partial key presence may propagate invalid coordinate triples.Line 42 uses OR logic (
||), so if any one of the three keys exists, all three values (including 0.0 defaults for missing keys) are propagated. For example, if onlyprev_zoomis present,featuredArgumentswill receive(zoom=X, latitude=0.0, longitude=0.0), which points to the middle of the Atlantic Ocean rather than indicating "no position data."Consider using AND logic (
&&) to require all three keys, or omit propagation entirely if any key is missing:-if (bundle.containsKey("prev_zoom") || bundle.containsKey("prev_latitude") || bundle.containsKey("prev_longitude")) { +if (bundle.containsKey("prev_zoom") && bundle.containsKey("prev_latitude") && bundle.containsKey("prev_longitude")) { featuredArguments.putDouble("prev_zoom", zoom) featuredArguments.putDouble("prev_latitude", latitude) featuredArguments.putDouble("prev_longitude", longitude) }app/src/main/java/fr/free/nrw/commons/explore/map/ExploreMapFragment.kt (1)
424-424: Inconsistent detection: uses non-zero check instead of containsKey.Line 424's
isCameFromNearbyMapproperty checksprevZoom != 0.0 || prevLatitude != 0.0 || prevLongitude != 0.0, which is the old detection pattern. The other two files (ExploreFragment.kt lines 144-146, ExploreMapRootFragment.kt line 42) now usecontainsKeychecks.With the new approach where arguments are only assigned if keys exist (lines 402-404), this non-zero check is inconsistent and could fail to detect "came from Nearby" if all three values happen to be 0.0 (though unlikely for zoom).
For consistency, update to use the presence of arguments:
val isCameFromNearbyMap: Boolean - get() = prevZoom != 0.0 || prevLatitude != 0.0 || prevLongitude != 0.0 + get() = arguments?.containsKey("prev_zoom") == true + && arguments?.containsKey("prev_latitude") == true + && arguments?.containsKey("prev_longitude") == true(Note: Use AND logic
&&to require all three keys, as discussed in earlier comments on the other files.)
🧹 Nitpick comments (3)
app/src/main/java/fr/free/nrw/commons/explore/ExploreMapRootFragment.kt (1)
32-34: Verify handling of legitimate zero values vs. missing keys.The
containsKeyguards with 0.0 defaults introduce ambiguity: if Nearby legitimately passes 0.0 for latitude (equator) or longitude (prime meridian), this code cannot distinguish those from missing keys. While rare in practice, consider whether the semantics require distinguishing "key absent" from "key present with value 0.0".For context, the zoom clamping logic in ExploreMapFragment.kt (lines 376-380) treats
prevZoom < 1.0specially, which suggests 0.0 is interpreted as "no zoom provided"—but that interpretation isn't consistently applied to latitude/longitude here.app/src/main/java/fr/free/nrw/commons/explore/map/ExploreMapFragment.kt (2)
300-304: Verify deferral doesn't skip remaining permission checks.The deferral at lines 300-304 only affects
performMapReadyActions(), but the method continues to lines 305-318 which handle permissions. If not resumed, should those permission checks also be deferred, or is the early return on line 305 (if (!isVisible)) sufficient?The logic flow could be clearer. Consider whether the deferral block should return early to prevent any further execution when not resumed.
372-374: Extract magic numbers as constants.The values 1.0, 22.0, and 15.0 are hardcoded. Define them as named constants (e.g.,
MIN_ZOOM,MAX_ZOOM,DEFAULT_ZOOM) at the class or companion object level for maintainability.companion object { private const val MIN_ZOOM = 1.0 private const val MAX_ZOOM = 22.0 private const val DEFAULT_ZOOM = 15.0 fun newInstance(): ExploreMapFragment { val fragment = ExploreMapFragment() fragment.setRetainInstance(true) return fragment } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/main/java/fr/free/nrw/commons/explore/ExploreFragment.kt(2 hunks)app/src/main/java/fr/free/nrw/commons/explore/ExploreMapRootFragment.kt(1 hunks)app/src/main/java/fr/free/nrw/commons/explore/map/ExploreMapFragment.kt(5 hunks)
🔇 Additional comments (4)
app/src/main/java/fr/free/nrw/commons/explore/ExploreFragment.kt (1)
124-132: LGTM—safe argument loading.The
containsKeyguards prevent reading missing keys, and fields are only updated when keys are present. This is the correct pattern for optional arguments.app/src/main/java/fr/free/nrw/commons/explore/map/ExploreMapFragment.kt (3)
105-105: LGTM—sensible lifecycle deferral mechanism.The flag ensures map-ready actions are replayed after resume when the fragment wasn't yet resumed at the time of the request. This prevents potential crashes or incorrect state from performing map operations before the fragment is fully ready.
285-288: LGTM—deferred actions correctly replayed.The onResume handler properly checks the flag and performs the deferred actions, ensuring map initialization completes even when initially deferred.
402-404: Approve guarded argument loading, but note 0.0 ambiguity.The
containsKeyguards are correct. However, defaulting to 0.0 when keys are absent creates ambiguity (see earlier comments on lines 372-384). If the semantics require distinguishing "absent" from "present with value 0.0," consider using nullableDouble?fields instead of primitives with 0.0 defaults.
app/src/main/java/fr/free/nrw/commons/explore/ExploreFragment.kt
Outdated
Show resolved
Hide resolved
| val minZoom = 1.0 | ||
| val maxZoom = 22.0 | ||
| val defaultNearbyZoom = 15.0 | ||
|
|
||
| val safeZoom = when { | ||
| prevZoom > maxZoom -> maxZoom | ||
| prevZoom < minZoom -> defaultNearbyZoom | ||
| else -> prevZoom | ||
| } | ||
|
|
||
| moveCameraToPosition( | ||
| GeoPoint(prevLatitude, prevLongitude), | ||
| prevZoom, | ||
| safeZoom, |
There was a problem hiding this comment.
Critical: Zoom clamping breaks detection of "no zoom provided."
Lines 376-380 clamp prevZoom, but when prevZoom < minZoom (including the 0.0 default from line 402 when the key is missing), safeZoom is set to defaultNearbyZoom (15.0). This means:
- If Nearby didn't provide a zoom (key absent → 0.0 default), the map incorrectly zooms to 15.0 instead of using fallback logic.
- The intent appears to be "use 15.0 for invalid zoom," but 0.0 legitimately signals "no zoom provided," not "invalid zoom."
The clamping should detect absence vs. invalidity. Apply this diff to preserve the "no zoom provided" case:
// if we came from 'Show in Explore' in Nearby, load Nearby map center and zoom
if (isCameFromNearbyMap) {
- val minZoom = 1.0
- val maxZoom = 22.0
- val defaultNearbyZoom = 15.0
-
- val safeZoom = when {
- prevZoom > maxZoom -> maxZoom
- prevZoom < minZoom -> defaultNearbyZoom
- else -> prevZoom
- }
-
moveCameraToPosition(
GeoPoint(prevLatitude, prevLongitude),
- safeZoom,
+ prevZoom.coerceIn(1.0, 22.0),
1L
)Or, if you want to distinguish "no zoom" (0.0) from "invalid zoom," use:
+ val minZoom = 1.0
+ val maxZoom = 22.0
+ val defaultNearbyZoom = 15.0
+
+ val safeZoom = when {
+ prevZoom == 0.0 -> defaultNearbyZoom // Key was absent, use default
+ prevZoom > maxZoom -> maxZoom
+ prevZoom < minZoom -> minZoom // Clamp to minimum, not default
+ else -> prevZoom
+ }Note: The second approach still conflates "key absent" (0.0 default) with "Nearby passed 0.0," so the root issue is defaulting to 0.0 at line 402. Consider nullable types or sentinel values.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/src/main/java/fr/free/nrw/commons/explore/map/ExploreMapFragment.kt (1)
371-376: Consider handling missing zoom explicitly.The current implementation clamps
prevZoomto the range [1.0, 22.0]. However, whenprevZoomis 0.0 (because the key was absent in the bundle) andprevLatitude/prevLongitudeare non-zero,isCameFromNearbyMap(line 414) returns true, causing the map to zoom to 1.0 (world view). This may not provide the expected user experience when navigating from Nearby with an incomplete zoom value.Consider explicitly detecting when zoom was not provided and using a sensible default (e.g., 15.0 for village-level zoom) instead of clamping to the minimum:
- moveCameraToPosition( - GeoPoint(prevLatitude, prevLongitude), - prevZoom.coerceIn(1.0, 22.0), - 1L - ) + val zoom = if (prevZoom == 0.0) 15.0 else prevZoom.coerceIn(1.0, 22.0) + moveCameraToPosition( + GeoPoint(prevLatitude, prevLongitude), + zoom, + 1L + )Alternatively, consider using nullable types for these values to distinguish "not provided" from actual zero values.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/main/java/fr/free/nrw/commons/explore/ExploreFragment.kt(2 hunks)app/src/main/java/fr/free/nrw/commons/explore/map/ExploreMapFragment.kt(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/fr/free/nrw/commons/explore/ExploreFragment.kt
🔇 Additional comments (4)
app/src/main/java/fr/free/nrw/commons/explore/map/ExploreMapFragment.kt (4)
105-105: LGTM: Lifecycle flag added correctly.The flag properly tracks whether map-ready actions need to be deferred until the fragment resumes.
285-288: LGTM: Deferred actions replayed correctly.The logic correctly handles the case where map-ready actions were deferred and need to be performed once the fragment resumes.
300-304: LGTM: Deferral logic implemented correctly.The conditional logic appropriately defers map-ready actions when the fragment is not yet resumed, ensuring they execute at the right lifecycle moment.
392-394: LGTM: Bundle reads properly guarded.The
containsKeychecks correctly prevent reading missing keys, allowing the values to remain at their defaults when not provided. This is good defensive programming.
|
✅ Generated APK variants! |
2 participants
Description (required)
Fixed a bug where when we go to "Show in Explorer" option from Nearby it would set map position incorrectly.
Fixes #6530
What changes did you make and why?
Tests performed (required)
Tested {build variant, e.g. ProdDebug} on {name of device or emulator} with API level {API level}.
Screenshots (for UI changes only)
Need help? See https://support.google.com/android/answer/9075928
Note: Please ensure that you have read CONTRIBUTING.md if this is your first pull request.
Summary by CodeRabbit