Skip to content

Commit 608480f

Browse files
faogustavofaogustavo
committed
[JEWEL-941] Reducing space used by images on markdown image error
- When the line contains only images, replaced the Text component with FlowRow - Using a flow row we can skip the Paragraph component that was taking extra space
1 parent c14e322 commit 608480f

File tree

5 files changed

+127
-55
lines changed

5 files changed

+127
-55
lines changed

‎platform/jewel/markdown/core/src/main/kotlin/org/jetbrains/jewel/markdown/Markdown.kt‎

Lines changed: 25 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,10 @@ package org.jetbrains.jewel.markdown
33
import androidx.compose.foundation.layout.Arrangement
44
import androidx.compose.foundation.layout.Column
55
import androidx.compose.foundation.layout.PaddingValues
6+
import androidx.compose.foundation.layout.padding
67
import androidx.compose.foundation.lazy.LazyColumn
78
import androidx.compose.foundation.lazy.LazyListState
8-
import androidx.compose.foundation.lazy.items
9+
import androidx.compose.foundation.lazy.itemsIndexed
910
import androidx.compose.foundation.lazy.rememberLazyListState
1011
import androidx.compose.foundation.text.selection.SelectionContainer
1112
import androidx.compose.runtime.Composable
@@ -302,12 +303,29 @@ public fun LazyMarkdown(
302303
blockRenderer: MarkdownBlockRenderer = JewelTheme.markdownBlockRenderer,
303304
) {
304305
MaybeSelectable(selectable, modifier) {
305-
LazyColumn(
306-
state = state,
307-
contentPadding = contentPadding,
308-
verticalArrangement = Arrangement.spacedBy(markdownStyling.blockVerticalSpacing),
309-
) {
310-
items(blocks) { block -> blockRenderer.RenderBlock(block, enabled, onUrlClick, Modifier) }
306+
LazyColumn(state = state, contentPadding = contentPadding) {
307+
itemsIndexed(blocks) { index, block ->
308+
blockRenderer.RenderBlock(
309+
block = block,
310+
enabled = enabled,
311+
onUrlClick = onUrlClick,
312+
modifier =
313+
Modifier.padding(
314+
top =
315+
if (index == 0) {
316+
0.dp
317+
} else {
318+
(markdownStyling.blockVerticalSpacing / 2)
319+
},
320+
bottom =
321+
if (index == blocks.lastIndex) {
322+
0.dp
323+
} else {
324+
(markdownStyling.blockVerticalSpacing / 2)
325+
},
326+
),
327+
)
328+
}
311329
}
312330
}
313331
}

‎platform/jewel/markdown/core/src/main/kotlin/org/jetbrains/jewel/markdown/extensions/ImageRendererExtension.kt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,5 +23,5 @@ public interface ImageRendererExtension {
2323
* @param image The image data, containing information like the image URL and alt text.
2424
* @return An [InlineTextContent] that will be embedded in the text flow, which will be used to display the image.
2525
*/
26-
@Composable public fun renderImageContent(image: InlineMarkdown.Image): InlineTextContent
26+
@Composable public fun renderImageContent(image: InlineMarkdown.Image): InlineTextContent?
2727
}

‎platform/jewel/markdown/core/src/main/kotlin/org/jetbrains/jewel/markdown/rendering/DefaultMarkdownBlockRenderer.kt‎

Lines changed: 87 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -3,12 +3,15 @@ package org.jetbrains.jewel.markdown.rendering
33
import androidx.compose.foundation.background
44
import androidx.compose.foundation.border
55
import androidx.compose.foundation.layout.Arrangement
6+
import androidx.compose.foundation.layout.Box
67
import androidx.compose.foundation.layout.Column
8+
import androidx.compose.foundation.layout.FlowRow
79
import androidx.compose.foundation.layout.Row
810
import androidx.compose.foundation.layout.Spacer
911
import androidx.compose.foundation.layout.fillMaxWidth
1012
import androidx.compose.foundation.layout.height
1113
import androidx.compose.foundation.layout.padding
14+
import androidx.compose.foundation.layout.size
1215
import androidx.compose.foundation.layout.width
1316
import androidx.compose.foundation.layout.widthIn
1417
import androidx.compose.foundation.text.InlineTextContent
@@ -18,6 +21,7 @@ import androidx.compose.runtime.CompositionLocalProvider
1821
import androidx.compose.runtime.collectAsState
1922
import androidx.compose.runtime.getValue
2023
import androidx.compose.runtime.movableContentOf
24+
import androidx.compose.runtime.mutableStateMapOf
2125
import androidx.compose.runtime.remember
2226
import androidx.compose.ui.Alignment
2327
import androidx.compose.ui.Modifier
@@ -29,6 +33,7 @@ import androidx.compose.ui.graphics.isSpecified
2933
import androidx.compose.ui.graphics.takeOrElse
3034
import androidx.compose.ui.input.pointer.PointerIcon
3135
import androidx.compose.ui.input.pointer.pointerHoverIcon
36+
import androidx.compose.ui.platform.LocalDensity
3237
import androidx.compose.ui.text.AnnotatedString
3338
import androidx.compose.ui.text.TextLayoutResult
3439
import androidx.compose.ui.text.TextStyle
@@ -75,7 +80,7 @@ private const val DISABLED_CODE_ALPHA = .5f
7580
*
7681
* @see MarkdownBlockRenderer
7782
*/
78-
@Suppress("OVERRIDE_DEPRECATION")
83+
@Suppress("OVERRIDE_DEPRECATION", "LargeClass")
7984
@ApiStatus.Experimental
8085
@ExperimentalJewelApi
8186
public open class DefaultMarkdownBlockRenderer(
@@ -193,23 +198,30 @@ public open class DefaultMarkdownBlockRenderer(
193198
softWrap: Boolean,
194199
maxLines: Int,
195200
) {
196-
val renderedContent = rememberRenderedContent(block, styling.inlinesStyling, enabled, onUrlClick)
197-
val textColor =
198-
styling.inlinesStyling.textStyle.color
199-
.takeOrElse { LocalContentColor.current }
200-
.takeOrElse { styling.inlinesStyling.textStyle.color }
201-
val mergedStyle = styling.inlinesStyling.textStyle.merge(TextStyle(color = textColor))
201+
val onlyImages = remember(block) { block.inlineContent.all { it is InlineMarkdown.Image } }
202+
val images = renderedImages(block)
202203

203-
Text(
204-
modifier = modifier,
205-
text = renderedContent,
206-
overflow = overflow,
207-
softWrap = softWrap,
208-
maxLines = maxLines,
209-
onTextLayout = onTextLayout,
210-
inlineContent = renderedImages(block),
211-
style = mergedStyle,
212-
)
204+
if (onlyImages) {
205+
RenderImages(images, modifier)
206+
} else {
207+
val renderedContent = rememberRenderedContent(block, styling.inlinesStyling, enabled, onUrlClick)
208+
val textColor =
209+
styling.inlinesStyling.textStyle.color
210+
.takeOrElse { LocalContentColor.current }
211+
.takeOrElse { styling.inlinesStyling.textStyle.color }
212+
val mergedStyle = styling.inlinesStyling.textStyle.merge(TextStyle(color = textColor))
213+
214+
Text(
215+
modifier = modifier,
216+
text = renderedContent,
217+
overflow = overflow,
218+
softWrap = softWrap,
219+
maxLines = maxLines,
220+
onTextLayout = onTextLayout,
221+
inlineContent = images,
222+
style = mergedStyle,
223+
)
224+
}
213225
}
214226

215227
@Composable
@@ -263,19 +275,29 @@ public open class DefaultMarkdownBlockRenderer(
263275
onUrlClick: (String) -> Unit,
264276
modifier: Modifier,
265277
) {
266-
val renderedContent = rememberRenderedContent(block, styling.inlinesStyling, enabled, onUrlClick)
278+
val onlyImages = remember(block) { block.inlineContent.all { it is InlineMarkdown.Image } }
279+
val images = renderedImages(block)
280+
281+
if (onlyImages && images.isEmpty()) return
282+
267283
Column(modifier = modifier.padding(styling.padding)) {
268-
val textColor =
269-
styling.inlinesStyling.textStyle.color.takeOrElse {
270-
LocalContentColor.current.takeOrElse { styling.inlinesStyling.textStyle.color }
271-
}
272-
val mergedStyle = styling.inlinesStyling.textStyle.merge(TextStyle(color = textColor))
273-
Text(
274-
text = renderedContent,
275-
style = mergedStyle,
276-
modifier = Modifier.focusProperties { this.canFocus = false },
277-
inlineContent = this@DefaultMarkdownBlockRenderer.renderedImages(block),
278-
)
284+
if (onlyImages) {
285+
RenderImages(images)
286+
} else {
287+
val renderedContent = rememberRenderedContent(block, styling.inlinesStyling, enabled, onUrlClick)
288+
289+
val textColor =
290+
styling.inlinesStyling.textStyle.color.takeOrElse {
291+
LocalContentColor.current.takeOrElse { styling.inlinesStyling.textStyle.color }
292+
}
293+
val mergedStyle = styling.inlinesStyling.textStyle.merge(TextStyle(color = textColor))
294+
Text(
295+
text = renderedContent,
296+
style = mergedStyle,
297+
modifier = Modifier.focusProperties { this.canFocus = false },
298+
inlineContent = images,
299+
)
300+
}
279301

280302
if (styling.underlineWidth > 0.dp && styling.underlineColor.isSpecified) {
281303
Spacer(Modifier.height(styling.underlineGap))
@@ -685,15 +707,22 @@ public open class DefaultMarkdownBlockRenderer(
685707
}
686708

687709
@Composable
688-
private fun renderedImages(blockInlineContent: WithInlineMarkdown): Map<String, InlineTextContent> =
689-
rendererExtensions
690-
.firstNotNullOfOrNull { it.imageRendererExtension }
691-
?.let { imagesRenderer ->
692-
getImages(blockInlineContent).associate { image ->
693-
image.source to imagesRenderer.renderImageContent(image)
694-
}
710+
private fun renderedImages(blockInlineContent: WithInlineMarkdown): Map<String, InlineTextContent> {
711+
val map = remember(blockInlineContent) { mutableStateMapOf<String, InlineTextContent>() }
712+
713+
val imagesRenderer = rendererExtensions.firstNotNullOfOrNull { it.imageRendererExtension }
714+
715+
for (image in getImages(blockInlineContent)) {
716+
val renderedImage = imagesRenderer?.renderImageContent(image)
717+
if (renderedImage == null) {
718+
map.remove(image.source)
719+
} else {
720+
map[image.source] = renderedImage
695721
}
696-
.orEmpty()
722+
}
723+
724+
return map
725+
}
697726

698727
@Composable
699728
protected fun MaybeScrollingContainer(
@@ -710,6 +739,25 @@ public open class DefaultMarkdownBlockRenderer(
710739
}
711740
}
712741

742+
@Composable
743+
private fun RenderImages(images: Map<String, InlineTextContent>, modifier: Modifier = Modifier) {
744+
if (images.isEmpty()) return
745+
746+
val density = LocalDensity.current
747+
FlowRow(modifier) {
748+
images.map { (text, inlineBlock) ->
749+
Box(
750+
modifier =
751+
with(density) {
752+
Modifier.size(inlineBlock.placeholder.width.toDp(), inlineBlock.placeholder.height.toDp())
753+
}
754+
) {
755+
inlineBlock.children(text)
756+
}
757+
}
758+
}
759+
}
760+
713761
public override fun createCopy(
714762
rootStyling: MarkdownStyling?,
715763
rendererExtensions: List<MarkdownRendererExtension>?,
@@ -734,9 +782,11 @@ private fun getImages(input: WithInlineMarkdown): List<InlineMarkdown.Image> = b
734782
is InlineMarkdown.Image -> {
735783
if (item.source.isNotBlank()) add(item)
736784
}
785+
737786
is WithInlineMarkdown -> {
738787
collectImagesRecursively(item.inlineContent)
739788
}
789+
740790
else -> {
741791
// Ignored
742792
}

‎platform/jewel/markdown/extensions/images/src/main/kotlin/org/jetbrains/jewel/markdown/extensions/images/Coil3ImageRendererExtensionImpl.kt‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,10 +45,11 @@ internal class Coil3ImageRendererExtensionImpl(private val imageLoader: ImageLoa
4545
* @return An [InlineTextContent] that can be used by a `Text` or `BasicText` composable to render the image inline.
4646
*/
4747
@Composable
48-
public override fun renderImageContent(image: InlineMarkdown.Image): InlineTextContent {
48+
public override fun renderImageContent(image: InlineMarkdown.Image): InlineTextContent? {
4949
val imageSourceResolver = LocalMarkdownImageSourceResolver.current
5050
val resolvedImageSource = remember(image.source) { imageSourceResolver.resolve(image.source) }
5151
var imageResult by remember(resolvedImageSource) { mutableStateOf<SuccessResult?>(null) }
52+
var hasError by remember { mutableStateOf(false) }
5253

5354
val painter =
5455
rememberAsyncImagePainter(
@@ -59,18 +60,25 @@ internal class Coil3ImageRendererExtensionImpl(private val imageLoader: ImageLoa
5960
.size(Size.ORIGINAL)
6061
.build(),
6162
imageLoader = imageLoader,
63+
onLoading = { hasError = false },
6264
onSuccess = { successState ->
65+
hasError = false
6366
// onSuccess should only be called once, but adding additional protection from
6467
// unnecessary rerender
6568
if (imageResult == null) {
6669
imageResult = successState.result
6770
}
6871
},
6972
onError = { error ->
73+
hasError = true
7074
JewelLogger.getInstance(this.javaClass).warn("AsyncImage loading failed.", error.result.throwable)
7175
},
7276
)
7377

78+
if (hasError) {
79+
return null
80+
}
81+
7482
val placeholder =
7583
imageResult?.let {
7684
val imageSize = it.image

‎platform/jewel/markdown/extensions/images/src/test/kotlin/org/jetbrains/jewel/markdown/extensions/images/Coil3ImageRendererExtensionImplTest.kt‎

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -33,14 +33,14 @@ import org.junit.Test
3333
public class Coil3ImageRendererExtensionImplTest {
3434
@get:Rule public val composeTestRule: ComposeContentTestRule = createComposeRule()
3535

36-
private val platformContext: PlatformContext = PlatformContext.Companion.INSTANCE
36+
private val platformContext: PlatformContext = PlatformContext.INSTANCE
3737
private val imageUrl = "https://example.com/image.png"
3838

3939
@Test
4040
public fun `image renders with correct size on success`() {
4141
val fakeImageWidth = 150
4242
val fakeImageHeight = 100
43-
val fakeImage = ColorImage(Color.Companion.Red.toArgb(), width = fakeImageWidth, height = fakeImageHeight)
43+
val fakeImage = ColorImage(Color.Red.toArgb(), width = fakeImageWidth, height = fakeImageHeight)
4444

4545
val engine = FakeImageLoaderEngine.Builder().intercept({ it == imageUrl }, fakeImage).build()
4646

@@ -76,11 +76,7 @@ public class Coil3ImageRendererExtensionImplTest {
7676

7777
setContent(extension, imageMarkdown)
7878

79-
composeTestRule
80-
.onNodeWithContentDescription("Failed to load image")
81-
.assertExists()
82-
.assertWidthIsEqualTo(0.dp)
83-
.assertHeightIsEqualTo(1.dp)
79+
composeTestRule.onNodeWithContentDescription("Failed to load image").assertDoesNotExist()
8480
}
8581

8682
@OptIn(ExperimentalCoroutinesApi::class)
@@ -90,7 +86,7 @@ public class Coil3ImageRendererExtensionImplTest {
9086

9187
val fakeImageWidth = 150
9288
val fakeImageHeight = 150
93-
val fakeImage = ColorImage(Color.Companion.Red.toArgb(), width = fakeImageWidth, height = fakeImageHeight)
89+
val fakeImage = ColorImage(Color.Red.toArgb(), width = fakeImageWidth, height = fakeImageHeight)
9490
val engine =
9591
FakeImageLoaderEngine.Builder()
9692
.intercept(
@@ -129,7 +125,7 @@ public class Coil3ImageRendererExtensionImplTest {
129125

130126
private fun setContent(extension: Coil3ImageRendererExtensionImpl, image: InlineMarkdown.Image) {
131127
composeTestRule.setContent {
132-
val inlineContent = mapOf("inlineTextContent" to extension.renderImageContent(image))
128+
val inlineContent = buildMap { extension.renderImageContent(image)?.let { put("inlineTextContent", it) } }
133129
val annotatedString = buildAnnotatedString {
134130
append("Rendered inline text image: ")
135131
appendInlineContent("inlineTextContent", "[rendered image]")

0 commit comments

Comments
 (0)