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.

kevinaboos

Thanks for the redesign effort.

I need more info in order to determine whether this is the correct approach, because right now I feel that your latest MediaCache redesign is too complicated again.

Recently, you said that a thumbnail version of an image may have a different MxcUri than the URI of the full-size image. That makes sense, but i have some questions:

Are the URIs ever the same? If so, what are the cases where this happens?
How do we know if two URIs point to two different versions of the same image? Can we know this? If so, where in the code is this known?
Do we learn about the URI for a full-size image separately from the URI for a thumbnail, or are they both known together?

This is your new redesign:

BTreeMap<OwnedMxcUri, (Option<OwnedMxcUri>, MediaCacheEntryRef, MediaCacheEntryRef)>

a few questions about this design:

Is the key of the BTreeMap the URI for the full image, or for a thumbnail? or can it be either?
What is the optional URI in the first part of the tuple?
- Why is that part of the value itself? If you keep this in the value part of the map, you can't search for it, so that seems fairly pointless.
Why are there still two MediaCacheEntryRefs within the map value?

aaravlu · 2025-03-19T02:30:23Z

Are the URIs ever the same? If so, what are the cases where this happens?

Never same, always different

How do we know if two URIs point to two different versions of the same image? Can we know this? If so, where in the code is this known?

Note fn set_keys:

robrix/src/media_cache.rs

Lines 61 to 76 in 90e1910

    
           pub fn set_keys(&mut self, original_uri: &OwnedMxcUri, thumbnail_uri: Option<OwnedMxcUri>) { 
        
               if let Some(uri) = thumbnail_uri.clone() { 
        
                   if let Entry::Vacant(v) = self.cache.entry(uri.clone()) { 
        
                       v.insert((None, Arc::new(Mutex::new(MediaCacheEntry::Null)), Arc::new(Mutex::new(MediaCacheEntry::default())))); 
        
                   } 
        
                   if let Entry::Vacant(v) = self.cache.entry(original_uri.clone()) { 
        
                       v.insert((thumbnail_uri.clone(), Arc::new(Mutex::new(MediaCacheEntry::Null)), Arc::new(Mutex::new(MediaCacheEntry::default())))); 
        
                   } 
        
               } else { 
        
                   if let Entry::Vacant(v) = self.cache.entry(original_uri.clone()) { 
        
                       v.insert((None, Arc::new(Mutex::new(MediaCacheEntry::default())), Arc::new(Mutex::new(MediaCacheEntry::default())))); 
        
                   } 
        
               } 
        
           }

Do we learn about the URI for a full-size image separately from the URI for a thumbnail, or are they both known together?

I afraid i dont know the meaning of learn about here.

I have to say, we actually store 3 different clarity level: original & File, thumbnail & 400x400, original & 400x400,

original & 400x400 is only for the image which doesnot possess the a thumbnail uri.

Is the key of the BTreeMap the URI for the full image, or for a thumbnail? or can it be either?

Both, as the same as you said either.

Why is that part of the value itself? If you keep this in the value part of the map, you can't search for it, so that seems fairly pointless.

We do 1 or 2 queries:

First query, we will get the Option<MxcUri> in the tuple value, these are all the cases:

Some(MxcUri):

For timeline => We use this mxcuri as the key do the second query, then get or fetch thumbnail & 400x400, return the last value in the tuple of the second query.

For image viewer => get or fetch Original & File, we dont do the second query, directly return the last value in the tuple.(it is enough to confirm the input uri of fn try_get_media_or_fetch is original instead of thumbnail)

None:

2 case here:

the input uri of fn try_get_media_or_fetch is original because the matching image is unclear enough.
the input uri of fn try_get_media_or_fetch is thumbail.

we need to rely on prefer_thumbnail to divide this two condition, user must know if it prefers thumbnail.

if prefer_thumbnail, directly return the last value of the tuple, it is _ & 400x400

else, this condition must for image viewer, return the medium value of the tuple, maybe thumbnail uri, maybe original uri, So we return _ & File, it is in the medium value of the tuple. (if it has not been fetched, we return the last value of the tuple temporarily(it is _ & 400x400), when the _ & File was fetched, it could autoly replace it.)

aaravlu · 2025-03-19T07:26:07Z

What is the optional URI in the first part of the tuple?

The thumbnail uri, we know it is an Option type in matrix sdk, we just simple clone it to insert into btreemap as the value.

Also, if Some, we unwrap and clone it to insert it into btreemap as the key, and the matching uri value is None.

Why is that part of the value itself? If you keep this in the value part of the map, you can't search for it, so that seems fairly pointless.

Because there are possible secondary queries here.

Why are there still two MediaCacheEntryRefs within the map value?

The middle value exists to deal with a rare case, where the middle value is rarely used.

aaravlu · 2025-03-19T07:37:53Z

and the Image fit mode can be set to fit: Biggest (or Smallest, based on your desired choice).

We should not use Biggest for ever, a diagrame willed be provided later.

kevinaboos · 2025-03-25T01:20:13Z

I have to say, we actually store 3 different clarity level: original & File, thumbnail & 400x400, original & 400x400,

ok i think this is what I missed. What does "original" mean here? I presumed we were only going to store two possible sizes: the full-size file, and a 400x400 thumbnail.

kevinaboos · 2025-03-25T01:40:41Z

src/image_viewer.rs

+    // We need to distinct these two similar actions because a potential bug.
+    // if we combine these 2 actions into 1, will appear a rare bug-condition:
+    //
+    // User click some image and **quickly close** the image viewer,
+    // firstly, the `_ & File` version image has not been fetched, `try_get_image_or_fetch` would return the matching thumbnail to as temporariness,
+    // Once the `_ & File` version image is beening fetched, it would autoly open image viewer and show the image,
+    // but image viewer was already closed by user.
+    // That is, image viewer will show up for no reason in users' view.
+    // feel free to modify comments.
+    Show(Arc<[u8]>),
+    MakeItClearer(Arc<[u8]>),


ok, so the difference between these two actions is:

Show will always show the image viewer, even if it is not currently shown.

MakeItClearer will reload the image displayed within the ImageViewer, but will not show the ImageViewer if it is not already shown.

Makes sense. I would rename MakeItClearer to be less vague... perhaps something like "ReplaceImage" since that's the only thing that should be done upon that action occurring.

Then in the docs for each action, you can describe what that action does.

kevinaboos · 2025-03-25T01:47:38Z

src/shared/text_or_image.rs

+        // We only handle events if the status is `Image`.
+        if TextOrImageStatus::Image != self.status() { return };


no, you cannot do this here. You still need to forward all events to the underlying self.view (by calling self.view.handle_event()), so don't skip that.

What you actually want to do here is to only call the event.hits() function if an image is being shown.

You are right.

Resolved in #443

kevinaboos · 2025-03-25T01:48:11Z

src/shared/text_or_image.rs

 impl TextOrImage {
    /// Sets the text content, which will be displayed on future draw operations.
    ///
    /// ## Arguments
    /// * `text`: the text that will be displayed in this `TextOrImage`, e.g.,
    ///   a message like "Loading..." or an error message.
-    pub fn show_text<T: AsRef<str>>(&mut self, cx: &mut Cx, text: T) {
+    fn show_text<T: AsRef<str>>(&mut self, cx: &mut Cx, text: T) {


why did you make these private? please restore them as public since they are public APIs.

impl TextOrImageRef should be public.
So impl TextOrImage no need to be public.

hmm, not quite true. You can't always get a widgetref in the code, so these functions should be public too.

If a widgetref function is public, the inner function implemented on that widget should also be public.

kevinaboos · 2025-03-25T01:53:25Z

src/home/room_screen.rs

+                    if let MediaCacheEntry::Loaded(data) = tl.media_cache.try_get_media_or_fetch(mxc_uri, false, image_viewer_insert_into_cache) {
+                        Cx::post_action(ImageViewerAction::Show(data));
+                    }


imo, this offers a poor user experience. What happens if the user clicks an image but this if statement is not true? Nothing would happen, which would confuse the user.

If that cannot possibly happen (which I think is true here), then you should redesign the TextOrImageAction::Click type to avoid this problem.

There is performance issue, if the user triple click the image. The overlay should appear immediately after user clicks the image to prevent further clicking.
There is a memory spike when multiple clicking.

kevinaboos · 2025-03-25T01:54:53Z

src/media_cache.rs

+            // `unwrap` is definitely safe here because we have set keys already.
+            let (thumbnail_uri, better_entry, entry) = self.cache.get(mxc_uri).unwrap().clone();


considering that this is a public function, you are incorrect here -- you cannot assume that the caller has explicitly called set_keys() before this. Even if you could assume that, you should not.

kevinaboos · 2025-03-25T01:59:29Z

I have left a few comments but have not done a formal review. To be honest, I'm having difficulty understanding some of your answers and also the code in the try_get_media_or_fetch() function. It still seems massively overcomplicated to me.

In my mind, a media cache (or really, any kind of cache) just stores some data that is referenced by a key. It seems like we agree that a MxcUri key can represent either a thumbnail or a full-size image. All we're trying to do is add the extra feature of connecting a thumbnail in the cache to a full-size image in the cache, or the opposite of that (connecting a full-size image to a thumbnail).

I still don't see why we need such complexity in the cache with 4 different cases that are handled so many different ways. The code is just very very hard to read/understand.

Given that I will be busy for the next 10-11 days with conference travel, I would recommend that your next step be to meet with Alex or your fellow interns and present your design to them to see if they can help you think of a better, simpler design. Then feel free to report back here.

alanpoon

There's too much code changes. There is no need to change the text_or_image widget and the function "try_get_media_or_fetch". You can refer to how OpenMessageContextMenu works by using the item id without storing any message content inside MessageDetails. You should create something like "OpenImageContextViewer".

alanpoon · 2025-04-30T03:36:07Z

src/home/room_screen.rs

+                    if let MediaCacheEntry::Loaded(data) = tl.media_cache.try_get_media_or_fetch(mxc_uri, false, image_viewer_insert_into_cache) {
+                        Cx::post_action(ImageViewerAction::Show(data));
+                    }


There is performance issue, if the user triple click the image. The overlay should appear immediately after user clicks the image to prevent further clicking.
There is a memory spike when multiple clicking.

alanpoon · 2025-04-30T03:37:19Z

src/image_viewer.rs

+        if self.view.button(id!(close_button)).clicked(actions) {
+            self.close(cx);
+        }


Allow key press event Esc to close the image viewer

alanpoon · 2025-04-30T05:52:32Z

src/home/room_screen.rs

+        let MediaSource::Plain(original_mxc_uri) = original_source else { return };
+
+        let mxc_uri_for_timeline = thumbnail_mxc_uri.clone().unwrap_or(original_mxc_uri.clone());
+
+        media_cache.set_keys(original_mxc_uri, thumbnail_mxc_uri);


Reduce the number of clone here.
`let mxc_uri_for_timeline = thumbnail_mxc_uri.as_ref().unwrap_or(&original_mxc_uri);

media_cache.set_keys(&original_mxc_uri, thumbnail_mxc_uri.as_ref());`

pub fn set_keys(&mut self, original_uri: &OwnedMxcUri, thumbnail_uri: Option<&OwnedMxcUri>) { match thumbnail_uri { Some(uri) => { if let Entry::Vacant(v) = self.cache.entry(uri.clone()) { v.insert((None, Arc::new(Mutex::new(MediaCacheEntry::Null)), Arc::new(Mutex::new(MediaCacheEntry::default())))); } if let Entry::Vacant(v) = self.cache.entry(original_uri.clone()) { v.insert((thumbnail_uri.cloned(), Arc::new(Mutex::new(MediaCacheEntry::Null)), Arc::new(Mutex::new(MediaCacheEntry::default())))); } } None => { if let Entry::Vacant(v) = self.cache.entry(original_uri.clone()) { v.insert((None, Arc::new(Mutex::new(MediaCacheEntry::default())), Arc::new(Mutex::new(MediaCacheEntry::default())))); } } } }

alanpoon · 2025-04-30T05:55:23Z

src/media_cache.rs

 impl Deref for MediaCache {
-    type Target = BTreeMap<OwnedMxcUri, MediaCacheValue>;
+    type Target = BTreeMap<OwnedMxcUri, (Option<OwnedMxcUri>, MediaCacheEntryRef, MediaCacheEntryRef)>;


Not sure why is there a better_entry?
BtreeMap's value should not be a tuple.

alanpoon · 2025-04-30T06:14:40Z

src/media_cache.rs

-        &mut self,
-        mxc_uri: OwnedMxcUri,
-        requested_format: MediaFormat,
-    ) -> (MediaCacheEntry, MediaFormat) {


Unnecessary removal of Return type MediaFormat

alanpoon · 2025-04-30T07:14:59Z

src/media_cache.rs

-                },
+            &mut self,
+            mxc_uri: &OwnedMxcUri,
+            prefer_thumbnail: bool,


I don't think you need to add a new parameter prefer_thumbnail. You can obtain the item_id of tl_state.items when clicking the image, find the mxc_uri from the tl_state and then fetch the full image in the image viewer. That way, you wouldn't need to save the mxc_uri into text_or_image widget.

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

aaravlu added 3 commits March 13, 2025 19:32

Not Fixed

a5b3f05

Refactor again and finish image viewer

0f3135c

Remove 'test' file

0c8fe17

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 14, 2025

aaravlu added 4 commits March 14, 2025 17:18

Add case of matching mediacache entry

3b07fee

Add comments

68842c1

Fix typos

b072a73

Dont close viewer when clicking blank area

90e1910

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 19, 2025

kevinaboos requested changes Mar 19, 2025

View reviewed changes

Fix Closing quickly caused a bug

f824f99

aaravlu requested a review from kevinaboos March 19, 2025 07:53

aaravlu added 2 commits March 20, 2025 10:48

Fix a panic on cliking same image in timeline quickly

04ee5f9

Clarify match in 'fn insert_keys'

8cd8aa8

kevinaboos reviewed Mar 25, 2025

View reviewed changes

aaravlu mentioned this pull request Mar 26, 2025

Add full-screen ImageViewer widget. Support storing original files and thumbnails in the MediaCache #443

Open

aaravlu removed the waiting-on-author This issue is waiting on the original author for a response label Apr 2, 2025

alanpoon reviewed Apr 30, 2025

View reviewed changes

alanpoon closed this Apr 30, 2025

		// We only handle events if the status is `Image`.
		if TextOrImageStatus::Image != self.status() { return };

		// `unwrap` is definitely safe here because we have set keys already.
		let (thumbnail_uri, better_entry, entry) = self.cache.get(mxc_uri).unwrap().clone();

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

Uh oh!

Conversation

aaravlu commented Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alanpoon commented Jan 23, 2025

Uh oh!

aaravlu commented Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aaravlu commented Jan 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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.

Uh oh!

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.

Uh oh!

kevinaboos left a comment

Choose a reason for hiding this comment

Uh oh!

aaravlu commented Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Some(MxcUri):

None:

Uh oh!

aaravlu commented Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aaravlu commented Mar 19, 2025

Uh oh!

kevinaboos commented Mar 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinaboos commented Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alanpoon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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 Mar 19, 2025 •

edited

Loading

aaravlu commented Mar 19, 2025 •

edited

Loading

kevinaboos commented Mar 25, 2025 •

edited

Loading