py-sdl / py-sdl2

Python ctypes wrapper around SDL2
Other
304 stars 49 forks source link

Handle SDL 2.23+ new versioning scheme #230

Closed smcv closed 2 years ago

smcv commented 2 years ago

PR Description

Continuation from #229. Fixes #228 (really this time).

Merge Checklist

smcv commented 2 years ago

To avoid having a gigantic diff, I haven't replaced all uses of dll.version with version_tuple, just one use for each library. It's fine to keep using code like dll.version <= 2018 for old code paths, but for new code that cares about new versions, it'll be clearer to use dll.version_tuple <= (2, 24, 1) rather than dll.version <= 4401.

a-hurst commented 2 years ago

@smcv Oh wow, I can't believe I forgot to fix the patch version check in the tests. Thanks for fixing my fix! The version_tuple is a valuable addition too, especially in light of the new version scheme.

My only requested change: I know it's non-standard and a little bit of a kludge, but can you please revert the _version_str_to_int and _version_int_to_str functions to keep separate paths for 2.0.x and 2.x.x versioning, so 2.24.1 becomes 2241 and not the SDL_VERSIONNUM-style 4401? Those version ints aren't part of the public API and are used internally for easily checking versions for sdl2.ext functions and unit tests, so since I'm really the only person using it I prefer being able to compare versions with ints and without mental math (e.g. if sdl2.dll.version > 2240 instead of if sdl2.dll.version_tuple > (2, 24, 0) or if sdl2.dll.version > 4400). It's faster to write and makes it easier to keep line lengths reasonable.

Thanks again!

smcv commented 2 years ago

Those version ints aren't part of the public API

sdl2.dll.version and sdl2.dll.DLL.version looked like public API to me, from the naming convention. Are they not?

a-hurst commented 2 years ago

Those version ints aren't part of the public API

sdl2.dll.version and sdl2.dll.DLL.version looked like public API to me, from the naming convention. Are they not?

They're accessible but not documented (and not exported as part of the sdl2 namespace): it currently only exists to facilitate internal stuff. A proper more Pythonic API for getting SDL and ttf/mixer/image version numbers is a good idea, but would make more sense to add to sdl2.ext since that's where other convenience wrappers around base SDL2 functions live (I'd use tuples there, for sure).

smcv commented 2 years ago

I removed _version_str_to_int and _version_int_to_str because they were no longer needed: the only function that works with the 4-digit representation in this PR is _version_tuple_to_int.

I can easily put back the 2241 representation in _version_tuple_to_int, and we can avoid it causing problems in future by having it "saturate" at minor version 99 and micro version 9, for example:

_version_tuple_to_int((2, 24, 15)) == 2249
_version_tuple_to_int((2, 103, 6)) == 2999

(so in the unlikely event that you need to distinguish between higher numbers, then you have to use the tuple form). Does that make sense?

I can put back _version_str_to_int and _version_int_to_str if you want, but there won't be anything calling them any more.

a-hurst commented 2 years ago

That's perfect. Thanks so much!