-
-
Notifications
You must be signed in to change notification settings - Fork 161
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use SDL_TTF_VERSION_ATLEAST
#3003
Use SDL_TTF_VERSION_ATLEAST
#3003
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from a minor review comment this PR LGTM. Thanks for the PR 🎉
@@ -989,8 +983,7 @@ font_set_direction(PyObject *self, PyObject *arg, PyObject *kwarg) | |||
writing this) This bug flips the top-to-bottom and bottom-to-top rendering. | |||
So, this is a compat patch for that behavior | |||
*/ | |||
#if SDL_VERSIONNUM(SDL_TTF_MAJOR_VERSION, SDL_TTF_MINOR_VERSION, \ | |||
SDL_TTF_PATCHLEVEL) < SDL_VERSIONNUM(2, 22, 0) | |||
#if !SDL_TTF_VERSION_ATLEAST(2, 22, 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that you made this PR, it got me thinking of whether this should be a compiled-version check or a linked-version check.
It isn't a big deal as probably over 99% of our users have matching compiled and linked versions, but it could be something to look into if you'd like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for the purposes of compat with the original code, it should be kept as-is in this pull. If we want to change that extreme edge-case behavior, that should be a separate pull IMO (maybe one that addresses it in a bunch of places that I'm sure have a similar problem)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it would dynamically link if it was lower.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Leaving unmerged just in case others disagree with me about the compiled-vs-linked discussion
Since #2464 our SDL_ttf minimum version is 2.0.15, so we can use
SDL_TTF_VERSION_ATLEAST