mirror of
https://github.com/vitorpamplona/amethyst.git
synced 2026-08-09 08:04:45 +00:00
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YU8YLcjH9ALr4PgdAkGPZh
This commit is contained in:
+42
-7
@@ -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<GeoPoint?>(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(),
|
||||
)
|
||||
|
||||
+29
-15
@@ -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<GeoPoint>(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<GeoPoint>(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()
|
||||
},
|
||||
)
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user