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

Add support for MaximizeWindow in DesktopGL platforms (uses SDL_MaximizeWindow) #7870

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

Conversation

HS-Dave
Copy link

@HS-Dave HS-Dave commented Jul 28, 2022

No description provided.

@HS-Dave HS-Dave changed the title Feature/desktopgl maximize window Add support for MaximizeWindow in DesktopGL platforms (uses SDL_MaximizeWindow) Jul 28, 2022
@mrhelmut
Copy link
Contributor

If we're adding a method to the public API, we should implement it for all platforms to not mislead users with platform-specific public APIs.

@HS-Dave
Copy link
Author

HS-Dave commented Jul 28, 2022

If we're adding a method to the public API, we should implement it for all platforms to not mislead users with platform-specific public APIs.

I agree in principle, although the work to add to other platforms is a margin higher - but there is already precedent for platform only features in this class such as IsBordless to hide the border which docs specify "Currently only supported on the WindowsDX and DesktopGL platforms." and FileDrop which obviously only works on desktop platforms.

@mrhelmut
Copy link
Contributor

Yes, but we should avoid making an exception to be the rule if we want the framework to be more accessible. At minimum, I'd like to suggest that we have the public API available for all platforms, and per-platform throwing NotImpletedException than having no public API at all.

@damian-666

This comment was marked as off-topic.

@HS-Dave
Copy link
Author

HS-Dave commented Jul 28, 2022

that seems an insane amount of external work to do for what is currently 18 lines in API and 1 call in implementation. I still strongly disagree that adding a method like this that is only available for specific platforms waters down the API in any meaningful way (and no more than throwing a not implemented exception) but I would rather implement the feature for whichever platforms support it than over engineer an external window manager. All we are trying to do here is maximise an already existing window without having to manually fetch the screen dimensions and set them ourselves :)

@mrhelmut
Copy link
Contributor

I still strongly disagree that adding a method like this that is only available for specific platforms waters down the API in any meaningful way (and no more than throwing a not implemented exception)

I should have started with that I guess. We have the plan to unify the public API and cleanse it of platform-specific #if. For that project to happen, we need the public API to be the same for all platforms.
We also believe that it is a better experience to have a code that compiles as-is for all platform (and proceed silently) and that doesn't oblige multi-platform projects to have #if too.

@damian-666

This comment was marked as off-topic.

@HS-Dave
Copy link
Author

HS-Dave commented Jul 28, 2022

I still strongly disagree that adding a method like this that is only available for specific platforms waters down the API in any meaningful way (and no more than throwing a not implemented exception)

I should have started with that I guess. We have the plan to unify the public API and cleanse it of platform-specific #if. For that project to happen, we need the public API to be the same for all platforms. We also believe that it is a better experience to have a code that compiles as-is for all platform (and proceed silently) and that doesn't oblige multi-platform projects to have #if too.

in context of your plan this now makes a lot more sense - I will see what I can do to support the non SDL platforms with this same feature as I feel its a pretty useful base feature, leave it with me a couple of evenings

@damian-666

This comment was marked as off-topic.

@damian-666

This comment was marked as off-topic.

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.

None yet

3 participants