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

Add refined debouncer #480

Merged
merged 15 commits into from
May 15, 2023
Merged

Add refined debouncer #480

merged 15 commits into from
May 15, 2023

Conversation

dfaust
Copy link
Member

@dfaust dfaust commented May 9, 2023

This is another take at event debouncing.
Unlike the mini debouncer, the refined debouncer is much more opinionated.
It tries its best to simplify event handling and in particular can use file system ids to stitch together rename events in case the notification back-end doesn't support cookies.

Features:

  • Only emits a single Rename event if the rename From and To events can be matched
  • Merges multiple Rename events
  • Optionally keeps track of the file system IDs all files and stitches rename events together (FSevents, Windows)
  • Emits only one Remove event when deleting a directory (inotify)
  • Doesn't emit duplicate create events
  • Doesn't emit Modify events after a Create event

This PR also removes the rename thread from the inotify back-end and simplifies the rename event handling.
Because of that I bumped the version number to 6.0.0.

I haven't done much testing, yet. But it works well on Linux so far.

This PR also implements Copy for EventKind and ModifyKind (#458).
And I changed the dependency declarations to use workspace dependencies. (Requires Cargo 1.64)

@0xpr03
Copy link
Member

0xpr03 commented May 9, 2023

FYI: I'll have time to look at this after the 12th may

@0xpr03
Copy link
Member

0xpr03 commented May 9, 2023

I also want to throw another idea at you: I was thinking about whether we could make a "fat" debouncer that also detects filesystems that require usage of pollwatcher. It could then automatically use the pollwatcher backend in addition to the native one. (See #475 ) This would of course mean detecting the added path in the mount section for linux. A simple approach* would already help with missing events for WSL environments that watch windows host folders.

* just checking at the time of adding the path, not later on or for sub folders

@JohnTitor
Copy link
Member

Meanwhile, could you split #458 as a separate PR? It's essentially unrelated to the debouncer change here and it's worth releasing it as v5.2.0.

@dfaust
Copy link
Member Author

dfaust commented May 9, 2023

I was thinking about whether we could make a "fat" debouncer ...

@0xpr03
I like the name "fat" debouncer. "refined" was the best name I could come up with.
I'm not sure how combining the poll watcher with the native watcher would look like,
but there are a few areas where a debouncer and the poll watcher can be integrated closer.
I was thinking about sharing the file ID cache (this is also a reason why I made the FileIdCache trait).
Going even further it may be possible to create some sort of "offline" change detection, where events are reported for changes that happened while the application wasn't running.

could you split #458 as a separate PR?

@JohnTitor
Sure.

Cargo.toml Outdated Show resolved Hide resolved
Copy link
Member

@0xpr03 0xpr03 left a comment

Choose a reason for hiding this comment

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

Looks overall pretty good. A ton more logic in the debouncer now, but this will give a much better user experience.

As noted I would suggest renaming to -debouncer-full.
Fat is also an option, but sounds more negative than it has to.

I would drop 1.56 compatibility and move on, as for the crossbeam things we could really need some functionality from 1.60+

I really like the testing harness.
Thanks for the effort!

Cargo.toml Show resolved Hide resolved
examples/Cargo.toml Show resolved Hide resolved
notify-debouncer-refined/src/lib.rs Outdated Show resolved Hide resolved
notify-debouncer-refined/Cargo.toml Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
@0xpr03 0xpr03 merged commit 7b89553 into notify-rs:main May 15, 2023
@0xpr03
Copy link
Member

0xpr03 commented May 15, 2023

I think we should merge it first and then improve it.

If you have something you want to merge pre 6.0 release leave me comment, otherwise I'd publish a v6 + deboncer-full tomorrow.

@dfaust
Copy link
Member Author

dfaust commented May 15, 2023

@0xpr03
Will you update the debouncer-full as well, after merging #467, or should I do that? Please let me know.

I think a new release is great, but as I said, I haven't done much testing, yet.
Maybe we could include a call for testing in the release notes.

@0xpr03
Copy link
Member

0xpr03 commented May 15, 2023

Yeah I'll try to do that as well.

call for testing

We could do a RC (but this time not for 2 years)

@dfaust
Copy link
Member Author

dfaust commented May 15, 2023

We could do a RC (but this time not for 2 years)

😄 I'll leave that decision to you.
Personally, since this is the first release of the full debouncer, I would release it as version 0.1.0 and just add a few lines to the release notes saying something like: "This release contains a new debouncer. It should still be considered experimental at this point. Please let us know if you encounter any issues with it."

@0xpr03
Copy link
Member

0xpr03 commented May 15, 2023

Ah yeah I meant RC for v6 of notify. But I guess we'll skip that. And for the debouncer-full a 0.1.0 is fine.

@0xpr03
Copy link
Member

0xpr03 commented May 17, 2023

This got a little bit hairy (apart from me not noticing that the updated versions weren't all correct), crates.io does require publishing file-id. So that's now also up there.

error: failed to verify package tarball

Caused by:
  no matching package named `file-id` found
  location searched: registry `crates-io`
  required by package `notify-debouncer-full v0.1.0

But notify v6.0.0, v5.2.0, debouncer-mini v0.3.0 and debouncer-full v0.1.0 are now on crates.io

@dfaust dfaust deleted the refined-debouncer branch June 12, 2023 18:39
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