Merge pull request #3719 from davotoula/fix/audio-player-overflows-note-layout

Stop the inline audio player painting over the note
2026-08-09 08:04:45 +00:00 · 2026-07-26 16:43:33 -04:00
parent 3813e248c2 482be60e4a
commit eedc8c7a08
5 changed files with 318 additions and 10 deletions
@@ -0,0 +1,113 @@
+/*
+ * 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.ui.components
+
+import androidx.compose.foundation.layout.Box
+import androidx.compose.foundation.layout.size
+import androidx.compose.ui.Alignment
+import androidx.compose.ui.Modifier
+import androidx.compose.ui.layout.ContentScale
+import androidx.compose.ui.layout.onGloballyPositioned
+import androidx.compose.ui.layout.positionInRoot
+import androidx.compose.ui.test.junit4.createComposeRule
+import androidx.compose.ui.unit.dp
+import androidx.test.ext.junit.runners.AndroidJUnit4
+import com.vitorpamplona.amethyst.service.playback.composable.audioSquare
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertTrue
+import org.junit.Rule
+import org.junit.Test
+import org.junit.runner.RunWith
+
+/**
+ * The inline audio player sizes itself as a square so the visualizer and controls get room, which is
+ * taller than the 16:9 box [ZoomableContentView] builds for a video of unknown dimensions. That box
+ * is centre-aligned and nothing between it and NoteComposeLayout clips, so caging the square in it
+ * makes the player paint over the note header above and the reactions row below.
+ *
+ * These tests recreate that enclosure — the real [mediaSizingModifier] over the real
+ * [unknownMediaAspectRatio], holding the real [audioSquare] — and pin both halves of the contract:
+ * audio must not be given a ratio, and video must keep the 16:9 assumption that stops live streams
+ * from letterboxing on first play.
+ */
+@RunWith(AndroidJUnit4::class)
+class AudioPlayerBoxOverflowTest {
+    @get:Rule val rule = createComposeRule()
+
+    private class Bounds {
+        var top = 0f
+        var bottom = 0f
+        var width = 0
+        var height = 0
+
+        fun modifier() =
+            Modifier.onGloballyPositioned {
+                top = it.positionInRoot().y
+                width = it.size.width
+                height = it.size.height
+                bottom = top + height
+            }
+    }
+
+    private fun measure(
+        url: String,
+        square: Boolean,
+    ): Pair<Bounds, Bounds> {
+        val box = Bounds()
+        val player = Bounds()
+
+        rule.setContent {
+            Box(Modifier.size(width = 400.dp, height = 1200.dp)) {
+                Box(
+                    modifier = mediaSizingModifier(unknownMediaAspectRatio(null, url), ContentScale.Fit).then(box.modifier()),
+                    contentAlignment = Alignment.Center,
+                ) {
+                    Box((if (square) Modifier.audioSquare() else Modifier).then(player.modifier()))
+                }
+            }
+        }
+        rule.waitForIdle()
+
+        return box to player
+    }
+
+    @Test
+    fun squareAudioPlayerStaysInsideItsMediaBox() {
+        val (box, player) = measure("https://haven.sdbitcoiners.com/f28a5a2e.mp3", square = true)
+
+        assertTrue(
+            "player overflows above the media box by ${box.top - player.top}px",
+            player.top >= box.top,
+        )
+        assertTrue(
+            "player overflows below the media box by ${player.bottom - box.bottom}px",
+            player.bottom <= box.bottom,
+        )
+        assertEquals("the box should wrap the square", player.height, box.height)
+    }
+
+    @Test
+    fun videoOfUnknownSizeKeeps16by9() {
+        val (box, _) = measure("https://example.com/a.mp4", square = false)
+
+        assertEquals(16f / 9f, box.width.toFloat() / box.height, 0.01f)
+    }
+}
@@ -87,6 +87,7 @@ import com.vitorpamplona.amethyst.commons.richtext.MediaUrlContent
 import com.vitorpamplona.amethyst.commons.richtext.MediaUrlImage
 import com.vitorpamplona.amethyst.commons.richtext.MediaUrlPdf
 import com.vitorpamplona.amethyst.commons.richtext.MediaUrlVideo
+import com.vitorpamplona.amethyst.commons.richtext.RichTextParser
 import com.vitorpamplona.amethyst.commons.richtext.toCoilModel
 import com.vitorpamplona.amethyst.commons.ui.components.LoadingAnimation
 import com.vitorpamplona.amethyst.model.MediaAspectRatioCache
@@ -144,6 +145,23 @@ private const val SHARED_VIDEO_CLEANUP_DELAY_MS = 120_000L
 // place; [MediaAspectRatioCache] then corrects anything unusual once the decoder reports its size.
 private const val DEFAULT_VIDEO_ASPECT_RATIO = 16f / 9f

+/**
+ * Shape to assume for a [MediaUrlVideo] nobody has reported dimensions for — no imeta `dim` and
+ * nothing in [MediaAspectRatioCache].
+ *
+ * Video gets [DEFAULT_VIDEO_ASPECT_RATIO]. Audio gets null, meaning "no ratio, wrap whatever the
+ * player asks for": audio has no picture, and the inline player sizes itself as a square (see
+ * `AudioPlayerSquare.audioSquare`) so the visualizer and controls get room. A square is taller than
+ * 16:9, so caging it in a 16:9 box — which is centre-aligned and unclipped all the way up through
+ * NoteComposeLayout — makes the player paint over the note header above it and the reactions row
+ * below. Audio also never reaches the cache, since it has no video track to report a size, so the
+ * miss is permanent rather than first-play-only.
+ */
+internal fun unknownMediaAspectRatio(
+    mimeType: String?,
+    url: String,
+): Float? = if (RichTextParser.isAudioContent(mimeType, url)) null else DEFAULT_VIDEO_ASPECT_RATIO
+
@Composable
 fun ZoomableContentView(
    content: BaseMediaContent,
@@ -203,7 +221,16 @@ fun ZoomableContentView(
        }

        is MediaUrlVideo -> {
-            val ratio = content.dim?.aspectRatio() ?: MediaAspectRatioCache.get(content.url) ?: DEFAULT_VIDEO_ASPECT_RATIO
+            // The fallback classifies the URL string, so compute it once per content — for audio
+            // the cache miss is permanent and this branch re-runs on every recomposition.
+            val fallbackRatio =
+                remember(content.url, content.mimeType) {
+                    unknownMediaAspectRatio(content.mimeType, content.url)
+                }
+            val ratio =
+                content.dim?.aspectRatio()
+                    ?: MediaAspectRatioCache.get(content.url)
+                    ?: fallbackRatio
            val bridgedUrl =
                remember(content.url, useLocalBlossomBridge) {
                    content.toCoilModel(useLocalBlossomBridge)
@@ -0,0 +1,63 @@
+/*
+ * 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.ui.components
+
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertNull
+import org.junit.Test
+
+/**
+ * The audio player sizes itself as a square (see AudioPlayerSquare.audioSquare), which is taller than
+ * 16:9. Handing audio the video default therefore cages the square in a shorter, centre-aligned box
+ * that nothing clips, and the player — visualizer included — paints over the note header above and
+ * the reactions row below. Audio must stay ratio-less so the box wraps the square instead.
+ */
+class UnknownMediaAspectRatioTest {
+    @Test
+    fun unknownVideoAssumes16by9() {
+        assertEquals(16f / 9f, unknownMediaAspectRatio(null, "https://example.com/a.mp4"))
+    }
+
+    @Test
+    fun liveStreamPlaylistKeeps16by9() {
+        assertEquals(16f / 9f, unknownMediaAspectRatio(null, "https://example.com/stream.m3u8"))
+    }
+
+    @Test
+    fun bareAudioUrlHasNoRatio() {
+        assertNull(unknownMediaAspectRatio(null, "https://haven.sdbitcoiners.com/f28a5a2e.mp3"))
+    }
+
+    @Test
+    fun audioMimeHasNoRatioEvenWithoutAnExtension() {
+        assertNull(unknownMediaAspectRatio("audio/mpeg", "https://example.com/download?id=7"))
+    }
+
+    @Test
+    fun videoMimeWinsOverNothing() {
+        assertEquals(16f / 9f, unknownMediaAspectRatio("video/mp4", "https://example.com/download?id=7"))
+    }
+
+    @Test
+    fun unknownEverythingAssumes16by9() {
+        assertEquals(16f / 9f, unknownMediaAspectRatio(null, "https://example.com/download?id=7"))
+    }
+}
@@ -495,12 +495,25 @@ class RichTextParser {
            )

        val imageExt = listOf("png", "jpg", "gif", "bmp", "jpeg", "webp", "svg", "avif")
-        val videoExt = listOf("mp4", "avi", "wmv", "mpg", "amv", "webm", "mov", "mp3", "m3u8", "ogg", "wav", "flac", "aac", "opus", "m4a", "f4a")
+
+        // Audio is folded into [videoExt] because both play through the same video pipeline — but
+        // audio has no picture, so anything that reasons about the shape of the media (aspect
+        // ratios, player sizing) has to tell them apart. `m3u8` stays video-only: a playlist
+        // carries either.
+        //
+        // Composing [videoExt] out of [audioExt] is what keeps "every audio extension is also a
+        // video extension" true by construction rather than by convention — the two lists cannot
+        // drift apart. It is also why [audioExt] is declared first; the compiler rejects the
+        // reverse order outright ("Variable 'audioExt' must be initialized"), so this note is
+        // intent, not a guard rail.
+        val audioExt = listOf("mp3", "ogg", "wav", "flac", "aac", "opus", "m4a", "f4a")
+        val videoExt = listOf("mp4", "avi", "wmv", "mpg", "amv", "webm", "mov", "m3u8") + audioExt
        val pdfExt = listOf("pdf")

        val imageExtensions = imageExt + imageExt.map { it.uppercase() }
        val videoExtensions = videoExt + videoExt.map { it.uppercase() }
        val pdfExtensions = pdfExt + pdfExt.map { it.uppercase() }
+        val audioExtensions = audioExt + audioExt.map { it.uppercase() }

        val tagIndex = Regex("\\#\\[([0-9]+)\\](.*)")
        val hashTagsPattern: Regex =
@@ -512,14 +525,13 @@ class RichTextParser {
                    it.uppercase()
                }

-        private fun removeQueryParamsForExtensionComparison(fullUrl: String): String =
-            if (fullUrl.contains("?")) {
-                fullUrl.split("?")[0]
-            } else if (fullUrl.contains("#")) {
-                fullUrl.split("#")[0]
-            } else {
-                fullUrl
-            }
+        private fun removeQueryParamsForExtensionComparison(fullUrl: String): String {
+            // Called per URL during feed render — substringBefore allocates nothing when the
+            // separator is absent, unlike split().
+            val queryStart = fullUrl.indexOf('?')
+            if (queryStart >= 0) return fullUrl.substring(0, queryStart)
+            return fullUrl.substringBefore('#')
+        }

        fun isImageExtension(ext: String) = imageExtensions.any { it == ext }

@@ -542,6 +554,18 @@ class RichTextParser {
            return videoExtensions.any { removedParamsFromUrl.endsWith(it) }
        }

+        fun isAudioUrl(url: String): Boolean {
+            val removedParamsFromUrl = removeQueryParamsForExtensionComparison(url)
+            return audioExtensions.any { removedParamsFromUrl.endsWith(it) }
+        }
+
+        // A declared MIME type is authoritative when present; the URL extension is only a fallback
+        // for the common bare-URL case.
+        fun isAudioContent(
+            mimeType: String?,
+            url: String,
+        ): Boolean = mimeType?.startsWith("audio/") ?: isAudioUrl(url)
+
        // Mirrors the canonical HLS-playlist MIME list also kept in MediaItemCache.toExoPlayerMimeType.
        // Called per URL during feed render — uses `equals(ignoreCase)` instead of `lowercase()` to
        // avoid a per-call String allocation on the common non-HLS path.
@@ -0,0 +1,81 @@
+/*
+ * 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.richtext
+
+import kotlin.test.Test
+import kotlin.test.assertFalse
+import kotlin.test.assertTrue
+
+class RichTextParserAudioUrlTest {
+    @Test
+    fun mp3IsAudio() {
+        assertTrue(RichTextParser.isAudioUrl("https://haven.sdbitcoiners.com/f28a5a2e.mp3"))
+    }
+
+    @Test
+    fun everyAudioContainerIsAudio() {
+        RichTextParser.audioExt.forEach {
+            assertTrue(RichTextParser.isAudioUrl("https://example.com/a.$it"), it)
+        }
+    }
+
+    @Test
+    fun uppercaseIsAudio() {
+        assertTrue(RichTextParser.isAudioUrl("https://example.com/A.MP3"))
+    }
+
+    @Test
+    fun queryParamsAndFragmentsAreIgnored() {
+        assertTrue(RichTextParser.isAudioUrl("https://example.com/a.mp3?x=1"))
+        assertTrue(RichTextParser.isAudioUrl("https://example.com/a.mp3#t=10"))
+    }
+
+    @Test
+    fun videoIsNotAudio() {
+        assertFalse(RichTextParser.isAudioUrl("https://example.com/a.mp4"))
+        assertFalse(RichTextParser.isAudioUrl("https://example.com/a.webm"))
+        assertFalse(RichTextParser.isAudioUrl("https://example.com/a.mov"))
+    }
+
+    @Test
+    fun hlsPlaylistIsNotAudio() {
+        // A .m3u8 carries either, and a live stream is the reason the 16:9 default exists.
+        assertFalse(RichTextParser.isAudioUrl("https://example.com/stream.m3u8"))
+    }
+
+    @Test
+    fun imagesAndUnknownAreNotAudio() {
+        assertFalse(RichTextParser.isAudioUrl("https://example.com/a.jpg"))
+        assertFalse(RichTextParser.isAudioUrl("https://example.com/nothing"))
+    }
+
+    @Test
+    fun mimeTypeIsAuthoritativeOverTheUrl() {
+        assertTrue(RichTextParser.isAudioContent("audio/mpeg", "https://example.com/download?id=7"))
+        assertFalse(RichTextParser.isAudioContent("video/mp4", "https://example.com/a.mp3"))
+    }
+
+    @Test
+    fun urlExtensionIsTheFallbackWithoutAMimeType() {
+        assertTrue(RichTextParser.isAudioContent(null, "https://example.com/a.mp3"))
+        assertFalse(RichTextParser.isAudioContent(null, "https://example.com/a.mp4"))
+    }
+}