fix: second audit of the commonsUI move - PDF snapshot close off main, shared formatters, tests

- PdfPreviewCard closed its cache snapshot on the main thread (the use{} ran after withContext(IO) returned), taking Coil's global DiskLruCache lock there; pre-existing. PdfFetcher.useSnapshot fetches, runs the block and closes inside one IO block, with no suspension point for cancellation to leak through. The PDF viewer's onDispose closes its handle on the app IO scope, under the render mutex. - JVM and iOS built a time-of-day formatter per feed item; they share one cached instance now (NSDateFormatter is costly to build). - timeAgoShort no longer allocates an unused fallback lambda per tick on Android: the expect returns null where the platform has no short span. - The iOS skeleton formatter keys on the locale only, like Android: iOS caches its system zone until reset, so keying on it cost lookups and never saw a change. KDocs now say which platforms follow what. - JumboEmojiTest covers emoji counting (ZWJ, skin tone, flag, keycap) and the bubble shapes; ScreenLayoutTest moves to commonsUI jvmTest beside its code. - The plan records the audit, and that the packaged Desktop runtime lacks jdk.localedata, which these formatters need before Desktop uses them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UjQQN9CgWHVtNCSWqnKoqY
2026-10-06 03:38:23 +00:00 · 2026-09-26 23:40:07 +00:00
parent 78e7561fb9
commit 417f4060a7
11 changed files with 255 additions and 95 deletions
@@ -61,8 +61,6 @@ import com.vitorpamplona.amethyst.ui.components.ShareMediaAction
 import com.vitorpamplona.amethyst.ui.screen.loggedIn.AccountViewModel
 import com.vitorpamplona.quartz.utils.Log
 import kotlinx.coroutines.CancellationException
-import kotlinx.coroutines.Dispatchers
-import kotlinx.coroutines.withContext

 // Hard ceiling on the inline thumbnail bitmap, in pixels. Prevents OOM on very tall/large pages.
 private const val THUMBNAIL_MAX_DIM_PX = 1600
@@ -158,12 +156,12 @@ private fun LoadedPdfPreviewCard(
        value =
            try {
                PdfFetcher
-                    .fetchSnapshot(content.url, { Amethyst.instance.diskCache }) { url ->
-                        accountViewModel.httpClientBuilder.okHttpClientForPreview(url)
-                    }.use { snapshot ->
-                        withContext(Dispatchers.IO) {
-                            renderFirstPage(snapshot.data.toFile(), targetWidthPx)
-                        }
+                    .useSnapshot(
+                        url = content.url,
+                        diskCache = { Amethyst.instance.diskCache },
+                        okHttpClient = { url -> accountViewModel.httpClientBuilder.okHttpClientForPreview(url) },
+                    ) { snapshot ->
+                        renderFirstPage(snapshot.data.toFile(), targetWidthPx)
                    }.also { result ->
                        // Same cache the image and video paths use, so a PDF that has been rendered
                        // once lays out at its real shape on every later visit instead of growing
@@ -82,6 +82,7 @@ import kotlinx.coroutines.delay
 import kotlinx.coroutines.flow.collectLatest
 import kotlinx.coroutines.flow.debounce
 import kotlinx.coroutines.flow.distinctUntilChanged
+import kotlinx.coroutines.launch
 import kotlinx.coroutines.sync.Mutex
 import kotlinx.coroutines.sync.withLock
 import kotlinx.coroutines.withContext
@@ -223,7 +224,14 @@ private fun PdfViewerContent(
    val handleForDispose = handleState
    DisposableEffect(handleForDispose) {
        onDispose {
-            handleForDispose?.close()
+            // Off the main thread: closing the cache snapshot takes Coil's global DiskLruCache
+            // lock, which its cleanup pass holds across bursts of unlinks. Under the render mutex,
+            // so a page render still in flight finishes before the renderer is closed under it.
+            handleForDispose?.let { handle ->
+                Amethyst.instance.applicationIOScope.launch {
+                    handle.mutex.withLock { handle.close() }
+                }
+            }
        }
    }

@@ -1439,7 +1439,7 @@ functional bugs. The follow-ups:

 | What | To | The blocker, and how it went |
 |---|---|---|
-| `ScreenLayout` tier logic, `CappedScreenContent`, the pane widths | `commonsUI/…/ui/layouts/ScreenLayout.kt` | `material3-window-size-class` (app-only): its width breakpoints (Medium ≥ 600dp, Expanded ≥ 840dp) are inlined, and `ScreenLayoutTest`'s 17 cases pass against them. `LocalConfiguration` stays in the app, which asks the shared `rememberScreenLayoutSpec(widthDp, heightDp)`, so Desktop can supply its own window size. |
+| `ScreenLayout` tier logic, `CappedScreenContent`, the pane widths | `commonsUI/…/ui/layouts/ScreenLayout.kt` | `material3-window-size-class` (app-only): its width breakpoints (Medium ≥ 600dp, Expanded ≥ 840dp) are inlined, and `ScreenLayoutTest` (18 cases, now in commonsUI jvmTest) passes against them. `LocalConfiguration` stays in the app, which asks the shared `rememberScreenLayoutSpec(widthDp, heightDp)`, so Desktop can supply its own window size. |
 | `animatedViewerChromeInset`, `rememberViewerControlsVisibility` | `commonsUI/…/ui/components/ViewerChrome.kt` | They sat in a file full of Android window code. Only `ImmersiveSystemBarsEffect` (Window/insets controller) stays. |
 | `SlidingCarousel` | `commonsUI/…/ui/components` | needed the inset above |
 | `AudioWaveformReadOnly` | `commonsUI/…/ui/components` | two enums from the Android-only audiowaveform library, now local enums with the same values |
@@ -1472,12 +1472,13 @@ Desktop still has its own `ToggleableTimeAgoText` and the older `commons/…/uti

 A transitive-blocker sweep of the 1,385 files under `amethyst/…/ui/`, after this round:

- **80 files have no blocker left.** Many are headless and belong in `commons`, not `commonsUI`:
-  - filter assemblers and `*LastRead`;
-  - `NewMessageTagger`, `SplitConversor`, `PubKeyFormatter`, `SettingsCatalog`;
-  - the Tor status/dialog VM;
-  - `ChatBubbleLayout` + `ChatGroupPosition` + `JumboEmoji` + `NewDateOrSubjectDivisor` + `AutoScrollToNewest`.
-  - The chat bubble set is the next obvious batch: Desktop's `ui/chats/ChatBubbleLayout.kt` is an older fork of it.
+- **80 files have no blocker left.**
+  - Many are headless and belong in `commons`, not `commonsUI`: the filter assemblers and
+    `*LastRead`, `NewMessageTagger`, `SplitConversor`, `PubKeyFormatter`, `SettingsCatalog`,
+    and the Tor status/dialog VM.
+  - The Compose ones include the chat bubble set (`ChatBubbleLayout`, `ChatGroupPosition`,
+    `JumboEmoji`, `NewDateOrSubjectDivisor`, `AutoScrollToNewest`). It moved to `commonsUI`
+    in the follow-up below. Desktop's `ui/chats/ChatBubbleLayout.kt` is an older fork of it.
 - **`AccountViewModel` is the wall:**
  - 892 files touch it, and 310 touch nothing else app-side. Yet swapping it for an interface frees only 76 files by itself, because the rest call hub composables that are blocked themselves.
  - The ui files use 210 distinct members of it. The note renderers use 28.
@@ -1515,12 +1516,40 @@ A transitive-blocker sweep of the 1,385 files under `amethyst/…/ui/`, after th
  - `PdfFetcher` takes the disk cache as a provider, read on the IO dispatcher, so a PDF
    card composing on a cold start doesn't build the app's lazy cache on the main thread.
  - The two DM lists resolve `TimeAgoLabels` once per list instead of once per row.
-  - The iOS `DateSkeletonFormatter` rebuilds on a locale or time-zone change, like the
-    other actuals.
+  - The iOS `DateSkeletonFormatter` rebuilds on a locale change, as Android's does. (The
+    JVM one also keys on the time zone; iOS caches its system zone until reset, so keying on
+    it there would cost lookups and still not notice.)
  - `ScreenLayoutTest` pins the inclusive 600dp boundary.
  - Two stale KDoc links and a same-package import are fixed.
  - The generated baseline profile has its stable-name entries repointed:
    `NowProviderKt`, and the five theme functions now in `AmethystThemeKt`. Its R8
    lambda entries were already stale before this round (it still lists
-    `MarkDownStyleOnDark` under `ThemeKt`), so **regenerate the profile** after these
-    moves.
+    `MarkDownStyleOnDark` under `ThemeKt`), and the new theme root
+    (`AmethystThemeKt;->AmethystMaterialTheme`, `isDarkTheme`) has no entries at all, so
+    **regenerate the profile** after these moves.
+
+### Second audit, and one Desktop decision
+
+- **Fixed:**
+  - `PdfPreviewCard` closed its cache snapshot on the main thread, after `withContext(IO)`
+    returned. That was older than this branch. Closing takes the same global DiskLruCache
+    lock as opening, so `PdfFetcher.useSnapshot` now fetches, runs the block and closes, all
+    in one IO block, leaving no suspension point for cancellation to leak through.
+  - The PDF viewer's `onDispose` now closes its handle on the app IO scope, under the
+    render mutex.
+  - JVM and iOS built a time-of-day formatter per feed item; they now share one cached
+    instance. `NSDateFormatter` is costly to build.
+  - `timeAgoShort` no longer allocates an unused fallback lambda per tick on Android.
+  - The iOS skeleton formatter keys on the locale only, like Android. iOS caches its system
+    zone until reset, so keying on the zone cost lookups and still never saw a change.
+  - New tests: `JumboEmojiTest` (counts, ZWJ/skin tone/flag/keycap sequences, bubble
+    shapes). `ScreenLayoutTest` moved to commonsUI jvmTest, beside the code it tests.
+- **Desktop needs `jdk.localedata` before it uses these formatters.**
+  - `desktopApp/build.gradle.kts` `nativeDistributions.modules(...)` doesn't include it,
+    so the packaged runtime carries only en/root CLDR data.
+  - `DateTimeFormatter.ofLocalizedPattern` then gives en-GB `Jan 5, 2024` and de-DE
+    `2024 Jan 5`, and the JDK tests (full runtime) can't see it.
+  - Desktop's existing `java.time` formatting (`DesktopScheduleAtPicker`) already has the
+    same gap.
+  - Adding the module costs about +28 MB to the unpacked runtime. That is a packaging
+    call, left for the Desktop phase.
@@ -60,12 +60,11 @@ actual fun rememberTimeOfDayFormatter(): (epochMillis: Long) -> String {
    return remember(context) { { epochMillis -> DateFormat.getTimeFormat(context).format(Date(epochMillis)) } }
 }

-actual fun relativeTimeSpanShort(
+actual fun relativeTimeSpanShortOrNull(
    epochMillis: Long,
    nowMillis: Long,
    nowLabel: String,
-    fallback: () -> String,
-): String {
+): String? {
    val humanReadable =
        DateUtils
            .getRelativeTimeSpanString(
@@ -25,7 +25,8 @@ import androidx.compose.runtime.Composable
 /**
 * Formats instants with a Unicode LDML date skeleton ("yMMMd", "MMMd", "yMMM"), letting the
 * platform pick the default locale's field order ("MMM d, y" in en-US, "d MMM y" in en-GB).
- * Safe to call from any thread. Rebuilds itself when the default locale changes.
+ * Safe to call from any thread. Rebuilds itself when the default locale changes (the JVM actual
+ * also on a time-zone change; Android and iOS keep the zone they were built with).
 */
 expect class DateSkeletonFormatter(
    skeleton: String,
@@ -37,20 +38,19 @@ expect class DateSkeletonFormatter(
 expect fun calendarYearAndDay(epochMillis: Long): Int

 /**
- * A time-of-day formatter ("14:32" / "2:32 PM") that follows the platform's own 12/24-hour
- * preference, which on Android is a system setting rather than a locale property.
+ * A time-of-day formatter ("14:32" / "2:32 PM"). Android and iOS follow the system 12/24-hour
+ * setting; the JVM follows the locale. Identity-stable, so it can key a `remember`, and cheap to
+ * call for every item in a feed: platforms whose formatter is costly share one cached instance.
 */
@Composable
 expect fun rememberTimeOfDayFormatter(): (epochMillis: Long) -> String

 /**
 * The platform's abbreviated relative span ("5 min. ago", "Yesterday"), [nowLabel] for an
- * instant that rounds to now or lies ahead, and [fallback] where the platform has no such
- * formatter.
+ * instant that rounds to now or lies ahead, or null where the platform has no such formatter.
 */
-expect fun relativeTimeSpanShort(
+expect fun relativeTimeSpanShortOrNull(
    epochMillis: Long,
    nowMillis: Long,
    nowLabel: String,
-    fallback: () -> String,
-): String
+): String?
@@ -375,5 +375,5 @@ fun timeAgoShort(
    labels: TimeAgoLabels,
 ): String {
    if (time == null) return " "
-    return relativeTimeSpanShort(time * 1000, TimeUtils.nowMillis(), labels.now) { timeAgoWith(time, labels, prefix = "") }
+    return relativeTimeSpanShortOrNull(time * 1000, TimeUtils.nowMillis(), labels.now) ?: timeAgoWith(time, labels, prefix = "")
 }
@@ -21,7 +21,6 @@
 package com.vitorpamplona.amethyst.commons.ui.note

 import androidx.compose.runtime.Composable
-import androidx.compose.runtime.remember
 import platform.Foundation.NSCalendar
 import platform.Foundation.NSCalendarUnitDay
 import platform.Foundation.NSCalendarUnitYear
@@ -30,10 +29,8 @@ import platform.Foundation.NSDateFormatter
 import platform.Foundation.NSDateFormatterNoStyle
 import platform.Foundation.NSDateFormatterShortStyle
 import platform.Foundation.NSLocale
-import platform.Foundation.NSTimeZone
 import platform.Foundation.currentLocale
 import platform.Foundation.dateWithTimeIntervalSince1970
-import platform.Foundation.defaultTimeZone
 import platform.Foundation.localeIdentifier
 import kotlin.concurrent.Volatile

@@ -41,7 +38,9 @@ private fun dateOf(epochMillis: Long) = NSDate.dateWithTimeIntervalSince1970(epo

 /**
 * NSDateFormatter is thread-safe for formatting on iOS 7+; the template picks the locale's order.
- * Rebuilt when the current locale or time zone changes, like the other actuals.
+ * Rebuilt when the current locale changes, as on Android. The time zone is the one in effect when
+ * it was built: iOS caches the system zone until `NSTimeZone.resetSystemTimeZone()`, so keying
+ * on it would cost two lookups per call and still not notice a change.
 */
 actual class DateSkeletonFormatter actual constructor(
    private val skeleton: String,
@@ -55,7 +54,7 @@ actual class DateSkeletonFormatter actual constructor(

    actual fun format(epochMillis: Long): String {
        val locale = NSLocale.currentLocale
-        val key = locale.localeIdentifier + "|" + NSTimeZone.defaultTimeZone.name
+        val key = locale.localeIdentifier
        val formatter =
            cached?.takeIf { it.key == key }?.formatter
                ?: NSDateFormatter()
@@ -75,21 +74,22 @@ actual fun calendarYearAndDay(epochMillis: Long): Int {
    return year * 1000 + day
 }

-@Composable
-actual fun rememberTimeOfDayFormatter(): (epochMillis: Long) -> String =
-    remember {
-        val formatter =
-            NSDateFormatter().apply {
-                dateStyle = NSDateFormatterNoStyle
-                timeStyle = NSDateFormatterShortStyle
-            }
-        val format: (Long) -> String = { epochMillis -> formatter.stringFromDate(dateOf(epochMillis)) }
-        format
+// NSDateFormatter is costly to build, so every feed item shares this one instead of each
+// composing its own. It follows the system 12/24-hour setting (the short time style does).
+private val timeOfDayFormatter by lazy {
+    NSDateFormatter().apply {
+        dateStyle = NSDateFormatterNoStyle
+        timeStyle = NSDateFormatterShortStyle
    }
+}

-actual fun relativeTimeSpanShort(
+private val timeOfDay: (Long) -> String = { epochMillis -> timeOfDayFormatter.stringFromDate(dateOf(epochMillis)) }
+
+@Composable
+actual fun rememberTimeOfDayFormatter(): (epochMillis: Long) -> String = timeOfDay
+
+actual fun relativeTimeSpanShortOrNull(
    epochMillis: Long,
    nowMillis: Long,
    nowLabel: String,
-    fallback: () -> String,
-): String = fallback()
+): String? = null
@@ -42,39 +42,63 @@ object PdfFetcher {
        url: String,
        diskCache: () -> DiskCache,
        okHttpClient: (String) -> OkHttpClient,
-    ): DiskCache.Snapshot =
-        withContext(Dispatchers.IO) {
-            val diskCache = diskCache()
-            // Covers the cache-hit fast path too, not just the download below it. openSnapshot()
-            // contends on the global DiskLruCache lock, which Coil's cleanup pass holds across a
-            // burst of unlink syscalls (see DeferredDeleteFileSystem) — calling it from a caller
-            // that happens to be on the main thread stalls the frame for that whole burst, and the
-            // hit path is exactly the one a feed takes when a PDF card scrolls back into view.
-            diskCache.openSnapshot(url)?.let { return@withContext it }
+    ): DiskCache.Snapshot = withContext(Dispatchers.IO) { openOrDownload(url, diskCache, okHttpClient) }

-            val editor = diskCache.openEditor(url) ?: throw IOException("Unable to open cache editor for $url")
-            try {
-                val request =
-                    Request
-                        .Builder()
-                        .url(url)
-                        .get()
-                        .build()
+    /** The fetch itself. Callers run it on the IO dispatcher. */
+    private suspend fun openOrDownload(
+        url: String,
+        diskCache: () -> DiskCache,
+        okHttpClient: (String) -> OkHttpClient,
+    ): DiskCache.Snapshot {
+        val cache = diskCache()
+        // Covers the cache-hit fast path too, not just the download below it. openSnapshot()
+        // contends on the global DiskLruCache lock, which Coil's cleanup pass holds across a
+        // burst of unlink syscalls (see DeferredDeleteFileSystem) — calling it from a caller
+        // that happens to be on the main thread stalls the frame for that whole burst, and the
+        // hit path is exactly the one a feed takes when a PDF card scrolls back into view.
+        cache.openSnapshot(url)?.let { return it }

-                okHttpClient(url).newCall(request).executeAsync().use { response ->
-                    if (!response.isSuccessful) {
-                        throw IOException("PDF download failed: ${response.code}")
-                    }
-                    diskCache.fileSystem.write(editor.data) {
-                        val bytes = writeAll(response.body.source())
-                        if (bytes == 0L) throw IOException("PDF download failed: empty response body")
-                    }
+        val editor = cache.openEditor(url) ?: throw IOException("Unable to open cache editor for $url")
+        try {
+            val request =
+                Request
+                    .Builder()
+                    .url(url)
+                    .get()
+                    .build()
+
+            okHttpClient(url).newCall(request).executeAsync().use { response ->
+                if (!response.isSuccessful) {
+                    throw IOException("PDF download failed: ${response.code}")
+                }
+                cache.fileSystem.write(editor.data) {
+                    val bytes = writeAll(response.body.source())
+                    if (bytes == 0L) throw IOException("PDF download failed: empty response body")
                }
-
-                editor.commitAndOpenSnapshot() ?: throw IOException("Unable to commit cache editor for $url")
-            } catch (t: Throwable) {
-                runCatching { editor.abort() }
-                throw t
            }
+
+            return editor.commitAndOpenSnapshot() ?: throw IOException("Unable to commit cache editor for $url")
+        } catch (t: Throwable) {
+            runCatching { editor.abort() }
+            throw t
+        }
+    }
+
+    /**
+     * Fetches like [fetchSnapshot], runs [block] on the snapshot, and closes it, all on the IO
+     * dispatcher. Prefer this to `fetchSnapshot(...).use { }` from a main-thread caller: there the
+     * close would run on main, and closing takes the same global DiskLruCache lock as opening.
+     * The snapshot is created by the last non-suspending step of the fetch and handed straight
+     * to `use`, inside one `withContext`, so a cancellation cannot land between the two and leak
+     * it open (which returning through [fetchSnapshot]'s own `withContext` could).
+     */
+    suspend fun <T> useSnapshot(
+        url: String,
+        diskCache: () -> DiskCache,
+        okHttpClient: (String) -> OkHttpClient,
+        block: (DiskCache.Snapshot) -> T,
+    ): T =
+        withContext(Dispatchers.IO) {
+            openOrDownload(url, diskCache, okHttpClient).use(block)
        }
 }
@@ -21,7 +21,6 @@
 package com.vitorpamplona.amethyst.commons.ui.note

 import androidx.compose.runtime.Composable
-import androidx.compose.runtime.remember
 import java.time.Instant
 import java.time.ZoneId
 import java.time.format.DateTimeFormatter
@@ -58,17 +57,31 @@ actual class DateSkeletonFormatter actual constructor(
    }
 }

-@Composable
-actual fun rememberTimeOfDayFormatter(): (epochMillis: Long) -> String =
-    remember {
-        val formatter = DateTimeFormatter.ofLocalizedTime(FormatStyle.SHORT).withZone(ZoneId.systemDefault())
-        val format: (Long) -> String = { epochMillis -> formatter.format(Instant.ofEpochMilli(epochMillis)) }
-        format
-    }
+private class CachedTimeOfDay(
+    val locale: Locale,
+    val zone: ZoneId,
+    val formatter: DateTimeFormatter,
+)

-actual fun relativeTimeSpanShort(
+@Volatile private var cachedTimeOfDay: CachedTimeOfDay? = null
+
+/** One formatter for every item, rebuilt only when the locale or zone changes. */
+private val timeOfDay: (Long) -> String = { epochMillis ->
+    val locale = Locale.getDefault()
+    val zone = ZoneId.systemDefault()
+    val formatter =
+        cachedTimeOfDay?.takeIf { it.locale == locale && it.zone == zone }?.formatter
+            ?: DateTimeFormatter.ofLocalizedTime(FormatStyle.SHORT).withLocale(locale).withZone(zone).also {
+                cachedTimeOfDay = CachedTimeOfDay(locale, zone, it)
+            }
+    formatter.format(Instant.ofEpochMilli(epochMillis))
+}
+
+@Composable
+actual fun rememberTimeOfDayFormatter(): (epochMillis: Long) -> String = timeOfDay
+
+actual fun relativeTimeSpanShortOrNull(
    epochMillis: Long,
    nowMillis: Long,
    nowLabel: String,
-    fallback: () -> String,
-): String = fallback()
+): String? = null
@@ -0,0 +1,92 @@
+/*
+ * 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.chats.ui
+
+import com.vitorpamplona.amethyst.commons.ui.theme.ChatBubbleShapeMe
+import com.vitorpamplona.amethyst.commons.ui.theme.ChatBubbleShapeMeTop
+import com.vitorpamplona.amethyst.commons.ui.theme.ChatBubbleShapeThemBottom
+import com.vitorpamplona.amethyst.commons.ui.theme.ChatBubbleShapeThemMiddle
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertSame
+import org.junit.Test
+
+// Emoji are written as escapes so the file stays unambiguous on disk (see CLAUDE.md).
+class JumboEmojiTest {
+    private val grin = "\uD83D\uDE00" // U+1F600
+    private val thumbsUpMedium = "\uD83D\uDC4D\uD83C\uDFFD" // U+1F44D + skin tone U+1F3FD
+    private val family = "\uD83D\uDC69\u200D\uD83D\uDC67" // woman ZWJ girl: draws as one
+    private val flagUs = "\uD83C\uDDFA\uD83C\uDDF8" // two regional indicators
+    private val keycapOne = "1\uFE0F\u20E3" // '1' + VS16 + combining keycap
+    private val redHeart = "\u2764\uFE0F" // U+2764 + VS16
+
+    @Test
+    fun countsUpToThreeEmoji() {
+        assertEquals(1, jumboEmojiCount(grin))
+        assertEquals(2, jumboEmojiCount(grin + redHeart))
+        assertEquals(3, jumboEmojiCount(grin + grin + grin))
+    }
+
+    @Test
+    fun moreThanThreeIsNotJumbo() = assertEquals(0, jumboEmojiCount(grin + grin + grin + grin))
+
+    @Test
+    fun anyTextMakesItNotJumbo() {
+        assertEquals(0, jumboEmojiCount("hi $grin"))
+        assertEquals(0, jumboEmojiCount("${grin}a"))
+    }
+
+    @Test
+    fun whitespaceIsIgnored() = assertEquals(2, jumboEmojiCount(" $grin \n $grin "))
+
+    @Test
+    fun emptyOrBlankIsZero() {
+        assertEquals(0, jumboEmojiCount(""))
+        assertEquals(0, jumboEmojiCount("   "))
+    }
+
+    @Test
+    fun modifiersAndZwjSequencesCountAsOne() {
+        assertEquals(1, jumboEmojiCount(thumbsUpMedium))
+        assertEquals(1, jumboEmojiCount(family))
+        assertEquals(1, jumboEmojiCount(redHeart))
+    }
+
+    @Test
+    fun flagsNeedBothRegionalIndicators() {
+        assertEquals(1, jumboEmojiCount(flagUs))
+        assertEquals(0, jumboEmojiCount("\uD83C\uDDFA")) // half a flag
+    }
+
+    @Test
+    fun keycapsNeedTheirMark() {
+        assertEquals(1, jumboEmojiCount(keycapOne))
+        assertEquals(0, jumboEmojiCount("1")) // a bare digit is text
+        assertEquals(0, jumboEmojiCount("1 $grin"))
+    }
+
+    @Test
+    fun bubbleShapeFollowsSideAndPosition() {
+        assertSame(ChatBubbleShapeMe, chatBubbleShapeFor(true, ChatGroupPosition.SINGLE))
+        assertSame(ChatBubbleShapeMeTop, chatBubbleShapeFor(true, ChatGroupPosition.TOP))
+        assertSame(ChatBubbleShapeThemMiddle, chatBubbleShapeFor(false, ChatGroupPosition.MIDDLE))
+        assertSame(ChatBubbleShapeThemBottom, chatBubbleShapeFor(false, ChatGroupPosition.BOTTOM))
+    }
+}
@@ -18,11 +18,8 @@
 * 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.ui.layouts
+package com.vitorpamplona.amethyst.commons.ui.layouts

-import com.vitorpamplona.amethyst.commons.ui.layouts.NavigationStyle
-import com.vitorpamplona.amethyst.commons.ui.layouts.decideNavigationStyle
-import com.vitorpamplona.amethyst.commons.ui.layouts.hasRoomForNotificationPanel
 import org.junit.Assert.assertEquals
 import org.junit.Assert.assertFalse
 import org.junit.Assert.assertTrue