From 78e641b3b283f6af0466cd39643e233024434ef3 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 18 Jul 2026 23:43:02 +0000 Subject: [PATCH] fix(location): audit fixes for the geohash picker Bugs: - "Use my location" could spin forever when a fix never arrives (location off, indoors, permission granted but unavailable). Wait with a 20s timeout and stop on any terminal outcome so the button always resets. - With no seed, an initial osmdroid layout-scroll at the opening center could be mistaken for a pick, auto-selecting the mid-Atlantic and enabling Confirm before the user moved. Gate selection behind a real pan/search/GPS/tap (hasSelection). Performance: - The map's AndroidView update ran setColorFilter + overlays.removeAll + invalidate() on every scroll-driven recomposition. Move the dark-mode tile filter to a theme-keyed effect, and rebuild the marker + invalidate only when the marker point actually changes. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01YU8YLcjH9ALr4PgdAkGPZh --- .../location/GeohashLocationPickerDialog.kt | 49 ++++++++++++++++--- .../creators/location/LocationPickerMap.kt | 44 +++++++++++------ 2 files changed, 71 insertions(+), 22 deletions(-) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/creators/location/GeohashLocationPickerDialog.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/creators/location/GeohashLocationPickerDialog.kt index 8a9ddf00d1..cba3c257ef 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/creators/location/GeohashLocationPickerDialog.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/creators/location/GeohashLocationPickerDialog.kt @@ -87,7 +87,9 @@ import com.vitorpamplona.quartz.experimental.bitchat.geohash.GeohashChannelLevel import com.vitorpamplona.quartz.nip01Core.tags.geohash.GeoHash import kotlinx.coroutines.delay import kotlinx.coroutines.flow.first +import kotlinx.coroutines.withTimeoutOrNull import org.osmdroid.util.GeoPoint +import kotlin.math.abs /** Zoom the map animates to after a search hit or a "use my location" tap. */ private const val RECENTER_ZOOM = 14.0 @@ -98,6 +100,19 @@ private const val SEEDED_ZOOM = 13.0 /** How far out to start when the picker opens with nothing selected yet. */ private const val WORLD_ZOOM = 2.5 +/** Neutral starting center (mid-Atlantic) when the picker opens with no seed. */ +private const val WORLD_CENTER_LAT = 20.0 +private const val WORLD_CENTER_LON = 0.0 + +/** + * Minimum center shift (degrees) from the opening center that counts as a real pan, + * so an initial osmdroid layout-scroll at the opening center is not mistaken for a pick. + */ +private const val SELECT_MOVE_EPS = 0.0005 + +/** Give up waiting for a GPS fix after this long so the button never spins forever. */ +private const val GPS_FIX_TIMEOUT_MS = 20_000L + /** * A full-screen, map-first location picker dialog that produces a geohash string. * @@ -160,18 +175,25 @@ fun GeohashLocationPickerContent( val seed = remember(initialGeohash) { initialGeohash?.takeIf { it.isNotBlank() }?.let { GeoHash.decode(it) } } val seedLen = initialGeohash?.trim()?.length ?: 0 + // The map opens centered here. Without a seed there is no real selection yet — and + // osmdroid can emit an initial scroll at this exact center, which must NOT be treated + // as a pick (else the picker would auto-select the mid-Atlantic and enable Confirm). + val initialLat = seed?.centerLat ?: WORLD_CENTER_LAT + val initialLon = seed?.centerLon ?: WORLD_CENTER_LON + var pickedLat by remember { mutableStateOf(seed?.centerLat) } var pickedLon by remember { mutableStateOf(seed?.centerLon) } + var hasSelection by remember { mutableStateOf(seed != null) } var level by remember { mutableStateOf(GeohashChannelLevel.forChars(seedLen) ?: GeohashChannelLevel.CITY) } var recenter by remember { mutableStateOf(null) } val cell = - remember(pickedLat, pickedLon, level) { + remember(pickedLat, pickedLon, level, hasSelection) { val lat = pickedLat val lon = pickedLon - if (lat != null && lon != null) GeoHash.encode(lat, lon, level.chars).toString() else null + if (hasSelection && lat != null && lon != null) GeoHash.encode(lat, lon, level.chars).toString() else null } // Debounce the reverse-geocode: panning changes [cell] constantly, and we don't @@ -207,11 +229,18 @@ fun GeohashLocationPickerContent( } LaunchedEffect(wantsMyLocation) { if (wantsMyLocation) { - val fix = locationManager.preciseGeohashStateFlow.first { it is LocationState.LocationResult.Success } - val hash = (fix as LocationState.LocationResult.Success).geoHash - recenter = GeoPoint(hash.centerLat, hash.centerLon) - pickedLat = hash.centerLat - pickedLon = hash.centerLon + // Wait for the first real fix, but bail on a timeout so the button never spins + // forever (permission granted yet location off, indoors, emulator with no fix…). + val fix = + withTimeoutOrNull(GPS_FIX_TIMEOUT_MS) { + locationManager.preciseGeohashStateFlow.first { it is LocationState.LocationResult.Success } + } as? LocationState.LocationResult.Success + if (fix != null) { + recenter = GeoPoint(fix.geoHash.centerLat, fix.geoHash.centerLon) + pickedLat = fix.geoHash.centerLat + pickedLon = fix.geoHash.centerLon + hasSelection = true + } wantsMyLocation = false } } @@ -248,6 +277,7 @@ fun GeohashLocationPickerContent( recenter = GeoPoint(hit.latitude, hit.longitude) pickedLat = hit.latitude pickedLon = hit.longitude + hasSelection = true results = emptyList() query = "" keyboard?.hide() @@ -266,11 +296,16 @@ fun GeohashLocationPickerContent( onCenterChanged = { lat, lon -> pickedLat = lat pickedLon = lon + // A pan/zoom away from the opening center is the user's first real pick. + if (!hasSelection && (abs(lat - initialLat) > SELECT_MOVE_EPS || abs(lon - initialLon) > SELECT_MOVE_EPS)) { + hasSelection = true + } }, onPick = { lat, lon -> recenter = GeoPoint(lat, lon) pickedLat = lat pickedLon = lon + hasSelection = true }, modifier = Modifier.fillMaxSize(), ) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/creators/location/LocationPickerMap.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/creators/location/LocationPickerMap.kt index f5b0db80c1..1ee0bb3a89 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/creators/location/LocationPickerMap.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/creators/location/LocationPickerMap.kt @@ -23,6 +23,7 @@ package com.vitorpamplona.amethyst.ui.note.creators.location import androidx.compose.material3.MaterialTheme import androidx.compose.runtime.Composable import androidx.compose.runtime.DisposableEffect +import androidx.compose.runtime.LaunchedEffect import androidx.compose.runtime.getValue import androidx.compose.runtime.remember import androidx.compose.runtime.rememberUpdatedState @@ -84,6 +85,10 @@ fun LocationPickerMap( // same [recenter] doesn't yank the map back while the user is panning. val lastRecenter = remember { arrayOfNulls(1) } + // Tracks the last marker point so we only rebuild the marker overlay (and invalidate) + // when it actually changes — not on every scroll-driven recomposition. + val lastMarker = remember { arrayOfNulls(1) } + val mapView = remember(context) { Configuration.getInstance().userAgentValue = context.packageName @@ -141,6 +146,13 @@ fun LocationPickerMap( } } + // Follow the app theme: dim the bright MAPNIK tiles in dark mode (matching the + // display-only LocationPreviewMap). Applied only when the theme flips, not per frame. + LaunchedEffect(mapView, darkTheme) { + mapView.overlayManager.tilesOverlay.setColorFilter(if (darkTheme) NIGHT_TILE_FILTER else null) + mapView.invalidate() + } + AndroidView( modifier = modifier, factory = { mapView }, @@ -154,22 +166,24 @@ fun LocationPickerMap( } } - // Follow the app theme: dim the bright MAPNIK tiles in dark mode, matching - // the display-only LocationPreviewMap. - map.overlayManager.tilesOverlay.setColorFilter(if (darkTheme) NIGHT_TILE_FILTER else null) - - map.overlays.removeAll { it is Marker } - if (pickedLatitude != null && pickedLongitude != null) { - val point = GeoPoint(pickedLatitude, pickedLongitude) - val marker = - Marker(map).apply { - position = point - setAnchor(Marker.ANCHOR_CENTER, Marker.ANCHOR_BOTTOM) - setInfoWindow(null) - } - map.overlays.add(marker) + // Only rebuild the marker overlay when the picked point actually changes. The + // update block runs on every scroll-driven recomposition, so unconditionally + // clearing/re-adding the marker and invalidating would be per-frame waste. + val marker = if (pickedLatitude != null && pickedLongitude != null) GeoPoint(pickedLatitude, pickedLongitude) else null + if (marker != lastMarker[0]) { + lastMarker[0] = marker + map.overlays.removeAll { it is Marker } + if (marker != null) { + map.overlays.add( + Marker(map).apply { + position = marker + setAnchor(Marker.ANCHOR_CENTER, Marker.ANCHOR_BOTTOM) + setInfoWindow(null) + }, + ) + } + map.invalidate() } - map.invalidate() }, ) }