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

Add support for images in Markdown previews #16192

Closed
wants to merge 33 commits into from

Conversation

dovakin0007
Copy link
Contributor

@dovakin0007 dovakin0007 commented Aug 14, 2024

Closes #13246

Release Notes:
added image preview for markdown

image

Release Notes:

  • Added support for images to the markdown preview

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Aug 14, 2024
@dovakin0007 dovakin0007 changed the title add markdown image preview Markdown: Image preview Aug 14, 2024
@Angelk90
Copy link

@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"/>

@dovakin0007
Copy link
Contributor Author

@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

@Angelk90
Copy link

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

@dovakin0007
Copy link
Contributor Author

dovakin0007 commented Aug 14, 2024

@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

@maxdeviant maxdeviant changed the title Markdown: Image preview markdown: Add image preview Aug 20, 2024
@maxdeviant maxdeviant changed the title markdown: Add image preview Add support for images in Markdown previews Aug 20, 2024
crates/markdown_preview/src/markdown_parser.rs Outdated Show resolved Hide resolved
crates/markdown_preview/src/markdown_parser.rs Outdated Show resolved Hide resolved
@dovakin0007
Copy link
Contributor Author

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

@mikayla-maki
Copy link
Contributor

mikayla-maki commented Aug 29, 2024

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 mikayla-maki marked this pull request as draft August 29, 2024 17:37
@bennetbo
Copy link
Collaborator

bennetbo commented Aug 30, 2024

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.

@dovakin0007
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

dovakin0007 commented Aug 31, 2024

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 dovakin0007 marked this pull request as ready for review September 7, 2024 11:46
@dovakin0007
Copy link
Contributor Author

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

@dovakin0007
Copy link
Contributor Author

@dovakin0007 : What are the remaining problems?

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

@Angelk90
Copy link

Angelk90 commented Oct 1, 2024

@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

@dovakin0007
Copy link
Contributor Author

@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

@Angelk90
Copy link

Angelk90 commented Oct 1, 2024

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

/// Get the size of this image, in pixels.
pub fn size(&self, frame_index: usize) -> Size<DevicePixels> {
let (width, height) = self.data[frame_index].buffer().dimensions();
size(width.into(), height.into())
}

@dovakin0007
Copy link
Contributor Author

dovakin0007 commented Oct 3, 2024

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

/// Get the size of this image, in pixels.
pub fn size(&self, frame_index: usize) -> Size<DevicePixels> {
let (width, height) = self.data[frame_index].buffer().dimensions();
size(width.into(), height.into())
}

yeah to get the frame index some changes need to be done in GPUI for now I am gonna leave it as it is
if let Some(data) = self.source.use_data(cx) {
if let Some(state) = &mut state {
let frame_count = data.frame_count();
if frame_count > 1 {
let current_time = Instant::now();
if let Some(last_frame_time) = state.last_frame_time {
let elapsed = current_time - last_frame_time;
let frame_duration =
Duration::from(data.delay(state.frame_index));
if elapsed >= frame_duration {
state.frame_index = (state.frame_index + 1) % frame_count;
state.last_frame_time =
Some(current_time - (elapsed - frame_duration));
}
} else {
state.last_frame_time = Some(current_time);
}
}
}
let image_size = data.size(frame_index);

@dovakin0007
Copy link
Contributor Author

@mikayla-maki I have done the changes let me know if anything else need to be changed

@dovakin0007
Copy link
Contributor Author

Hi @mikayla-maki the conflict has been resolved let me know if anything else needed to be changed

@mjdNjhNJ
Copy link

@dovakin0007 Thank you for the work! I hope this gets merged soon.

@dovakin0007 dovakin0007 marked this pull request as draft November 16, 2024 04:10
@dovakin0007
Copy link
Contributor Author

will re open once I add fallback text for images

@dovakin0007 dovakin0007 reopened this Nov 21, 2024
@dovakin0007 dovakin0007 marked this pull request as ready for review November 21, 2024 15:24
@dovakin0007
Copy link
Contributor Author

dovakin0007 commented Nov 21, 2024

Hi zed team please do review the code and let me know what are the changes need to be done and feel free to change it

Copy link
Contributor

@mikayla-maki mikayla-maki left a comment

Choose a reason for hiding this comment

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

Thanks for your patience on this! I think this PR looks ready to go, I pushed some code style nits to a different PR here: #21082, so I'm going to close this one for now. But it's all the same code and commits, thank you for implementing this!

mikayla-maki added a commit that referenced this pull request Nov 22, 2024
Closes #13246

Supersedes: #16192

I couldn't push to the git fork this user was using, so here's the exact
same PR but with some style nits implemented.


Release Notes:

- Added image rendering to the Markdown preview

---------

Co-authored-by: dovakin0007 <[email protected]>
Co-authored-by: dovakin0007 <[email protected]>
@dovakin0007
Copy link
Contributor Author

Thanks for your patience on this! I think this PR looks ready to go, I pushed some code style nits to a different PR here: #21082, so I'm going to close this one for now. But it's all the same code and commits, thank you for implementing this!

Thank you! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support images in Markdown preview
6 participants