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

Provide debounced versions of some oft-updating events #63

Closed
passcod opened this issue May 1, 2016 · 8 comments
Closed

Provide debounced versions of some oft-updating events #63

passcod opened this issue May 1, 2016 · 8 comments
Milestone

Comments

@passcod
Copy link
Member

passcod commented May 1, 2016

See #48 for reason and #60, #62 for context. In short: MODIFY events fire too often, or twice in a row, and it would be useful to only emit them once in [some time period].

@passcod passcod added this to the 3.1.0 milestone May 2, 2016
@dfaust
Copy link
Collaborator

dfaust commented Jul 29, 2016

@passcod

So I pushed a debounce module and it actually works almost the same way on all platforms. I think this is the last major change for now. So from now on I will concentrate on polishing things up.
There are a few things left to do though and I would like to hear what you think.

  • Move debounce::Event to lib.rs and rename it to DebouncedEvent
  • Rename Event to RawEvent
  • Rename Watcher::new to Watcher::new_raw and Watcher::debounced to Watcher::new_debounced
  • The Inotify back-end can emit events without a path. These aren't handled at the moment. Is there a valid reason why this would happen? If not, I would suggest emitting an error (eg. EventWithoutPath(op::Op)).
  • Improve documentation
  • Test with PollWatcher

An additional note: I have added quite a lot of tests. It seems that some of them (if not all) suffer from timing issues (mostly on os x). I don't have a solution to that and adding sleeps can change the entire nature of the test (in fact there are some tests that should be split in two, one with additional sleeps and on without). As more tests are added, the chance of hitting such a timing issue increases. So it will become more and more likely that tests fail, but I would argue that the issue is with the operating system and not with notify.

@passcod
Copy link
Member Author

passcod commented Jul 30, 2016

Can you think of a way to mark timing-sensitive tests as such in the output, such that it becomes easier to see whether a test run is a hard failure or may simply need a rerun to combat the timing?

@passcod
Copy link
Member Author

passcod commented Jul 30, 2016

👍 on EventWithoutPath

@dfaust
Copy link
Collaborator

dfaust commented Aug 6, 2016

Can you think of a way to mark timing-sensitive tests as such in the output, such that it becomes easier to see whether a test run is a hard failure or may simply need a rerun to combat the timing?

I think the debounce tests are more prone to timing problems but in general all tests seem to have that problem. It happens rarely, so I don't have much data (actually none, since I didn't write the failures down). So at this point I guess the best we can do is write down which tests fail how often and then look for a solution.

👍 on EventWithoutPath

Any opinion on the other issues? Or the behavior of the debounce module as a whole btw.?
I started by putting everything debounce related into a module, but I think it would be better to move some parts into the notify lib. I would guess that most people will use the debounce API anyway since it is easier to use.

@dfaust
Copy link
Collaborator

dfaust commented Oct 2, 2016

Were you able to make up your mind? I think we should wrap this up and release a new version.

@passcod
Copy link
Member Author

passcod commented Oct 12, 2016

I've been extremely busy at work the past two months, which has left me little motivation to work on this, and I apologise for the time it's taken to come back to it.

I very much like what you've done with debouncing and everything else. I do think debounce is going to be the primary interface for most people, so it should be within notify for the best out-the-box experience.

@dfaust
Copy link
Collaborator

dfaust commented Oct 18, 2016

I re-factored the debounce code a bit and I guess it's ready for release now. Another set of eyes wouldn't hurt of cause but thanks to cargo nothing should go up in flames anyway.
What remains to be changed is the example in the readme.

I had another look at the EventWithoutPath thing, but I realized that it wouldn't work as I first thought, so I didn't change it. My idea was to change the Option in RawEvent to a non-optional path, but that would only work, if it was wrapped in the Result, which it isn't.

@passcod
Copy link
Member Author

passcod commented Oct 30, 2016

This has been released in 3.0.0. Looked through everything, corrected a few typos in comments and documentation, and ran it all through rustfmt 🎉

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

No branches or pull requests

2 participants