Add wrapper for SDL_AddEventWatch and SDL_DelEventWatch

[TonalidadeHidrica]

Closes #1023.

The new function enables to use an event watcher in a safe, user-friendly way. We can call EventSubsystem::add_event_watch with a closure parameter as a callback to listen to the events. The callback is automatically disabled when dropped.

When I wrote the code I largely referenced AudioDevice::open, AudioSpecDesired::convert_to_ll and audio_callback_marshall in src/audio.rs. Great thanks to them.

Concerns:

• I'm not sure about the trait bounds and lifetimes. Is it correct? Also, the function may be called in multiple threads. Is it desired to add Send or Sync as additional bounds? (I'm not quite proficient at unsafe Rust, sorry)
• Is the "drop-to-disable" design good?

[Cobrand]

I'm not sure about the trait bounds and lifetimes. Is it correct? Also, the function may be called in multiple threads. Is it desired to add Send or Sync as additional bounds? (I'm not quite proficient at unsafe Rust, sorry)

Lifetimes seem correct, you added PhandomData as well which was necessary.

However I don't really know what you mean by "the function may be called in multiple threads".
If the closure you pass through is only executed once at a time, I think your implementation is correct. I don't think it needs Sync in that case (like F: FnMut(Event) -> () + Sync) because it is not accessed by multiple threads at once.
However, if the callback can be called twice at the same time (because an event1 is triggered in thread1 and event2 is triggered in thread2 at the same time for instance), then your implementation is unsafe. It that is the case, then your closure must be Fn(Event) -> () + Sync instead of FnMut(Event)

It all depends of when the callback is called exactly, which is probably explained somewhere in SDL2's internal code (the doc is not clear enough on this topic). If the acllback is called as soon as the event is triggered, then it's probably the latter, however if it's called right about "event_pump" processing is done, then it's probably single-threaded and the former implementation.

Is the "drop-to-disable" design good?

Yes, it's standard Rust to do it like so.

[TonalidadeHidrica]

Thank you for your reply! As for concurrency, I asked the community forum for the detail. As long as there's a feedback, I'll adjust is as needed. Is there some other things to do before it can be merged?

[Cobrand]

The callback is indeed run on push, but apparently there is a mutex for the watchers in PushEvent, so that the callbacks are never run at the same time:

https://github.com/libsdl-org/SDL/blob/main/src/events/SDL_events.c#L805

So your code seems correct. Tell me if you have anything else to add, otherwise I can merge this.

[TonalidadeHidrica]

@Cobrand Thanks for your reading the code! I realized that closures are actually difficult to describe its type, so I'm planning to extract it to a trait. As long as it's done it's ok to merge.

[TonalidadeHidrica]

@Cobrand Done!

[Cobrand]

Mmh I'm not sure it's the right call to create a trait just for that, is there a reason to not use FnMut like you did before?

[TonalidadeHidrica]

@Cobrand Several reasons.

• It was hard to describe the type. For example, if I wanted to return an event watch from a function, I needed to write like this:

fn create_event_watch() -> EventWatch<impl FnMut(Event) -> ()> {
  // generate
}

or more trickier (if you don't want to manipulate the event watch at all):

fn create_event_watch() -> impl Drop {
  // generate
}

The former is lengthy and the latter restricts the usability. If you want to store it into a struct, things get worse.

• It's the same way AudioCallback does.

Of course I loved the handy closure approach, but while I'm using I realized the problem mentioned above, and that's why I switched to trait.

[Cobrand]

Could you try to at least impl<F> EventWatchCallback for F where F: FnMut(Event)? It'll avoid to specifically create a struct if you don't want to. At least I think it'll work fine like that, but I'm not so sure since this is a public lib crate.

[TonalidadeHidrica]

@Cobrand That's a good idea, I didn't came up with that! I added.

[Cobrand]

Thanks! Let's merge this.