Skip to content

Image handling improvements #72

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

Jump to bottom
Merged
mikepenz merged 3 commits into from
Sep 1, 2023
Merged

Image handling improvements #72

mikepenz merged 3 commits into from
Sep 1, 2023

Conversation

@psuzn
Copy link
Contributor

@psuzn psuzn commented Aug 31, 2023

  • CompositionLocal based image loader
  • Resizing the image container(Placeholder) to better fit the different image sizes.
Before After
Screenshot_20230831-215836 Screenshot_20230831-215648

We can use a different image loader to load the images. Here is a snippet using Kamel to load the images

object KamelImageLoader : ImageTransformer {
    @Composable
    override fun transform(link: String): Painter? {
        return when (val resource = asyncPainterResource(link)) {
            is Resource.Failure -> null
            is Resource.Loading -> null
            is Resource.Success -> resource.value
        }
    }
}
val scrollState = rememberScrollState()

 Markdown(
      markdown,
      imageTransformer = KamelImageLoader,
      modifier = Modifier
          .fillMaxSize()
          .verticalScroll(scrollState)
  )

@psuzn
Copy link
Contributor Author

psuzn commented Aug 31, 2023

On a side note, while testing this I've created a sample project with multiplatform support(Android, JVM and IOS). We can also merge that here if it adds any value.

https://github.com/psuzn/multiplatform-markdown-renderer/compare/sp/image-handler...psuzn:multiplatform-markdown-renderer:sp/sample-multiplatform-project?expand=1

mikepenz
mikepenz reviewed Aug 31, 2023
View reviewed changes
Copy link
Owner

@mikepenz mikepenz left a comment

Choose a reason for hiding this comment

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

The PR looks great! Haven't had time to go into full depth yet, but overall this looks really promising.

Thank you so much for opening the PR!

Added a few comments and questions

...atform-markdown-renderer/src/commonMain/kotlin/com/mikepenz/markdown/compose/ComposeLocal.kt Outdated
* Local [ImageTransformer] provider
*/
val LocalImageTransformer = staticCompositionLocalOf<ImageTransformer> {
error("No local ImageLinkTransformer")
Copy link
Owner

@mikepenz mikepenz Aug 31, 2023

Choose a reason for hiding this comment

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

No local ImageTransformer

Copy link
Contributor Author

@psuzn psuzn Sep 1, 2023

Choose a reason for hiding this comment

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

We can provide the ImageTransformerImpl here and remove the imageTransformer parameter from Markdown() composable but doing that if the user wants to use a different imagetransformer they have to use provide the imageTransformer using composition local provider in their code. There is noting wrong with it but I think providing imageTransformer as a parameter is more intuitive.

Copy link
Owner

@mikepenz mikepenz Sep 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Oh I believe you missunderstood me. I only meant the error message. It states: ImageLinkTransformer instead of ImageTransformer

(the comment was basically repeating the error message but in fixed form :D)

Copy link
Contributor Author

@psuzn psuzn Sep 1, 2023

Choose a reason for hiding this comment

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

sorry my bad, I had named it ImageLinkTransformer initially.

...-markdown-renderer/src/commonMain/kotlin/com/mikepenz/markdown/model/ImageTransformerImpl.kt Outdated
import com.mikepenz.markdown.utils.imagePainter
import com.mikepenz.markdown.utils.painterIntrinsicSize

internal object ImageTransformerImpl : ImageTransformer {
Copy link
Owner

@mikepenz mikepenz Aug 31, 2023

Choose a reason for hiding this comment

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

I believe it would probably better to make this a normal class instead of an object. As we won't need it to exist if somebody provides their own implementation.

...n/com/mikepenz/markdown/utils/ImagePainterProvider.multiplatform-markdown-renderer.native.kt
@Composable
internal actual fun imagePainter(url: String): Painter? {
return null
return null as Painter? // just `return null` crashes on native
Copy link
Owner

@mikepenz mikepenz Aug 31, 2023

Choose a reason for hiding this comment

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

Thank you!

...rm-markdown-renderer/src/commonMain/kotlin/com/mikepenz/markdown/model/MarkdownImageState.kt
Comment on lines 31 to 35
if (parentSize.isUnspecified) {
TextUnitSize(180.sp, 180.sp)
} else if (intrinsicImageSize.isUnspecified) {
TextUnitSize(parentSize.width.toSp(), 180.sp)
} else {
Copy link
Owner

@mikepenz mikepenz Aug 31, 2023

Choose a reason for hiding this comment

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

Using TextSize for an image? Would it maybe make more sense to have this exposed as normal IntSize instead? 🤔

Copy link
Contributor Author

@psuzn psuzn Sep 1, 2023

Choose a reason for hiding this comment

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

I made this sp size class because the Placeholder needs sp dimensions and doing conversion here is better. I might be able to remove it. Let's see.

@psuzn psuzn requested a review from mikepenz September 1, 2023 10:10
@mikepenz mikepenz changed the title image handling improvements Image handling improvements Sep 1, 2023
@mikepenz
Copy link
Owner

mikepenz commented Sep 1, 2023

Once again thank you so much for this PR!

@mikepenz mikepenz merged commit 2c4e8fc into mikepenz:develop Sep 1, 2023
@mikepenz mikepenz mentioned this pull request Nov 4, 2023
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mikepenz mikepenz Awaiting requested review from mikepenz

Assignees

No one assigned

Labels

feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@psuzn @mikepenz