Article text, icons and picker fixes

.github/workflows/release.yml

@@ -53,7 +53,8 @@ jobs:
               echo "A newer push releases this"
               exit 0
             fi
-            latest=$(git ls-remote --tags --refs "https://github.com/$GITHUB_REPOSITORY" 'v*' \
+            tags=$(git ls-remote --tags --refs "https://github.com/$GITHUB_REPOSITORY" 'v*') || exit 1
+            latest=$(printf '%s\n' "$tags" \
               | sed 's|.*refs/tags/||' | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+$' | sort -V | tail -n 1)
             if [ -z "$latest" ]; then
               titles=$(printf '%s\n' "$MESSAGE" | head -n 1)

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

@@ -39,7 +39,10 @@ data class PendingMarkEntity(
     @PrimaryKey val itemId: String,
     val read: Boolean,
     val queuedAt: Long,
-    /** Set once a send has started, so the server may have it even if the reply never came. */
+    /**
+     * Set once a send has started, so the server may have it even if the reply never came. Also set
+     * on read marks a mark-all covers, since the mark-all may reach the server first.
+     */
     @ColumnInfo(defaultValue = "0") val attempted: Boolean = false,
 )
 

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

@@ -63,27 +63,29 @@ abstract class QueueDao {
     abstract suspend fun dropUnreadMarksForReadItems()
 
     @Query(
-        "DELETE FROM PendingMarkEntity WHERE read = 1 AND itemId IN " +
+        "UPDATE PendingMarkEntity SET attempted = 1 WHERE read = 1 AND itemId IN " +
             "(SELECT id FROM ItemEntity WHERE publishedSec <= :publishedAtOrBeforeSec)",
     )
-    protected abstract suspend fun dropReadMarksUpTo(publishedAtOrBeforeSec: Long)
+    protected abstract suspend fun markReadMarksAttemptedUpTo(publishedAtOrBeforeSec: Long)
 
     @Query(
-        "DELETE FROM PendingMarkEntity WHERE read = 1 AND itemId IN " +
+        "UPDATE PendingMarkEntity SET attempted = 1 WHERE read = 1 AND itemId IN " +
             "(SELECT id FROM ItemEntity WHERE feedId IN (:feedIds) AND publishedSec <= :publishedAtOrBeforeSec)",
     )
-    protected abstract suspend fun dropReadMarksOfFeedsUpTo(feedIds: List<String>, publishedAtOrBeforeSec: Long)
+    protected abstract suspend fun markReadMarksOfFeedsAttemptedUpTo(feedIds: List<String>, publishedAtOrBeforeSec: Long)
 
     /**
-     * Drops pending read marks for the items a mark-all covers (same scope and cutoff as
-     * [ItemDao.markReadWhere]), so a later unread mark on one of them is queued, not cancelled.
+     * Flags pending read marks for the items a mark-all covers (same scope and cutoff as
+     * [ItemDao.markReadWhere]) as attempted. A later unread mark on one of them then replaces it
+     * instead of cancelling it, and the read marks are still sent after the mark-all, so nothing
+     * is lost if the server applies the cutoff differently or the mark-all is dropped.
      */
     @Transaction
-    open suspend fun dropReadMarksCoveredBy(feedIds: List<String>?, publishedAtOrBeforeSec: Long) {
+    open suspend fun markReadMarksCoveredByAttempted(feedIds: List<String>?, publishedAtOrBeforeSec: Long) {
         if (feedIds == null) {
-            dropReadMarksUpTo(publishedAtOrBeforeSec)
+            markReadMarksAttemptedUpTo(publishedAtOrBeforeSec)
         } else {
-            feedIds.chunked(CHUNK).forEach { dropReadMarksOfFeedsUpTo(it, publishedAtOrBeforeSec) }
+            feedIds.chunked(CHUNK).forEach { markReadMarksOfFeedsAttemptedUpTo(it, publishedAtOrBeforeSec) }
         }
     }
 

app/src/main/java/io/github/bardesss/wick/html/HtmlText.kt

@@ -35,13 +35,14 @@ internal const val MAX_HTML_INPUT = 200_000
 
 /**
  * Turns feed HTML into styled text for the sheet. Returns null when nothing readable is left.
+ * Relative links resolve against [baseUrl], the article address; without one they stay unlinked.
  * Pure, so it can run on any thread.
  */
-fun htmlToAnnotated(html: String?): AnnotatedString? {
+fun htmlToAnnotated(html: String?, baseUrl: String? = null): AnnotatedString? {
     if (html.isNullOrBlank()) return null
     val out = TextWriter()
     // very long feeds are cut rather than parsed whole; the sheet only shows the start anyway
-    out.children(Jsoup.parseBodyFragment(html.take(MAX_HTML_INPUT)).body())
+    out.children(Jsoup.parseBodyFragment(html.take(MAX_HTML_INPUT), baseUrl.orEmpty()).body())
     return out.build()
 }
 
@@ -94,11 +95,13 @@ private class TextWriter {
             tag in headings || tag == "b" || tag == "strong" -> Bold
             tag == "i" || tag == "em" || tag == "cite" || tag == "blockquote" -> Italic
             tag == "pre" || tag == "code" || tag == "kbd" || tag == "samp" || tag == "tt" -> Mono
-            tag == "a" -> el.attr("href").trim().takeIf { href -> linkSchemes.any { href.startsWith(it, ignoreCase = true) } }
+            tag == "a" -> el.absUrl("href").ifEmpty { el.attr("href").trim() }.takeIf { href -> linkSchemes.any { href.startsWith(it, ignoreCase = true) } }
                 ?.let { LinkAnnotation.Url(it, LinkStyles) }
             else -> null
         }
         if (style != null) mark(style, start)
+        // cells share a line, so the next one needs a space; a row end clears it again
+        if (tag == "td" || tag == "th") space = true
         if (block) lineBreak(2)
     }
 

app/src/main/java/io/github/bardesss/wick/html/LeadImage.kt

@@ -7,8 +7,9 @@ private val hidden = Regex("display\\s*:\\s*none", RegexOption.IGNORE_CASE)
 
 /**
  * The address of the first image in feed HTML that is worth showing, or null. Relative addresses
- * are resolved against the article's [link]. Only http and https count, and an image that says it
- * is up to two pixels wide or high, or is hidden by its style (a tracking pixel), is passed over.
+ * are resolved against the article's [link]. Only http and https count; an http address is
+ * returned as https so the image loads on a secure connection. An image that says it is up to two
+ * pixels wide or high, or is hidden by its style (a tracking pixel), is passed over.
  */
 fun leadImageUrl(html: String?, link: String?): String? {
     if (html.isNullOrBlank()) return null
@@ -17,7 +18,8 @@ fun leadImageUrl(html: String?, link: String?): String? {
         if (side(img.attr("width")) in tinySides || side(img.attr("height")) in tinySides) continue
         if (hidden.containsMatchIn(img.attr("style"))) continue
         val url = img.absUrl("src").trim()
-        if (url.startsWith("http://", ignoreCase = true) || url.startsWith("https://", ignoreCase = true)) return url
+        if (url.startsWith("https://", ignoreCase = true)) return url
+        if (url.startsWith("http://", ignoreCase = true)) return "https://" + url.substring("http://".length)
     }
     return null
 }

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

@@ -100,7 +100,7 @@ class MarkQueue(
         ops.trySend {
             inTransaction {
                 itemDao.markReadWhere(feedIds, newestLoadedSec)
-                queueDao.dropReadMarksCoveredBy(feedIds, newestLoadedSec)
+                queueDao.markReadMarksCoveredByAttempted(feedIds, newestLoadedSec)
                 queueDao.dropUnreadMarksForReadItems()
                 queueDao.enqueueAll(
                     PendingMarkAllEntity(streamRaw = stream.raw, olderThanUsec = (newestLoadedSec + 1) * 1_000_000),

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

@@ -23,14 +23,15 @@ import java.util.concurrent.TimeUnit
 
 private const val KEY_FLUSH_ONLY = "flushOnly"
 
-/** How many runs a flush-only job gets before it leaves the marks for the next sync. */
+/** How many runs a flush-only job gets in total, counting the first, before it leaves the marks for the next sync. */
 internal const val MAX_FLUSH_ATTEMPTS = 5
 
 /**
  * Sends the queued marks and, unless [SyncWorker.input] asked for the flush alone, brings the
  * database up to date. A full sync always reports success: the next periodic run tries again,
  * so a dead server cannot cause a retry storm. A flush-only job asks to be retried with backoff
- * while marks are still queued, up to [MAX_FLUSH_ATTEMPTS] runs, then leaves them to the next sync.
+ * while marks are still queued, up to [MAX_FLUSH_ATTEMPTS] runs in all (runAttemptCount counts from 0),
+ * then leaves them to the next sync.
  */
 class SyncWorker(
     context: Context,
@@ -45,7 +46,7 @@ class SyncWorker(
         val flushOnly = inputData.getBoolean(KEY_FLUSH_ONLY, false)
         try {
             if (flushOnly) {
-                if (!queue().flush() && runAttemptCount < MAX_FLUSH_ATTEMPTS) return Result.retry()
+                if (!queue().flush() && runAttemptCount < MAX_FLUSH_ATTEMPTS - 1) return Result.retry()
             } else {
                 // a failure here is an ApiError that the next run tries again
                 engine().backgroundSync()
@@ -54,7 +55,7 @@ class SyncWorker(
             throw e
         } catch (e: Exception) {
             Log.e(LOG_TAG, "background sync failed", e)
-            if (flushOnly && runAttemptCount < MAX_FLUSH_ATTEMPTS) return Result.retry()
+            if (flushOnly && runAttemptCount < MAX_FLUSH_ATTEMPTS - 1) return Result.retry()
         }
         return Result.success()
     }

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

@@ -26,7 +26,7 @@ import coil3.compose.AsyncImagePainter
 import io.github.bardesss.wick.api.KnownApiPaths
 import io.github.bardesss.wick.ui.theme.InstrumentSans
 import io.github.bardesss.wick.ui.theme.WickColors
-import java.net.URI
+import okhttp3.HttpUrl.Companion.toHttpUrlOrNull
 
 /** The site the API lives on: the API URL without a trailing known API path, else the URL itself. */
 fun siteRoot(apiUrl: String): String {
@@ -36,19 +36,17 @@ fun siteRoot(apiUrl: String): String {
 }
 
 /**
- * Absolute URLs as they are, relative ones against the site of the API at [server], null when
- * there is no icon. Only http and https come back, so the image loader never reads a local file.
+ * Absolute URLs normalised by HttpUrl, relative ones resolved against the site of the API at
+ * [server], null when there is no icon. Only http and https come back, so the image loader never
+ * reads a local file.
  */
 fun resolveIcon(server: String, iconUrl: String?): String? {
     val icon = iconUrl?.trim()
     if (icon.isNullOrEmpty()) return null
-    val resolved = try {
-        val uri = URI(icon)
-        if (uri.isAbsolute) icon else URI(siteRoot(server) + "/").resolve(uri).toString()
-    } catch (e: Exception) {
-        return null
-    }
-    return resolved.takeIf(::isWebUrl)
+    // HttpUrl encodes spaces and similar characters and only accepts http and https
+    val resolved = icon.toHttpUrlOrNull()
+        ?: (siteRoot(server) + "/").toHttpUrlOrNull()?.resolve(icon)
+    return resolved?.toString()
 }
 
 private val TileShape = RoundedCornerShape(6.dp)

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

@@ -8,9 +8,21 @@ import coil3.fetch.SourceFetchResult
 import coil3.request.Options
 import coil3.svg.SvgDecoder
 import okio.Buffer
+import okio.BufferedSource
+import java.io.IOException
 
-// a declaration such as fill:color(display-p3 .98 .3 0), with or without its semicolon
-private val WideGamutDeclaration = Regex("""[a-zA-Z-]+\s*:\s*color\([^)]*\)\s*;?""")
+// a declaration such as fill:color(display-p3 .98 .3 0), with or without its semicolon; a match
+// only starts at the start of a property name, so a long run of letters stays linear
+private val WideGamutDeclaration = Regex("""(?<![a-zA-Z-])[a-zA-Z-]+\s*:\s*color\([^)]*\)\s*;?""")
+
+/** Largest svg the icon decoder reads; anything bigger fails the decode and the row keeps its letter tile. */
+internal const val MAX_SVG_BYTES = 1_000_000L
+
+/** Reads the whole source as text, throwing [IOException] when it holds more than [maxBytes]. */
+internal fun readSvgText(source: BufferedSource, maxBytes: Long = MAX_SVG_BYTES): String {
+    if (source.request(maxBytes + 1)) throw IOException("svg is larger than $maxBytes bytes")
+    return source.readUtf8()
+}
 
 /**
  * Design tools write a plain colour followed by a display-p3 one, for example
@@ -22,7 +34,7 @@ fun withoutWideGamutColours(svg: String): String = WideGamutDeclaration.replace(
 /** Decodes svg icons through [withoutWideGamutColours]; anything else is left to other decoders. */
 class SvgIconDecoder(private val result: SourceFetchResult, private val options: Options) : Decoder {
     override suspend fun decode(): DecodeResult {
-        val text = result.source.source().use { it.readUtf8() }
+        val text = result.source.source().use { readSvgText(it) }
         val source = ImageSource(Buffer().writeUtf8(withoutWideGamutColours(text)), options.fileSystem)
         return SvgDecoder(source, options).decode()
     }

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

@@ -324,7 +324,7 @@ class ListViewModel(
     suspend fun sheetBody(id: String, link: String?): SheetBody.Ready = withContext(work) {
         val html = itemDao.htmlOf(id)
         val image = if (settings.leadImages.first()) leadImageUrl(html, link) else null
-        SheetBody.Ready(htmlToAnnotated(html), image)
+        SheetBody.Ready(htmlToAnnotated(html, link), image)
     }
 
     /** Queued marks are sent by the flush job the app enqueues when it stops. */

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

@@ -140,12 +140,20 @@ fun LoginContent(state: LoginState, onSignIn: (String, String, String) -> Unit)
             onClick = submit,
             enabled = !state.busy && server.isNotBlank() && user.isNotBlank() && key.isNotBlank(),
             shape = RoundedCornerShape(10.dp),
-            colors = ButtonDefaults.buttonColors(
-                containerColor = WickColors.Amber,
-                contentColor = WickColors.Night,
-                disabledContainerColor = WickColors.Amber,
-                disabledContentColor = WickColors.Night,
-            ),
+            // while busy the button keeps its enabled look behind the spinner; blank fields get the usual disabled look
+            colors = if (state.busy) {
+                ButtonDefaults.buttonColors(
+                    containerColor = WickColors.Amber,
+                    contentColor = WickColors.Night,
+                    disabledContainerColor = WickColors.Amber,
+                    disabledContentColor = WickColors.Night,
+                )
+            } else {
+                ButtonDefaults.buttonColors(
+                    containerColor = WickColors.Amber,
+                    contentColor = WickColors.Night,
+                )
+            },
             modifier = Modifier
                 .padding(top = 28.dp)
                 .fillMaxWidth()

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

@@ -193,6 +193,7 @@ private fun EntryRow(
         Row(
             Modifier
                 .weight(1f)
+                .heightIn(min = TapTarget)
                 .semantics { this.selected = selected }
                 .clickable(role = Role.Button, onClick = onClick)
                 .padding(start = if (folder != null) 8.dp else start, end = 22.dp, top = 11.dp, bottom = 11.dp),

app/src/test/java/io/github/bardesss/wick/html/HtmlTextTest.kt

@@ -86,4 +86,36 @@ class HtmlTextTest {
         val out = htmlToAnnotated("<div><p>One</p><br><br><br><div></div><p>Two</p></div>")
         assertEquals("One\n\nTwo", out?.text)
     }
+
+    @Test
+    fun relative_link_resolves_against_the_article() {
+        val out = htmlToAnnotated("<p><a href=\"/p\">x</a></p>", "https://site.example/a/b")!!
+        val link = out.getLinkAnnotations(0, out.length).single()
+        assertEquals("https://site.example/p", (link.item as LinkAnnotation.Url).url)
+    }
+
+    @Test
+    fun protocol_relative_link_gets_the_scheme() {
+        val out = htmlToAnnotated("<p><a href=\"//cdn.example/x\">x</a></p>", "https://site.example/a/b")!!
+        val link = out.getLinkAnnotations(0, out.length).single()
+        assertEquals("https://cdn.example/x", (link.item as LinkAnnotation.Url).url)
+    }
+
+    @Test
+    fun relative_link_without_a_base_stays_unlinked() {
+        val out = htmlToAnnotated("<p><a href=\"/p\">x</a></p>")!!
+        assertEquals(0, out.getLinkAnnotations(0, out.length).size)
+    }
+
+    @Test
+    fun table_cells_are_separated() {
+        val out = htmlToAnnotated("<table><tr><td>Price</td><td>10</td></tr></table>")
+        assertEquals("Price 10", out?.text)
+    }
+
+    @Test
+    fun table_rows_stay_on_their_own_lines() {
+        val out = htmlToAnnotated("<table><tr><th>A</th><td>1</td></tr><tr><th>B</th><td>2</td></tr></table>")
+        assertEquals("A 1\n\nB 2", out?.text)
+    }
 }

app/src/test/java/io/github/bardesss/wick/html/LeadImageTest.kt

@@ -44,12 +44,18 @@ class LeadImageTest {
         assertEquals("https://e.x/real.jpg", leadImageUrl(html, null))
     }
 
+    @Test
+    fun http_image_is_upgraded_to_https() {
+        assertEquals("https://a.example/i.jpg", leadImageUrl("<img src=\"http://a.example/i.jpg\">", null))
+        assertEquals("https://a.example/i.jpg", leadImageUrl("<img src=\"HTTP://a.example/i.jpg\">", null))
+    }
+
     @Test
     fun anything_but_http_is_ignored() {
         val html = "<img src=\"data:image/png;base64,AAAA\"><img src=\"ftp://e.x/a.jpg\">" +
             "<img src=\"file:///sdcard/a.jpg\"><img src=\"http://e.x/ok.jpg\">"
 
-        assertEquals("http://e.x/ok.jpg", leadImageUrl(html, "https://e.x/post"))
+        assertEquals("https://e.x/ok.jpg", leadImageUrl(html, "https://e.x/post"))
     }
 
     @Test

app/src/test/java/io/github/bardesss/wick/sync/MarkQueueTest.kt

@@ -82,6 +82,42 @@ class MarkQueueTest {
         assertTrue(db.queueDao().pending().isEmpty())
     }
 
+    @Test
+    fun mark_all_keeps_the_read_marks_it_covers_and_sends_them_after_it() = runTest {
+        seed("a", "b")
+        val q = queue()
+        q.mark("a", read = true, manual = false)
+        q.markAll(StreamId.All, null, newestLoadedSec = 100)
+        runCurrent()
+
+        val pending = db.queueDao().pending()
+        assertEquals(listOf("a"), pending.map { it.itemId })
+        assertTrue(pending.single().attempted)
+
+        var markAllsBeforeSetRead = -1
+        api.beforeSetRead = { markAllsBeforeSetRead = api.markAllCalls.size }
+        assertTrue(q.flush())
+        assertEquals(1, markAllsBeforeSetRead)
+        assertEquals(listOf(listOf("a") to true), api.setReadCalls)
+        assertTrue(db.queueDao().pending().isEmpty())
+    }
+
+    @Test
+    fun mark_all_rejected_with_4xx_still_sends_the_read_marks_it_covered() = runTest {
+        seed("a", "b")
+        val q = queue()
+        q.mark("a", read = true, manual = false)
+        q.markAll(StreamId.All, null, newestLoadedSec = 100)
+        runCurrent()
+
+        api.failNext(1, ApiError.Http(404))
+        assertTrue(q.flush())
+        assertTrue(db.queueDao().pendingAll().isEmpty())
+        assertTrue(api.isRead("a"))
+        assertFalse(api.isRead("b"))
+        assertTrue(db.queueDao().pending().isEmpty())
+    }
+
     @Test
     fun mark_all_keeps_read_marks_it_does_not_cover() = runTest {
         seed("a", publishedSec = 100)