-
Notifications
You must be signed in to change notification settings - Fork 251
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
Second attempt on preventing an event buildup #569
Conversation
eebc8b2
to
9470c01
Compare
I've tested this locally for a bit and will keep using this for the next few days. So far, this looks good too. I know the PR requires a few finishing touches, I will do them once we agree on merging this. |
Works good on my machine. @thedodd any thoughts on this? |
@thedodd Is there anything wrong with this PR? |
I think I did everything asked for, implementing this twice. As I don't have any feedback and no idea why this doesn't get any attention, I will go ahead and fork this in the few weeks. I am absolutely open to bring all of this to the original project, but I also need something that others can easily install. |
Please see my comments here: #588 (comment) Also, I will do my best to get to these issues. I'll touch base with the other maintainers to see if they might have time to take a look as well. |
9470c01
to
bb06361
Compare
This is now released as part of the 0.17.5 release of |
Ok, looks like this is a known issue which folks are seeing: notify-rs/notify#259 As it turns out, this is not due to a library we are using, but comes from I've implemented a solution to address this. A cooldown period following a build. All events will be ignored following a build completion for the cooldown period, which I will hardcode to 1s for now. That should be enough without impacting the development flow. @ctron I will merge your changes here (you've worked hard on this), but I am going to update the watch.rs file after merging. I've experimented with this PR, and I was still seeing the infinite build loop. After I merge this, I'll open a PR to highlight the changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, merging this. Thanks @ctron
Follow up on: #516
See: #516 (review)