zed icon indicating copy to clipboard operation
zed copied to clipboard

Add support for images in Markdown previews

Open dovakin0007 opened this issue 1 year ago • 5 comments

Closes #13246

Release Notes: added image preview for markdown

https://github.com/user-attachments/assets/54569a4f-9d6c-4f9d-a8f6-94458846004e

dovakin0007 avatar Aug 14 '24 01:08 dovakin0007

@dovakin0007 : Does it also work using the img tag?

<img src="https://upload.wikimedia.org/wikipedia/commons/thumb/6/61/HTML5_logo_and_wordmark.svg/200px-HTML5_logo_and_wordmark.svg.png" alt="Image description" width="300"/>

Angelk90 avatar Aug 14 '24 06:08 Angelk90

@dovakin0007 : Does it also work using the img tag?

<img src="https://upload.wikimedia.org/wikipedia/commons/thumb/6/61/HTML5_logo_and_wordmark.svg/200px-HTML5_logo_and_wordmark.svg.png" alt="Image description" width="300"/>

no sir

dovakin0007 avatar Aug 14 '24 06:08 dovakin0007

@dovakin0007 : What is displayed in case of the alt message?

Angelk90 avatar Aug 14 '24 06:08 Angelk90

@dovakin0007 : What is displayed in case of the alt message?

the alt text like zed gpui image doesn't have a way to display the alt text so for now I am displaying alt text directly image this text will be shown if image fails or unable to find the image in path image else we display image interactive image needs to be added though

dovakin0007 avatar Aug 14 '24 06:08 dovakin0007

I want to raise one more issue it seems like the image is not rendering in same line image like the above example any ideas and contributions will be helpful here

dovakin0007 avatar Aug 29 '24 16:08 dovakin0007

Seems like there's some test failures as well. I'm going to mark this as draft for now, once things are ready to go feel free to mark it as ready :)

As for your issues with the layout, perhaps you need to add some more styling rules on the img perhaps usingblock() (equivalent to display: block)?

mikayla-maki avatar Aug 29 '24 17:08 mikayla-maki

I haven't looked at the PR in the detail, but I assume that we will run into the same issues I encountered in #10565. In particular there are these two issues:

  1. Gpui cannot handle images in the middle of a paragraph, I think there are two ways we can address this:
  • Split up paragraph's that contain images and render each text and image as their own block (This is what I did in #10565)
  • Add support for this in gpui (seems like a lot of work though)
  1. The img element needs better error handling, right now if a link is malformed or does not return an image, we fetch it over and over again. This wastes unnecessary resources and network bandwidth.

Rendering images in separate lines feels like a good enough workaround for now, but 2. definitely feels like a blocker to me.

bennetbo avatar Aug 30 '24 10:08 bennetbo

Seems like there's some test failures as well. I'm going to mark this as draft for now, once things are ready to go feel free to mark it as ready :)

As for your issues with the layout, perhaps you need to add some more styling rules on the img perhaps usingblock() (equivalent to display: block)?

yeah that should work. regarding the layout we have to render the image within text StyledText can't handle images for now

dovakin0007 avatar Aug 30 '24 10:08 dovakin0007

I haven't looked at the PR in the detail, but I assume that we will run into the same issues I encountered in #10565. In particular there are these two issues:

  1. Gpui cannot handle images in the middle of a paragraph, I think there are two ways we can address this:
  • Split up paragraph's that contain images and render each text and image as their own block (This is what I did in markdown preview: Image support #10565)
  • Add support for this in gpui (seems like a lot of work though)
  1. The img element needs better error handling, right now if a link is malformed or does not return an image, we fetch it over and over again. This wastes unnecessary resources and network bandwidth.

Rendering images in separate lines feels like a good enough workaround for now, but 2. definitely feels like a blocker to me.

yeah I am running into the same issues that you have mentioned.

dovakin0007 avatar Aug 30 '24 10:08 dovakin0007

I haven't looked at the PR in the detail, but I assume that we will run into the same issues I encountered in #10565. In particular there are these two issues:

  1. Gpui cannot handle images in the middle of a paragraph, I think there are two ways we can address this:
  • Split up paragraph's that contain images and render each text and image as their own block (This is what I did in markdown preview: Image support #10565)
  • Add support for this in gpui (seems like a lot of work though)
  1. The img element needs better error handling, right now if a link is malformed or does not return an image, we fetch it over and over again. This wastes unnecessary resources and network bandwidth.

Rendering images in separate lines feels like a good enough workaround for now, but 2. definitely feels like a blocker to me. Maybe we can split the text into multiple blocks which will allow us to render image between text that can fix 1?

dovakin0007 avatar Aug 31 '24 04:08 dovakin0007

still img() doesn't return error for bad request

dovakin0007 avatar Sep 07 '24 11:09 dovakin0007

I was able to render the image within

https://github.com/user-attachments/assets/dd5dcbf1-68a0-4a90-b635-4dcb3018a02e

text

dovakin0007 avatar Sep 08 '24 18:09 dovakin0007

Support for inline elements in the text system is probably the better choice if the text and inline elements needs to wrap properly. Splitting the text and images into their own blocks is just avoiding the problem that will eventually be needed to be implemented. It may be fine for now.

I've tried to implement it myself, but there are some changes and some decisions which I'm not sure what is best.

I have had a issue up to track this https://github.com/zed-industries/zed/issues/10916

MatinAniss avatar Sep 13 '24 02:09 MatinAniss

Support for inline elements in the text system is probably the better choice if the text and inline elements needs to wrap properly. Splitting the text and images into their own blocks is just avoiding the problem that will eventually be needed to be implemented. It may be fine for now.

I've tried to implement it myself, but there are some changes and some decisions which I'm not sure what is best.

I have had a issue up to track this https://github.com/zed-industries/zed/issues/10916

Yeah I did try to do it but I wasn't on the right track so I scrapped it then for now implemented to render image as its separate own block rather than a part of interactive text yeah you are right once interactive text supports inline images this has to be changed

dovakin0007 avatar Sep 13 '24 06:09 dovakin0007

@mikayla-maki I have made the changes please do review when you have time thanks

dovakin0007 avatar Sep 17 '24 15:09 dovakin0007

@dovakin0007 : Does the following code display correctly?

| A | B |
|---|---|
| ![A](https://upload.wikimedia.org/wikipedia/commons/thumb/6/61/HTML5_logo_and_wordmark.svg/200px-HTML5_logo_and_wordmark.svg.png) | ![B](https://upload.wikimedia.org/wikipedia/commons/thumb/9/99/Unofficial_JavaScript_logo_2.svg/440px-Unofficial_JavaScript_logo_2.svg.png) |
A B
A B

With the img tag it still doesn't work, right?

Angelk90 avatar Sep 19 '24 08:09 Angelk90

image

the image will be misaligned due to https://github.com/zed-industries/zed/issues/11024

dovakin0007 avatar Sep 19 '24 09:09 dovakin0007

@dovakin0007 : This problem also had @bennetbo in #10565.

Angelk90 avatar Sep 19 '24 09:09 Angelk90

@dovakin0007 : This problem also had @bennetbo in #10565.

Yeah I am running into same issues as @bennetbo mentioned

dovakin0007 avatar Sep 19 '24 09:09 dovakin0007

@dovakin0007 : Look #18315.

Angelk90 avatar Sep 25 '24 17:09 Angelk90

A few nitpicks, then I'd be happy to merge this!

Will fix those thanks !

dovakin0007 avatar Oct 01 '24 06:10 dovakin0007

@dovakin0007 : The problem with the tables has probably been solved in #18315. By updating with the latest commit of zed you can see if the problem with the images in the tables still persists or has been solved.

Angelk90 avatar Oct 01 '24 11:10 Angelk90

@dovakin0007 : The problem with the tables has probably been solved in #18315. By updating with the latest commit of zed you can see if the problem with the images in the tables still persists or has been solved.

okay so it seems like there is a merge conflict due to
`for (index, cell) in parsed.header.children.iter().enumerate() { let length = cell.contents.len(); max_lengths[index] = length; }

for row in &parsed.body {
    for (index, cell) in row.children.iter().enumerate() {
        let length = cell.contents.len();
        if length > max_lengths[index] {
            max_lengths[index] = length;
        }
    }
}`

dovakin0007 avatar Oct 01 '24 13:10 dovakin0007

@dovakin0007 : The problem with the tables has probably been solved in #18315. By updating with the latest commit of zed you can see if the problem with the images in the tables still persists or has been solved.

image A part of the issue has been fixed

dovakin0007 avatar Oct 01 '24 13:10 dovakin0007

@dovakin0007 : What are the remaining problems?

Angelk90 avatar Oct 01 '24 16:10 Angelk90

@dovakin0007 : What are the remaining problems?

Adjust table length based on image size issue is no way to get image length for now

dovakin0007 avatar Oct 01 '24 16:10 dovakin0007

@dovakin0007 : Are you also referring to the problem that the html logo image is not aligned correctly to the center like vscode and github does?

Vscode: Screenshot 2024-10-01 alle 19 08 56

Angelk90 avatar Oct 01 '24 17:10 Angelk90

@dovakin0007 : Are you also referring to the problem that the html logo image is not aligned correctly to the center like vscode and github does?

Vscode:

Screenshot 2024-10-01 alle 19 08 56

Yes and if I add it and the table width does change based on the image width

dovakin0007 avatar Oct 01 '24 17:10 dovakin0007

@dovakin0007 : Picking from something similar using the image dependency.

https://github.com/zed-industries/zed/blob/9b148f3dcc5e4281bfadd515efd817bcdbf21bc3/crates/gpui/src/assets.rs#L74-L78

Angelk90 avatar Oct 01 '24 18:10 Angelk90

@dovakin0007 : Picking from something similar using the image dependency.

https://github.com/zed-industries/zed/blob/9b148f3dcc5e4281bfadd515efd817bcdbf21bc3/crates/gpui/src/assets.rs#L74-L78 yeah to get the frame index some changes need to be done in GPUI for now I am gonna leave it as it is https://github.com/zed-industries/zed/blob/9cd42427d88afad3423fa2546ed656839295cf3f/crates/gpui/src/elements/img.rs#L167-L188

dovakin0007 avatar Oct 03 '24 09:10 dovakin0007