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

[core] Add more missing implementations to SDL #3439

Merged
merged 13 commits into from
Oct 18, 2023
Merged

[core] Add more missing implementations to SDL #3439

merged 13 commits into from
Oct 18, 2023

Conversation

Bigfoot71
Copy link
Contributor

As requested in #3313 here is the implementation of the SetWindowState, ClearWindowState, and SetWindowIcon functions for rcore_desktop_sdl.c.

The functions SetWindowState and ClearWindowState don't handle all flags, but it seems that some things can't be managed directly through SDL. Please take a look at the notes in the comments.

Otherwise, SetWindowIcon seems to work fine on my end on Cinnamon (Linux Mint). I don't have access to Windows to test it right now.

I'm sharing this work now so that if you have any feedback, I can make corrections, or you can merge it directly if everything looks good.

Add functions: `SetWindowState`, `ClearWindowState`, `SetWindowIcon`
@Bigfoot71
Copy link
Contributor Author

Uh wait, I'm still going to put tracelogs for the unmanaged flags

@Bigfoot71
Copy link
Contributor Author

It's good, the SetWindowState and ClearWindowState functions have been completed (I added the handling of certain flags I had forgotten), along with the correction proposed by @ubkp and some TraceLog statements for the flags that are not yet supported.

I've also added VSync support.

if (flags & FLAG_BORDERLESS_WINDOWED_MODE)
{
// NOTE: Same as FLAG_WINDOW_UNDECORATED with SDL ?
SDL_SetWindowBordered(platform.window, SDL_FALSE);
Copy link

Choose a reason for hiding this comment

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

@Bigfoot71 Could you replace it with ToggleBorderlessWindowed()? This state has a bit more going on (L200-L272) behind the scene.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ubkp If I understand correctly what the code in rcore_desktop.c does with GLFW, wouldn’t this parameter be enough SDL_WINDOW_FULLSCREEN_DESKTOP in SDL? (doc)

Or should we do exactly the same thing, i.e. remove the decoration and define the size of the window with that of the screen?

Copy link

Choose a reason for hiding this comment

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

Or should we do exactly the same thing,

@Bigfoot71 I think we should.

Unfortunately SDL_SetWindowBordered just removes the window decoration.

FLAG_BORDERLESS_WINDOWED_MODE through ToggleBorderlessWindowed does quite a lot of handling (L200-L272). E.g.: leaves real fullscreen if present, saves window pos/sizes (for toggling), removes the decoration, sets it top most, resize it to match the fullscreen and reposition it on the correct monitor. Which is necessary to simulate a fullscreen desktop (which although "present" on SDL, is still a different thing because SDL's fullscreen desktop will still cause the window to get minimized when alt tabbing).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I do that!

Copy link

Choose a reason for hiding this comment

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

@Bigfoot71 If you want (and have the time), feel free to take ToggleBorderlessWindowed. Note: it probably could be done at the same time with ToggleFullscreen, SetWindowMonitor and GetMonitorPosition since they share a lot of the same handling. Just let me know. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ubkp If you, or anyone else, can try it on their system, to see if you have the same problem as with the taskbar which remains in front of the window with ToggleBordelessWindowed that would be cool. Bigfoot71@232a061

Copy link

Choose a reason for hiding this comment

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

@Bigfoot71 Tested it right now on Fluxbox (X11) and MATE (X11) on Linux Mint 21.1 64-bit and it's working great! Very good job! 👍

I'll spin a Fedora 38 64-bit here to test it on Gnome and KDE with Wayland (may take me an hour to report back tho).

Are you on Cinnamon (X11)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ubkp Yes, I'm on Cinnamon, so it's probably related to that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ubkp I have finished implementing the functions you mentioned to me. However, I only have one screen, so I believe (by deduction) that the implementation of 'SetWindowMonitor' should be correct, but I have no idea if it works, and furthermore, it is incomplete.

So, if you or someone else could try and correct this function, it would be great! (Bigfoot71@db0674c)

Copy link

Choose a reason for hiding this comment

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

@Bigfoot71 SetWindowMonitor has a bit more handling (L615-L643). But don't worry about it, I'll finish it up. Thanks so much for all the work! 👍

We now get the size of the monitor where the window is located
NOTE: The function is implemented but incomplete
Fixed a delta retrieval issue with `GetMouseDelta` when the mouse is in relative mode. Solution by @ubkp
An issue caused `IsKeyPressed` to continuously return true for most keys when pressed
@raysan5 raysan5 merged commit d7a098e into raysan5:master Oct 18, 2023
12 checks passed
@raysan5
Copy link
Owner

raysan5 commented Oct 18, 2023

@Bigfoot71 @ubkp Wow! Amazing work! Thank you very much! 👍🙂

@Bigfoot71 Bigfoot71 deleted the patch-1 branch October 18, 2023 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants