MidiInput::connect user data is not necessary

[Boscop]

The user data passed to MidiInput::connect() is not necessary because anything that the closure wants to reference can be captured.
(It's only necessary in the underlying C lib because it doesn't have closures.)
So user_data can be entirely removed from HandlerData, right?

(I always pass () for user data in MidiInput::connect() anyway, and capture what I need.)

[Boddlnagg]

I thought about removing it, but there is a difference compared to closure captures, which make it still useful:
Rust's lifetime system does not know about a closure type that can take ownership of its captures, can execute multiple times, but will never be called concurrently with itself. Also, after the MIDI connection is closed, you can get ownership of the passed data back, which is also not possible with normal closures.

I agree that for most cases, closure captures are probably sufficient ... so maybe the normal methods should not have the additional data parameter, and a second variant should be added that has it. I don't know if that would make it simpler or harder for users of the library.

Anyway, passing () is fine, and since it's zero-sized, there will be no overhead compared to removing the data parameter completely.

[Boscop]

I don't really have a problem with passing () but I can't imagine that anyone would miss if the userdata param didn't exist...
Usually I capture a std Sender and send all received messages through that for further processing.

Do you have a specific use case in mind that would make use of the user data? :)

Btw, even as it is now, it doesn't have to be Option<T>, it's only used as Some(data) and is then unwrapped, so it can just be T.

[Boddlnagg]

Yeah, probably there is no usecase that couldn't also be solved with a channel (sender/receiver). But I'd like to have some more feedback from others ...

Concerning the Option<T>, I had some problem with the borrow checker which I could only solve using Option::take(). You can try to remove the Option if you want to, I'd gladly accept a PR!

[Boddlnagg]

Note to self: Actually there is already a branch (which I forgot about) where the user_data is removed: https://github.com/Boddlnagg/midir/tree/remove-userdata

[vklquevs]

I like userdata because the connection takes ownership of it exactly while it is connected, so you can do this and UserData doesn't need to be Clone:

enum MaybeConnection {
    Disconnected(MidiInput, UserData),
    Connected(MidiInputConnection<UserData>),
}

[edit] Although if MidiInput::connect fails, you don't get the userdata back.

[Boddlnagg]

@vklquevs Would it be useful if MidiInput::connect also lets you get the userdata back? I can have a look if I can change that.

In general I think that I won't remove the userdata.

[vklquevs]

It would certainly be nice for symmetry, although at the moment I'm just giving it an mpsc::channel so it's not critical. I did have a look at how much work it would be to do that, but there are a lot of ways the connection can fail!

[Boddlnagg]

@vklquevs: Can you have a look at #44 and evaluate whether that would be useful to you so that you wouldn't need the user data anymore? I'm still trying to find the best solution here. With #44 you would have two options to pass data into the closure:

• Move it into the closure (similar to UserData right now, but you wouldn't be able to get it back out)
• Borrow it within the closure, but then your MaybeConnection wouldn't work because it would have to be designed as Connected(MidiInputConnection<'a>, UserData) and the input connection would have to borrow the UserData, which would make the struct self-referential, which is not easily possible in Rust.

[vklquevs]

Yes, if I've read it correctly then that would still work well. Even if I'm wrong (it's sadly been a while since I last looked at my project), it does look like removing user data is the way to go, so please do what you feel works best and I can work with it.

[kangalio]

Rust's lifetime system does not know about a closure type that can take ownership of its captures, can execute multiple times, but will never be called concurrently with itself.

FnMut, no? It can take ownership and be called multiple times (e.g. let mut n = 0; let mut increment_n = move || n += 1; increment_n(); increment_n();), but will never be concurrently with itself because calling it requires &mut self and Rust disallows multiple concurrent mutable references