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: Added initial support for dialog, path and fs plugins #68

Open
wants to merge 21 commits into
base: v2
Choose a base branch
from

Conversation

emirror-de
Copy link

@emirror-de emirror-de commented Dec 4, 2024

This PR introduces the initial support for dialog, path and fs plugins.

This is not yet 100% complete and tested, but at least an initial implementation to build on.

Updates that have been done:

  • Renaming FsOptions::dir to FsOptions::base_dir
  • Added #[serde(rename_all = "camelCase")] to FsOptions
  • Mapping of Paths has been updated

Tested functions:

  • Confirm dialog
  • Pick file dialog
  • Open file dialog
  • Write binary file

src/fs.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@bicarlsen bicarlsen left a comment

Choose a reason for hiding this comment

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

Thanks! I had some questions about some implementation details. A few occurred multiple times, so I copy-pasted the comment to mark each spot I noticed it.

Also, in the future, please break up PR's into smaller chunks. i.e. This could have been broken into 3: one for each of fs, path, and dialog.

src/fs.rs Outdated Show resolved Hide resolved
src/fs.rs Outdated Show resolved Hide resolved
src/fs.rs Outdated
/// ```
///
/// Requires [`allowlist > fs > readDir`](https://tauri.app/v1/api/js/fs) to be enabled.
pub async fn read_dir(path: &Path, dir: BaseDirectory) -> crate::Result<Vec<FileEntry>> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It doesn't appear that readDir gives any structure to what is read, and I don't see where you assemble the strcutre, using the children field of FileEntry.

Copy link
Author

Choose a reason for hiding this comment

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

I am not sure if I get what is required, but I changed the ReadDirOptions as a first step towards a solution. See 3201ccf8

If I get the API right, path should be relative to base_dir?

src/fs.rs Outdated
/// ```
///
/// Requires [`allowlist > fs > readDir`](https://tauri.app/v1/api/js/fs) to be enabled.
pub async fn read_dir_all(path: &Path, dir: BaseDirectory) -> crate::Result<Vec<FileEntry>> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

See comments for read_dir.

Copy link
Author

Choose a reason for hiding this comment

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

As far as I can see, there is no readDirAll function in v2? I only can find readDir

src/fs.rs Outdated Show resolved Hide resolved
src/path.rs Outdated Show resolved Hide resolved
src/path.rs Outdated Show resolved Hide resolved
src/path.rs Outdated Show resolved Hide resolved
src/path.rs Outdated Show resolved Hide resolved
src/path.rs Outdated Show resolved Hide resolved
@bicarlsen
Copy link
Collaborator

bicarlsen commented Dec 16, 2024

@emirror-de Can you resolve the conversations which you've addressed, so I know if you're still working on things, or if I should review again.

@emirror-de
Copy link
Author

Yes sure, I will also re-request a review as soon as I finished walking through.

@emirror-de
Copy link
Author

@bicarlsen thanks for the review. I must admit, the initial PR state was just a copy-paste of v1 version with collected JS and adjusted to work for my urgent requirements. But I wanted to share and show that there has been some work started in case anyone is happy to work on it as long as I am not able to.

Despite to this, I adjusted most of your suggestions. Unfinished threads are still left open.

I think there is much more work to do on this, as I did not update the documentation yet to match the ones from the reference.

Some ideas I had during my walk through the code:

  • In path.rs should we replace the functions that return base directories and add a generic one that takes BaseDirectory as param? This would reduce the amount of code on Rust side quite a bit.
  • I now sometimes added the options parameter as second argument to the Rust functions as for example in fs::remove. Should we adjust the rest as well to get closer to the JS API? Or decide it by the amount of parameters in the specific functions?

@bicarlsen
Copy link
Collaborator

@emirror-de, thanks for all the work :)

1

For the BaseDirectory functions, I leave it to you to choose whatever feels more ergonomic. I don't use these functions, so don't have a sense. The code for the individual functions is simple enough I'm not worried about maintaining the extra lines.

2

In general I would prefer deviating from the JS API if it means we can be more Rusty.
e.g.

function myFunction(requiredArg, optionalArg)

should go to

fn my_function(required_arg)
fn my_function_with_options(required_arg, optional_arg)

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.

2 participants