Skip to content

Attachment upload with file dialog#674

Open
alanpoon wants to merge 32 commits intoproject-robius:mainfrom
alanpoon:image_upload
Open

Attachment upload with file dialog#674
alanpoon wants to merge 32 commits intoproject-robius:mainfrom
alanpoon:image_upload

Conversation

@alanpoon
Copy link
Contributor

Screen.Recording.2026-01-26.at.4.09.11.PM.mov
  • macOS
  • Android
  • Linux
  • Windows

@alanpoon alanpoon self-assigned this Jan 26, 2026
@kevinaboos
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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 alanpoon added the waiting-on-review This issue is waiting to be reviewed label Jan 30, 2026
@alanpoon alanpoon marked this pull request as ready for review January 30, 2026 16:18
@alanpoon
Copy link
Contributor Author

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.

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 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 kevinaboos removed the waiting-on-review This issue is waiting to be reviewed label Feb 5, 2026
@kevinaboos kevinaboos assigned kevinaboos and alanpoon and unassigned alanpoon Feb 5, 2026
@kevinaboos kevinaboos added the waiting-on-author This issue is waiting on the original author for a response label Feb 5, 2026
@kevinaboos
Copy link
Member

  • 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.

alanpoon and others added 4 commits February 24, 2026 16:35
Co-authored-by: Kevin Boos <1139460+kevinaboos@users.noreply.github.com>
Co-authored-by: Kevin Boos <1139460+kevinaboos@users.noreply.github.com>
@alanpoon alanpoon added waiting-on-review This issue is waiting to be reviewed waiting-on-author This issue is waiting on the original author for a response and removed waiting-on-author This issue is waiting on the original author for a response waiting-on-review This issue is waiting to be reviewed labels Feb 24, 2026
@alanpoon 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 25, 2026
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.

Looks pretty good, just a few minor comments.

@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 Feb 25, 2026
alanpoon and others added 8 commits February 26, 2026 08:25
Co-authored-by: Kevin Boos <1139460+kevinaboos@users.noreply.github.com>
Co-authored-by: Kevin Boos <1139460+kevinaboos@users.noreply.github.com>
Co-authored-by: Kevin Boos <1139460+kevinaboos@users.noreply.github.com>
@alanpoon 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 26, 2026
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 for the fixes.

I see that you ran an auto-formatter on the code, which makes it impossible for me to review the key changes that you actually made. Take a look at the room_input_bar diff, for example, and you'll see just how many unnecessary changes were made.

A friendly reminder to not run tools like rustfmt on whole files because it destroys our existing custom formatting decisions. Please undo all unnecessary formatting-only changes across all changed files (most of which are not an improvement, particularly in the room_input_bar.rs file), and then manually inspect your own PR's diff to ensure that there are no superfluous changes. Then, after that, please re-request a review from me.

Comment on lines +114 to +119
/// Whether the view is showing an error state with retry option
#[rust]
is_error_state: bool,
/// File data to retry if in error state
#[rust]
retry_file_data: Option<FileData>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do you need both an Option and a boolean, isn't that redundant?

If you do need both, then it's much better and clearer to create a single enum that distinguishes between the 3 or 4 different possible cases, and only includes the FileData in the variant(s) that it is needed for.

@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 Mar 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

waiting-on-author This issue is waiting on the original author for a response

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants