Skip to content

issues #81: Display link previews beneath a message containing a link #384

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

Open
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

laonger
Copy link

@laonger laonger commented Feb 10, 2025

Fixes #81
Remaining Issue:

1.	Due to the complexity of the page, many web pages cannot display the preview.
2.	The preview card requires scrolling the page to be displayed.

…taining a link

Remaining Issue:

	1.	Due to the complexity of the page, many web pages cannot display the preview.
	2.	The preview card requires scrolling the page to be displayed.
@laonger laonger marked this pull request as draft February 11, 2025 16:31
@ZhangHanDong ZhangHanDong added the waiting-on-author This issue is waiting on the original author for a response label Feb 21, 2025
@laonger laonger marked this pull request as ready for review February 21, 2025 07:02
@laonger laonger marked this pull request as draft February 21, 2025 07:03
@laonger laonger marked this pull request as ready for review February 26, 2025 05:56
@ZhangHanDong ZhangHanDong added waiting-on-review This issue is waiting to be reviewed and removed waiting-on-author This issue is waiting on the original author for a response labels Feb 27, 2025
@tyreseluo
Copy link
Contributor

You need to pass the ci first.

@laonger
Copy link
Author

laonger commented Feb 28, 2025

You need to pass the ci first.

done

Copy link
Member

@kevinaboos kevinaboos left a comment

Choose a reason for hiding this comment

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

Thanks, this is a great start on this new feature!

Some questions:

  1. do we actually need a dedicated cache type for link previews? Seems like a general map would work just fine, since it's a simple case.
  2. The name "card cache" is confusing. What are "cards" in this context? Isn't it just a cache of link previews? Please change the term "card" accordingly throughout your whole PR.
  3. Undo all formatting changes. All those unnecessary changes make it really hard for me to properly review this PR.
  4. The widget structure of where you placed the LinkPreviewCard is ... unusual. See my comments about moving it into the Message widget's content subview within its body.

@kevinaboos kevinaboos added waiting-on-author This issue is waiting on the original author for a response and removed waiting-on-review This issue is waiting to be reviewed labels Mar 3, 2025
@tyreseluo
Copy link
Contributor

One other issue, when there is more than one link, is the display of this card only one or, all of them, and when displaying too many cards, does it take up too much vertical space in the room_screen?

@tyreseluo
Copy link
Contributor

tyreseluo commented Mar 11, 2025

  1. Other than that, I think our MatrixRequest could have been simpler.

we just need url to fetch the description, title and preview image.

MatrixRequest::FetchLinkPreview { url } => {
    let _fetch_task = Handle::current().spawn(async move {
        let preview_service = url_preview::PreviewService::with_no_cache();
        match preview_service.generate_preview(&url).await {
            Ok(preview) => {
                log!("preview data: {:?}", preview);
            },
            Err(e) => {
                log!("Failed to fetch link preview card for {url}: {e}");
            }
        };
    });
}

when we get the preview data, then we store to chace or use Cx::post_action(Some); pass the data to the widget

  1. We can use LinkPreview name to replace the Card name. also same to change the card_cache.

  2. It seems we don't need to change much of the code in room_screen, just handle it in html_or_plaintext.

you can see this PR #390

@laonger laonger requested a review from kevinaboos April 4, 2025 17:11
@laonger laonger requested a review from ZhangHanDong April 4, 2025 17:19
@ZhangHanDong ZhangHanDong added the blocked Blocked on another issue or missing feature label May 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Blocked on another issue or missing feature waiting-on-author This issue is waiting on the original author for a response
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Display link previews beneath a message containing a link
4 participants