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

Ship-side part of the Mac fullscreen fix #5060

Open
wants to merge 15 commits into
base: develop
Choose a base branch
from

Conversation

larsy1995
Copy link
Contributor

@larsy1995 larsy1995 commented Feb 13, 2025

Changes to SohMenuBar.cpp needed to remove fullscreen mode toggle when in fullscreen and change the toggle text from windowed fullscreen to exclusive fullscreen.

Thanks @ReddestDream for making the fullscreen toggle fix.

Dependent on:
Kenix3/libultraship#817

Build Artifacts

Copy link
Contributor

@briaguya-ai briaguya-ai left a comment

Choose a reason for hiding this comment

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

after looking at this and the LUS PR i'm not quite following the logic here

what i understand:

  • this changes the meaning of the windowed fullscreen cvar on mac
  • this hides the toggle when in fullscreen on mac

i guess my main question is when would someone want to check the box vs not check the box? does just always using macos native fullscreen for fullscreen not work?

@larsy1995
Copy link
Contributor Author

larsy1995 commented Feb 14, 2025

Exclusive mode enabled the direct rendering mode on macOS instead of just the composited rendering mode. The reason to keep the button at the moment is to not remove a feature that someone potentially might want to use, it is at least useful for testing and it keeps a sort of feature parity with the other operating systems.

But as it is currently, exclusive fullscreen is the default which is not ideal in a macOS context, thus I am inverting the CVAR on LUS and correcting the name on ship.

@briaguya-ai
Copy link
Contributor

someone potentially might want to use, it is at least useful for testing

i'm still not fully understanding why someone would want to use one or the other

exclusive fullscreen is the default which is not ideal in a macOS context

why is this not ideal?

and my last question is about hiding the toggle when in fullscreen - that just feels like strange UX to me

@larsy1995
Copy link
Contributor Author

We hide the toggle because otherwise we end up fullscreening twice if the fullscreen mode changes when in native fullscreen mode. This is a way to keep the behaviour consistent while keeping the mouse cursor hiding functionality when the imGUI menu is visible or hidden.
Exclusive fullscreen blocks the macOS menubar and dock access as well as block out the notifications pop-up. This is not how most macOS applications handle this and this toggle is to make it more consistent with the macOS ecosystem as a whole.
Removing exclusive fullscreen mode and just keeping native would be easier, but I was advised against removing functionality.

@briaguya-ai
Copy link
Contributor

@Archez and/or @garrettjoecox probably have a better sense of the "why* for these things. my main hangup is inverting the cvar so on mac it means the opposite of what the value implies.

@larsy1995
Copy link
Contributor Author

The other option would be to have the checkbox enabled by default, but I feel like that is a worse inconsistency.

@briaguya-ai
Copy link
Contributor

I was advised against removing functionality

who advised against that? did they give a reason? it sounds to me like just always using native fullscreen on mac is the logical choice

@larsy1995
Copy link
Contributor Author

I can't remember, my memory doesn't keep information like that usually.
I can definitely change the code to just use native fullscreen, that would simplify the code a fair bit, but what you're saying then is that we should not keep the direct rendering mode available and just permanently remove the toggle on macOS?
I can do that.

@briaguya-ai
Copy link
Contributor

i'd want to hear what @Archez and/or @garrettjoecox think about that idea first, but that feels reasonable to me

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