diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/favorites/BrowserHistoryRegistry.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/favorites/BrowserHistoryRegistry.kt index 5564139886..57494e95f9 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/favorites/BrowserHistoryRegistry.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/favorites/BrowserHistoryRegistry.kt @@ -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): List = list .sortedByDescending { it.lastVisitedAt } - .distinctBy { it.url } + .distinctBy { historyKey(it.url) } .take(MAX_ENTRIES) private inline fun update(transform: (List) -> List) { @@ -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 +} diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/browser/BrowserScreen.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/browser/BrowserScreen.kt index 06f038fb22..f65ff428d5 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/browser/BrowserScreen.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/browser/BrowserScreen.kt @@ -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 , 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 } +} diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/favorites/BrowserHistoryKeyTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/favorites/BrowserHistoryKeyTest.kt new file mode 100644 index 0000000000..842fe99427 --- /dev/null +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/favorites/BrowserHistoryKeyTest.kt @@ -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")) + } +} diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/favorites/RecentSubtitleTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/favorites/RecentSubtitleTest.kt new file mode 100644 index 0000000000..988495bced --- /dev/null +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/favorites/RecentSubtitleTest.kt @@ -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")) + } +} diff --git a/commonsUI/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/browser/ui/pill/BrowserPill.kt b/commonsUI/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/browser/ui/pill/BrowserPill.kt index ca09cc1bc8..b0b5d88e14 100644 --- a/commonsUI/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/browser/ui/pill/BrowserPill.kt +++ b/commonsUI/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/browser/ui/pill/BrowserPill.kt @@ -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( diff --git a/commonsUI/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/browser/ui/pill/BalancedRowSizesTest.kt b/commonsUI/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/browser/ui/pill/BalancedRowSizesTest.kt new file mode 100644 index 0000000000..787661cdf2 --- /dev/null +++ b/commonsUI/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/browser/ui/pill/BalancedRowSizesTest.kt @@ -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)) + } +}