Skip to content

[JEWEL-941] Reducing space used by images on markdown image error #3285

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

[JEWEL-941] Reducing space used by images on markdown image error #3285

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 25 additions & 7 deletions platform/jewel/markdown/core/src/main/kotlin/org/jetbrains/jewel/markdown/Markdown.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,10 @@ package org.jetbrains.jewel.markdown
import androidx.compose.foundation.layout.Arrangement
import androidx.compose.foundation.layout.Column
import androidx.compose.foundation.layout.PaddingValues
import androidx.compose.foundation.layout.padding
import androidx.compose.foundation.lazy.LazyColumn
import androidx.compose.foundation.lazy.LazyListState
import androidx.compose.foundation.lazy.items
import androidx.compose.foundation.lazy.itemsIndexed
import androidx.compose.foundation.lazy.rememberLazyListState
import androidx.compose.foundation.text.selection.SelectionContainer
import androidx.compose.runtime.Composable
Expand Down Expand Up @@ -302,12 +303,29 @@ public fun LazyMarkdown(
blockRenderer: MarkdownBlockRenderer = JewelTheme.markdownBlockRenderer,
) {
MaybeSelectable(selectable, modifier) {
LazyColumn(
state = state,
contentPadding = contentPadding,
verticalArrangement = Arrangement.spacedBy(markdownStyling.blockVerticalSpacing),
) {
items(blocks) { block -> blockRenderer.RenderBlock(block, enabled, onUrlClick, Modifier) }
LazyColumn(state = state, contentPadding = contentPadding) {
itemsIndexed(blocks) { index, block ->
blockRenderer.RenderBlock(
block = block,
enabled = enabled,
onUrlClick = onUrlClick,
modifier =
Modifier.padding(
top =
if (index == 0) {
0.dp
} else {
(markdownStyling.blockVerticalSpacing / 2)
},
bottom =
if (index == blocks.lastIndex) {
0.dp
} else {
(markdownStyling.blockVerticalSpacing / 2)
},
),
)
}
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion ...wn/core/src/main/kotlin/org/jetbrains/jewel/markdown/extensions/ImageRendererExtension.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,5 +23,5 @@ public interface ImageRendererExtension {
* @param image The image data, containing information like the image URL and alt text.
* @return An [InlineTextContent] that will be embedded in the text flow, which will be used to display the image.
*/
@Composable public fun renderImageContent(image: InlineMarkdown.Image): InlineTextContent
@Composable public fun renderImageContent(image: InlineMarkdown.Image): InlineTextContent?
}
124 changes: 87 additions & 37 deletions ...re/src/main/kotlin/org/jetbrains/jewel/markdown/rendering/DefaultMarkdownBlockRenderer.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,15 @@ package org.jetbrains.jewel.markdown.rendering
import androidx.compose.foundation.background
import androidx.compose.foundation.border
import androidx.compose.foundation.layout.Arrangement
import androidx.compose.foundation.layout.Box
import androidx.compose.foundation.layout.Column
import androidx.compose.foundation.layout.FlowRow
import androidx.compose.foundation.layout.Row
import androidx.compose.foundation.layout.Spacer
import androidx.compose.foundation.layout.fillMaxWidth
import androidx.compose.foundation.layout.height
import androidx.compose.foundation.layout.padding
import androidx.compose.foundation.layout.size
import androidx.compose.foundation.layout.width
import androidx.compose.foundation.layout.widthIn
import androidx.compose.foundation.text.InlineTextContent
Expand All @@ -18,6 +21,7 @@ import androidx.compose.runtime.CompositionLocalProvider
import androidx.compose.runtime.collectAsState
import androidx.compose.runtime.getValue
import androidx.compose.runtime.movableContentOf
import androidx.compose.runtime.mutableStateMapOf
import androidx.compose.runtime.remember
import androidx.compose.ui.Alignment
import androidx.compose.ui.Modifier
Expand All @@ -29,6 +33,7 @@ import androidx.compose.ui.graphics.isSpecified
import androidx.compose.ui.graphics.takeOrElse
import androidx.compose.ui.input.pointer.PointerIcon
import androidx.compose.ui.input.pointer.pointerHoverIcon
import androidx.compose.ui.platform.LocalDensity
import androidx.compose.ui.text.AnnotatedString
import androidx.compose.ui.text.TextLayoutResult
import androidx.compose.ui.text.TextStyle
Expand Down Expand Up @@ -75,7 +80,7 @@ private const val DISABLED_CODE_ALPHA = .5f
*
* @see MarkdownBlockRenderer
*/
@Suppress("OVERRIDE_DEPRECATION")
@Suppress("OVERRIDE_DEPRECATION", "LargeClass")
@ApiStatus.Experimental
@ExperimentalJewelApi
public open class DefaultMarkdownBlockRenderer(
Expand Down Expand Up @@ -193,23 +198,30 @@ public open class DefaultMarkdownBlockRenderer(
softWrap: Boolean,
maxLines: Int,
) {
val renderedContent = rememberRenderedContent(block, styling.inlinesStyling, enabled, onUrlClick)
val textColor =
styling.inlinesStyling.textStyle.color
.takeOrElse { LocalContentColor.current }
.takeOrElse { styling.inlinesStyling.textStyle.color }
val mergedStyle = styling.inlinesStyling.textStyle.merge(TextStyle(color = textColor))
val onlyImages = remember(block) { block.inlineContent.all { it is InlineMarkdown.Image } }
val images = renderedImages(block)

Text(
modifier = modifier,
text = renderedContent,
overflow = overflow,
softWrap = softWrap,
maxLines = maxLines,
onTextLayout = onTextLayout,
inlineContent = renderedImages(block),
style = mergedStyle,
)
if (onlyImages) {
RenderImages(images, modifier)
} else {
val renderedContent = rememberRenderedContent(block, styling.inlinesStyling, enabled, onUrlClick)
val textColor =
styling.inlinesStyling.textStyle.color
.takeOrElse { LocalContentColor.current }
.takeOrElse { styling.inlinesStyling.textStyle.color }
val mergedStyle = styling.inlinesStyling.textStyle.merge(TextStyle(color = textColor))

Text(
modifier = modifier,
text = renderedContent,
overflow = overflow,
softWrap = softWrap,
maxLines = maxLines,
onTextLayout = onTextLayout,
inlineContent = images,
style = mergedStyle,
)
}
Copy link

@cursor cursor bot Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Spacing lost when paragraph has only failed images

When a paragraph contains only images that all fail to load, RenderImages returns early without applying the modifier, causing spacing to be lost in LazyMarkdown. The modifier contains padding for block spacing, but when images.isEmpty() triggers the early return in RenderImages, this padding is never applied. This causes the spacing between blocks to collapse when a paragraph with only failed images is present in a LazyMarkdown context.

Fix in Cursor Fix in Web

}

@Composable
Expand Down Expand Up @@ -263,19 +275,29 @@ public open class DefaultMarkdownBlockRenderer(
onUrlClick: (String) -> Unit,
modifier: Modifier,
) {
val renderedContent = rememberRenderedContent(block, styling.inlinesStyling, enabled, onUrlClick)
val onlyImages = remember(block) { block.inlineContent.all { it is InlineMarkdown.Image } }
val images = renderedImages(block)

if (onlyImages && images.isEmpty()) return
Copy link

@cursor cursor bot Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Spacing lost when heading has only failed images

When a heading contains only images that all fail to load, the function returns early without applying the modifier, causing spacing to be lost in LazyMarkdown. The modifier contains padding for block spacing, but the early return at line 281 prevents the Column from being created, so the modifier is never applied. This causes incorrect spacing between blocks in a LazyMarkdown context.

Fix in Cursor Fix in Web


Column(modifier = modifier.padding(styling.padding)) {
val textColor =
styling.inlinesStyling.textStyle.color.takeOrElse {
LocalContentColor.current.takeOrElse { styling.inlinesStyling.textStyle.color }
}
val mergedStyle = styling.inlinesStyling.textStyle.merge(TextStyle(color = textColor))
Text(
text = renderedContent,
style = mergedStyle,
modifier = Modifier.focusProperties { this.canFocus = false },
inlineContent = [email protected](block),
)
if (onlyImages) {
RenderImages(images)
} else {
val renderedContent = rememberRenderedContent(block, styling.inlinesStyling, enabled, onUrlClick)

val textColor =
styling.inlinesStyling.textStyle.color.takeOrElse {
LocalContentColor.current.takeOrElse { styling.inlinesStyling.textStyle.color }
}
val mergedStyle = styling.inlinesStyling.textStyle.merge(TextStyle(color = textColor))
Text(
text = renderedContent,
style = mergedStyle,
modifier = Modifier.focusProperties { this.canFocus = false },
inlineContent = images,
)
}

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

@Composable
private fun renderedImages(blockInlineContent: WithInlineMarkdown): Map<String, InlineTextContent> =
rendererExtensions
.firstNotNullOfOrNull { it.imageRendererExtension }
?.let { imagesRenderer ->
getImages(blockInlineContent).associate { image ->
image.source to imagesRenderer.renderImageContent(image)
}
private fun renderedImages(blockInlineContent: WithInlineMarkdown): Map<String, InlineTextContent> {
val map = remember(blockInlineContent) { mutableStateMapOf<String, InlineTextContent>() }

val imagesRenderer = rendererExtensions.firstNotNullOfOrNull { it.imageRendererExtension }

for (image in getImages(blockInlineContent)) {
val renderedImage = imagesRenderer?.renderImageContent(image)
if (renderedImage == null) {
map.remove(image.source)
} else {
map[image.source] = renderedImage
}
.orEmpty()
}

return map
}
Copy link

@cursor cursor bot Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Duplicate images not rendered in image-only blocks

When a paragraph or heading contains only images and multiple images share the same source URL, only one image is rendered instead of all of them. The renderedImages function uses a map keyed by image.source, so duplicate sources overwrite each other. When RenderImages iterates through this map, it only renders one image per unique source. For example, ![alt1](a.png) ![alt2](a.png) would render only one image instead of two. This differs from the text path where duplicate images are rendered correctly.

Fix in Cursor Fix in Web


@Composable
protected fun MaybeScrollingContainer(
Expand All @@ -710,6 +739,25 @@ public open class DefaultMarkdownBlockRenderer(
}
}

@Composable
private fun RenderImages(images: Map<String, InlineTextContent>, modifier: Modifier = Modifier) {
if (images.isEmpty()) return

val density = LocalDensity.current
FlowRow(modifier) {
images.map { (text, inlineBlock) ->
Box(
modifier =
with(density) {
Modifier.size(inlineBlock.placeholder.width.toDp(), inlineBlock.placeholder.height.toDp())
}
) {
inlineBlock.children(text)
}
}
}
}

public override fun createCopy(
rootStyling: MarkdownStyling?,
rendererExtensions: List<MarkdownRendererExtension>?,
Expand All @@ -734,9 +782,11 @@ private fun getImages(input: WithInlineMarkdown): List<InlineMarkdown.Image> = b
is InlineMarkdown.Image -> {
if (item.source.isNotBlank()) add(item)
}

is WithInlineMarkdown -> {
collectImagesRecursively(item.inlineContent)
}

else -> {
// Ignored
}
Expand Down
10 changes: 9 additions & 1 deletion .../kotlin/org/jetbrains/jewel/markdown/extensions/images/Coil3ImageRendererExtensionImpl.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,11 @@ internal class Coil3ImageRendererExtensionImpl(private val imageLoader: ImageLoa
* @return An [InlineTextContent] that can be used by a `Text` or `BasicText` composable to render the image inline.
*/
@Composable
public override fun renderImageContent(image: InlineMarkdown.Image): InlineTextContent {
public override fun renderImageContent(image: InlineMarkdown.Image): InlineTextContent? {
val imageSourceResolver = LocalMarkdownImageSourceResolver.current
val resolvedImageSource = remember(image.source) { imageSourceResolver.resolve(image.source) }
var imageResult by remember(resolvedImageSource) { mutableStateOf<SuccessResult?>(null) }
var hasError by remember { mutableStateOf(false) }

val painter =
rememberAsyncImagePainter(
Expand All @@ -59,18 +60,25 @@ internal class Coil3ImageRendererExtensionImpl(private val imageLoader: ImageLoa
.size(Size.ORIGINAL)
.build(),
imageLoader = imageLoader,
onLoading = { hasError = false },
onSuccess = { successState ->
hasError = false
// onSuccess should only be called once, but adding additional protection from
// unnecessary rerender
if (imageResult == null) {
imageResult = successState.result
}
},
onError = { error ->
hasError = true
JewelLogger.getInstance(this.javaClass).warn("AsyncImage loading failed.", error.result.throwable)
},
)

if (hasError) {
return null
}

val placeholder =
imageResult?.let {
val imageSize = it.image
Expand Down
14 changes: 5 additions & 9 deletions ...lin/org/jetbrains/jewel/markdown/extensions/images/Coil3ImageRendererExtensionImplTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,14 @@ import org.junit.Test
public class Coil3ImageRendererExtensionImplTest {
@get:Rule public val composeTestRule: ComposeContentTestRule = createComposeRule()

private val platformContext: PlatformContext = PlatformContext.Companion.INSTANCE
private val platformContext: PlatformContext = PlatformContext.INSTANCE
private val imageUrl = "https://example.com/image.png"

@Test
public fun `image renders with correct size on success`() {
val fakeImageWidth = 150
val fakeImageHeight = 100
val fakeImage = ColorImage(Color.Companion.Red.toArgb(), width = fakeImageWidth, height = fakeImageHeight)
val fakeImage = ColorImage(Color.Red.toArgb(), width = fakeImageWidth, height = fakeImageHeight)

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

Expand Down Expand Up @@ -76,11 +76,7 @@ public class Coil3ImageRendererExtensionImplTest {

setContent(extension, imageMarkdown)

composeTestRule
.onNodeWithContentDescription("Failed to load image")
.assertExists()
.assertWidthIsEqualTo(0.dp)
.assertHeightIsEqualTo(1.dp)
composeTestRule.onNodeWithContentDescription("Failed to load image").assertDoesNotExist()
}

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

val fakeImageWidth = 150
val fakeImageHeight = 150
val fakeImage = ColorImage(Color.Companion.Red.toArgb(), width = fakeImageWidth, height = fakeImageHeight)
val fakeImage = ColorImage(Color.Red.toArgb(), width = fakeImageWidth, height = fakeImageHeight)
val engine =
FakeImageLoaderEngine.Builder()
.intercept(
Expand Down Expand Up @@ -129,7 +125,7 @@ public class Coil3ImageRendererExtensionImplTest {

private fun setContent(extension: Coil3ImageRendererExtensionImpl, image: InlineMarkdown.Image) {
composeTestRule.setContent {
val inlineContent = mapOf("inlineTextContent" to extension.renderImageContent(image))
val inlineContent = buildMap { extension.renderImageContent(image)?.let { put("inlineTextContent", it) } }
val annotatedString = buildAnnotatedString {
append("Rendered inline text image: ")
appendInlineContent("inlineTextContent", "[rendered image]")
Expand Down
Loading