Configure for each watchee?

[sjoshid]

@passcod
Im integrating the new OnGoingWrite config (issue #146) in xi_editor. If Im not mistaken you can have many watchees for a single watcher. If that is the case then the behavior of OnGoingWrite event config would be incorrect.

let mut watcher: RecommendedWatcher = try!(Watcher::new(tx, Duration::from_secs(2)));
watcher.config(Config(Some(Duration::from_millis(10000))));
try!(watcher.watch("/home/test/notify1", RecursiveMode::Recursive));
try!(watcher.watch("/home/test/notify2", RecursiveMode::Recursive));

We do something similar in xi-editor.
What we have right now needs to be tweaked a little bit.
If you agree I can get this fixed.

[passcod]

Can you explain how it would be incorrect?

[sjoshid]

Lets suppose there are two files A and B where ongoing write is happening and ongoing duration is 10. A sends an event at i and B sends at i + 1. Since there is only one ongoing_write_event in timer the event at i + 1 will not be scheduled because there is already an event from A scheduled to be fired at i + 10.
In short: until event from A is fired no other event can be scheduled. This is incorrect.
What should happen is both events, from A and B must be scheduled to be fired at i + 10 and i + 11 respectively.

[passcod]

Ah, quite. Also note that because of recursive watches, this isn't really related to having multiple watches per watcher (that threw me off) A fix would be appreciated then.

…

On Sun, 7 Apr 2019, 02:05 Sujit Joshi, ***@***.***> wrote: Lets suppose there are two files A and B where ongoing write is happening and ongoing duration is 10. A sends an event at i and B sends at i + 1. Since there is only one ongoing_write_event in timer the event at i + 1 will not be scheduled because there is already an event from A scheduled to be fired at i + 10. In short: until event from A is fired no other event can be scheduled. This is incorrect. What should happen is both events, from A and B must be scheduled to be fired at i + 10 and i + 11 respectively. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#191 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJgi5YmLjvEkRPKxLS1tMZ1DwQ2puc4ks5veJuKgaJpZM4ce-Wb> .

[sjoshid]

Yes. I have started work on a fix. But if I cannot get this done by mid-april release (work is going to be crazy this week)...I would like to have the Configure feature reverted. No point in having half-baked feature in a new release.

On a different note... I get a bunch of build errors on Windows on main branch. Is it a WIP?

[passcod]

Main branch is currently WIP, yes, although it should build on Windows (so says Travis CI, anyway). I'm pretty sure I have fixed the issue you bring up, can you review the latest?

[sjoshid]

Thanks for fixing this. Sorry I couldn't get it to sooner. I got a chance to review this and I must say your implementation is far better, clean and easy to read. The WHAT part is similar to what I was doing but the HOW part is a lot different thanks to CHashMap. Im new to Rust so thanks for introducing me to this data structure!!

Few comments/questions:

• Are we not allowing each path to have different delay?
• There is a tiny bug in handle_ongoing_write. We need to update the value after firing the event. So adding *fire_at = Instant::now() + delay; after firing the event is necessary.

Other than that .. I didnt see any issues.

[passcod]

- Are we not allowing each path to have different delay? No. Remember that watches are often recursive: we would need to either

offer a way to provide a default delay and then a custom delay per path, which gets messy, or have a delay per watch. Both of these then run into the thorny problem of path resolution: because of links, binds, and transparent path rewriting (a thing on windows and sometimes some fuse impls) it is possible that eg `path/a` and `path/b` both point to the same canonical file. In that case, assuming different delays on each of these, which applies? Besides, notify now uses crossbeam channels, which means you can start multiple watchers, configured differently, on the *same* reporting channel. This is an advanced use, but I intend it to be very much a supported use. (I'll write tests for it later.)

- There is a tiny bug in handle_ongoing_write. We need to update the value after firing the event. So adding *fire_at = Instant::now() + delay; after firing the event is necessary.

Thanks, will fix.

…