diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/MediaAspectRatioCache.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/MediaAspectRatioCache.kt index c5b9acfee8..5057f55847 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/MediaAspectRatioCache.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/MediaAspectRatioCache.kt @@ -56,12 +56,16 @@ object MediaAspectRatioCache : MutableMediaAspectRatioCache { override fun get(url: String): Float? = entry(url).value + // Both sides are checked, not just the divisor: a zero width divides cleanly to 0f, and 0f is + // just as unusable downstream as a division by zero — `Modifier.aspectRatio` throws on it. A + // reported size that cannot be laid out is stored as no size at all, leaving the entry empty + // for a later, better report to fill. override fun add( url: String, width: Int, height: Int, ) { - if (height > 1) { + if (width > 0 && height > 1) { entry(url).value = width.toFloat() / height.toFloat() } } diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/GifVideoView.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/GifVideoView.kt index e251521024..c93211bafa 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/GifVideoView.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/GifVideoView.kt @@ -66,11 +66,11 @@ fun GifVideoView( accountViewModel: AccountViewModel, thumbhash: String? = null, ) { - // Pure read path — DimensionTag.aspectRatio() is a one-line int division and + // Pure read path — DimensionTag.aspectRatioOrNull() is a one-line int division and // MediaAspectRatioCache.get() is a synchronized LruCache lookup. Wrapping this in // remember() to avoid the recompute would cost more (slot read + N equality checks) // than the work it saves; that's why this stays as a plain expression. - val ratio = dimensions?.aspectRatio() ?: MediaAspectRatioCache.get(videoUri) + val ratio = dimensions?.aspectRatioOrNull() ?: MediaAspectRatioCache.get(videoUri) val autoPlay = accountViewModel.settings.autoPlayVideos() val borderModifier = if (roundedCorner) MaterialTheme.colorScheme.imageModifier else Modifier val context = LocalContext.current diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/ImageGallery.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/ImageGallery.kt index 450125d2a7..7e0290a40d 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/ImageGallery.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/ImageGallery.kt @@ -62,11 +62,7 @@ private data class FirstImageOrientation( val isLandscape: Boolean, ) -private fun MediaUrlImage.resolvedAspectRatio(): Float? = - dim - ?.takeIf { it.hasSize() } - ?.aspectRatio() - ?: MediaAspectRatioCache.get(url) +private fun MediaUrlImage.resolvedAspectRatio(): Float? = dim?.aspectRatioOrNull() ?: MediaAspectRatioCache.get(url) private fun MediaUrlImage?.resolveOrientation( landscapeDefaultAspectRatio: Float, diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/SensitivityWarning.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/SensitivityWarning.kt index fd8d7b7e29..6aff101a1d 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/SensitivityWarning.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/SensitivityWarning.kt @@ -240,13 +240,23 @@ fun BlurhashGridBackdrop(media: List) { } } +/** + * Sizes a media container from a reported width/height ratio, falling back to width-only when + * there is no usable one. + * + * A ratio that is not finite and positive counts as no ratio: `Modifier.aspectRatio` throws + * `IllegalArgumentException` on `0f` and on `NaN`, which would take down the composition around + * the media rather than just mis-sizing it. Callers are expected to hand over a clean value + * (`DimensionTag.aspectRatioOrNull`, `MediaAspectRatioCache`), so this is the backstop for the one + * that forgets, not the primary guard. + */ fun mediaSizingModifier( ratio: Float?, contentScale: ContentScale, ): Modifier = when { contentScale == ContentScale.Crop -> Modifier.fillMaxSize() - ratio != null -> Modifier.fillMaxWidth().aspectRatio(ratio) + ratio != null && ratio > 0f && ratio.isFinite() -> Modifier.fillMaxWidth().aspectRatio(ratio) else -> Modifier.fillMaxWidth() } diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/ZoomableContentDialog.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/ZoomableContentDialog.kt index f211bbfa4f..4239c9be68 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/ZoomableContentDialog.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/ZoomableContentDialog.kt @@ -628,7 +628,7 @@ private fun RenderImageOrVideo( Modifier.fillMaxWidth() } - val ratio = content.dim?.aspectRatio() ?: MediaAspectRatioCache.get(content.url) + val ratio = content.dim?.aspectRatioOrNull() ?: MediaAspectRatioCache.get(content.url) val useLocalBlossomBridge by accountViewModel.useLocalBlossomBridge.collectAsStateWithLifecycle() val bridgedUrl = remember(content.url, useLocalBlossomBridge) { @@ -694,7 +694,7 @@ private fun RenderImageOrVideo( } content.localFile?.let { - val ratio = content.dim?.aspectRatio() ?: MediaAspectRatioCache.get(it.toUri().toString()) + val ratio = content.dim?.aspectRatioOrNull() ?: MediaAspectRatioCache.get(it.toUri().toString()) val modifier = if (ratio != null) { diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/ZoomableContentView.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/ZoomableContentView.kt index 1122b73a57..0928ad0c5c 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/ZoomableContentView.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/ZoomableContentView.kt @@ -181,7 +181,7 @@ fun ZoomableContentView( when (content) { is MediaUrlImage -> { - val ratio = content.dim?.aspectRatio() ?: MediaAspectRatioCache.get(content.url) + val ratio = content.dim?.aspectRatioOrNull() ?: MediaAspectRatioCache.get(content.url) val bridgedUrl = remember(content.url, useLocalBlossomBridge) { content.toCoilModel(useLocalBlossomBridge) @@ -228,7 +228,7 @@ fun ZoomableContentView( unknownMediaAspectRatio(content.mimeType, content.url) } val ratio = - content.dim?.aspectRatio() + content.dim?.aspectRatioOrNull() ?: MediaAspectRatioCache.get(content.url) ?: fallbackRatio val bridgedUrl = @@ -386,7 +386,7 @@ fun LocalImageView( ) } - val ratio = remember(content) { content.dim?.aspectRatio() ?: MediaAspectRatioCache.get(content.localFile.toString()) } + val ratio = remember(content) { content.dim?.aspectRatioOrNull() ?: MediaAspectRatioCache.get(content.localFile.toString()) } val context = LocalContext.current val imageModel = if (fullResolution) { @@ -510,7 +510,7 @@ fun UrlImageView( alwayShowImage: Boolean = false, fullResolution: Boolean = false, ) { - val ratio = content.dim?.aspectRatio() ?: MediaAspectRatioCache.get(content.url) + val ratio = content.dim?.aspectRatioOrNull() ?: MediaAspectRatioCache.get(content.url) val showImage = remember { diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/pdf/PdfFetcher.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/pdf/PdfFetcher.kt index 4932b0a97f..c1fcee999a 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/pdf/PdfFetcher.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/pdf/PdfFetcher.kt @@ -41,11 +41,16 @@ object PdfFetcher { suspend fun fetchSnapshot( url: String, okHttpClient: (String) -> OkHttpClient, - ): DiskCache.Snapshot { - val diskCache = Amethyst.instance.diskCache - diskCache.openSnapshot(url)?.let { return it } + ): DiskCache.Snapshot = + withContext(Dispatchers.IO) { + val diskCache = Amethyst.instance.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 } - return withContext(Dispatchers.IO) { val editor = diskCache.openEditor(url) ?: throw IOException("Unable to open cache editor for $url") try { val request = @@ -71,5 +76,4 @@ object PdfFetcher { throw t } } - } } diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/pdf/PdfPreviewCard.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/pdf/PdfPreviewCard.kt index 0b975ede46..fd3b91b9ab 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/pdf/PdfPreviewCard.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/pdf/PdfPreviewCard.kt @@ -26,6 +26,7 @@ import android.os.ParcelFileDescriptor import androidx.compose.foundation.ExperimentalFoundationApi import androidx.compose.foundation.Image import androidx.compose.foundation.combinedClickable +import androidx.compose.foundation.layout.Box import androidx.compose.foundation.layout.Column import androidx.compose.foundation.layout.Spacer import androidx.compose.foundation.layout.aspectRatio @@ -36,6 +37,7 @@ import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.produceState import androidx.compose.runtime.remember +import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier import androidx.compose.ui.graphics.FilterQuality import androidx.compose.ui.graphics.asImageBitmap @@ -44,11 +46,15 @@ import androidx.compose.ui.platform.LocalWindowInfo import androidx.core.graphics.createBitmap import com.vitorpamplona.amethyst.commons.icons.symbols.MaterialSymbols import com.vitorpamplona.amethyst.commons.richtext.MediaUrlPdf +import com.vitorpamplona.amethyst.commons.ui.components.LoadingAnimation +import com.vitorpamplona.amethyst.model.MediaAspectRatioCache import com.vitorpamplona.amethyst.ui.components.ClickableUrl import com.vitorpamplona.amethyst.ui.components.FileAttachmentRow import com.vitorpamplona.amethyst.ui.components.ShareMediaAction import com.vitorpamplona.amethyst.ui.screen.loggedIn.AccountViewModel import com.vitorpamplona.amethyst.ui.theme.DoubleVertSpacer +import com.vitorpamplona.amethyst.ui.theme.Size40dp +import com.vitorpamplona.amethyst.ui.theme.Size6dp import com.vitorpamplona.amethyst.ui.theme.innerPostModifier import com.vitorpamplona.quartz.utils.Log import kotlinx.coroutines.CancellationException @@ -58,11 +64,43 @@ 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 +// Floor for the width/height ratio the card lays out with. A pathologically tall page (a receipt, +// a single-column banner) would otherwise reserve a screenful of height for a sliver of content. +private const val MIN_PREVIEW_ASPECT_RATIO = 0.2f + +// Shape to assume when the page reports no usable size at all. US Letter portrait — the +// overwhelmingly common page shape, and the least surprising box to hold open for an unknown one. +private const val DEFAULT_PAGE_ASPECT_RATIO = 612f / 792f + +/** + * The ratio the card actually lays out with, given a page's natural width/height. The placeholder + * and the loaded thumbnail both go through here so the box reserved while loading is the exact box + * the rendered page lands in — the clamp has to be applied on both sides or the reservation is + * wrong for the very pages it exists to protect. + * + * This is also the one place that guarantees `Modifier.aspectRatio` is handed a finite, positive + * number. It throws `IllegalArgumentException` on `0f` and on `NaN`, and the clamp below does not + * catch either: `NaN.coerceAtLeast(x)` is `NaN`, because every comparison against `NaN` is false. + * Both are reachable from real input — a malformed PDF whose first page measures 0x0, or an imeta + * `dim` that survives DimensionTag.parse's only-rejects-literal-"0x0" check as 0x0 anyway + * (`"0.4x0.4"` truncates to it). Without this guard either one takes down the whole feed's + * composition, from a tag any relay can carry. + */ +internal fun previewAspectRatio(pageAspectRatio: Float): Float = + if (!pageAspectRatio.isFinite() || pageAspectRatio <= 0f) { + DEFAULT_PAGE_ASPECT_RATIO + } else { + pageAspectRatio.coerceAtLeast(MIN_PREVIEW_ASPECT_RATIO) + } + data class PdfPreview( val thumbnail: Bitmap, val pageCount: Int, - val aspectRatio: Float, -) + val pageWidth: Int, + val pageHeight: Int, +) { + val aspectRatio: Float = pageWidth.toFloat() / pageHeight.toFloat() +} private sealed class PdfLoadState { data object Loading : PdfLoadState() @@ -104,6 +142,14 @@ private fun LoadedPdfPreviewCard( containerWidthPx.coerceAtMost(THUMBNAIL_MAX_DIM_PX).coerceAtLeast(1) } + // Read in composition — never inside a remember — so this recomposes the moment the render + // below fills the entry in. On a revisit the entry is already there and the placeholder can + // reserve the right box from the first frame, which is the whole point of caching it. + // The cache is consulted ahead of the imeta `dim` because it holds the page size this card + // measured itself: an author-supplied `dim` that disagrees would guarantee the jump on every + // single visit, which is exactly what this is here to stop. + val knownAspectRatio = MediaAspectRatioCache.get(content.url) ?: content.dim?.aspectRatioOrNull() + @Suppress("ProduceStateDoesNotAssignValue") val state by produceState(initialValue = PdfLoadState.Loading, key1 = content.url, key2 = targetWidthPx) { value = @@ -115,6 +161,13 @@ private fun LoadedPdfPreviewCard( withContext(Dispatchers.IO) { 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 + // from a bare filename row into a full page. + if (result is PdfLoadState.Ready) { + MediaAspectRatioCache.add(content.url, result.preview.pageWidth, result.preview.pageHeight) + } } } catch (e: Exception) { if (e is CancellationException) throw e @@ -134,7 +187,7 @@ private fun LoadedPdfPreviewCard( when (val current = state) { is PdfLoadState.Loading -> { - PdfSkeletonCard(filename) + PdfSkeletonCard(filename, knownAspectRatio) } is PdfLoadState.Failed -> { @@ -150,15 +203,20 @@ private fun LoadedPdfPreviewCard( onLongClick = { sharePopupExpanded.value = true }, ), ) { + // asImageBitmap() allocates a fresh wrapper on every call and the wrapper compares + // by identity, so calling it inline would defeat the remember(bitmap) that Image + // uses to hold its BitmapPainter — every recomposition would rebuild the painter. + val thumbnail = remember(current.preview.thumbnail) { current.preview.thumbnail.asImageBitmap() } + Image( - bitmap = current.preview.thumbnail.asImageBitmap(), + bitmap = thumbnail, contentDescription = content.description ?: filename, contentScale = ContentScale.FillWidth, filterQuality = FilterQuality.High, modifier = Modifier .fillMaxWidth() - .aspectRatio(current.preview.aspectRatio.coerceAtLeast(0.2f)), + .aspectRatio(previewAspectRatio(current.preview.aspectRatio)), ) FilenameRow(filename = filename, subtitle = pageCountLabel(current.preview.pageCount)) @@ -187,8 +245,26 @@ private fun PlaceholderPdfCard( } @Composable -private fun PdfSkeletonCard(filename: String) { +private fun PdfSkeletonCard( + filename: String, + aspectRatio: Float?, +) { Column(modifier = MaterialTheme.colorScheme.innerPostModifier.fillMaxWidth()) { + // Only known on a revisit — the first render is what fills the cache — so the first sight of + // a PDF still grows into place. From then on the page's box is held open while it renders + // and the feed stays put. + if (aspectRatio != null) { + Box( + modifier = + Modifier + .fillMaxWidth() + .aspectRatio(previewAspectRatio(aspectRatio)), + contentAlignment = Alignment.Center, + ) { + LoadingAnimation(Size40dp, Size6dp) + } + } + FilenameRow(filename = filename, subtitle = "Loading…") Spacer(modifier = DoubleVertSpacer) } @@ -223,7 +299,8 @@ private fun renderFirstPage( PdfPreview( thumbnail = bitmap, pageCount = pageCount, - aspectRatio = page.width.toFloat() / page.height.toFloat(), + pageWidth = page.width, + pageHeight = page.height, ), ) } diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/types/PictureDisplay.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/types/PictureDisplay.kt index f5fc594951..6143af7dd4 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/types/PictureDisplay.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/types/PictureDisplay.kt @@ -107,7 +107,7 @@ fun PictureDisplay( } if (images.size == 1) { - val ratio = first.dim?.aspectRatio() ?: MediaAspectRatioCache.get(first.url) + val ratio = first.dim?.aspectRatioOrNull() ?: MediaAspectRatioCache.get(first.url) ContentWarningGate( isSensitive = isSensitive, reasons = reasons, diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/types/VideoDisplay.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/types/VideoDisplay.kt index 4f5c661810..afe66a4224 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/types/VideoDisplay.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/types/VideoDisplay.kt @@ -92,7 +92,7 @@ fun JustVideoDisplay( ) } - val ratio = imeta.dimension?.aspectRatio() ?: MediaAspectRatioCache.get(imeta.url) + val ratio = imeta.dimension?.aspectRatioOrNull() ?: MediaAspectRatioCache.get(imeta.url) ContentWarningGate( isSensitive = isSensitive, diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/pictures/PictureCardCompose.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/pictures/PictureCardCompose.kt index 361515edd1..3ca8de4921 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/pictures/PictureCardCompose.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/pictures/PictureCardCompose.kt @@ -125,7 +125,7 @@ private fun PictureCardImage( if (images.size == 1) { val single = images.first() - val ratio = single.dim?.aspectRatio() ?: MediaAspectRatioCache.get(single.url) + val ratio = single.dim?.aspectRatioOrNull() ?: MediaAspectRatioCache.get(single.url) ContentWarningGate( isSensitive = isSensitive, reasons = reasons, diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/shorts/VideoCardCompose.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/shorts/VideoCardCompose.kt index 9e52ec0362..1c54ac585d 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/shorts/VideoCardCompose.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/shorts/VideoCardCompose.kt @@ -141,7 +141,7 @@ private fun VideoCardImage( ) } - val ratio = imeta.dimension?.aspectRatio() ?: MediaAspectRatioCache.get(imeta.url) + val ratio = imeta.dimension?.aspectRatioOrNull() ?: MediaAspectRatioCache.get(imeta.url) ContentWarningGate( isSensitive = isSensitive, diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/video/FileHeaderCardCompose.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/video/FileHeaderCardCompose.kt index d5b18976f2..112c9af02e 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/video/FileHeaderCardCompose.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/video/FileHeaderCardCompose.kt @@ -125,7 +125,7 @@ private fun FileHeaderCardImage( } val isImage = content is MediaUrlImage - val ratio = dimensions?.aspectRatio() ?: MediaAspectRatioCache.get(fullUrl) + val ratio = dimensions?.aspectRatioOrNull() ?: MediaAspectRatioCache.get(fullUrl) ContentWarningGate( isSensitive = isSensitive, diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/components/pdf/PdfPreviewAspectRatioTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/components/pdf/PdfPreviewAspectRatioTest.kt new file mode 100644 index 0000000000..5750780d29 --- /dev/null +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/components/pdf/PdfPreviewAspectRatioTest.kt @@ -0,0 +1,100 @@ +/* + * 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.pdf + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * The placeholder shown while a previously-seen PDF re-renders reserves its box from the ratio + * cached in `MediaAspectRatioCache`, and the loaded thumbnail sizes itself from the ratio the + * renderer reports. Those are the same number, so both sides must clamp it identically — a + * placeholder that skipped the clamp would reserve a different box than the page lands in, which is + * the jump it exists to remove. + */ +class PdfPreviewAspectRatioTest { + @Test + fun ordinaryPortraitPageIsUnchanged() { + // US Letter, in PostScript points. + assertEquals(612f / 792f, previewAspectRatio(612f / 792f), 0.0001f) + } + + @Test + fun landscapePageIsUnchanged() { + assertEquals(792f / 612f, previewAspectRatio(792f / 612f), 0.0001f) + } + + @Test + fun pathologicallyTallPageIsClamped() { + // A 200x4000 receipt would otherwise reserve 20 screens of height for a sliver of content. + assertEquals(0.2f, previewAspectRatio(200f / 4000f), 0.0001f) + } + + @Test + fun theClampIsTheSameNumberBothSidesUse() { + val cachedOnRevisit = 200f / 4000f + val reportedByTheRenderer = 200f / 4000f + + assertEquals(previewAspectRatio(reportedByTheRenderer), previewAspectRatio(cachedOnRevisit), 0.0f) + } + + /** + * `Modifier.aspectRatio` requires a finite ratio greater than zero and throws otherwise, so + * every one of these would crash the composition of the whole feed if it reached the modifier. + * A page measuring 0x0 and an imeta `dim` of `"0.4x0.4"` (which truncates to 0x0, slipping past + * the parser's literal-`"0x0"` rejection) both arrive here as 0/0. + */ + @Test + fun degenerateSizesNeverReachTheModifier() { + assertUsable(previewAspectRatio(ratioOf(0, 0))) + assertUsable(previewAspectRatio(ratioOf(100, 0))) + assertUsable(previewAspectRatio(ratioOf(0, 100))) + assertUsable(previewAspectRatio(-1f)) + } + + /** + * Pins the reason the guard above cannot just be the clamp: every comparison against NaN is + * false, so `coerceAtLeast` returns NaN unchanged rather than lifting it to the floor. + */ + @Test + fun clampingAloneDoesNotCatchNaN() { + assertTrue(ratioOf(0, 0).coerceAtLeast(0.2f).isNaN()) + } + + @Test + fun unknownSizedPagesFallBackToPortrait() { + // Taller than wide, so an unknown page reserves a portrait box rather than a squat one. + assertTrue(previewAspectRatio(ratioOf(0, 0)) < 1f) + } + + // Computed rather than written as a literal so the compiler doesn't fold it into a + // division-by-zero warning — these degenerate sizes are the point of the test. + private fun ratioOf( + width: Int, + height: Int, + ): Float = width.toFloat() / height.toFloat() + + private fun assertUsable(ratio: Float) { + assertTrue("$ratio is not finite", ratio.isFinite()) + assertTrue("$ratio is not > 0", ratio > 0f) + } +} diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip94FileMetadata/tags/DimensionTag.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip94FileMetadata/tags/DimensionTag.kt index 0ec356b6d3..d502939609 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip94FileMetadata/tags/DimensionTag.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip94FileMetadata/tags/DimensionTag.kt @@ -24,15 +24,44 @@ import androidx.compose.runtime.Stable import com.vitorpamplona.quartz.nip01Core.core.has import com.vitorpamplona.quartz.utils.ensure import kotlinx.serialization.Serializable +import kotlinx.serialization.Transient @Serializable @Stable class DimensionTag( val width: Int, val height: Int, + /** + * The ratio exactly as the author declared it, kept because [width] and [height] are whole + * pixels and truncating to them can lose it — entirely, for a dim below `1` on either axis. + * Only [parse] supplies it, and it is deliberately [Transient]: it is derived from the tag + * text, so a deserialized instance simply falls back to the pixel counts. + */ + @Transient private val declaredRatio: Float? = null, ) { + /** + * The raw width/height ratio in whole pixels, which is only meaningful when [hasSize] is true. + * Anywhere the result reaches a layout, use [aspectRatioOrNull] instead. + */ fun aspectRatio() = width.toFloat() / height.toFloat() + /** + * The ratio to lay out with, or null when the tag carries no usable shape. + * + * Prefers the declared ratio over the truncated pixel counts, so a fractional dim keeps the + * shape its author meant: `"0.75x1"` is a legitimate 3:4 even though it rounds down to `0x1`, + * and `"317.9x498.4"` is fractionally more accurate than `317x498`. For whole-number dims — + * every well-formed tag — the two are the same number. + * + * Returning null matters as much as the value. [parse] only rejects the literal string + * `"0x0"`, so a tag can still arrive with no usable shape at all, and [aspectRatio] then + * returns `0/0`, which is `NaN`. Compose's `Modifier.aspectRatio` throws + * `IllegalArgumentException` on `NaN` and on `0f`, and clamping does not rescue either: every + * comparison against `NaN` is false, so `coerceAtLeast` returns it unchanged. A tag any relay + * can carry would otherwise crash the composition around it. + */ + fun aspectRatioOrNull(): Float? = declaredRatio ?: if (hasSize()) aspectRatio() else null + fun hasSize() = width > 0 && height > 0 override fun toString() = "${width}x$height" @@ -59,15 +88,28 @@ class DimensionTag( // Some clients (e.g. Primal) emit floating-point dimensions like "317.0x498.0" // in NIP-92 imeta tags. Parse as Double and truncate to keep those tags usable // for pre-load layout reservation. - val width = parts[0].toDouble().toInt() - val height = parts[1].toDouble().toInt() + val width = parts[0].toDouble() + val height = parts[1].toDouble() - DimensionTag(width, height) + DimensionTag(width.toInt(), height.toInt(), declaredRatio(width, height)) } catch (e: Exception) { null } } + /** + * The declared ratio, or null when the declared size cannot describe a shape at all. + * Computed before the truncation to whole pixels, which is the whole point: a dim under + * `1` on either axis truncates to `0` and takes its shape with it. + */ + private fun declaredRatio( + width: Double, + height: Double, + ): Float? { + if (width <= 0.0 || height <= 0.0) return null + return (width / height).toFloat().takeIf { it.isFinite() && it > 0f } + } + fun assemble(dim: DimensionTag) = arrayOf(TAG_NAME, dim.toString()) } } diff --git a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip94FileMetadata/tags/DimensionTagTest.kt b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip94FileMetadata/tags/DimensionTagTest.kt index e97ecbf833..abec85a647 100644 --- a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip94FileMetadata/tags/DimensionTagTest.kt +++ b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip94FileMetadata/tags/DimensionTagTest.kt @@ -22,8 +22,10 @@ package com.vitorpamplona.quartz.nip94FileMetadata.tags import kotlin.test.Test import kotlin.test.assertEquals +import kotlin.test.assertFalse import kotlin.test.assertNotNull import kotlin.test.assertNull +import kotlin.test.assertTrue class DimensionTagTest { @Test @@ -72,4 +74,102 @@ class DimensionTagTest { assertNotNull(tag) assertEquals(317f / 498f, tag.aspectRatio()) } + + /** + * [rejectsZeroByZero] passes by comparing the string, so the float path of the Primal + * tolerance above reaches the same 0x0 pixel counts by truncation and is kept. + */ + @Test + fun subPixelFloatsTruncateToNoPixels() { + val tag = DimensionTag.parse("0.4x0.4") + assertNotNull(tag) + assertEquals(0, tag.width) + assertEquals(0, tag.height) + assertFalse(tag.hasSize()) + } + + /** + * ...but the shape the author declared survives that truncation. A square is a square whether + * it was declared as "0.4x0.4" or "400x400"; only the pixel counts are unusable. + */ + @Test + fun subPixelFloatsKeepTheDeclaredShape() { + assertEquals(1f, DimensionTag.parse("0.4x0.4")?.aspectRatioOrNull()) + assertEquals(0.75f, DimensionTag.parse("0.75x1")?.aspectRatioOrNull()) + assertEquals(2f, DimensionTag.parse("0.5x0.25")?.aspectRatioOrNull()) + } + + /** + * The declared ratio wins over the truncated one wherever they differ, being the more faithful + * of the two. They differ only for fractional dims — for whole numbers it is the same number. + */ + @Test + fun declaredRatioBeatsTheTruncatedOne() { + val tag = DimensionTag.parse("317.9x498.4") + assertNotNull(tag) + assertEquals((317.9 / 498.4).toFloat(), tag.aspectRatioOrNull()) + assertEquals(317f / 498f, tag.aspectRatio()) + } + + @Test + fun wholeNumberDimsAreUnaffected() { + val tag = DimensionTag.parse("1920x1080") + assertNotNull(tag) + assertEquals(tag.aspectRatio(), tag.aspectRatioOrNull()) + } + + /** + * The declared ratio is derived from the tag text, so it is not serialized — a tag rebuilt + * from its width and height falls back to the pixel counts rather than carrying a stale one. + */ + @Test + fun aDirectlyConstructedTagFallsBackToPixelCounts() { + assertEquals(1920f / 1080f, DimensionTag(1920, 1080).aspectRatioOrNull()) + assertNull(DimensionTag(0, 0).aspectRatioOrNull()) + } + + /** + * Why [DimensionTag.aspectRatioOrNull] exists: `Modifier.aspectRatio` throws on both of these, + * so a tag that reaches a layout through the raw accessor crashes the composition around it. + */ + @Test + fun rawAspectRatioOfAZeroSizedTagIsNotLayoutSafe() { + assertTrue(DimensionTag(0, 0).aspectRatio().isNaN()) + assertEquals(0f, DimensionTag(0, 100).aspectRatio()) + } + + /** + * A fractional dim keeps its shape (see [subPixelFloatsKeepTheDeclaredShape]); one that + * declares no shape at all still has to come back null, since `Modifier.aspectRatio` throws on + * everything this would otherwise produce. + */ + @Test + fun aspectRatioOrNullRefusesEveryShapeALayoutCannotUse() { + assertNull(DimensionTag.parse("0x5")?.aspectRatioOrNull()) + assertNull(DimensionTag.parse("-3x4")?.aspectRatioOrNull()) + assertNull(DimensionTag(0, 0).aspectRatioOrNull()) + assertNull(DimensionTag(100, 0).aspectRatioOrNull()) + assertNull(DimensionTag(0, 100).aspectRatioOrNull()) + assertNull(DimensionTag(-1, 10).aspectRatioOrNull()) + } + + @Test + fun aspectRatioOrNullKeepsUsableSizes() { + assertEquals(317f / 498f, DimensionTag(317, 498).aspectRatioOrNull()) + } + + /** + * `"NaN"` and `"Infinity"` are legal input to Kotlin's `String.toDouble()` and a relay can + * carry either, so both reach the ratio maths. The `<= 0.0` rejection cannot stop `NaN` — + * every comparison against it is false — which is why the finite check is the one holding the + * line. Whatever comes out, it is never a shape a layout would throw on. + */ + @Test + fun nonNumericDoublesNeverProduceAnUnusableShape() { + assertNull(DimensionTag.parse("NaNxNaN")?.aspectRatioOrNull()) + assertNull(DimensionTag.parse("0.4xNaN")?.aspectRatioOrNull()) + + // Infinity saturates to Int.MAX_VALUE on both axes: garbage in, square out, never a throw. + assertEquals(1f, DimensionTag.parse("InfinityxInfinity")?.aspectRatioOrNull()) + } }