Add image viewer widget #565

alanpoon · 2025-08-06T09:48:25Z

Fixes #327

in replacement of #443

Waiting for this PR to be merged: makepad/makepad#788.

alanpoon · 2025-08-11T04:45:09Z

Intention of adding an image magnifier and the ability to pan image.

The image's image_scale, image_pan will always be set to default in draw_walk. Hence unable to dynamically scale and pan image.
makepad/makepad#785

kevinaboos · 2025-10-17T09:08:38Z

Intention of adding an image magnifier and the ability to pan image.

The image's image_scale, image_pan will always be set to default in draw_walk. Hence unable to dynamically scale and pan image. makepad/makepad#785

@alanpoon I just checked with Rik, and this line should not be present in the Image widget. It's some errant code left over from dealing with animations in a strange way.

You can submit a PR to makepad that removes that line, and then continue with this issue here.

alanpoon · 2025-10-18T02:58:24Z

Sure, the makepad PR is here: makepad/makepad#788

alanpoon · 2025-10-19T03:01:23Z

kevinaboos

Thanks Alan, nice work here.

My main comment is that we should decouple the ImageViewer widget from the RoomScreen. It doesn't need to know anything about the RoomScreen except for being able to access the RoomScreen's MediaCache instance (technically even that is not required, see Aarav's PR for an example of just passing the image data directly from the media fetch background task to the ImageViewer via an action).

Also, Aarav and I had a lot of discussions about using actions to communicate with the ImageViewer widget in PR#443. I think you can combine your approach (of storing images in the RoomScreen's MediaCache) with the action-based design from #443 (in which you emit an action including the image data/status instead of directly accessing the widget and calling a function on it). Actions are more idiomatic and also allow easy communication from any context, both in a background task and in a RoomScreen TimelineUpdate handler.

src/home/room_screen.rs

src/app.rs

src/shared/styles.rs

src/shared/text_or_image.rs

src/shared/image_viewer_modal.rs

src/app.rs

kevinaboos · 2025-11-05T01:58:19Z

src/app.rs

+        // If the image viewer modal is really opened, handles non-Draw events using the modal.
+        let image_viewer_modal = self.ui.modal(ids!(image_viewer));
+        if image_viewer_modal.is_open() && image_viewer_modal.area().rect(cx).size.y > 0.0 {
+            let scope = &mut Scope::with_data(&mut self.app_state);
+            self.ui
+                .view(ids!(popup_list))
+                .handle_event(cx, event, scope);
+            self.ui
+                .modal(ids!(image_viewer))
+                .handle_event(cx, event, scope);
+            // Pass the Signal event to the underlying room screen, so as to populate the full image in the image viewer.
+            if let Event::Signal = event {
+                self.ui.handle_event(cx, event, scope);
+            }
+            if let Event::Actions(actions) = event {
+                for action in actions {
+                    if self.handle_image_viewer_action(cx, action) {
+                        continue;
+                    }
+                }
+            }
+            return;
+        }


why is this one modal so uniquely different than all the other modals? I would only expect you to have to handle opening and closing the ImageViewer modal here, just like all other modals.

I don't understand why you need to explicitly pass events to the modal; this is almost certainly incorrect and will result in the event being delivered to the modal twice.

I forgot to comment on the rationale. Modal widget is not behaving as expected with the events being propagated to underneath layer. This results in other image being clicked while the image viewer modal is opened.

I see, that's concerning. Can you open an issue on the Makepad repo with a minimal example that reproduces this, and then link to it in a code comment here? Thanks.

I'll try to dig into modal issues this week to see if we can fix it there.

I created a branch at https://github.com/alanpoon/makepad/tree/modal_underneath_test to test modal. The event does not propagate to underneath layer. But it is happening to Robrix. Not sure why.

After testing, this line

robrix/src/app.rs

Line 535 in c2ea9a1

self.ui.view(ids!(login_screen_view)).set_visible(cx, show_login);

is the culprit as to why event propagates to the underneath layer.

Created a tracking issue here: #626. Hope to be settle this issue after this PR is merged.

ok ... strange. Perhaps we need to redesign the LoginScreen to be something other than just a simple overlay view.

However, I don't understand why the existence of the LoginScreen (or whether it's currently visible) would impact how events are delivered to other widgets in that same overlay view (within the app body). I don't think that's actually the problem, because all the other modals in that same overlay view work just fine.

see one of my latest comments — i think if you just put the ImageViewer in a Modal (like with the verification modal or other modals), it will work as expected. I've never had any problems with modals in the top-level app not working as intended.

src/app.rs

src/media_cache.rs

src/utils.rs

src/home/room_screen.rs

kevinaboos

Thanks for addressing my comments, I left a few more comments below.

One thing I don't quite understand is why any code outside of the ImageViewer needs to be concerned with the size/dimensions of the image. Only the ImageViewer itself would care about that.... and actually, why does the ImageViewer even care? Doesn't it just show the image in a full-screen view with width: Fill, height: Fill?

(perhaps you need image dimension data for handling pan/zoom, but either way, it shouldn't be relevant outside of the ImageViewer widget code)

kevinaboos · 2025-11-08T01:42:56Z

src/home/room_screen.rs

+        let Some(item) = tl_state.items.get(index) else { return; };
+        let Some(event_tl_item) = item.as_event() else { return; };
+
+        let (capped_width, capped_height) = constrain_image_dimensions(size.x, size.y, screen_width);


why does this part of the code need to be concerned with the dimensions of the image or the size of the app window?

Ideally, any code module anywhere within Robrix should be able to just tell the ImageViewer to "show this image" without having to figure out any more details or calculate any dimensions. The ImageViewer will just display it in a full-screen view (width: FIll, height: Fill) so i'm not sure why you even need to calculate or constrain the dimensions at all.

Either way, it definitely shouldn't be done here, because that means that anywhere else I want to use the ImageViewer, i have to copy all of this complex code to populate it. We want to make it super easy to show an image in the ImageViewer.

Removed this logic

kevinaboos · 2025-11-08T01:47:30Z

src/app.rs

                            login_screen = <LoginScreen> {}
                        }

+                        <RoomImageViewer> { }


i think the problems with the event delivery and drawing will both be solved if you put this inside of a Modal, just like the verification modal a few lines below this. That way, the events will automatically be propagated to it when appropriate.

Also, the ImageViewer is a modal if you think about it, so that makes sense.

verification.mov

It seems like verification modal also has this problem. In this example, it is possible to click the link preview underneath the verification Modal.

oh damn, that's not good. Thanks for discovering that! Looks like we'll need to fix the modal behavior asap, I will take a look as soon as I can.

For now, in this PR, just assume that modals work as expected, and don't add in any weird hacks to workaround that problem. We'll fix the modals properly so that you don't have to make special exceptions here.

See my response in #626. If you merge in the latest changes from main, you should no longer be able to click widgets that are behind an open Modal. (however, scroll events will still propagate through... nothing I can do about that.)

kevinaboos · 2025-11-08T01:53:12Z

src/home/room_image_viewer.rs

+//! Room Image Viewer components for displaying images in full-screen modal.
+//!
+//! The main component is `RoomImageViewerDetail` which provides an overlay showing:
+//! - Sender information (avatar, username, timestamp) 
+//! - Image metadata (filename, file size)


i like the general concept of splitting the ImageViewer into two elements: the core full-screen image view, and some metadata in an overlay.

There are a lot of different places in Robrix that I would like to be able to use your new Image Viewer, e.g., for user/room avatars, thumbnails of uploaded images, etc.

Thus, I would encourage you to make this "Room Image Viewer" into a more generic "Image viewer with metadata", in which the overlay view just shows any arbitrary metadata that the "caller" of the image viewer wanted to include alongside the image. Room message can be one example of that metadata, but this widget definitely shouldn't just be room-specific, because there are many other use cases where an image may not be associated with a room message.

kevinaboos · 2025-11-08T01:54:46Z

src/home/room_screen.rs

+                let content_message = wr.text_or_image(ids!(content.message));
+                if let TextOrImageAction::Clicked(mxc_uri) = actions.find_widget_action(content_message.widget_uid()).cast() {
+                    let Some((texture, size)) = content_message.get_texture_and_size(cx) else { continue; };
+                    let texture = std::rc::Rc::new(texture);
+                    let screen_width = wr.area().rect(cx).size.x;
+
+                    self.handle_image_click(
+                        cx,
+                        mxc_uri,
+                        texture,
+                        size,
+                        screen_width,
+                        index,
+                        room_screen_widget_uid,
+                        scope,
+                    );
+                    continue;
+                }


very nice, this looks a lot better!

In accordance with my other comment, I would recommend moving even more of this code (the stuff dealing with texture size and screen width) into the ImageViewer itself, if it's even needed at all.

Remember, we want it to be as easy as possible to send images to the image viewer from any other part of the app too.

src/utils.rs

kevinaboos

oops, accidentally left a comment review instead of requesting changes. See above.

alanpoon added 4 commits July 10, 2025 11:45

move MEDIA_CACHE to global

e51b874

Merge branch 'main' into image_viewer#327

f323303

some testing

e1d2aaf

Merge branch 'main' into image_viewer#327

e4fe2c8

alanpoon self-assigned this Aug 6, 2025

changed default image

440bde3

alanpoon force-pushed the image_viewer#327 branch from 91bf416 to 440bde3 Compare August 11, 2025 01:10

alanpoon added the blocked-on-makepad Blocked on a Makepad bug or missing Makepad feature label Aug 11, 2025

alanpoon added 2 commits August 27, 2025 14:54

Merge branch 'main' into image_viewer#327

d029bbc

Image Viewer with zoom and pann

ddd04b9

alanpoon force-pushed the image_viewer#327 branch from 1b7875f to ddd04b9 Compare September 1, 2025 23:49

alanpoon added 6 commits September 2, 2025 07:54

fix spelling

d641657

simplify code for populate_image_modal

9fe9ddc

Deallocate image buffer

27ebbd7

Merge branch 'main' into image_viewer#327

92d78b3

remove default_image

de263cc

Merge branch 'main' into image_viewer#327

43137a1

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

alanpoon added 4 commits October 18, 2025 21:54

Merge branch 'main' into image_viewer#327

03e4120

code improvement image_viewer

c9f2484

Merge branch 'main' into image_viewer#327

81cf00a

update makepad

ec774a0

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

kevinaboos requested changes Oct 19, 2025

View reviewed changes

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

alanpoon force-pushed the image_viewer#327 branch from 2d14df8 to 91e324c Compare November 4, 2025 01:45

added room_image_viewer_footer

68179c5

alanpoon force-pushed the image_viewer#327 branch from 9919e8a to 68179c5 Compare November 4, 2025 07:16

alanpoon 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 Nov 4, 2025

fix clippy

e55568c

kevinaboos requested changes Nov 5, 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 Nov 5, 2025

alanpoon added 4 commits November 7, 2025 14:21

Fix remove_cache_entry

30ca041

removed ImageViewer opening LinkPreview's image

1a35f45

combine into room_image_viewer.rs

964b798

revert LinkPreview

1136030

alanpoon 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 Nov 7, 2025

alanpoon added 2 commits November 7, 2025 23:30

minor code improvement

897952c

Merge branch 'main' into image_viewer#327

571c888

kevinaboos reviewed Nov 8, 2025

View reviewed changes

kevinaboos added the waiting-on-author This issue is waiting on the original author for a response label Nov 8, 2025

kevinaboos requested changes Nov 8, 2025

View reviewed changes

kevinaboos removed the waiting-on-review This issue is waiting to be reviewed label Nov 11, 2025

kevinaboos mentioned this pull request Nov 11, 2025

Events propagating underneath Opened Modal in Robrix #626

Closed

remove get_texture_and_size

b43a96e

alanpoon force-pushed the image_viewer#327 branch from 1e85297 to b43a96e Compare November 12, 2025 02:55

alanpoon added 2 commits November 12, 2025 10:55

Merge branch 'main' into image_viewer#327

b4f81ab

fix clippy

dd6555b

alanpoon 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 Nov 12, 2025

fix grammar issues

9226992

Add image viewer widget #565

Are you sure you want to change the base?

Add image viewer widget #565

Conversation

alanpoon commented Aug 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alanpoon commented Aug 11, 2025

Uh oh!

kevinaboos commented Oct 17, 2025

Uh oh!

alanpoon commented Oct 18, 2025

Uh oh!

alanpoon commented Oct 19, 2025

Uh oh!

kevinaboos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kevinaboos 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kevinaboos left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

alanpoon commented Aug 6, 2025 •

edited

Loading