Skip to content
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.

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

[JEWEL-746] Load inline markdown images using Coil3 #2924

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

obask
Copy link
Collaborator

@obask obask commented Jan 21, 2025

It supports every image as an inline node

@rock3r rock3r self-requested a review January 21, 2025 17:18
@obask obask force-pushed the images branch 3 times, most recently from b0ba3c3 to 7fdb5ac Compare January 23, 2025 15:31
@obask
Copy link
Collaborator Author

obask commented Jan 24, 2025

Screenshot 2025-01-24 at 1 19 26 PM
Screenshot 2025-01-24 at 1 19 31 PM
currently there are 2 differences with a JCEF render: JCEF renders SVG as a giant image while we keep it small(I guess it just depends on a default size parameter), and the text in build|failing is not shown for some reason...

@rock3r
Copy link
Collaborator

rock3r commented Jan 24, 2025

They look like entirely different images... Are you sure it's the same image it's being displayed, and that it's loading correctly in the first case? That to me looks like a "broken image" placeholder (which also is way too big)

SVG files have an intrinsic size declared in their root node, and any renderer should adhere to that

@obask
Copy link
Collaborator Author

obask commented Jan 24, 2025

images I posted earlier are examples of how JCEF renders my "README.md" which this is how compose shows the same things:
Screenshot 2025-01-24 at 3 27 15 PM
README.md

@rock3r
Copy link
Collaborator

rock3r commented Jan 27, 2025

I see. I had misinterpreted the original message, sorry 🙏

The first image (the broken square) has a declared size of 800x800 px, encoded as width="800px" height="800px", which browsers seem to accept but maybe the Compose renderer doesn't like, and falls back to the viewBox size of 16x16. That would explain why it's larger on JCEF.

As for the latter, I think it's because the text for whatever reason just doesn't get rendered:

<g text-anchor="middle" font-family="DejaVu Sans,Verdana,Geneva,sans-serif" font-size="11">
    <use x="5.5" y="2" fill="#010101" fill-opacity=".3" href="#logo" />
    <use x="5.5" y="1" fill="#fff" href="#logo" />
  <text x="40.5" y="15" fill="#010101" fill-opacity=".3">build</text>
  <text x="40.5" y="14" fill="#fff">build</text>
  <text x="83.5" y="15" fill="#010101" fill-opacity=".3">passing</text>
  <text x="83.5" y="14" fill="#fff">passing</text>
</g>

Again, possibly some unsupported value in there that Skia ignores.

That's sort of weird since Skia can interpret image sizes with units, and render text — it does in Chrome! But maybe there is some setup that is missing, or Chrome does some magic stuff. Either way, these are either CMP or Skiko bugs :)

@obask obask changed the title [JEWEL-TBD] Load markdown images using Coil3 [JEWEL-327] Load markdown images using Coil3 Jan 31, 2025
@obask obask changed the title [JEWEL-327] Load markdown images using Coil3 [JEWEL-746] Load inline markdown images using Coil3 Jan 31, 2025
@obask
Copy link
Collaborator Author

obask commented Jan 31, 2025

@obask obask marked this pull request as ready for review January 31, 2025 13:26
@obask
Copy link
Collaborator Author

obask commented Jan 31, 2025

The first image (the broken square) has a declared size of 800x800 px, encoded as width="800px" height="800px", which browsers seem to accept but maybe the Compose renderer doesn't like, and falls back to the viewBox size of 16x16. That would explain why it's larger on JCEF.

As for the latter, I think it's because the text for whatever reason just doesn't get rendered:

Again, possibly some unsupported value in there that Skia ignores.

I'm going to create a separate issue for SVG regressions. Should it go to youtrack?

knownSize.value = state.result.image.let { ImageSize(it.width, it.height) }
}
},
// TODO: log onError as warning
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is there an easy way to log errors inside of compose rendering?

It supports every image as an inline node;
Using built-in coroutine library and ktor2 from the platform;
Added SVG support using a coil dependency.
@rock3r
Copy link
Collaborator

rock3r commented Jan 31, 2025

Should it go to youtrack?

Please, no. Stick with GH until further notice. You'll know together with everyone else when YT is usable. We're still waiting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants