Fix version detection by calling GetVersion before GetWindowWMInfo

[lewisclement]

On certain systems not doing this will cause a "Couldn't get SDL window info: Application not compiled with SDL 2.0" error.

[Cobrand]

the SDL_GetVersion function already exists here, use this one instead. A comment about why we must fill this field with version would be great too (a link to the doc where you found this is fine).

Otherwise we're good to go!

[lewisclement]

the SDL_GetVersion function already exists here, use this one instead.

True, but the raw_window_handle module defines it's own SDL_Version struct. The already existing SDL_GetVersion expects sdl2_sys::SDL_version instead of sdl2::raw_window_handle::SDL_version. I don't really understand why this module redefines the structs, but I went with it since the code was already there I went along with it.

[lewisclement]

I would like for someone to check whether the example still works on their system, as I've done some more extensive alterations this time.

Maybe @lmcgrath as he's the one that pushed most of this code in #962 ?

[ghost]

@lewisclement I’m not at a computer until tonight, but I’ll check then :)

Some context on why sdl2::raw_window_handle redefines a bunch of structs: the primary sdl2_sys module appears to be generated on a single platform, and using bindgen to get platform-specific bindings breaks code elsewhere. Basically it was going to be a breaking API change to support bindgen so I hardcoded the necessary structs instead.

What you can probably do is implement Into for the hardcoded SDL_Version struct to turn it into the sdl2_sys struct.

[lewisclement]

Some context on why sdl2::raw_window_handle redefines a bunch of structs: the primary sdl2_sys module appears to be generated on a single platform, and using bindgen to get platform-specific bindings breaks code elsewhere. Basically it was going to be a breaking API change to support bindgen so I hardcoded the necessary structs instead.

Wouldn't you encounter this specific problem in other places too then? Having each module redefine the types doesn't seem like a very efficient solution to me.

[ghost]

It’s absolutely inefficient. Unfortunately the better solution was not the acceptable solution 🤷‍♀️

[ghost]

Your changes pull in the non cross-platform compatible sdl2_sys crate. It doesn't work on macos:

error[E0609]: no field `cocoa` on type `sys::SDL_SysWMinfo__bindgen_ty_1`
  --> src/sdl2/raw_window_handle.rs:60:54
   |
60 |                     ns_window: unsafe { wm_info.info.cocoa }.window as *mut libc::c_void,
   |                                                      ^^^^^ unknown field
   |
   = note: available fields are: `x11`, `wl`, `dummy`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0609`.
error: could not compile `sdl2`.

The bindings in sdl2_sys should only have ever been generated-only rather than checked into source control because of all the twiddles and knobs around features and platform compatibility. Unfortunately in the interest of not breaking anyone that uses what's currently there, the the raw-window-handle code rolls its own implementation of what it needs. If you're needing specific interop between the raw-window-handle code and the rest of the sdl2_sys crate, you'll have to add a translation function to convert between the structs. Lemme know if you need a hand with it, and I'm also available for macos-based testing.

[lewisclement]

What you can probably do is implement Into for the hardcoded SDL_Version struct to turn it into the sdl2_sys struct.

I ended up doing it the other way around, as copying the values wouldn't really get the desired result thanks to calling a method that takes &mut sys::SDL_version. The results would have to be copied into the crate::SDL_version anyways.

[ghost]

I think macos is the oddball in this one by SDL_GetWindowWMInfo working despite not checking the version number (it really should, and it's my bad for not doing it before anyway!) so it makes sense to grab it for all platforms, which the documentation subtly indicates should be done.

I also checked to see if the interleaving of dependencies on the sys crate could be reduced, and it looks like all that's necessary to make this work is binding a local version of SDL_GetVersion that works with the rest of the local structs. The wm_info struct is already mutable, which makes wm_info.version mutable so no copying is necessary. Here's code changes:

// Make certain to retrieve version before querying `SDL_GetWindowWMInfo`
// as that gives an error on certain systems
unsafe {
    SDL_GetVersion(&mut wm_info.version);
}

// ......

extern "C" {
    fn SDL_GetVersion(ver: *mut SDL_version); // ADD THIS

    fn SDL_GetWindowWMInfo(window: *mut SDL_Window, info: *mut SDL_SysWMinfo) -> SDL_bool;
}

The above works on both macos and arch.

[lewisclement]

fn SDL_GetVersion(ver: *mut SDL_version); // ADD THIS

Okay, that's roughly my first submission. But @Cobrand requested this to be changed. Or is that purely because we weren't yet aware why the types were redefined?

[ghost]

Yeah I see that above. I think this should be done with a local bindings. The type conversion is pretty heavy and this is an "internally handled" call anyway so dealing with it in the context of a black box with no external dependencies makes sense. @Cobrand what do you think?

[Cobrand]

Ideally we must avoid the extern "C" calls in the sdl2 crate, as they should be in the sdl2-sys. I understand that the case of SDL_GetWindowWMInfo and SDL_SysWMinfo it's an exception because it's very dependant on the OS, but I don't get why it should be the case for SDL_GetVersion.

and it looks like all that's necessary to make this work is binding a local version of SDL_GetVersion that works with the rest of the local structs

Why? Why not use sys::SDL_Version in the declaration of SDL_SysWMinfo and in the delcaration of SDL_GetWindowWMInfo? I find it pretty weird to re-define exactly the same structs in two separate modules, and implement an Into that is only doing a identity operation. If you do that, you don't have to redefine a local SDL_GetVersion either.

Or perhaps I misunderstood something or I'm missing something?

[lewisclement]

Why not use sys::SDL_Version in the declaration of SDL_SysWMinfo and in the delcaration of SDL_GetWindowWMInfo?

That's probably because of the derive of Default, which isn't in the sdl2_sys crate.

[Cobrand]

If that's the only issue, finding a way to not use Default will be much simpler than everything else. You can perhaps even use mem::MaybeUninit for this case, it would probably work too.

[lewisclement]

If that's the only issue, finding a way to not use Default will be much simpler than everything else. You can perhaps even use mem::MaybeUninit for this case, it would probably work too.

Yeah, that's what I figured too. I'm not really able to test anything on macos though, which seems to be the oddball here. @lmcgrath is the problem was that the sdl2_sys crate and it's types are missing altogether on mac? Because the zeroed version didn't work, did it?

Maybe let's apply it just to the SDL_version component.

[lewisclement]

Does this last commit work for you, @lmcgrath ?

[ghost]

I works for me!

[Cobrand]

Looks good to me, anyone has anything else to add?