notify icon indicating copy to clipboard operation
notify copied to clipboard

Add method for native watcher initialization with fallback

Open 0xpr03 opened this issue 1 year ago • 3 comments

Ads a new method for initializing the recommended watcher and falling back to pollwatcher on failure to do so

Fixes #423

0xpr03 avatar Aug 31 '22 15:08 0xpr03

This is a draft because:

  1. I'd like to also change the recommended_watcher function to just call the new one, giving it more value, but would have to change its result value. Not sure if we can do that semver compatible (or just ignore it).
  2. I've opted for an enum type instead of Box<dyn Watcher>, I could also imagine using that.
  3. I've only handled the specific #423 error. We could also fallback for any error.
  4. Do we want to report the original error with the fallback ? Add some additional error kind "unavailable API" to at least report it?
  5. This requires EventHandler to be declared either Copy or Clone for this fallback function. Even more of a SemVer break for recommended_watcher

Todo:

  • [ ] Add docs entry in the issue for using that
  • [ ] Change example to use the new function
  • [ ] allow debouncer init with fallback (maybe add an easier init)

CC @samuelcolvin as possible consumer

0xpr03 avatar Aug 31 '22 15:08 0xpr03

  1. I'd like to also change the recommended_watcher function to just call the new one, giving it more value, but would have to change its result value. Not sure if we can do that semver compatible (or just ignore it).

I'd call it a breaking change if it changes behavior.

  1. I've opted for an enum type instead of Box<dyn Watcher>, I could also imagine using that.

I think it's totally fine while we have take_boxed.

  1. I've only handled the specific Function not implemented (os error 38) when running on docker in M1 MacOS #423 error. We could also fallback for any error.

These could come later :)

  1. Do we want to report the original error with the fallback ? Add some additional error kind "unavailable API" to at least report it?

I don't have a strong opinion on that, maybe we could prepare an optional value?

  1. This requires EventHandler to be declared either Copy or Clone for this fallback function. Even more of a SemVer break for recommended_watcher

Hmm, that sounds like a rather difficult issue and I cannot prepare a solution for that soon (I'm now fixing my dev env and cannot check your code deeply).

JohnTitor avatar Sep 02 '22 14:09 JohnTitor

Please review again, the addition of Deref/DerefMut allows direct access to the watcher in our enum, making the whole stack allocation worthwhile, such that boxing is not required. Example

Open Questions:

  • Do we deprecate recommended_watcher() to reflect that it's kind of unhelpful as a function (in light of RecommendedWatcher) ?
  • Do we migrate all examples (except for one) and the docs to recommend using recommended_watcher_fallback over RecommendedWatcher ? Technically it is the only way which we can get to actually work on all systems (and can work around future issues). In a way comptime-selection was fun while it lasted. On the other hand a lot of people don't seem to care about the specific issue on M1 + Docker (you can also say people should not expect such weird emulator setups to work), but I don't see a drawback for people already using RecommendedWatcher to migrate to the dynamic approach.
  • Bikeshed: Is there a better name than recommended_watcher_fallback ? It's kinda long when for normal users I'd say it is the recommended way.
  • I have no good answer to the copy/clone requirement of the fallback logic. We need it for recommended_watcher_fallback, so it's an additional requirement. We may just leave it as that.

0xpr03 avatar Sep 04 '22 19:09 0xpr03

Bikeshed: Is there a better name than recommended_watcher_fallback ? It's kinda long when for normal users I'd say it is the recommended way. @0xpr03

How about ensure_watcher? From my experience, ensure has a stronger implication that a resource will be obtained.

juliusl avatar Oct 12 '22 22:10 juliusl