Skip to content
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

Port window.c to SDL3 #3251

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Port window.c to SDL3 #3251

wants to merge 1 commit into from

Conversation

ankith26
Copy link
Member

@ankith26 ankith26 commented Dec 2, 2024

Continuing the SDL3 porting work, window.c now compiles in SDL3.

@ankith26 ankith26 requested a review from a team as a code owner December 2, 2024 17:09
@damusss damusss added sdl3 window pygame.Window labels Dec 2, 2024
@ankith26 ankith26 force-pushed the ankith26-window-sdl3 branch from e0cd9b4 to a6eb1f4 Compare December 2, 2024 17:25
@@ -288,6 +376,10 @@ window_focus(pgWindowObject *self, PyObject *args, PyObject *kwargs)
if (!PyArg_ParseTupleAndKeywords(args, kwargs, "|p", kwids, &input_only)) {
return NULL;
}
#if SDL_VERSION_ATLEAST(3, 0, 0)
/* input_only ignored on SDL3 */
SDL_RaiseWindow(self->_win);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In SDL2, SDL_RaiseWindow returns void.
In SDL3, SDL_RaiseWindow returns a bool to indicate if an error occured.
So an error check is needed here.

Copy link
Contributor

@yunline yunline left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SDL3 changed the return values of some functions. In SDL2 they return void. In SDL3 they return bool. We need to add error check for them in SDL3.
Here is a list of this kind of function (may be incomplete):

  • SDL_RaiseWindow
  • SDL_HideWindow
  • SDL_ShowWindow
  • SDL_RestoreWindow
  • SDL_MaximizeWindow
  • SDL_MinimizeWindow
  • SDL_SetWindowIcon
  • SDL_SetWindowMouseGrab
  • SDL_SetWindowKeyboardGrab
  • SDL_SetWindowTitle
  • SDL_SetWindowResizable
  • SDL_SetWindowBordered
  • SDL_SetWindowAlwaysOnTop
  • SDL_SetWindowSize
  • SDL_GetWindowSize
  • SDL_GetWindowMinimumSize
  • SDL_SetWindowMinimumSize
  • SDL_GetWindowMaximumSize
  • SDL_SetWindowMaximumSize
  • SDL_SetWindowPosition
  • SDL_GetWindowPosition

@ankith26
Copy link
Member Author

ankith26 commented Dec 9, 2024

Definitely a good catch here, thanks!

But it has gotten me thinking - raising errors is technically a behaviour change and while I think it is definitely a step in the right direction, I wonder if it's gonna be very disruptive.

@damusss
Copy link
Member

damusss commented Dec 9, 2024

But it has gotten me thinking - raising errors is technically a behaviour change and while I think it is definitely a step in the right direction, I wonder if it's gonna be very disruptive.

I'm not sure if it's a good idea to throw out so many errors. I wouldn't exactly want my program to crash just because non-critical functionality X didn't work. (for example, not sure how could changing the title fail, but even if it failed, I wouldn't want my program crashing). Otherwise you'd need to try-except basically every window operation and with that you basically lose the flexibility of OOP. If we do want to do something I think warnings are the best strategy.

@yunline
Copy link
Contributor

yunline commented Dec 10, 2024

But it has gotten me thinking - raising errors is technically a behaviour change and while I think it is definitely a step in the right direction, I wonder if it's gonna be very disruptive.

The errors added in SDL3 are more like "sanity check". If users don't write code in a really weirld way in SDL2 (like setting an invalid surface as the window icon), they won't get an error in SDL3. So I think it's OK to raise those errors.

I'm not sure if it's a good idea to throw out so many errors. I wouldn't exactly want my program to crash just because non-critical functionality X didn't work. (for example, not sure how could changing the title fail, but even if it failed, I wouldn't want my program crashing). Otherwise you'd need to try-except basically every window operation and with that you basically lose the flexibility of OOP. If we do want to do something I think warnings are the best strategy.

If SDL raises an error, it's usually because the user is giving a wrong input to the function. To eliminate the error, what user needs to do is not simply adding a "try" statement to ignore the error. They need to fix where the code is wrong, making the code more robust. So I think there is no need to replace errors with warnings to decrease the use of "try". Users can fix those errors without "try" easily.

@ankith26
Copy link
Member Author

Consider SDL_SetWindowPosition for instance. For now on SDL2 it will silently fail on platforms like wayland, for reasons completely out of the users control. Whereas SDL3 will actively raise an error in this case.

@yunline
Copy link
Contributor

yunline commented Dec 10, 2024

One way to solve this is to add a option for users to turn on or turn off these errors.
Like:

pygame.compat_ignore_sdl3_errors(True)

@damusss
Copy link
Member

damusss commented Dec 10, 2024

If SDL raises an error, it's usually because the user is giving a wrong input to the function. To eliminate the error, what user needs to do is not simply adding a "try" statement to ignore the error. They need to fix where the code is wrong, making the code more robust. So I think there is no need to replace errors with warnings to decrease the use of "try". Users can fix those errors without "try" easily.

I get what you mean, but I'm not exactly talking about that. Things that fail for user input are already raising errors. I'm talking about nonsensical errors like ankith mentioned (wayland doesn't allow to change window position). or like I said if your title string has no errors the code should not error just because the os failed to update the title for some reason. a warning is enough. I don't want to try excpet window.title = "test"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sdl3 window pygame.Window
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants