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

Fix Wayland window_can_set_mode ExclusiveFullScreen #101566

Conversation

TCROC
Copy link
Contributor

@TCROC TCROC commented Jan 15, 2025

I'm currently testing out all of our supported platforms for the game we are getting ready to release. I noticed that ExclusiveFullScreen didn't work on Wayland. I dug in, fixed it, then found this PR beat me to it: #100898

But it appears the PR missed the window_can_set_mode method so here I am updating that as well. Let me know if you would like me to change anything :)

@TCROC TCROC requested a review from a team as a code owner January 15, 2025 01:25
Copy link

@Mecanerd Mecanerd left a comment

Choose a reason for hiding this comment

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

If possible, I suggest adding a before and after screenshot or some other information on how this was tested. You can also show what the other PR missed by forgetting to adjust window_can_set_mode().

@TCROC
Copy link
Contributor Author

TCROC commented Jan 15, 2025

Tbh, I just set everything that supports FullScreen to also support ExclusiveFullScreen. I noticed the other pr missed a case statement in this method. I am not sure if it has any visual effect. Maybe someone with more knowledge knows how this method is used internally? I presume it comes into effect at some point. Even if not, I suspect it should still return true for the sake of correctness since the other pr went through and now Wayland does indeed support ExclusiveFullScreen by performing the same behavior as FullScreen.

Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

Shouldn't make any difference, since window_can_set_mode seems to be only called for minimized and maximized modes.

But all other platforms and other Wayland code fall back from EXFS to regular FS, so it makes sense.

Copy link
Contributor

@Riteo Riteo left a comment

Choose a reason for hiding this comment

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

Ye this is an internal method and nothing currently uses that branch (for now) so this is more of a correctness change, which I do approve! :DDDD

@akien-mga akien-mga changed the title Fix wayland window_can_set_mode ExclusiveFullScreen Fix Wayland window_can_set_mode ExclusiveFullScreen Jan 15, 2025
@Repiteo Repiteo merged commit 6820cce into godotengine:master Jan 16, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Jan 16, 2025

Thanks!

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

Successfully merging this pull request may close these issues.

5 participants