Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(dialog): set len and modified_at fields in FileResponse on desktop #1295

Merged
merged 3 commits into from
May 13, 2024

Conversation

luis-cicada
Copy link
Contributor

Description:

This pull request introduces a feature enhancement to the FileResponse struct in the Tauri project. The enhancement allows the FileResponse struct to automatically compute and include the size of the file in bytes when creating a new instance.

Changes:

  • Added functionality to retrieve the size of a file in bytes.
  • Implemented logic to calculate the size of the file using the standard library's std::fs::metadata method.
  • Updated the FileResponse struct's new function to include the size of the file as a field.

Benefits:

  • Users of the FileResponse struct can now easily access the size of a file without needing to manually compute it.
  • Simplifies code for developers by providing a convenient way to include file size information in FileResponse instances.

Considerations:

  • Error handling for file metadata retrieval is implemented using unwrap(). Further improvements can be made to handle errors more gracefully.
  • File size is reported in bytes. No unit conversion (e.g., kilobytes or megabytes) is performed at this stage.

@luis-cicada luis-cicada requested a review from a team as a code owner May 7, 2024 15:30
@luis-cicada luis-cicada changed the title feat(dialog) - Adding fix for file size on dialog plugin feat(dialog) - Adding fix for file size in dialog plugin May 7, 2024
@amrbashir
Copy link
Member

@FabianLars why are we returning metadata in the dialog plugin? shouldn't that be left to be handled by the fs plugin?

@luis-cicada
Copy link
Contributor Author

@amrbashir we want to get the file size right from the input rather than then using another plugin to read it. Our business case is to prevent files from being processed if they are larger than 100mb

@FabianLars
Copy link
Member

@FabianLars why are we returning metadata in the dialog plugin? shouldn't that be left to be handled by the fs plugin?

no idea, wasn't involved in that at all. i assume it makes sense to do so on mobile (not so much on desktop then which is why it's all None etc)

@amrbashir
Copy link
Member

@amrbashir we want to get the file size right from the input rather than then using another plugin to read it. Our business case is to prevent files from being processed if they are larger than 100mb

It is not the responsibility of the dialog plugin to access the metadata of the file, it should be done by the fs plugin, this ensures that plugins are composable and work together effortlessly. This also removes the duplicated code that we will have to maintain.

@amrbashir
Copy link
Member

@FabianLars why are we returning metadata in the dialog plugin? shouldn't that be left to be handled by the fs plugin?

no idea, wasn't involved in that at all. i assume it makes sense to do so on mobile (not so much on desktop then which is why it's all None etc)

maybe @lucasfernog has better idea, it should be removed imo

@luis-cicada
Copy link
Contributor Author

@amrbashir we want to get the file size right from the input rather than then using another plugin to read it. Our business case is to prevent files from being processed if they are larger than 100mb

It is not the responsibility of the dialog plugin to access the metadata of the file, it should be done by the fs plugin, this ensures that plugins are composable and work together effortlessly. This also removes the duplicated code that we will have to maintain.

Are you saying then that we should invoke dialog and fs just to get the size of the file? I understand the separation of concerns between the 2 modules, but in my opinion metadata access from the dialog is usefull to have on the fly.

@amrbashir
Copy link
Member

amrbashir commented May 13, 2024

Are you saying then that we should invoke dialog and fs just to get the size of the file?

yes, this is what I meant.

I understand the separation of concerns between the 2 modules, but in my opinion metadata access from the dialog is usefull to have on the fly.

It will still call the same APIs under the hood, and even though it is an extra call from JS side, which can be considered more overhead and a bit slower, it is still very very miniscule.

That said, I am gonna close the PR and open another to remove the metadata from the dialog plugin, I recommend you either use fs plugin to read the metadata, or create your own custom command to open the dialog and read the metadata at the same time.

@amrbashir amrbashir closed this May 13, 2024
@amrbashir amrbashir reopened this May 13, 2024
@amrbashir amrbashir closed this May 13, 2024
@amrbashir amrbashir reopened this May 13, 2024
@amrbashir
Copy link
Member

Looks like FileResponse metadata is used in the mobile portion for the plugin, and since fs plugin doesn't support mobile atm, I will accept this PR but in the future this will probably be removed as soon as fs plugin supports mobile.

Copy link
Member

@amrbashir amrbashir 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 contribution

@amrbashir amrbashir changed the title feat(dialog) - Adding fix for file size in dialog plugin feat(dialog): set len and modified_at fields in FileResponse on desktop May 13, 2024
@amrbashir amrbashir merged commit cd57dcd into tauri-apps:v2 May 13, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants