laantungir/amethyst
1
0
Fork 0
mirror of https://github.com/vitorpamplona/amethyst.git synced 2026-10-06 03:38:23 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(browser): the two things that looked wrong in the pill and the Recent list

Browse Source 
**The tile grid stranded its remainder.** Six actions filled a row of four and
left two beside a hole. The rows now share the tiles out evenly — six is 3 + 3,
five is 3 + 2 — and a gap only appears when the count will not divide, which
for seven is 4 + 3 either way. Rows are still padded back out to four where
they draw, so every tile keeps the same width.

`balancedRowSizes` returns the row sizes rather than one column count: the
first shape of this returned a single number and `chunked` it, which quietly
gave 4 + 4 + 2 for ten instead of 4 + 3 + 3. The test caught that, so the
function changed rather than the expectation.

**Two rows in Recent both read "Primal / primal.net".** They are not
duplicates — they are `primal.net/home` and `primal.net`, two pages that
happen to share a `<title>`. The second line was only ever the host, so there
was no way to tell them apart or to know which one a tap would open. It now
shows host and path, minus the scheme and a bare trailing slash:
`primal.net/home` against `primal.net`.

While in there, `BrowserHistoryRegistry` now keys on a normalised URL instead
of the raw string. That is not what caused the pair above, but a trailing
slash, a different case in the host or a leftover `#fragment` would each have
produced a genuine duplicate, and `distinctBy { it.url }` could not see it.

Verified on an SM-T220: the Recent rows read `primal.net/home` and
`primal.net`, and the six-tile pill renders as two rows of three.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Vitor Pamplona
2026-09-26 20:42:32 -04:00
co-authored by Claude Opus 5
parent dc5932a812
commit 0dca3cb11b
6 changed files with 299 additions and 10 deletions
Download Patch File Download Diff File Expand all files Collapse all files
amethyst/src/main/java/com/vitorpamplona/amethyst/favorites/BrowserHistoryRegistry.kt
+32 -4
View File
@@ -101,8 +101,9 @@ object BrowserHistoryRegistry {
) {
val host = OmniboxInput.hostOf(url) ?: url
val now = System.currentTimeMillis()
val key = historyKey(url)
update { current ->
val existing = current.firstOrNull { it.url == url }
val existing = current.firstOrNull { historyKey(it.url) == key }
val entry =
if (existing != null) {
existing.copy(
@@ -114,18 +115,22 @@ object BrowserHistoryRegistry {
} else {
BrowserHistoryEntry(url = url, title = title, host = host, lastVisitedAt = now, visitCount = 1)
}
(listOf(entry) + current.filterNot { it.url == url }).take(MAX_ENTRIES)
(listOf(entry) + current.filterNot { historyKey(it.url) == key }).take(MAX_ENTRIES)
}
}
fun remove(url: String) = update { current -> current.filterNot { it.url == url } }
fun remove(url: String) =
update { current ->
val key = historyKey(url)
current.filterNot { historyKey(it.url) == key }
}
fun clear() = update { emptyList() }
private fun dedupeNewestFirst(list: List<BrowserHistoryEntry>): List<BrowserHistoryEntry> =
list
.sortedByDescending { it.lastVisitedAt }
.distinctBy { it.url }
.distinctBy { historyKey(it.url) }
.take(MAX_ENTRIES)
private inline fun update(transform: (List<BrowserHistoryEntry>) -> List<BrowserHistoryEntry>) {
@@ -152,3 +157,26 @@ object BrowserHistoryRegistry {
emptyList()
}
}
/**
* What counts as "the same page" in the history list.
*
* Keying on the raw URL string put `primal.net` in the list twice: a trailing slash, a different case
* in the host, or a leftover `#fragment` reads as one page and compares as two Strings. So the scheme
* and host are lowercased, a fragment is dropped, and a trailing slash is dropped when the path is
* nothing but that slash.
*
* The path and the query are kept and compared as-is. Two pages of the same site are two entries, and
* guessing which query parameters are load-bearing is how you lose one of them.
*/
internal fun historyKey(url: String): String {
val noFragment = url.substringBefore('#')
val schemeEnd = noFragment.indexOf("://")
if (schemeEnd <= 0) return noFragment
val scheme = noFragment.substring(0, schemeEnd).lowercase()
val rest = noFragment.substring(schemeEnd + 3)
val authorityEnd = rest.indexOfFirst { it == '/' || it == '?' }
val authority = (if (authorityEnd < 0) rest else rest.substring(0, authorityEnd)).lowercase()
val tail = if (authorityEnd < 0) "" else rest.substring(authorityEnd)
return scheme + "://" + authority + if (tail == "/") "" else tail
}
amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/browser/BrowserScreen.kt
+22 -1
View File
@@ -745,7 +745,11 @@ private fun RecentRow(
overflow = TextOverflow.Ellipsis,
)
Text(
entry.host,
// Host and path, not just the host: two pages of one site usually carry the
// same <title>, so `primal.net/home` and `primal.net` arrived as two rows
// reading "Primal / primal.net" and there was no way to tell them apart or
// to know which one a tap would open.
remember(entry.url, entry.host) { recentSubtitle(entry.url, entry.host) },
style = MaterialTheme.typography.bodySmall,
color = MaterialTheme.colorScheme.onSurfaceVariant,
maxLines = 1,
@@ -818,3 +822,20 @@ private fun SiteIcon(
/** The host of [url] for a favorite's default label, falling back to the raw string. */
private fun hostOf(url: String): String = OmniboxInput.hostOf(url) ?: url
/**
* The second line of a Recent row: what a reader needs to tell two rows of one site apart.
*
* The scheme and a bare trailing slash are noise at this size, so they go; everything after the
* host stays, because that is the part that differs. Falls back to the host when the URL has
* nothing more to say.
*/
internal fun recentSubtitle(
url: String,
host: String,
): String {
val schemeEnd = url.indexOf("://")
val afterScheme = if (schemeEnd > 0) url.substring(schemeEnd + 3) else url
val trimmed = afterScheme.removeSuffix("/")
return trimmed.ifBlank { host }
}
amethyst/src/test/java/com/vitorpamplona/amethyst/favorites/BrowserHistoryKeyTest.kt
+69
View File
@@ -0,0 +1,69 @@
/*
* Copyright (c) 2025 Vitor Pamplona
*
* Permission is hereby granted, free of charge, to any person obtaining a copy of
* this software and associated documentation files (the "Software"), to deal in
* the Software without restriction, including without limitation the rights to use,
* copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the
* Software, and to permit persons to whom the Software is furnished to do so,
* subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in all
* copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS
* FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR
* COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN
* AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
* WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
*/
package com.vitorpamplona.amethyst.favorites
import org.junit.Assert.assertEquals
import org.junit.Assert.assertNotEquals
import org.junit.Test
/** The list showed `primal.net` twice; these are the pairs that produced that. */
class BrowserHistoryKeyTest {
@Test
fun `a trailing slash on the root is the same page`() {
assertEquals(historyKey("https://primal.net"), historyKey("https://primal.net/"))
}
@Test
fun `the host is compared without case`() {
assertEquals(historyKey("https://Primal.NET/"), historyKey("https://primal.net/"))
}
@Test
fun `a fragment is not a different page`() {
assertEquals(historyKey("https://primal.net/home"), historyKey("https://primal.net/home#top"))
}
@Test
fun `a path is still its own page`() {
assertNotEquals(historyKey("https://primal.net/"), historyKey("https://primal.net/home"))
}
@Test
fun `a query is still its own page`() {
assertNotEquals(historyKey("https://x.com/search?q=a"), historyKey("https://x.com/search?q=b"))
}
@Test
fun `a trailing slash deeper in the path is left alone`() {
// Servers are free to treat these as different, so we do not decide for them.
assertNotEquals(historyKey("https://primal.net/home"), historyKey("https://primal.net/home/"))
}
@Test
fun `http and https are different origins`() {
assertNotEquals(historyKey("http://primal.net/"), historyKey("https://primal.net/"))
}
@Test
fun `something that is not a url is returned as itself`() {
assertEquals("not a url", historyKey("not a url"))
}
}
amethyst/src/test/java/com/vitorpamplona/amethyst/favorites/RecentSubtitleTest.kt
+62
View File
@@ -0,0 +1,62 @@
/*
* Copyright (c) 2025 Vitor Pamplona
*
* Permission is hereby granted, free of charge, to any person obtaining a copy of
* this software and associated documentation files (the "Software"), to deal in
* the Software without restriction, including without limitation the rights to use,
* copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the
* Software, and to permit persons to whom the Software is furnished to do so,
* subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in all
* copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS
* FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR
* COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN
* AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
* WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
*/
package com.vitorpamplona.amethyst.favorites
import com.vitorpamplona.amethyst.ui.screen.loggedIn.browser.recentSubtitle
import org.junit.Assert.assertEquals
import org.junit.Assert.assertNotEquals
import org.junit.Test
/** The two rows that both read "Primal / primal.net" and could not be told apart. */
class RecentSubtitleTest {
@Test
fun `two pages of one site no longer read the same`() {
assertNotEquals(
recentSubtitle("https://primal.net/", "primal.net"),
recentSubtitle("https://primal.net/home", "primal.net"),
)
}
@Test
fun `the root shows the bare host`() {
assertEquals("primal.net", recentSubtitle("https://primal.net/", "primal.net"))
}
@Test
fun `a path is shown after the host`() {
assertEquals("primal.net/home", recentSubtitle("https://primal.net/home", "primal.net"))
}
@Test
fun `a port is kept, since it is part of where you are going`() {
assertEquals("localhost:8000/t.html", recentSubtitle("http://localhost:8000/t.html", "localhost"))
}
@Test
fun `a query is kept`() {
assertEquals("x.com/search?q=nostr", recentSubtitle("https://x.com/search?q=nostr", "x.com"))
}
@Test
fun `a url with nothing to add falls back to the host`() {
assertEquals("primal.net", recentSubtitle("", "primal.net"))
}
}
commonsUI/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/browser/ui/pill/BrowserPill.kt
+34 -5
View File
@@ -353,7 +353,7 @@ private fun OutOfScopeBanner(
}
}
/** Page actions as tiles, four per row; toggles (desktop site, text size) fill while on. */
/** Page actions as tiles, at most four per row; toggles (desktop site, text size) fill while on. */
@Composable
private fun TileGrid(
actions: List<Action>,
@@ -361,9 +361,16 @@ private fun TileGrid(
textSizeOpen: Boolean,
onAction: (Action) -> Unit,
) {
val columns = 4
// Spread over the rows we are going to use anyway, rather than filling each
// one to four and leaving the remainder stranded. Six actions read as 3 + 3
// instead of 4 + 2 with a hole beside it, and five as 3 + 2 instead of 4 + 1.
// Every row is chunked to the same width, so the tiles stay a uniform size
// and a gap appears only when the count is not divisible — seven is 4 + 3
// either way.
var taken = 0
val rows = balancedRowSizes(actions.size, MAX_TILE_COLUMNS).map { size -> actions.subList(taken, taken + size).also { taken += size } }
Column(verticalArrangement = Arrangement.spacedBy(8.dp)) {
actions.chunked(columns).forEach { rowActions ->
rows.forEach { rowActions ->
Row(horizontalArrangement = Arrangement.spacedBy(8.dp)) {
rowActions.forEach { action ->
// The hand-off tile names the browser it will actually open when the
@@ -384,13 +391,35 @@ private fun TileGrid(
modifier = Modifier.weight(1f),
)
}
// Keep the last row's tiles the same width as the others.
repeat(columns - rowActions.size) { Spacer(Modifier.weight(1f)) }
// Keep every row's tiles the same width, whatever the row holds.
repeat(MAX_TILE_COLUMNS - rowActions.size) { Spacer(Modifier.weight(1f)) }
}
}
}
}
/** Most tiles on one row. Four is what fits at the pill's width without the labels wrapping twice. */
private const val MAX_TILE_COLUMNS = 4
/**
* How many tiles go on each row, spread as evenly as the count allows.
*
* The number of rows is fixed by [max] either way — this only decides how the
* items are shared out between them, so the leftovers do not all land on the
* last row. Ten tiles are 4 + 3 + 3, not 4 + 4 + 2. Rows are padded back out
* to [max] where they draw, so the tiles stay a uniform width throughout.
*/
internal fun balancedRowSizes(
count: Int,
max: Int,
): List<Int> {
if (count <= 0) return emptyList()
val rows = (count + max - 1) / max
val base = count / rows
val remainder = count % rows
return List(rows) { index -> base + if (index < remainder) 1 else 0 }
}
/** Text size: a stepped slider between a small and a large "A", the value, and a way back to 100%. */
@Composable
fun TextSizeControl(
commonsUI/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/browser/ui/pill/BalancedRowSizesTest.kt
+80
View File
@@ -0,0 +1,80 @@
/*
* Copyright (c) 2025 Vitor Pamplona
*
* Permission is hereby granted, free of charge, to any person obtaining a copy of
* this software and associated documentation files (the "Software"), to deal in
* the Software without restriction, including without limitation the rights to use,
* copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the
* Software, and to permit persons to whom the Software is furnished to do so,
* subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in all
* copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS
* FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR
* COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN
* AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
* WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
*/
package com.vitorpamplona.amethyst.commons.browser.ui.pill
import kotlin.test.Test
import kotlin.test.assertEquals
class BalancedRowSizesTest {
private fun rows(
count: Int,
max: Int = 4,
) = balancedRowSizes(count, max)
@Test
fun `six fills two rows evenly instead of stranding two`() {
assertEquals(listOf(3, 3), rows(6))
}
@Test
fun `five is three and two, not four and one`() {
assertEquals(listOf(3, 2), rows(5))
}
@Test
fun `seven cannot be even and stays four and three`() {
assertEquals(listOf(4, 3), rows(7))
}
@Test
fun `a full row is left alone`() {
assertEquals(listOf(4), rows(4))
assertEquals(listOf(4, 4), rows(8))
}
@Test
fun `no row is ever wider than the maximum, and every tile is placed`() {
(1..24).forEach { count ->
val rows = rows(count)
assertEquals(true, rows.all { it in 1..4 }, "count=$count rows=$rows")
assertEquals(count, rows.sum(), "count=$count rows=$rows")
}
}
@Test
fun `rows never differ by more than one`() {
(1..24).forEach { count ->
val rows = rows(count)
assertEquals(true, rows.max() - rows.min() <= 1, "count=$count rows=$rows")
}
}
@Test
fun `it never puts everything on one row it cannot fit`() {
assertEquals(listOf(4, 4, 4), rows(12))
assertEquals(listOf(4, 3, 3), rows(10))
}
@Test
fun `an empty grid does not divide by zero`() {
assertEquals(emptyList(), balancedRowSizes(0, 4))
}
}