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(fs/watch): migrate to notify-debouncer-full #885

Merged
merged 7 commits into from
Jan 18, 2024
Merged

Conversation

dfaust
Copy link
Contributor

@dfaust dfaust commented Jan 10, 2024

  • Adds support for notify-debouncer-full

    This complicates the API some more and I'm not super happy with it, yet. But at least this allows Tauri users to take advantage of the more involved notify-debouncer-full.

  • Adds a watch section to the file system page of the demo app

    image

I'm a contributor to the notify crate, on which the fs watcher is built. So if you have any questions or suggestions, please let me/the rest of the notify team know.
One thing that I noticed is that Tauri might be the first application that really uses the event serialization. At least for me this feature has been more of an afterthought until now. I'm not happy with the way events are serialized, maybe we can change that - I'm thinking of using #[serde(tag = "kind")] for enums.

@FabianLars
Copy link
Member

Hi, thanks so much for contributing! Do you see any value in keeping the mini implementation around?

Also, before you add more commits:
We require all commits of a pr to be signed - here's a guide: https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature-verification#ssh-commit-signature-verification - i recommend the ssh approach because i think it's easier. A guide for gpg is right above that though

And we'll also need a changefile like this one: https://github.com/tauri-apps/plugins-workspace/blob/v2/.changes/dialog-async-message-dialog.md, but it probably makes sense to wait for that in case we end up dropping the mini impl.

@amrbashir
Copy link
Member

let's just move to the full version and remove mini

@dfaust
Copy link
Contributor Author

dfaust commented Jan 11, 2024

Do you see any value in keeping the mini implementation around?

The full debouncer has more features, but is also more opinionated. Still, I think most people would prefer it.
Then again, as the author of the full debouncer I'm biased :). Maybe @0xpr03, the author of the mini debouncer has an opinion on this?
Removing the mini debouncer would simplify the API and we could always add it back if the need arises.

@0xpr03
Copy link

0xpr03 commented Jan 11, 2024

From what I can see Tauri is a full OS abstraction to most users. With that in mind I think you want the full debouncer. Debouncer-Mini is just a bare debouncer drop-in for when the old one got removed from notify. So it's for people who want nothing more than a thin abstraction on top and who can live with some surprising behaviour from the filesystem.

Even more: You probably want more features than the full debouncer currently offers. Specifically filesystem detection and automatically choosing backends for paths.

@dfaust dfaust force-pushed the v2 branch 2 times, most recently from cb8a5c3 to d6da17f Compare January 12, 2024 18:16
@dfaust
Copy link
Contributor Author

dfaust commented Jan 12, 2024

I updated the PR.

  • I removed notify-debouncer-mini
  • Renamed RawEvent to Event because the full debouncer emits the same event type
  • Added a .changes files
  • And signed my commits

plugins/fs/guest-js/index.ts Outdated Show resolved Hide resolved
plugins/fs/src/watcher.rs Outdated Show resolved Hide resolved
plugins/fs/guest-js/index.ts Outdated Show resolved Hide resolved
@amrbashir amrbashir changed the title Add support for notify-debouncer-full feat(fs/watch): migrate to notify-debouncer-full Jan 18, 2024
@amrbashir amrbashir merged commit 61edbbe into tauri-apps:v2 Jan 18, 2024
35 of 36 checks passed
@amrbashir
Copy link
Member

Thank you

@dfaust
Copy link
Contributor Author

dfaust commented Jan 18, 2024

Thanks for merging, I'll create another PR for the track-file-ids feature then.

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.

4 participants