When the user clicks a thumbnail image, show the full-size image in an image viewer widget #341

aaravlu · 2025-01-22T04:33:01Z

issue 327: When the user clicks a thumbnail image, show the full-size image in an image viewer widget

alanpoon · 2025-01-23T02:53:33Z

There is no close button for Image previewer. Only the button to close the entire program.
https://github.com/user-attachments/assets/d0a2db15-1ee4-42a7-8756-46d5fd031408

aaravlu · 2025-01-23T03:47:05Z

There is no close button for Image previewer. Only the button to close the entire program. https://github.com/user-attachments/assets/d0a2db15-1ee4-42a7-8756-46d5fd031408

~~Yeah, many bugs currently, all the widgetsabout image viewer, I set absolutly position, so maybe you cannot see the button.~~

Now it's better, but still some bugs :(
Screencast from 2025-01-23 11-49-27.webm

aaravlu · 2025-01-24T08:22:19Z

We cannot set height or width Fit for <ImageViewerModal>, because it doesn't know how long or wide the image is, it is waiting for the image's actual length or width, but the image has not been fetched yet.

robrix/src/image_viewer_modal.rs

Line 15 in cd9c6a4

width: 1600, height: 900

A widget wrapped by <Modal> cannot handle_actions it self when the <Modal> is closed. I do this in app.rs so it can be avoided.

robrix/src/app.rs

Line 210 in cd9c6a4

match action.downcast_ref() {

After a large size image being fetched and filled to image viewer, we cannot see the image, I guess it cousts too large amout of time fetching the image to "forget" to redraw, so that it cannot be seen.
Fetching image to image viewer can block ui threads, I have no idea for this currently.

We can keep this pr open and I shall put my energies elsewhere issues, IMHO

kevinaboos · 2025-01-24T22:31:49Z

We cannot set height or width Fit for <ImageViewerModal>, because it doesn't know how long or wide the image is, it is waiting for the image's actual length or width, but the image has not been fetched yet.

Right, but in general, the Matrix SDK can provide image metadata including dimensions, so you can use that as a placeholder until you download the entire image. Even if you don't know the size of the image, you can just show the thumbnail image in the ImageViewer widget until the full-size image has been fully-fetched, and then you will know the size of the full image.

Regardless, it should not matter what the image's size is. The ImageViewer should be full-screen (width: Fill, height: Fill), and the Image fit mode can be set to fit: Biggest (or Smallest, based on your desired choice).

A widget wrapped by <Modal> cannot handle_actions it self when the <Modal> is closed. I do this in app.rs so it can be avoided.

You're correct, the Modal widget doesn't. We can modify it, but in general, the Modal widget has lots of issues right now. Don't use the Modal widget; you can copy my LoadingPane widget instead.

After a large size image being fetched and filled to image viewer, we cannot see the image, I guess it cousts too large amout of time fetching the image to "forget" to redraw, so that it cannot be seen.

I don't understand what this means, sorry.

Fetching image to image viewer can block ui threads, I have no idea for this currently.

Yes, of course, you'll have to do it on a background thread/task. We already have the infrastructure for this.

In summary, it sounds like you're a bit stuck on this, which is totally fine. Thanks for submitting your work thus far; in the future, I (or others) can help you complete the ImageViewer widget.

aaravlu · 2025-02-24T04:35:38Z

I removed the spin loader in this pr while would take it back if #389 is OK.

lastest fix:
Screencast from 2025-02-24 10-53-01.webm

kevinaboos · 2025-02-24T19:09:14Z

All the media's decoding can be put into sliding_syc to leave it to tokio to auto handle to avoid blocking ui-thread

Yes, we have planned to do this for all images for a while now, just haven't gotten around to it. The general idea of offloading image decoding to a background thread has been floating around our discussions for a few months at least, and yes, we should absolutely do it -- but not just for the ImageViewer, for all images. Let's keep that as part of a separate PR though, in the future.

kevinaboos

The design of the ImageViewer widget itself is way too complicated. We don't need a fancy BTreeMap or a MediaCache within it --- the only thing it needs to include is a simple <Image> or <TextOrImage> instance. This is because the actual image data (for the image that will be shown) will come from another part of the code, so we don't need to store anything within the ImageViewer instance itself.

kevinaboos · 2025-02-24T19:21:23Z

src/home/room_screen.rs

-        match media_cache.try_get_media_or_fetch(mxc_uri.clone(), Some(MEDIA_THUMBNAIL_FORMAT.into())) {
-            MediaCacheEntry::Loaded(data) => {
+
+    let mut fetch_and_show_image_uri = |cx: &mut Cx2d, original_source: MediaSource, mxc_uri: OwnedMxcUri, image_info: Option<&ImageInfo>| {


now that we have multiple different image "sources", we need to name them more clearly. What is original_source? Is that the source info for the full-sized image?

And then what is mxc_uri for? We need to name them better so we know what each one means.

Same question for image_info -- which image does that apply to?

kevinaboos · 2025-02-24T19:22:03Z

src/home/room_screen.rs

+
+                if let MediaSource::Plain(original_uri) = original_source {
+                    let original_thumbnail = OriginalAndThumbnail::new(original_uri, thumbnail_data);
+                    Cx::post_action(ImageViewerAction::SetData {text_or_image_uid, original_thumbnail});


why do you need to pass in the widget UID of the TextOrImage instance? that doesn't make any sense --- there's only one ImageViewer, so this widget UID will never be of any use.

kevinaboos · 2025-02-24T19:22:13Z

src/home/room_screen.rs

@@ -3452,7 +3459,10 @@ fn populate_image_message_content(
        }
    };

-    let mut fetch_and_show_media_source = |cx: &mut Cx2d, media_source: MediaSource, image_info: Option<&ImageInfo>| {
+
+    let mut fetch_and_show_media_source = |cx: &mut Cx2d, original_source: MediaSource, thumbnail_source: Option<MediaSource>, image_info: Option<&ImageInfo>| {


same comment here about naming the arguments.

kevinaboos · 2025-02-24T19:23:37Z

src/home/room_screen.rs

-                .and_then(|image_info| image_info.thumbnail_source)
-                .unwrap_or(original_source);
-            fetch_and_show_media_source(cx, media_source, image_info.as_ref());
+            let thumbnail_source = image_info.clone()


do we need to repeatedly (and eagerly) clone all of the image info? That seems unnecessary. Couldn't you just pass in a reference to all of these image_info instances?

kevinaboos · 2025-02-24T19:29:46Z

src/image_viewer.rs

+    /// Key is uid of `TextOrImage`, val is the corresponded image uri and its thumbnail data.
+    #[rust]
+    map: BTreeMap<WidgetUid, OriginalAndThumbnail>,
+    /// We use a standalone `MediaCache` to store the original image data.
+    #[rust]
+    media_cache: MediaCache,


this is a strange approach for designing this widget. It should literally just contain a single image instance (maybe a TextOrImage instead, if we want to show a loading message, which would be a good idea), and nothing else. This widget should be very simple, no need to overcomplicate it.

We don't need this weird map or a media_cache here at all. Including those do not make any sense, because that information is already available from the room's timeline message or user avatar that actually provided the image source.

src/image_viewer.rs

src/shared/styles.rs

src/shared/text_or_image.rs

src/utils.rs

aaravlu · 2025-02-26T04:23:48Z

need to refactor MediaCache, blocked by #405

aaravlu · 2025-03-13T05:36:29Z

src/home/room_screen.rs

-    let mut fetch_and_show_image_uri = |cx: &mut Cx2d, mxc_uri: OwnedMxcUri, image_info: Option<&ImageInfo>| {
-        match media_cache.try_get_media_or_fetch(mxc_uri.clone(), MEDIA_THUMBNAIL_FORMAT.into()) {
+    //
+    // I refactor those two closure here, feel free to renamed it.


@kevinaboos , Just feel free to rename this closure here.

aaravlu added 8 commits January 18, 2025 11:23

Add imageviewer base

fc81c57

Add image viewer base

da49f8f

Finished image viewer

bf3106f

Wrong path to get room screen

2345679

Finish image viewer base

ade49c8

Remove '.log' File

e141b6b

Add doc

a3fcb9d

Merge branch 'main' into fix271-2-2

c0957ee

aaravlu changed the title ~~Fix327~~ When the user clicks a thumbnail image, show the full-size image in an image viewer widget Jan 22, 2025

aaravlu requested a review from kevinaboos January 22, 2025 04:38

alanpoon assigned aaravlu Jan 23, 2025

alanpoon added the waiting-on-author This issue is waiting on the original author for a response label Jan 23, 2025

aaravlu added 4 commits January 23, 2025 11:25

Fix align for image viewer

e8dfb49

Remove '.log' file

9cd54b5

Move related logic from 'app.rs' to 'image_viewer_modal.rs'

e074794

Remove '.log' file

2e293a4

aaravlu added 6 commits January 23, 2025 14:52

Resolve conflicts in 'room_screen.rs'

4a091f9

Merge branch 'main' into fix327

15eb0a3

Merge branch 'main' into fix327

a519e68

Resolve conflicts

094f76f

Collate related logic about fetching image

84ae954

Block by makepad because of <Modal>

cd9c6a4

aaravlu added blocked-on-makepad Blocked on a Makepad bug or missing Makepad feature and removed waiting-on-author This issue is waiting on the original author for a response labels Jan 24, 2025

kevinaboos added help wanted Looking for help from anyone! and removed blocked-on-makepad Blocked on a Makepad bug or missing Makepad feature labels Jan 24, 2025

Fix media format

1a0d1dc

aaravlu 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 Feb 24, 2025

aaravlu added 3 commits February 24, 2025 16:11

Revert because one mxc_uri to multi image

7d91225

Remove .log file

8207a50

Remove redundant 'MatrixRequest'

d95d4b8

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 Feb 24, 2025

kevinaboos requested changes Feb 24, 2025

View reviewed changes

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 Feb 24, 2025

aaravlu mentioned this pull request Feb 26, 2025

Refactor MediaCache #405

Closed

aaravlu added blocked Blocked on another issue or missing feature and removed waiting-on-author This issue is waiting on the original author for a response labels Feb 26, 2025

aaravlu added waiting-on-author This issue is waiting on the original author for a response and removed blocked Blocked on another issue or missing feature labels Mar 13, 2025

aaravlu added 2 commits March 13, 2025 10:23

Merge branch 'main' into fix327

0908831

Resolve conflicts and finish the functionality

9714b24

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 Mar 13, 2025

aaravlu added 2 commits March 13, 2025 13:33

Feel free to rename the closure

cc41e6f

TextOrImage fit revert to 'Smallest'

0051f17

aaravlu commented Mar 13, 2025

View reviewed changes

aaravlu requested a review from kevinaboos March 13, 2025 05:38

Remove Default trait of 'MediaFormatConst'

91b9a9f

aaravlu 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 13, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

When the user clicks a thumbnail image, show the full-size image in an image viewer widget #341

When the user clicks a thumbnail image, show the full-size image in an image viewer widget #341

aaravlu commented Jan 22, 2025 •

edited

Loading

alanpoon commented Jan 23, 2025

aaravlu commented Jan 23, 2025 •

edited

Loading

aaravlu commented Jan 24, 2025 •

edited

Loading

We cannot set `height` or `width` `Fit` for `<ImageViewerModal>`, because it doesn't know how long or wide the image is, it is waiting for the image's actual length or width, but the image has not been fetched yet.

A widget wrapped by `<Modal>` cannot `handle_actions` it self when the `<Modal>` is closed. I do this in `app.rs` so it can be avoided.

After a large size image being fetched and filled to image viewer, we cannot see the image, I guess it cousts too large amout of time fetching the image to "forget" to redraw, so that it cannot be seen.

Fetching image to image viewer can block ui threads, I have no idea for this currently.

kevinaboos commented Jan 24, 2025

We cannot set `height` or `width` `Fit` for `<ImageViewerModal>`, because it doesn't know how long or wide the image is, it is waiting for the image's actual length or width, but the image has not been fetched yet.

A widget wrapped by `<Modal>` cannot `handle_actions` it self when the `<Modal>` is closed. I do this in `app.rs` so it can be avoided.

After a large size image being fetched and filled to image viewer, we cannot see the image, I guess it cousts too large amout of time fetching the image to "forget" to redraw, so that it cannot be seen.

Fetching image to image viewer can block ui threads, I have no idea for this currently.

aaravlu commented Feb 24, 2025 •

edited

Loading

kevinaboos commented Feb 24, 2025

All the media's decoding can be put into `sliding_syc` to leave it to tokio to auto handle to avoid blocking ui-thread

kevinaboos left a comment

kevinaboos Feb 24, 2025

kevinaboos Feb 24, 2025

kevinaboos Feb 24, 2025

kevinaboos Feb 24, 2025

kevinaboos Feb 24, 2025

aaravlu commented Feb 26, 2025

aaravlu Mar 13, 2025 •

edited

Loading

When the user clicks a thumbnail image, show the full-size image in an image viewer widget #341

Are you sure you want to change the base?

When the user clicks a thumbnail image, show the full-size image in an image viewer widget #341

Conversation

aaravlu commented Jan 22, 2025 • edited Loading

alanpoon commented Jan 23, 2025

aaravlu commented Jan 23, 2025 • edited Loading

aaravlu commented Jan 24, 2025 • edited Loading

We cannot set height or width Fit for <ImageViewerModal>, because it doesn't know how long or wide the image is, it is waiting for the image's actual length or width, but the image has not been fetched yet.

robrix/src/image_viewer_modal.rs Line 15 in cd9c6a4 width: 1600, height: 900

A widget wrapped by <Modal> cannot handle_actions it self when the <Modal> is closed. I do this in app.rs so it can be avoided.

robrix/src/app.rs Line 210 in cd9c6a4 match action.downcast_ref() {

After a large size image being fetched and filled to image viewer, we cannot see the image, I guess it cousts too large amout of time fetching the image to "forget" to redraw, so that it cannot be seen.

Fetching image to image viewer can block ui threads, I have no idea for this currently.

kevinaboos commented Jan 24, 2025

We cannot set height or width Fit for <ImageViewerModal>, because it doesn't know how long or wide the image is, it is waiting for the image's actual length or width, but the image has not been fetched yet.

A widget wrapped by <Modal> cannot handle_actions it self when the <Modal> is closed. I do this in app.rs so it can be avoided.

After a large size image being fetched and filled to image viewer, we cannot see the image, I guess it cousts too large amout of time fetching the image to "forget" to redraw, so that it cannot be seen.

Fetching image to image viewer can block ui threads, I have no idea for this currently.

aaravlu commented Feb 24, 2025 • edited Loading

kevinaboos commented Feb 24, 2025

All the media's decoding can be put into sliding_syc to leave it to tokio to auto handle to avoid blocking ui-thread

kevinaboos left a comment

Choose a reason for hiding this comment

kevinaboos Feb 24, 2025

Choose a reason for hiding this comment

kevinaboos Feb 24, 2025

Choose a reason for hiding this comment

kevinaboos Feb 24, 2025

Choose a reason for hiding this comment

kevinaboos Feb 24, 2025

Choose a reason for hiding this comment

kevinaboos Feb 24, 2025

Choose a reason for hiding this comment

aaravlu commented Feb 26, 2025

aaravlu Mar 13, 2025 • edited Loading

Choose a reason for hiding this comment

aaravlu commented Jan 22, 2025 •

edited

Loading

aaravlu commented Jan 23, 2025 •

edited

Loading

aaravlu commented Jan 24, 2025 •

edited

Loading

We cannot set `height` or `width` `Fit` for `<ImageViewerModal>`, because it doesn't know how long or wide the image is, it is waiting for the image's actual length or width, but the image has not been fetched yet.

robrix/src/image_viewer_modal.rs

Line 15 in cd9c6a4

width: 1600, height: 900

A widget wrapped by `<Modal>` cannot `handle_actions` it self when the `<Modal>` is closed. I do this in `app.rs` so it can be avoided.

robrix/src/app.rs

Line 210 in cd9c6a4

match action.downcast_ref() {

We cannot set `height` or `width` `Fit` for `<ImageViewerModal>`, because it doesn't know how long or wide the image is, it is waiting for the image's actual length or width, but the image has not been fetched yet.

A widget wrapped by `<Modal>` cannot `handle_actions` it self when the `<Modal>` is closed. I do this in `app.rs` so it can be avoided.

aaravlu commented Feb 24, 2025 •

edited

Loading

All the media's decoding can be put into `sliding_syc` to leave it to tokio to auto handle to avoid blocking ui-thread

aaravlu Mar 13, 2025 •

edited

Loading