- Notifications
You must be signed in to change notification settings - Fork3.8k
Tweaking version getters for sdl modules and freetype#3379
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
src_c/imageext.c Outdated
| returnNULL; | ||
| } | ||
| if (!compiled) { |
andrewhong04Aug 11, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 it'll be neater to switch these around; not a big issue though.
if (compiled) {}else {}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.
The reason I left it like this for now is that if it's decided to give everything the same default behavior and the same keyword, I only have to change some stuff in 2 or 3 of them, and sincelinked is kinda the opposite ofcompiled, leaving practically the same structure eliminates some intermediate confusion at the moment
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 believe the commit I just pushed resolves this
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.
For some initial consistency, I believe all functions should have the same keyword, regardless of whether that defaults toTrue orFalse. Now changing the default value needs more discussion
I think the linked version is what should be reported. If pygame was built with SDL 2.0.14 but it's running on a Linux system and sees a newer libsdl, say SDL 2.0.20, it will use that version instead. This could be significant for joystick support, for instance, where we've frequently seen SDL versions be the entire difference "100% working" and "0% working." For the runtime behavior, the linked version is more important. This is why the existing functions that have this argument use linked=True. This is a change in behavior, but it should have very minimal backwards compatibility implications.
|
I agree, return the linked version by default for everything to synch them up. It is a v. minor change that I doubt will affect anyone and is easily resolved if it does. |
I think "returning the linked version by default for everything" is reasonable, increases consistency and won't hurt. Of course it should be documented accordingly and be mentioned in the change log I guess. |
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.
As I wrote in the comment. I think we should just havelinked=True for all these getters. Could add a smallversion changed note in the docs for the two where this is a change.
Looks good to me 👍 |
Thanks-very-cool. Note pygame.font.get_sdl_ttf_version isn't in this PR but is mentioned in the description. There's going to be a change shortly for 2.1.3 to rename it from pygame.font.get_ttf_version, so maybe wait for that to be merged first. |
Sorry for the confusion because of the misnamed PR, get_sdl_ttf_version is currently implemented. |
This is because it's already correct and there's nothing to do in this PR. I was just addressing that they are all of the same form now. |
This PR is missing unit tests though, maybe a follow up PR to add some basic unit tests would be nice? |
Uh oh!
There was an error while loading.Please reload this page.
I've worked on syncing up the version getters
pygame.get_sdl_version,pygame.mixer.get_sdl_mixer_version,pygame.font.get_sdl_ttf_version,pygame.freetype.get_version, andpygame.image.get_sdl_image_version. Now they all accept a keyword arg to change whether they return the linked version of the indicated module or the compiled version.By default,
pygame.get_sdl_version,pygame.mixer.get_sdl_mixer_version, andpygame.font.get_sdl_ttf_versionall return the linked version and use thelinkedboolean keyword to toggle which version number is grabbed.linked=Falsewill grab the compiled version.By default,
pygame.freetype.get_version, andpygame.image.get_sdl_image_versionall return the compiled version and use thecompiledboolean keyword to toggle which version number is grabbed.compiled=Falsewill grab the linked version.This is behavior that already existed for some of these functions, and for those that I had to introduce the keyword toggle, I kept the old behavior as the default. Ideally, all 5 of them would use the same keyword and default behavior for simplicity, but that would come at the cost of changing the default behavior for a few of the methods listed here.
Question: should they be left as-is to avoid the (slight) break of changing the default behavior for some methods or should they all be made consistent across the board for simplicity?
This is intended toclose#3087