Merge dc3a6c2bf3222387733456ec3fa6dde3c1cf894c into 1f24973b06ee77492742ba522002e028f38f4381

app/src/main/java/com/bitchat/android/geohash/GeohashBookmarksStore.kt

@@ -9,6 +9,8 @@ import com.google.gson.Gson
 import com.google.gson.reflect.TypeToken
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.Job
+import kotlinx.coroutines.ensureActive
 import kotlinx.coroutines.launch
 import kotlinx.coroutines.flow.MutableStateFlow
 import kotlinx.coroutines.flow.StateFlow
@@ -20,7 +22,10 @@ import java.util.Locale
  * - Persistence: SharedPreferences (JSON string array)
  * - Semantics: geohashes are normalized to lowercase base32 and de-duplicated
  */
-class GeohashBookmarksStore private constructor(private val context: Context) {
+class GeohashBookmarksStore private constructor(
+    private val context: Context,
+    private val resolveNames: Boolean
+) {
 
     companion object {
         private const val TAG = "GeohashBookmarksStore"
@@ -30,10 +35,18 @@ class GeohashBookmarksStore private constructor(private val context: Context) {
         @Volatile private var INSTANCE: GeohashBookmarksStore? = null
         fun getInstance(context: Context): GeohashBookmarksStore {
             return INSTANCE ?: synchronized(this) {
-                INSTANCE ?: GeohashBookmarksStore(context.applicationContext).also { INSTANCE = it }
+                INSTANCE ?: GeohashBookmarksStore(context.applicationContext, resolveNames = true)
+                    .also { INSTANCE = it }
             }
         }
 
+        /**
+         * Isolated store for unit tests. Reverse geocoding is disabled so a test never
+         * reaches the network; the persistence and bookkeeping paths are unchanged.
+         */
+        internal fun createForTest(context: Context): GeohashBookmarksStore =
+            GeohashBookmarksStore(context.applicationContext, resolveNames = false)
+
         private val allowedChars = "0123456789bcdefghjkmnpqrstuvwxyz".toSet()
         fun normalize(raw: String): String {
             return raw.trim().lowercase(Locale.US)
@@ -55,16 +68,20 @@ class GeohashBookmarksStore private constructor(private val context: Context) {
 
     // For throttling / preventing duplicate geocode lookups
     private val resolving = mutableSetOf<String>()
+    private val resolveJobs = mutableMapOf<String, Job>()
 
     init { load() }
 
+    @Synchronized
     fun isBookmarked(geohash: String): Boolean = membership.contains(normalize(geohash))
 
+    @Synchronized
     fun toggle(geohash: String) {
         val gh = normalize(geohash)
         if (membership.contains(gh)) remove(gh) else add(gh)
     }
 
+    @Synchronized
     fun add(geohash: String) {
         val gh = normalize(geohash)
         if (gh.isEmpty() || membership.contains(gh)) return
@@ -76,6 +93,7 @@ class GeohashBookmarksStore private constructor(private val context: Context) {
         resolveNameIfNeeded(gh)
     }
 
+    @Synchronized
     fun remove(geohash: String) {
         val gh = normalize(geohash)
         if (!membership.contains(gh)) return
@@ -142,6 +160,7 @@ class GeohashBookmarksStore private constructor(private val context: Context) {
 
     // MARK: - Destructive Reset
 
+    @Synchronized
     fun clearAll() {
         try {
             membership.clear()
@@ -151,7 +170,11 @@ class GeohashBookmarksStore private constructor(private val context: Context) {
                 .remove(STORE_KEY)
                 .remove(NAMES_STORE_KEY)
                 .apply()
-            // Clear any in-flight resolutions to avoid repopulating
+            // Clear any in-flight resolutions to avoid repopulating, and
+            // cancel the geocode so a panic clear does not still send the
+            // bookmark's coordinates to the geocoder.
+            resolveJobs.values.forEach { it.cancel() }
+            resolveJobs.clear()
             resolving.clear()
             Log.i(TAG, "Cleared all geohash bookmarks and names")
         } catch (e: Exception) {
@@ -162,15 +185,19 @@ class GeohashBookmarksStore private constructor(private val context: Context) {
 
     // MARK: - Friendly Name Resolution
 
+    @Synchronized
     fun resolveNameIfNeeded(geohash: String) {
+        if (!resolveNames) return
         val gh = normalize(geohash)
         if (gh.isEmpty()) return
         if (_bookmarkNames.value?.containsKey(gh) == true) return
         if (resolving.contains(gh)) return
 
         resolving.add(gh)
-        CoroutineScope(Dispatchers.IO).launch {
+        val job = CoroutineScope(Dispatchers.IO).launch {
             try {
+                ensureActive()
+                if (!isBookmarked(gh)) return@launch
                 val geocoderProvider = GeocoderFactory.get(context)
                 val name: String? = if (gh.length <= 2) {
                     // Composite admin name from multiple points
@@ -206,18 +233,39 @@ class GeohashBookmarksStore private constructor(private val context: Context) {
                     pickNameForLength(gh.length, a)
                 }
 
-                if (!name.isNullOrEmpty()) {
-                    val current = _bookmarkNames.value.toMutableMap()
-                    current[gh] = name
-                    _bookmarkNames.value = current
-                    persistNames(current)
-                }
+                commitResolvedName(gh, name)
             } catch (e: Exception) {
                 Log.w(TAG, "Bookmark name resolution failed")
             } finally {
-                resolving.remove(gh)
+                finishResolving(gh)
             }
         }
+        resolveJobs[gh] = job
+    }
+
+    @Synchronized
+    private fun finishResolving(geohash: String) {
+        resolving.remove(geohash)
+        resolveJobs.remove(geohash)
+    }
+
+    /**
+     * Stores a resolved friendly name, but only while the geohash is still bookmarked.
+     *
+     * A reverse geocode runs on the IO dispatcher and can outlive the bookmark it was
+     * started for. Without this check a lookup in flight during [remove] or [clearAll]
+     * writes the place name back to SharedPreferences after the wipe, so a panic clear
+     * leaves a resolved location name on disk for the next launch.
+     */
+    @Synchronized
+    internal fun commitResolvedName(geohash: String, name: String?) {
+        val gh = normalize(geohash)
+        if (gh.isEmpty() || name.isNullOrEmpty()) return
+        if (!membership.contains(gh)) return
+        val current = _bookmarkNames.value.toMutableMap()
+        current[gh] = name
+        _bookmarkNames.value = current
+        persistNames(current)
     }
 
     private fun pickNameForLength(len: Int, address: android.location.Address?): String? {

app/src/test/kotlin/com/bitchat/android/geohash/GeohashBookmarkNameCommitTest.kt

@@ -0,0 +1,84 @@
+package com.bitchat.android.geohash
+
+import android.content.Context
+import androidx.test.core.app.ApplicationProvider
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertNull
+import org.junit.Before
+import org.junit.Test
+import org.junit.runner.RunWith
+import org.robolectric.RobolectricTestRunner
+
+/**
+ * A reverse geocode started for a bookmark runs on the IO dispatcher and can finish after
+ * the bookmark is gone. These tests pin the commit step: a resolved name is stored only
+ * while the geohash is still bookmarked.
+ */
+@RunWith(RobolectricTestRunner::class)
+class GeohashBookmarkNameCommitTest {
+
+    // Artificial all-zero geohash and label; neither comes from a device or user location.
+
+    private lateinit var store: GeohashBookmarksStore
+
+    @Before
+    fun setUp() {
+        val context = ApplicationProvider.getApplicationContext<Context>()
+        store = GeohashBookmarksStore.createForTest(context)
+        store.clearAll()
+    }
+
+    @Test
+    fun `resolved name is stored for a bookmark that is still present`() {
+        store.add("000000")
+
+        store.commitResolvedName("000000", "Synthetic bookmark A")
+
+        assertEquals("Synthetic bookmark A", store.bookmarkNames.value["000000"])
+    }
+
+    @Test
+    fun `resolved name is dropped when the bookmark was removed while in flight`() {
+        store.add("000000")
+        store.remove("000000")
+
+        store.commitResolvedName("000000", "Synthetic bookmark A")
+
+        assertNull(store.bookmarkNames.value["000000"])
+    }
+
+    @Test
+    fun `resolved name is dropped when a panic clear happened while in flight`() {
+        store.add("000000")
+        store.clearAll()
+
+        store.commitResolvedName("000000", "Synthetic bookmark A")
+
+        assertNull(store.bookmarkNames.value["000000"])
+        assertEquals(emptyMap<String, String>(), store.bookmarkNames.value)
+    }
+
+    @Test
+    fun `a panic clear survives a restart when a lookup lands after the wipe`() {
+        store.add("000000")
+        store.clearAll()
+        store.commitResolvedName("000000", "Synthetic bookmark A")
+
+        // A fresh store reads back what was persisted, which is what the next launch sees.
+        val reloaded = GeohashBookmarksStore.createForTest(
+            ApplicationProvider.getApplicationContext<Context>()
+        )
+        assertEquals(emptyList<String>(), reloaded.bookmarks.value)
+        assertEquals(emptyMap<String, String>(), reloaded.bookmarkNames.value)
+    }
+
+    @Test
+    fun `an empty or blank name is never stored`() {
+        store.add("000000")
+
+        store.commitResolvedName("000000", null)
+        store.commitResolvedName("000000", "")
+
+        assertNull(store.bookmarkNames.value["000000"])
+    }
+}