List screen and recently read fixes

app/src/main/java/io/github/bardesss/wick/db/ItemDao.kt

@@ -47,10 +47,10 @@ abstract class ItemDao {
     @Query("SELECT id, readAtMs FROM ItemEntity WHERE readAtMs IS NOT NULL AND id IN (:ids)")
     protected abstract suspend fun readAtChunk(ids: List<String>): List<ReadAt>
 
-    /** Sets the read state of one item; marking it read also stamps it with [nowMs] for the recently read view. */
+    /** Sets the read state of one item; marking it read stamps it with [nowMs] for the recently read view, marking it unread clears the stamp. */
     @Query(
         "UPDATE ItemEntity SET read = :read, manualUnread = :manualUnread, " +
-            "readAtMs = CASE WHEN :read THEN :nowMs ELSE readAtMs END WHERE id = :id",
+            "readAtMs = CASE WHEN :read THEN :nowMs ELSE NULL END WHERE id = :id",
     )
     abstract suspend fun setRead(id: String, read: Boolean, manualUnread: Boolean, nowMs: Long)
 
@@ -99,7 +99,7 @@ abstract class ItemDao {
         "SELECT id FROM ItemEntity WHERE read = 1 " +
             "AND id NOT IN (SELECT itemId FROM PendingMarkEntity) " +
             "AND id NOT IN (SELECT itemId FROM SnapshotEntity) " +
-            "AND id NOT IN (SELECT id FROM ItemEntity WHERE readAtMs IS NOT NULL ORDER BY readAtMs DESC LIMIT :kept)",
+            "AND id NOT IN (SELECT id FROM ItemEntity WHERE read = 1 AND readAtMs IS NOT NULL ORDER BY readAtMs DESC, id ASC LIMIT :kept)",
     )
     protected abstract suspend fun prunable(kept: Int): List<String>
 
@@ -118,6 +118,10 @@ abstract class ItemDao {
     @Query("SELECT EXISTS(SELECT 1 FROM ItemEntity)")
     abstract suspend fun any(): Boolean
 
+    /** Whether the item is read locally, or null when it is not stored. */
+    @Query("SELECT read FROM ItemEntity WHERE id = :id")
+    abstract suspend fun isRead(id: String): Boolean?
+
     @Query("SELECT html FROM ItemEntity WHERE id = :id")
     abstract suspend fun htmlOf(id: String): String?
 

app/src/main/java/io/github/bardesss/wick/sync/MarkQueue.kt

@@ -74,16 +74,26 @@ class MarkQueue(
     }
 
     fun mark(id: String, read: Boolean, manual: Boolean) {
+        ops.trySend { stateLock.withLock { inTransaction { applyMark(id, read, manual) } } }
+    }
+
+    /** Flips the stored read state, decided when the op runs so quick taps cancel out. */
+    fun toggle(id: String) {
         ops.trySend {
             stateLock.withLock {
                 inTransaction {
-                    itemDao.setRead(id, read, manualUnread = manual && !read, nowMs = clock())
-                    queueDao.enqueueOrCancel(PendingMarkEntity(id, read, nextStamp()), inFlight = id in inFlight)
+                    val wasRead = itemDao.isRead(id)
+                    if (wasRead != null) applyMark(id, read = !wasRead, manual = wasRead)
                 }
             }
         }
     }
 
+    private suspend fun applyMark(id: String, read: Boolean, manual: Boolean) {
+        itemDao.setRead(id, read, manualUnread = manual && !read, nowMs = clock())
+        queueDao.enqueueOrCancel(PendingMarkEntity(id, read, nextStamp()), inFlight = id in inFlight)
+    }
+
     // The server marks only items strictly older than the cutoff, in whole seconds, so ask for
     // the start of the next second to include items published at newestLoadedSec.
     fun markAll(stream: StreamId, feedIds: List<String>?, newestLoadedSec: Long) {

app/src/main/java/io/github/bardesss/wick/ui/Links.kt

@@ -11,13 +11,15 @@ import io.github.bardesss.wick.data.LinkTarget
 import io.github.bardesss.wick.ui.theme.WickColors
 
 /** True for http and https links, the only ones a feed may open. */
-fun isWebUrl(url: String): Boolean =
-    url.startsWith("http://", ignoreCase = true) || url.startsWith("https://", ignoreCase = true)
+fun isWebUrl(url: String): Boolean {
+    val trimmed = url.trim()
+    return trimmed.startsWith("http://", ignoreCase = true) || trimmed.startsWith("https://", ignoreCase = true)
+}
 
 /** The intent that opens [url] for [target], or null for anything but http and https. */
 fun linkIntent(url: String, target: LinkTarget): Intent? {
     if (!isWebUrl(url)) return null
-    val uri = url.toUri()
+    val uri = url.trim().toUri().normalizeScheme()
     return when (target) {
         LinkTarget.DefaultApp -> Intent(Intent.ACTION_VIEW, uri)
         LinkTarget.InApp -> {
@@ -38,12 +40,13 @@ fun linkIntent(url: String, target: LinkTarget): Intent? {
 
 /**
  * Opens [url] the way [target] says. A Custom Tab falls back to a plain view intent when no
- * browser takes it. Other schemes are ignored.
+ * browser takes it. Other schemes are ignored. Returns whether an activity started.
  */
-fun openLink(context: Context, url: String, target: LinkTarget) {
-    val intent = linkIntent(url, target) ?: return
-    if (start(context, intent) || target != LinkTarget.InApp) return
-    start(context, Intent(Intent.ACTION_VIEW, url.toUri()))
+fun openLink(context: Context, url: String, target: LinkTarget): Boolean {
+    val intent = linkIntent(url, target) ?: return false
+    if (start(context, intent)) return true
+    if (target != LinkTarget.InApp) return false
+    return start(context, Intent(Intent.ACTION_VIEW, url.trim().toUri().normalizeScheme()))
 }
 
 // false when nothing on the device can open it

app/src/main/java/io/github/bardesss/wick/ui/list/ListScreen.kt

@@ -211,14 +211,19 @@ fun ListScreen(vm: ListViewModel, onOpenSettings: () -> Unit) {
     val markedRead = stringResource(R.string.list_marked_read)
     val markedUnread = stringResource(R.string.list_marked_unread)
     var sheetId by rememberSaveable { mutableStateOf<String?>(null) }
-    val sheetKept = remember { arrayOfNulls<RowModel>(1) }
 
     val linkTarget by vm.linkTarget.collectAsStateWithLifecycle(LinkTarget.InApp)
-    val open = remember(vm, context, linkTarget) {
+    val noBrowser = stringResource(R.string.list_no_browser)
+    val open = remember(vm, context, linkTarget, noBrowser) {
         { row: RowModel ->
             row.url?.takeIf(::isWebUrl)?.let { url ->
-                openLink(context, url, linkTarget)
-                vm.open(row.id)
+                // a row is read only once its link really opened
+                if (openLink(context, url, linkTarget)) {
+                    vm.open(row.id)
+                } else {
+                    toast[0]?.cancel()
+                    toast[0] = scope.launch { snackbar.showSnackbar(noBrowser) }
+                }
             }
             Unit
         }
@@ -290,8 +295,13 @@ fun ListScreen(vm: ListViewModel, onOpenSettings: () -> Unit) {
     }
 
     // a refresh may drop the row; the open sheet keeps showing it until the user closes it
-    val sheetRow = sheetId?.let { id -> rows?.firstOrNull { it.id == id } ?: sheetKept[0]?.takeIf { it.id == id } }
-    sheetKept[0] = sheetRow
+    // the kept row lives in the view model so it survives rotation too
+    val sheetRow = sheetId?.let { id -> rows?.firstOrNull { it.id == id } ?: vm.sheetKept?.takeIf { it.id == id } }
+    vm.sheetKept = sheetRow
+    // an id with no row anywhere (process death) must not reopen the sheet by itself later
+    LaunchedEffect(sheetId, rows) {
+        if (sheetId != null && rows != null && sheetRow == null) sheetId = null
+    }
     if (sheetRow != null) {
         val sheetState = rememberModalBottomSheetState(skipPartiallyExpanded = true)
         val body by produceState<SheetBody>(SheetBody.Loading, sheetRow.id) {
@@ -362,8 +372,16 @@ fun ListContent(
     var query by rememberSaveable(history) { mutableStateOf(searchPreview?.takeIf { history }) }
     BackHandler(enabled = query != null) { query = null }
     val found = remember(rows, query) { query?.let { q -> rows?.let { searchRows(it, q) } } ?: rows }
-    // each new search starts at the top of what it found
-    LaunchedEffect(query) { if (query != null) listState.scrollToItem(0) }
+    // each new search starts at the top of what it found; a restored one keeps its position
+    var scrolledFor by rememberSaveable { mutableStateOf<String?>(null) }
+    LaunchedEffect(query) {
+        if (query == null) {
+            scrolledFor = null
+        } else if (query != scrolledFor) {
+            listState.scrollToItem(0)
+            scrolledFor = query
+        }
+    }
     val frameSink by rememberUpdatedState(onFrame)
     val shownRows by rememberUpdatedState(found)
     LaunchedEffect(listState) {

app/src/main/java/io/github/bardesss/wick/ui/list/ListViewModel.kt

@@ -56,6 +56,8 @@ import kotlinx.coroutines.flow.mapLatest
 import kotlinx.coroutines.flow.shareIn
 import kotlinx.coroutines.flow.stateIn
 import kotlinx.coroutines.launch
+import kotlinx.coroutines.sync.Mutex
+import kotlinx.coroutines.sync.withLock
 import kotlinx.coroutines.withContext
 
 /** What a row shows apart from its read state, which changes on its own (see [ListViewModel.read]). */
@@ -103,6 +105,8 @@ class ListViewModel(
     private var busy = false
     private var again = false
     private var applying = false
+    // one snapshot write at a time, so the check of the view and the write go together
+    private val snapshotLock = Mutex()
     private var backgroundedAt: Long? = null
 
     /**
@@ -123,6 +127,9 @@ class ListViewModel(
         .flowOn(work)
         .shareIn(viewModelScope, SharingStarted.Eagerly, replay = 1)
 
+    /** The row the open sheet shows, so it outlives a refresh that drops it and a rotation. */
+    var sheetKept: RowModel? = null
+
     /** Ids of the snapshot rows that are read. */
     val read: StateFlow<Set<String>> = marks.map { it.read }
         .distinctUntilChanged()
@@ -238,7 +245,8 @@ class ListViewModel(
     /**
      * Marks the whole view read on the server. The cutoff is the newest of the rows loaded and the
      * unread items stored for the view, so it covers what the header counts, whether or not the
-     * list has caught up. With neither, it is now. Items that arrive later stay unread.
+     * list has caught up. With neither, it is now. Items that arrive later stay unread. The rows
+     * count only when the stored list belongs to this view; right after a switch it may not yet.
      */
     fun markAllRead() {
         val v = _view.value
@@ -247,7 +255,8 @@ class ListViewModel(
         if (loaded.isNullOrEmpty() && count.value <= 0) return
         viewModelScope.launch {
             val feedIds = v.feedIds(feedDao.feedsNow())
-            val newest = listOfNotNull(loaded?.maxOfOrNull { it.publishedSec }, itemDao.newestUnreadSec(feedIds)).maxOrNull()
+            val ours = if (snapshotDao.viewRaw() == v.raw) loaded else null
+            val newest = listOfNotNull(ours?.maxOfOrNull { it.publishedSec }, itemDao.newestUnreadSec(feedIds)).maxOrNull()
                 ?: (clock() / 1000)
             queue.markAll(v, feedIds, newest)
             // the refresh must see the mark-all in the queue
@@ -277,7 +286,8 @@ class ListViewModel(
         var firstIndex = Int.MAX_VALUE
         for (item in frame.items) {
             current[item.id] = item.index
-            if (item.index < firstIndex) firstIndex = item.index
+            // a row kept composed above the viewport must not hold the first index back
+            if (item.offset + item.size > frame.viewportStart && item.index < firstIndex) firstIndex = item.index
             if (item.id in readNow || item.size <= 0) continue
             val top = maxOf(item.offset, frame.viewportStart)
             val bottom = minOf(item.offset + item.size, frame.viewportEnd)
@@ -298,7 +308,7 @@ class ListViewModel(
     }
 
     fun toggleRead(id: String) {
-        if (id in read.value) queue.mark(id, read = false, manual = true) else queue.mark(id, read = true, manual = false)
+        queue.toggle(id)
     }
 
     /** Whether ticks are on; the screen follows it in its [io.github.bardesss.wick.ui.Haptics]. */
@@ -356,10 +366,10 @@ class ListViewModel(
                     }
                     result.fold(
                         onSuccess = { r ->
-                            if (v == _view.value) {
-                                replaceSnapshot(r.ids, clock(), v)
+                            if (replaceSnapshot(r.ids, clock(), v)) {
                                 _capped.value = r.capped
-                            } else {
+                            } else if (v != _view.value) {
+                                // only a new view is worth another round; a failed write alone would loop
                                 again = true
                             }
                             _message.value = if (r.marksStuck) Note(R.string.note_marks_unsent) else null
@@ -389,7 +399,7 @@ class ListViewModel(
         val synced = sync.backgroundSync()
         val ids = storedIds(StreamId.Read)
         return synced.map { Refreshed(ids, capped = false, marksStuck = false) }.onFailure {
-            if (_view.value == StreamId.Read) replaceSnapshot(ids, clock(), StreamId.Read)
+            replaceSnapshot(ids, clock(), StreamId.Read)
         }
     }
 
@@ -401,21 +411,29 @@ class ListViewModel(
             itemDao.unreadIds(view.feedIds(feedDao.feedsNow()), settings.oldestFirst.first())
         }
 
-    /** Swaps the snapshot with scroll marking paused. On failure the old list stays. */
-    private suspend fun replaceSnapshot(ids: List<String>, writtenAtMs: Long, view: StreamId) {
-        applying = true
-        try {
-            snapshotDao.replaceSnapshot(ids, writtenAtMs, view.raw)
-            tracker.reset()
-            lastFrameIndex = HashMap()
-        } catch (e: CancellationException) {
-            throw e
-        } catch (e: Exception) {
-            Log.w(LOG_TAG, "could not store the new list", e)
-        } finally {
-            applying = false
+    /**
+     * Swaps the snapshot with scroll marking paused, but only while [view] is still the one
+     * showing. Returns whether it wrote; on failure the old list stays.
+     */
+    private suspend fun replaceSnapshot(ids: List<String>, writtenAtMs: Long, view: StreamId): Boolean =
+        snapshotLock.withLock {
+            // the user moved on while the list was being built
+            if (_view.value != view) return@withLock false
+            applying = true
+            try {
+                snapshotDao.replaceSnapshot(ids, writtenAtMs, view.raw)
+                tracker.reset()
+                lastFrameIndex = HashMap()
+                true
+            } catch (e: CancellationException) {
+                throw e
+            } catch (e: Exception) {
+                Log.w(LOG_TAG, "could not store the new list", e)
+                false
+            } finally {
+                applying = false
+            }
         }
-    }
 
     private fun idsWhere(list: List<LocalState>, test: (LocalState) -> Boolean): Set<String> =
         list.filter(test).mapTo(HashSet()) { it.id }

app/src/main/java/io/github/bardesss/wick/ui/sheet/SummarySheet.kt

@@ -61,6 +61,7 @@ import io.github.bardesss.wick.ui.benchTags
 import io.github.bardesss.wick.ui.short
 import io.github.bardesss.wick.ui.spoken
 import io.github.bardesss.wick.ui.list.RowModel
+import io.github.bardesss.wick.ui.isWebUrl
 import io.github.bardesss.wick.ui.openLink
 import io.github.bardesss.wick.ui.theme.InstrumentSans
 import io.github.bardesss.wick.ui.theme.Newsreader
@@ -163,7 +164,9 @@ private fun SheetContent(row: RowModel, body: SheetBody, nowSec: Long, linkTarge
     // links in the text open the same way as the list does
     val links = remember(context, linkTarget) {
         object : UriHandler {
-            override fun openUri(uri: String) = openLink(context, uri, linkTarget)
+            override fun openUri(uri: String) {
+                openLink(context, uri, linkTarget)
+            }
         }
     }
     // the cap sits on the content; a height limit on the sheet itself throws off its anchors
@@ -211,7 +214,7 @@ private fun SheetContent(row: RowModel, body: SheetBody, nowSec: Long, linkTarge
                 }
             }
         }
-        if (row.url != null) Button(
+        if (row.url?.let(::isWebUrl) == true) Button(
             onClick = onOpen,
             shape = RoundedCornerShape(14.dp),
             colors = ButtonDefaults.buttonColors(containerColor = WickColors.Amber, contentColor = WickColors.Night),

app/src/main/res/values/strings.xml

@@ -24,6 +24,7 @@
     </plurals>
     <string name="list_marked_read">Marked read</string>
     <string name="list_marked_unread">Marked unread</string>
+    <string name="list_no_browser">No app can open this link</string>
     <string name="sheet_age">%1$s ago</string>
     <string name="age_now">now</string>
     <string name="age_minutes">%1$dm</string>

app/src/test/java/io/github/bardesss/wick/db/ItemDaoTest.kt

@@ -9,6 +9,7 @@ import kotlinx.coroutines.test.runTest
 import org.junit.After
 import org.junit.Assert.assertEquals
 import org.junit.Assert.assertFalse
+import org.junit.Assert.assertNull
 import org.junit.Assert.assertTrue
 import org.junit.Before
 import org.junit.Test
@@ -74,6 +75,37 @@ class ItemDaoTest {
         assertEquals(listOf("b"), items.recentlyRead(10))
     }
 
+    @Test
+    fun marking_unread_clears_the_read_time() = runTest {
+        items.upsertKeepingLocal(listOf(item("a")))
+        items.setRead("a", read = true, manualUnread = false, nowMs = 10)
+        items.setRead("a", read = false, manualUnread = true, nowMs = 20)
+
+        assertNull(row("a").readAtMs)
+    }
+
+    @Test
+    fun unread_then_mark_all_stays_out_of_recently_read() = runTest {
+        items.upsertKeepingLocal(listOf(item("x")))
+        items.setRead("x", read = true, manualUnread = false, nowMs = 10)
+        items.setRead("x", read = false, manualUnread = true, nowMs = 20)
+        items.markReadWhere(null, publishedAtOrBeforeSec = 1000)
+
+        assertFalse("x" in items.recentlyRead(10))
+    }
+
+    @Test
+    fun prune_keeps_the_configured_number_of_read_rows() = runTest {
+        val ids = (1..RECENT_KEPT + 5).map { "r$it" }
+        items.upsertKeepingLocal(ids.map { item(it) })
+        ids.forEachIndexed { i, id -> items.setRead(id, read = true, manualUnread = false, nowMs = i + 1L) }
+        ids.takeLast(5).forEach { items.setRead(it, read = false, manualUnread = true, nowMs = 1000) }
+
+        items.pruneReadNotIn(emptySet())
+
+        assertEquals(RECENT_KEPT, items.recentlyRead(RECENT_KEPT).size)
+    }
+
     @Test
     fun upsert_keeps_when_an_item_was_read_here() = runTest {
         items.upsertKeepingLocal(listOf(item("a")))