Attachment upload with file dialog by alanpoon · Pull Request #674 · project-robius/robrix

alanpoon · 2026-01-26T13:38:42Z

Screen.Recording.2026-01-26.at.4.09.11.PM.mov

macOS
Android
Linux
Windows

kevinaboos · 2026-01-27T08:01:22Z

awesome!

I know this isn't yet complete, but I wanted to quickly drop in and suggest using rfd instead of makepad's internal file API. rfd is the de facto crate for this kind of thing, and I plan to shortly create a robius crate that wraps rfd, so we'll end up with a similar API.

Plus, with rfd you'd get immediate support for all 3 desktop platforms.

alanpoon · 2026-01-28T13:10:50Z

awesome!

I know this isn't yet complete, but I wanted to quickly drop in and suggest using rfd instead of makepad's internal file API. rfd is the de facto crate for this kind of thing, and I plan to shortly create a robius crate that wraps rfd, so we'll end up with a similar API.

Plus, with rfd you'd get immediate support for all 3 desktop platforms.

Kindly refer to this implementation of the rfd. https://github.com/Vjze/YY_DPS/blob/edb15ffc85b646c27547081b30a0e6f0d8ba688b/src/export/export_view.rs#L158 This requires the tokio runtime in a field in export screen

kevinaboos · 2026-01-28T17:48:50Z

awesome!
I know this isn't yet complete, but I wanted to quickly drop in and suggest using rfd instead of makepad's internal file API. rfd is the de facto crate for this kind of thing, and I plan to shortly create a robius crate that wraps rfd, so we'll end up with a similar API.
Plus, with rfd you'd get immediate support for all 3 desktop platforms.

Kindly refer to this implementation of the rfd. https://github.com/Vjze/YY_DPS/blob/edb15ffc85b646c27547081b30a0e6f0d8ba688b/src/export/export_view.rs#L158 This requires the tokio runtime in a field in export screen

Ok, what's the issue with that? Is that problematic? Apologies, but I'm not quite sure what point you're trying to make.

Moly has used rfd in their app, works fine AFAIK.

alanpoon · 2026-01-30T16:19:32Z

awesome!
I know this isn't yet complete, but I wanted to quickly drop in and suggest using rfd instead of makepad's internal file API. rfd is the de facto crate for this kind of thing, and I plan to shortly create a robius crate that wraps rfd, so we'll end up with a similar API.
Plus, with rfd you'd get immediate support for all 3 desktop platforms.

Kindly refer to this implementation of the rfd. https://github.com/Vjze/YY_DPS/blob/edb15ffc85b646c27547081b30a0e6f0d8ba688b/src/export/export_view.rs#L158 This requires the tokio runtime in a field in export screen

Ok, what's the issue with that? Is that problematic? Apologies, but I'm not quite sure what point you're trying to make.

Moly has used rfd in their app, works fine AFAIK.

Thanks for referring me to Moly. File Dialog does not work well in asynchronously in macOS as file dialog is only allowing in main thread.

src/sliding_sync.rs

src/shared/progress_bar.rs

src/sliding_sync.rs

kevinaboos · 2026-02-05T02:44:27Z

src/sliding_sync.rs

+                    // Upload the file to the media repository and send the message
+                    let file_size_uint = UInt::try_from(file_meta.file_size).ok();
+                    let attachment_config = matrix_sdk::attachment::AttachmentConfig::new()
+                        .info(if mime_type.type_() == mime::IMAGE {
+                            matrix_sdk::attachment::AttachmentInfo::Image(
+                                matrix_sdk::attachment::BaseImageInfo {
+                                    height: None,
+                                    width: None,
+                                    size: file_size_uint,
+                                    blurhash: None,
+                                    is_animated: None,
+                                }
+                            )
+                        } else {
+                            matrix_sdk::attachment::AttachmentInfo::File(
+                                matrix_sdk::attachment::BaseFileInfo {
+                                    size: file_size_uint,
+                                }
+                            )
+                        });
+
+                    // Note: Matrix SDK doesn't currently support progress tracking via observable
+                    // The progress_sender is kept for future compatibility
+                    let Some(progress_sender) = progress_sender else {
+                        error!("Progress sender not provided for file upload to room {room_id}");
+                        return;
+                    };
+
+                    progress_sender.set(TransmissionProgress { total: file_meta.file_size as usize, current: 0 });
+
+                    let result = room.send_attachment(
+                        &file_meta.filename,
+                        &mime_type,
+                        file_data,
+                        attachment_config,
+                    ).with_send_progress_observable(progress_sender.clone()).store_in_cache().await;


you're doing a lot of stuff manually here, some of which (I think) the Timeline's functions already handle for you.

For example, there is Timeline::send_attachment() and Timeline::send_gallery(), and of course send() and send_reply() which we already use (and I see you have used here as well for replies).

I don't mind if we call the Room sending functions directly, but is there a particular reason to do so? I would expect it to be easier/simpler to use the TImeline-provided functions.

Can you explain your rationale / thought process here? perhaps there is a reason why we can't use the Timeline::send* functions?

Sorry, I did not check the timeline api, lost touch.

src/shared/file_previewer.rs

src/app.rs

kevinaboos

Thanks Alan, looks very good — impressive work here!

I left comments about a few major structural decisions, but it's mostly just about refactoring things into more modular widgets. I also left some questions about using higher-level Timeline APIs vs Room APIs for sending attachments.

kevinaboos · 2026-02-05T02:53:03Z

src/room/room_input_bar.rs

+/// Maximum file size allowed for upload (100 MB).
+/// Files larger than this will be rejected to prevent memory issues.
+const MAX_FILE_SIZE_BYTES: u64 = 100 * 1024 * 1024;


no need for artificial limits. The homeserver should be responsible for informing us when a file is too large.

While the main matrix.org homeserver typically uses 100MB limits, other private homeservers likely do not have those limits.

Removed limits

src/room/room_input_bar.rs

kevinaboos · 2026-02-05T02:59:12Z

src/room/room_input_bar.rs


+        // Handle the file attachment upload button being clicked.
+        if self.button(ids!(attachment_upload_button)).clicked(actions) {
+            if !cx.display_context.is_desktop() {


"is_desktop()" is confusingly named here, it just checks for the window size, not the actual platform.

Instead, you should check the built-in target_os like we do for all robius crates; that way you can correctly and explicitly exclude ios and android.

Changed to target_os.

src/room/room_input_bar.rs

kevinaboos · 2026-02-05T03:10:40Z

macOS

Android

Linux

Windows

Also, now that you're using rfd, can you confirm whether this also works for Linux and Windows? Theoretically they are also included as supported platforms in rfd.

kevinaboos

I recently added support for threads (and thread-focused timelines), which required adding a new type TimelineKind that identifies a timeline. You'll need to use that in your new MatrixRequest variants in order to distinguish between sending a message to a room's main timeline vs. sending it to a thread timeline within that room.

Once you merge in the latest changes from main and resolve conflicts, you'll see that new type/design being used everywhere.

Another top-level comment is that all of the new widgets you introduced in this PR unfortunately assume that they are app-wide singletons, but that is not true (or will not be true in the future). Be careful when you're using actions to update the state of something — if there are multiple instances of that widget (e.g., the ProgressBar), then they will all handle that action, which is incorrect.
Instead of using actions for these operations, you can use the TimelineUpdate infrastructure to directly send updates to only one timeline, which is more correct in general.

kevinaboos · 2026-02-19T20:10:38Z

src/home/upload_progress.rs

+            cancel_upload_button = <Button> {
+                width: Fit,
+                height: Fit,
+                padding: {top: 4, bottom: 4, left: 8, right: 8}
+                draw_text: {
+                    text_style: <REGULAR_TEXT>{font_size: 9},
+                    color: #fff
+                }
+                draw_bg: {
+                    color: #c44
+                }
+                text: "Cancel"
+            }
+        }


please use the existing RobrixIconButton for the sake of stylistic consistency. You can copy the style, icon, and coloring of other cancel buttons across robrix, which are red and have the "forbidden" icon.

kevinaboos · 2026-02-19T20:13:48Z

src/home/upload_progress.rs

+        // Poll for the upload task's abort handle (for cancellation support)
+        if let Some(receiver) = &self.upload_abort_receiver {
+            if let Ok(handle) = receiver.try_recv() {
+                self.upload_abort_handle = Some(handle);
+                self.upload_abort_receiver = None;
+            }
+        }


we definitely shouldn't be doing this on every event. Either use a UI Signal, or use a dedicated action type, or something similar.

kevinaboos · 2026-02-19T20:16:19Z

src/home/upload_progress.rs

+            enqueue_popup_notification(PopupItem {
+                message: String::from("Upload cancelled"),
+                kind: PopupKind::Info,
+                auto_dismissal_duration: Some(3.0)
+            });


we now have a new function API for this, please use it after resolving conflicts

kevinaboos · 2026-02-19T20:18:40Z

src/home/upload_progress.rs

+        }
+
+        for action in actions {
+            if let Some(ProgressBarAction::Update { current, total }) = action.downcast_ref() {


we can have multiple progress bars showing simultaneously, so how does this action identify which progress bar should be updated?

Seems like you'll need a unique progress bar ID (or better yet, just use a direct channel instead of an action that is broadcasted to all widgets) in order to avoid progress updates getting mixed up across multiple progress bar widgets.

kevinaboos · 2026-02-19T20:21:28Z

src/room/room_input_bar.rs

+    #[rust] background_task_id: u32,
+    #[rust] receiver: Option<(u32, FileLoadReceiver)>,


these are both vaguely named. Can you give them more specific names and also add doc comments to clarify what they represent?

kevinaboos · 2026-02-19T20:55:39Z

src/image_utils.rs

+use mime_guess::mime;
+
+/// Maximum dimensions for image thumbnails
+const THUMBNAIL_MAX_WIDTH: u32 = 600;


my previous comment said that the standard thumbnail size for matrix is 800x600.

Suggested change

const THUMBNAIL_MAX_WIDTH: u32 = 600;

const THUMBNAIL_MAX_WIDTH: u32 = 800;

kevinaboos · 2026-02-19T20:56:50Z

src/image_utils.rs

+
+        final_img.write_to(&mut std::io::Cursor::new(&mut bytes), ImageEncodingFormat::Jpeg)?;
+        let bytes_size = bytes.len() as u32;
+        Ok(Some(Thumbnail { data: bytes, content_type: mime::IMAGE_JPEG, height: UInt::from(thumb_height), width: UInt::from(thumb_width), size: UInt::from(bytes_size) }))


formatting nit

Suggested change

Ok(Some(Thumbnail { data: bytes, content_type: mime::IMAGE_JPEG, height: UInt::from(thumb_height), width: UInt::from(thumb_width), size: UInt::from(bytes_size) }))

Ok(Some(Thumbnail {

data: bytes,

content_type: mime::IMAGE_JPEG,

height: UInt::from(thumb_height),

width: UInt::from(thumb_width),

size: UInt::from(bytes_size),

}))

kevinaboos · 2026-02-19T20:59:07Z

src/sliding_sync.rs

            }

+            MatrixRequest::SendAttachment {
+                room_id,


use the new TimelineKind type, along with the new function get_timeline_and_sender() for convenience.

Use the returned sender to send a TimelineUpdate (or multiple TimelineUpdates) back to the proper timeline, which avoids the issues I mentioned previously about widgets getting improperly updated.

I think it makes the most sense to use TimelineUpdates for all of this: the progress bar updates, the abort handle, etc. Then, in the RoomScreen where you handle those new TimelineUpdate variants, you can just directly call various RoomInputBar functions to update the widgets that are actually in the progress bar.

kevinaboos · 2026-02-19T21:08:27Z

src/room/room_input_bar.rs

+                // Set up progress tracking for the upload
+                // Create a channel to receive the upload task's AbortHandle for cancellation support
+                let (abort_sender, abort_receiver) = crossbeam_channel::bounded(1);
+                let upload_progress_view = self.upload_progress_view(ids!(upload_progress_view));
+                upload_progress_view.set_abort_receiver(abort_receiver);
+                upload_progress_view.set_visible(cx, true);


just use the existing TimelineUpdate infrastructure for all of this. It's more correct to do it that way, since these things exist on a per-timeline basis and are specific to just one timeline.

kevinaboos · 2026-02-19T21:14:05Z

src/shared/file_upload_modal.rs

+/// Type alias for file data message sent through the channel.
+pub type FileData = Arc<(FilePreviewerMetaData, Option<Thumbnail>)>;


in accordance with my other comment, you can include a timeline_update_sender here in order to directly send updates to the one timeline that this upload is related to.

alanpoon added 7 commits January 23, 2026 21:46

add image upload

72020cb

added progress bar widget

824b6cf

file upload modal

ba038cc

added file_previewer

51e388c

code improvement

172462b

fix clippy

13109ff

add FilePreviewer in App

bdb0b34

alanpoon self-assigned this Jan 26, 2026

alanpoon added 2 commits January 30, 2026 15:54

image upload improvement

0ca00fb

Merge branch 'main' into image_upload

2afea24

alanpoon added the waiting-on-review This issue is waiting to be reviewed label Jan 30, 2026

alanpoon marked this pull request as ready for review January 30, 2026 16:18

image_upload improvement

f922cf2

alanpoon force-pushed the image_upload branch from eae0bfa to f922cf2 Compare January 31, 2026 01:38

fix clippy

59860ab

kevinaboos reviewed Feb 5, 2026

View reviewed changes

kevinaboos requested changes Feb 5, 2026

View reviewed changes

kevinaboos removed the waiting-on-review This issue is waiting to be reviewed label Feb 5, 2026

kevinaboos assigned kevinaboos and alanpoon and unassigned alanpoon Feb 5, 2026

kevinaboos added the waiting-on-author This issue is waiting on the original author for a response label Feb 5, 2026

alanpoon added 3 commits February 5, 2026 11:55

Merge branch 'main' into image_upload

cc335f7

send attachment improvement

36b2432

fix file upload modal white space issue

e41250c

Merge branch 'main' into image_upload

46efd21

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 Feb 7, 2026

kevinaboos requested changes Feb 19, 2026

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

		#[rust] background_task_id: u32,
		#[rust] receiver: Option<(u32, FileLoadReceiver)>,

	const THUMBNAIL_MAX_WIDTH: u32 = 600;
	const THUMBNAIL_MAX_WIDTH: u32 = 800;

		/// Type alias for file data message sent through the channel.
		pub type FileData = Arc<(FilePreviewerMetaData, Option<Thumbnail>)>;

Comments

Conversation

alanpoon commented Jan 26, 2026

Uh oh!

kevinaboos commented Jan 27, 2026

Uh oh!

alanpoon commented Jan 28, 2026

Uh oh!

kevinaboos commented Jan 28, 2026

Uh oh!

alanpoon commented Jan 30, 2026

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!

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kevinaboos commented Feb 5, 2026

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants