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

Fetch and display image thumbnails in the timeline instead of the full-resolution image #322

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

Conversation

aaravlu
Copy link
Contributor

@aaravlu aaravlu commented Jan 11, 2025

issue #271

To-do before merge:

  • Fetch thumbnail instead full origin media.

@aaravlu
Copy link
Contributor Author

aaravlu commented Jan 11, 2025

before:

Screenshot from 2025-01-11 13-05-11

after:

Screenshot from 2025-01-11 13-03-16

It's easy to see clarity difference of the image on my machine.
If you can alse see the difference, that's great!

@aaravlu aaravlu self-assigned this Jan 11, 2025
@aaravlu
Copy link
Contributor Author

aaravlu commented Jan 13, 2025

@kevinaboos
Thanks for noticing this comment.
I have finished the task1 while there is no need to review this time.
Now I am working on task2, which shoud make the image clickable as first step.
I change View to ClickableView here:

image_view = <ClickableView> {

There are a few of points confusing me:

  • There is no any widgets in robrix rely on ClickableView in shared/clickable_view.rs.
  • I have tested ClickableView for many times and seems it cannot work. And One widget called RoomPreview, which is a View, impl the clickable function itself instead relying on ClickableView:
    match event.hits(cx, self.view.area()) {

@aaravlu aaravlu requested a review from kevinaboos January 13, 2025 04:14
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.

@kevinaboos Thanks for noticing this comment. I have finished the task1 while there is no need to review this time. Now I am working on task2, which shoud make the image clickable as first step. I change View to ClickableView here:

Let's keep this PR simple and focused on just one thing -- the thumbnail support, which you've called "task1". We can handle task2 later in a separate PR, as it is easier to review multiple smaller PRs instead of one big PR.

So, please address my one comment, and clean up this PR such that it only includes changes related to thumbnails.

There are a few of points confusing me:

  • There is no any widgets in robrix rely on ClickableView in shared/clickable_view.rs.
  • I have tested ClickableView for many times and seems it cannot work. And One widget called RoomPreview, which is a View, impl the clickable function itself instead relying on ClickableView

Ah yes, ClickableView is no longer used and should be removed. Please don't use that for task2. We'll remove that code.

src/home/room_screen.rs Outdated Show resolved Hide resolved
@kevinaboos kevinaboos added the waiting-on-author This issue is waiting on the original author for a response label Jan 14, 2025
@aaravlu aaravlu marked this pull request as ready for review January 14, 2025 08:04
@aaravlu aaravlu 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 Jan 14, 2025
@aaravlu aaravlu requested a review from kevinaboos January 14, 2025 09:10
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, the new code to fetch the thumbnail or the full-size image as a fallback looks good now.

Please remove all ImageViewer stuff from this PR so I can merge in only the thumbnail-related changes. You can add the ImageViewer back in as part of a future PR.

@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 Jan 14, 2025
@aaravlu aaravlu 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 Jan 15, 2025
@alanpoon
Copy link
Contributor

alanpoon commented Jan 16, 2025

I am using commit 50f662a
Not thumbnail
It does not seem to be fetching thumbnails in the timeline.

Expected
Uploading Screenshot 2025-01-16 at 10.55.13 AM.png…

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-on-review This issue is waiting to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants