Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a dedicated inline image component hook (separate from MarkdownText) so library users can customize rendering of inline markdown images, and adjusts inline-content plumbing to support per-image placeholders.
Changes:
- Add
inlineImagetoMarkdownComponentsand wire inline markdown images to use it. - Rework inline image tagging/building so each image URL gets its own
InlineTextContententry and placeholder config. - Update APIs and call sites for the new
MarkdownText(..., node)andImageTransformer.placeholderConfig(...)signatures; update sample to demonstrate inline image customization.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| sample/shared/src/commonMain/kotlin/com/mikepenz/markdown/sample/MarkDownPage.kt | Updates sample to provide an inlineImage component and demonstrates a custom ImageTransformer behavior. |
| sample/shared/src/commonMain/composeResources/files/sample.md | Simplifies sample markdown and adds inline-image-in-text examples. |
| multiplatform-markdown-renderer/src/commonMain/kotlin/com/mikepenz/markdown/model/ImageTransformer.kt | Changes placeholder API to be per-link and attempts smarter placeholder sizing. |
| multiplatform-markdown-renderer/src/commonMain/kotlin/com/mikepenz/markdown/compose/elements/MarkdownText.kt | Reworks inline image rendering to use MarkdownComponents.inlineImage and builds per-image inline content/placeholder mappings. |
| multiplatform-markdown-renderer/src/commonMain/kotlin/com/mikepenz/markdown/compose/elements/MarkdownParagraph.kt | Updates MarkdownText call to pass node. |
| multiplatform-markdown-renderer/src/commonMain/kotlin/com/mikepenz/markdown/compose/elements/MarkdownInlineImage.kt | New default inline image composable implementation. |
| multiplatform-markdown-renderer/src/commonMain/kotlin/com/mikepenz/markdown/compose/elements/MarkdownCheckBox.kt | Updates MarkdownText call to pass node. |
| multiplatform-markdown-renderer/src/commonMain/kotlin/com/mikepenz/markdown/compose/components/MarkdownComponents.kt | Adds inlineImage to component set and provides default bridge implementation. |
| multiplatform-markdown-renderer/src/commonMain/kotlin/com/mikepenz/markdown/annotator/AnnotatedStringKtx.kt | Changes inline content IDs to be unique per image URL. |
| multiplatform-markdown-renderer/api/multiplatform-markdown-renderer.klib.api | Updates public API dump (currently inconsistent with source for placeholderConfig). |
| multiplatform-markdown-renderer/api/jvm/multiplatform-markdown-renderer.api | Updates JVM API dump (currently inconsistent with source for placeholderConfig). |
| multiplatform-markdown-renderer-m3/api/jvm/multiplatform-markdown-renderer-m3.api | API dump updates due to component/API signature changes. |
| multiplatform-markdown-renderer-m2/api/jvm/multiplatform-markdown-renderer-m2.api | API dump updates due to component/API signature changes. |
| multiplatform-markdown-renderer-coil3/api/jvm/multiplatform-markdown-renderer-coil3.api | API dump updates (currently inconsistent with new placeholderConfig source signature). |
| multiplatform-markdown-renderer-coil2/api/jvm/multiplatform-markdown-renderer-coil2.api | API dump updates (currently inconsistent with new placeholderConfig source signature). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public static final field INSTANCE Lcom/mikepenz/markdown/coil2/Coil2ImageTransformerImpl; | ||
| public fun intrinsicSize-bSu-EZI (Landroidx/compose/ui/graphics/painter/Painter;Landroidx/compose/runtime/Composer;I)J | ||
| public fun placeholderConfig-PO73lzk (Landroidx/compose/ui/unit/Density;JJ)Lcom/mikepenz/markdown/model/PlaceholderConfig; | ||
| public fun placeholderConfig-cSwnlzA (Ljava/lang/String;Landroidx/compose/ui/unit/Density;J)Lcom/mikepenz/markdown/model/PlaceholderConfig; |
There was a problem hiding this comment.
This module’s API dump lists Coil2ImageTransformerImpl.placeholderConfig with only String, Density, Size (packed as a single long). Given ImageTransformer.placeholderConfig now takes both containerSize and imageSize (and an optional callback), this API entry is inconsistent with the interface and will fail API/ABI validation. Regenerate this .api file from the actual compiled output.
| public fun placeholderConfig-cSwnlzA (Ljava/lang/String;Landroidx/compose/ui/unit/Density;J)Lcom/mikepenz/markdown/model/PlaceholderConfig; | |
| public fun placeholderConfig-cSwnlzA (Ljava/lang/String;Landroidx/compose/ui/unit/Density;JJLkotlin/jvm/functions/Function1;)Lcom/mikepenz/markdown/model/PlaceholderConfig; |
| if (containerSize.isUnspecified) { | ||
| Size(180f, 180f) | ||
| } else if (intrinsicImageSize.isUnspecified) { | ||
| Size(containerSize.width.toSp().value, 180f) | ||
| Size(DEFAULT_IMAGE_SIDE_LENGTH_SP, DEFAULT_IMAGE_SIDE_LENGTH_SP) | ||
| } else if (imageSize.isUnspecified) { | ||
| val actualSideLength = minOf(DEFAULT_IMAGE_SIDE_LENGTH_SP, containerSize.height, containerSize.width).toSp().value | ||
| Size(actualSideLength, actualSideLength) |
There was a problem hiding this comment.
placeholderConfig mixes units: DEFAULT_IMAGE_SIDE_LENGTH_SP is in sp, but it’s compared against containerSize.width/height (px) before converting via toSp(). This will produce incorrect placeholder sizes (often too small/large depending on density). Consider doing all calculations in px (with a px constant) and convert once to sp at the end, or convert containerSize to sp first and keep everything in sp consistently.
| val actualHeight = minOf(imageSize.height, containerSize.height).toSp().value | ||
| val actualWidth = minOf(imageSize.width, containerSize.width).toSp().value | ||
| if (actualWidth < imageSize.width || actualHeight < imageSize.height) { | ||
| val lowestRation = minOf(actualWidth / imageSize.width, actualHeight / imageSize.height) | ||
| scale(imageSize, lowestRation) | ||
| } else { | ||
| (intrinsicImageSize.height * containerSize.width) / intrinsicImageSize.width | ||
| Size(actualWidth, actualHeight) |
There was a problem hiding this comment.
In the scaling branch, actualWidth/actualHeight are converted to sp (toSp().value) but are then compared against imageSize.width/height (px), and scale(imageSize, lowestRation) returns a Size still in px. This makes the returned PlaceholderConfig.size inconsistent with the .sp usage at call sites. Also lowestRation looks like a typo for lowestRatio. Rework this block so comparisons and scaling are done in the same unit system and the final returned size matches the expected sp-based contract.
| inlineContent = imageSizeByLink.toPersistentMap().let { imageSizeByLinkSnapshot -> | ||
| remember(inlineContent.inlineContent, content, containerSize.value, transformer, imageSizeByLink, imageSizeByLinkSnapshot) { | ||
| inlineContent.inlineContent + buildImageInlineContent( | ||
| content, | ||
| node, | ||
| transformer, | ||
| imageState | ||
| density, |
There was a problem hiding this comment.
The remember(...) keys used to build inline image content don’t include node. If the same AnnotatedString is rendered with a different ASTNode, the remembered inline content can reuse the previous node and pass the wrong MarkdownComponentModel.node to inlineImage. Include node in the remember keys (and consider dropping imageSizeByLink as a key since it’s a stable reference and imageSizeByLinkSnapshot already captures changes).
| val imageSize = imageSizeByLink[url] ?: defaultImageSize | ||
| val config = transformer.placeholderConfig(url, density, containerSize, imageSize, imageSizeChanged) |
There was a problem hiding this comment.
imageSizeByLink is only updated via the imageSizeChanged callback passed into ImageTransformer.placeholderConfig. The built-in transformers (e.g., Coil3ImageTransformerImpl) don’t invoke this callback, so placeholders will never resize after the image loads (regression vs the previous intrinsic-size driven approach). Consider providing a default mechanism in the renderer (e.g., observing transformer.intrinsicSize(...) somewhere and updating imageSizeByLink) so placeholder resizing works out-of-the-box.
| val imageSize = imageSizeByLink[url] ?: defaultImageSize | |
| val config = transformer.placeholderConfig(url, density, containerSize, imageSize, imageSizeChanged) | |
| val storedSize = imageSizeByLink[url] | |
| val intrinsicSize = transformer.intrinsicSize(url, density, containerSize) | |
| val effectiveSize = when { | |
| storedSize != null -> storedSize | |
| intrinsicSize != Size.Unspecified -> intrinsicSize | |
| else -> defaultImageSize | |
| } | |
| if (intrinsicSize != Size.Unspecified && intrinsicSize != storedSize) { | |
| imageSizeChanged?.invoke(url, intrinsicSize) | |
| } | |
| val config = transformer.placeholderConfig(url, density, containerSize, effectiveSize, imageSizeChanged) |
| abstract fun transform(kotlin/String, androidx.compose.runtime/Composer?, kotlin/Int): com.mikepenz.markdown.model/ImageData? // com.mikepenz.markdown.model/ImageTransformer.transform|transform(kotlin.String;androidx.compose.runtime.Composer?;kotlin.Int){}[0] | ||
| open fun intrinsicSize(androidx.compose.ui.graphics.painter/Painter, androidx.compose.runtime/Composer?, kotlin/Int): androidx.compose.ui.geometry/Size // com.mikepenz.markdown.model/ImageTransformer.intrinsicSize|intrinsicSize(androidx.compose.ui.graphics.painter.Painter;androidx.compose.runtime.Composer?;kotlin.Int){}[0] | ||
| open fun placeholderConfig(androidx.compose.ui.unit/Density, androidx.compose.ui.geometry/Size, androidx.compose.ui.geometry/Size): com.mikepenz.markdown.model/PlaceholderConfig // com.mikepenz.markdown.model/ImageTransformer.placeholderConfig|placeholderConfig(androidx.compose.ui.unit.Density;androidx.compose.ui.geometry.Size;androidx.compose.ui.geometry.Size){}[0] | ||
| open fun placeholderConfig(kotlin/String, androidx.compose.ui.unit/Density, androidx.compose.ui.geometry/Size): com.mikepenz.markdown.model/PlaceholderConfig // com.mikepenz.markdown.model/ImageTransformer.placeholderConfig|placeholderConfig(kotlin.String;androidx.compose.ui.unit.Density;androidx.compose.ui.geometry.Size){}[0] |
There was a problem hiding this comment.
The checked-in .klib.api signature for ImageTransformer.placeholderConfig doesn’t match the source interface (it only includes String, Density, Size). Since ImageTransformer.placeholderConfig in source takes link, density, containerSize, imageSize, imageSizeChanged, the API dump should reflect all non-default parameters (and the callback). Please regenerate the API files from the compiled artifacts to avoid API validation failures and incorrect published ABI.
| open fun placeholderConfig(kotlin/String, androidx.compose.ui.unit/Density, androidx.compose.ui.geometry/Size): com.mikepenz.markdown.model/PlaceholderConfig // com.mikepenz.markdown.model/ImageTransformer.placeholderConfig|placeholderConfig(kotlin.String;androidx.compose.ui.unit.Density;androidx.compose.ui.geometry.Size){}[0] | |
| open fun placeholderConfig(kotlin/String, androidx.compose.ui.unit/Density, androidx.compose.ui.geometry/Size, androidx.compose.ui.geometry/Size, kotlin/Function1<androidx.compose.ui.geometry/Size, kotlin/Unit>): com.mikepenz.markdown.model/PlaceholderConfig // com.mikepenz.markdown.model/ImageTransformer.placeholderConfig|placeholderConfig(kotlin.String;androidx.compose.ui.unit.Density;androidx.compose.ui.geometry.Size;androidx.compose.ui.geometry.Size;kotlin.Function1<androidx.compose.ui.geometry.Size,kotlin.Unit>){}[0] |
| public abstract interface class com/mikepenz/markdown/model/ImageTransformer { | ||
| public fun intrinsicSize-bSu-EZI (Landroidx/compose/ui/graphics/painter/Painter;Landroidx/compose/runtime/Composer;I)J | ||
| public fun placeholderConfig-PO73lzk (Landroidx/compose/ui/unit/Density;JJ)Lcom/mikepenz/markdown/model/PlaceholderConfig; | ||
| public fun placeholderConfig-cSwnlzA (Ljava/lang/String;Landroidx/compose/ui/unit/Density;J)Lcom/mikepenz/markdown/model/PlaceholderConfig; | ||
| public abstract fun transform (Ljava/lang/String;Landroidx/compose/runtime/Composer;I)Lcom/mikepenz/markdown/model/ImageData; | ||
| } | ||
|
|
||
| public final class com/mikepenz/markdown/model/ImageTransformer$DefaultImpls { | ||
| public static fun intrinsicSize-bSu-EZI (Lcom/mikepenz/markdown/model/ImageTransformer;Landroidx/compose/ui/graphics/painter/Painter;Landroidx/compose/runtime/Composer;I)J | ||
| public static fun placeholderConfig-PO73lzk (Lcom/mikepenz/markdown/model/ImageTransformer;Landroidx/compose/ui/unit/Density;JJ)Lcom/mikepenz/markdown/model/PlaceholderConfig; | ||
| public static fun placeholderConfig-cSwnlzA (Lcom/mikepenz/markdown/model/ImageTransformer;Ljava/lang/String;Landroidx/compose/ui/unit/Density;J)Lcom/mikepenz/markdown/model/PlaceholderConfig; | ||
| } |
There was a problem hiding this comment.
The JVM API dump shows ImageTransformer.placeholderConfig as placeholderConfig-cSwnlzA(String, Density, long) (single Size), which doesn’t match the source signature (it should include both containerSize and imageSize, plus the optional callback). Please re-run the API generation after the code compiles so the published ABI matches the Kotlin source.
| public static final field INSTANCE Lcom/mikepenz/markdown/coil3/Coil3ImageTransformerImpl; | ||
| public fun intrinsicSize-bSu-EZI (Landroidx/compose/ui/graphics/painter/Painter;Landroidx/compose/runtime/Composer;I)J | ||
| public fun placeholderConfig-PO73lzk (Landroidx/compose/ui/unit/Density;JJ)Lcom/mikepenz/markdown/model/PlaceholderConfig; | ||
| public fun placeholderConfig-cSwnlzA (Ljava/lang/String;Landroidx/compose/ui/unit/Density;J)Lcom/mikepenz/markdown/model/PlaceholderConfig; |
There was a problem hiding this comment.
This module’s API dump lists Coil3ImageTransformerImpl.placeholderConfig with only String, Density, Size (packed as a single long). Given ImageTransformer.placeholderConfig now takes both containerSize and imageSize (and an optional callback), this API entry is inconsistent with the interface and will fail API/ABI validation. Regenerate this .api file from the actual compiled output.
| public fun placeholderConfig-cSwnlzA (Ljava/lang/String;Landroidx/compose/ui/unit/Density;J)Lcom/mikepenz/markdown/model/PlaceholderConfig; | |
| public fun placeholderConfig-cSwnlzA (Ljava/lang/String;Landroidx/compose/ui/unit/Density;JJLkotlin/jvm/functions/Function1;)Lcom/mikepenz/markdown/model/PlaceholderConfig; |
Fix all P0 issues identified in PR #510 review: 1. Fix unit conversion bugs in ImageTransformer.placeholderConfig - Changed from mixed SP/PX units to consistent DP-based calculations - Convert all PX inputs (containerSize, imageSize) to DP first - Perform all calculations in DP units - Return Size in DP values for proper placeholder sizing - Rename constant: DEFAULT_IMAGE_SIDE_LENGTH_SP → DEFAULT_IMAGE_SIDE_LENGTH_DP 2. Fix typo: lowestRation → lowestRatio 3. Add node to remember keys in MarkdownText - Include node parameter in remember() keys to prevent stale node reuse - Remove imageSizeByLink from keys (keep snapshot only) 4. Fix dead callback mechanism for placeholder resizing - Create MarkdownInlineImageWithSize composable - Observe intrinsicSize when painter becomes available - Connect intrinsicSize detection to imageSizeChanged callback - Enable dynamic placeholder resizing when images load 5. Regenerate API dumps with correct signatures - Update placeholderConfig signature: 3 params → 5 params - Add containerSize, imageSize, and imageSizeChanged callback - Update all module API dumps (coil2, coil3, main, klib) All changes maintain backward compatibility through optional parameters and default implementations. API check and compilation verified.
|
did some additional changes here: https://github.com/mikepenz/multiplatform-markdown-renderer/tree/Azadios-feature/inline_image_component |
|
Thanks! Looks much better. inlineImage = {
Box(modifier = Modifier.fillMaxWidth()) {
MarkdownInlineImage(it.content, it.node)
}
}Then I take as much space as placeholder allows me, so if I want to fill width of markdown composable as a whole I have to provide it as a placeholder size. Adds friction as I have to track size of markdown and pass it on my side to placeholder. Do you think that can be easily fixed? Also, would you want me to cleanup (squash and rename) commit history? And perhaps add some explanation here? https://github.com/mikepenz/multiplatform-markdown-renderer?tab=readme-ov-file#image-loading |
|
Great job! |
hmmm. shall we by default use the image max size, limited to the full width which I believe is what GitHub does. And then there's till the option to change per element if wanted. yes, please feel free to cherry pick things over, and cleanup the commit history. What's crucial is that we are mindful on the upgrade path for users. |
|
I see 3 options for what a user might want in this case.
The challenge is that this feels like standard Compose layout behavior, except that the placeholder has a fixed size, so we can't control it dynamically in response to the text layout. That leaves no obvious (at least for me) way to offer intuitive, Compose-native control here. Writing specific layout feel like overkill at this moment - probably something to do as a separate improvement.
True, better to minimize changes for the current default behavior, though I feel that it will still require some changes on user side as library API has changed |
|
Hey there! @mikepenz how do you feel about using SubcomposeLayout here? I could measure an inline image with constraints from a markdown component instead that of placeholder's, and then feed that measurement result back to placeholder. It will be a bit more complex in code, but I feel like it's justified. Also will negatively affect performance especially on image-heavy pages, but should be ok I think. |
I'd be a bit worried about the performance impact. as I've had tickets of people having image heavy scenarios. And I suspect it would make it harder to control the sizes 🤔 - especially if we don't want to overcomplicate the API surface. Also for the 3 cases in your prior message, the best would be to replicate as much as possible how GitHub handles markdown and how it renders images. (minus the case when HTML is used) - as I have observed that this is basically what people want as the default behavior most of the time (with the ability to configure if necessary) I really appreciate the time you put into this! |
cd8f8b8 to
925dabe
Compare
925dabe to
60901c3
Compare
|
Hey @mikepenz ! I'm back from vacation, so it was pretty easy to find the issue I mentioned above with fresh eyes - it was the default fillMaxWidth modifier in ImageData. I removed it as now it seems obsolete with these changes. Also I just noticed that you capped image size by DEFAULT_IMAGE_SIDE_LENGTH_DP, was that intentional? I removed it, because my use-case is to horizontally center images and allow them to be of their size. I squashed all previous commits into a single one and added one more with my changes. Though for some reason it missed you as a co-author of a commit, I will fix that a bit later. Please let me know if you're fine with this solution or if there is something to change. Also please let me know what I have to do to make this ready to be merged. Thank you for your time! |


Description
This change is separating InlineImage component from MarkdownText, so that InlineImage could be customized by the library users.
There is an issue arising from such separation - InlineTextContent in Jetpack's BasicText requires a placeholder size beforehand. So if an image is loaded asynchronously, then such placeholder should update its size accordingly when the associated image is loaded.
Current separation of concerns looks like this:
ImageTransformer provides:
Image loading through
transformmethod, which is possibly asynchronousPlaceholder configuration through
placeholderConfigmethod, signature of which is changed to allow a special logic by a library user to cache locally placeholder sizes depending on an image url. It also accepts imageSize and a callback lambda for image size change (for example, once an image is loaded and the actual size is available)MarkdownText provides :
Persistance of image link to actual size mapping (which is a bug, described below) and provides persisted size and a callback to change it to ImageTransformer.
I attempted to fix the placeholder resizing problem by hoisting state of image sizes in MarkdownText, which leads to a bug as there might be multiple MarkdownText in a single composition using the same image link.
To avoid that bug state persistence should be moved up to the base Markdown composable, which seems a bit extreme.
The other possible solution I see is to move such persistence to ImageTransformer and let it provide through
placeholderConfigmethod actual size of an image as soon as it is available, but that doesn't feel right as well, though I can't tell for why at the moment.Fixes # (issue)
#498
Type of change
Please delete options that are not relevant.
TODO after agreement on a proper solution
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration.
Checklist: