Re-order display fullscreen handling and ensure toggle_fullscreen works for maximized window#3497
Conversation
ankith26
left a comment
ankith26
left a comment
There was a problem hiding this comment.
Wow, I'm quite impressed!
This seemingly simple fix fixes 3 really complex-looking issues! 🥳 🎉
I could reproduce all three issues being fixed (the first 2 were spot on, the third toggle_fullscreen one was a bit problematic and I came across other minor and unrelated bugs, but the main issue in there is probably fixed)
We have other issues to be tracking toggle_fullscreen problems, and this PR does good progress already, thanks a ton! 🍰 🍕
(due to the nature of what this is fixing, I'd like if we could get more than 3 reviewers, and from my side I'm supporting this PR to quickly make it to the next dev release for wider testing)
|
I've updated it so that it now has the correct formatting and contains a comment for that specified section. I also wrote some further changes for the toggle_fullscreen so that it preserves these flags and the position as well. Also related issue #2698 is fixed by this PR. These changes will require further review to check that no issues are brought about by them. |
There was a problem hiding this comment.
I think the fullscreen_backup_x/fullscreen_backup_y stuff are redundant now. Those were added as part of #2460 to workaround a similar issue (and I suspect most of #2460 could now be reverted), because this fix of handling fullscreen first and then doing other display stuff also fixes the issue that PR was fixing
|
Unfortunately the backup positions are still required since those get modified when going into fullscreen as it changes the position to (0, 0) when entering fullscreen. Fullscreen was already handled before position anyway so it isn't affected by this change. This does however implement the same fix #2460 used for toggle_fullscreen now to enable position saving since that was left out when the PR was made. |
src_c/display.c
Outdated
|
|
||
| flags = SDL_GetWindowFlags(win) & SDL_WINDOW_FULLSCREEN_DESKTOP; | ||
| flags = SDL_GetWindowFlags(win); | ||
| /* SDL_WINDOW_FULLSCREEN_DESKTOP includes SDL_WINDOW_FULLSCREEN */ |
There was a problem hiding this comment.
Now that the line above is changed, the comment just below it seems a bit out of place. Should it be moved elsewhere?
| win = SDL_CreateWindow(state->title, wx, wy, w, h, flags); | ||
| if (win == NULL) { | ||
| return RAISE(pgExc_SDLError, SDL_GetError()); | ||
| } |
There was a problem hiding this comment.
The comment in this block of code says it's a hack for SDL 2.0.8 (which is a rather old version)... not sure whether it's relevant to this day? Would need someone running pygame on linux with x11 to check though
There was a problem hiding this comment.
This fixes the two main linked issues for me, with the slight caveat that one of them also seems to want a resize able window without a frame which isn't a thing in the Windows platform where you resize and move windows by dragging the frame around.
|
Alright fixed that comment so it's now where the flag is actually mentioned, looks like everything has been resolved. |
Re-order display fullscreen handling and ensure toggle_fullscreen works for maximized window
4 participants
This PR resolves #2711 and #3107 by changing the order that changing the fullscreen flag is handled so that it sets the window to be not fullscreen before handling the borderless and resizeable flag as when in fullscreen SDL ignores these changes.
It also partly fixes #2380 as it succeeds at creating a fullscreen window and when exiting retains the resize flag of the window but it still results in a warning message as it has to recreate the window. The toggle function could likely still be improved in this regard but for now this PR fixes the main issues found with it.