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

Implement center window function #81012

Merged
merged 1 commit into from Aug 28, 2023
Merged

Conversation

Jordyfel
Copy link
Contributor

@Jordyfel Jordyfel commented Aug 26, 2023

Following the discussion in #65843, implements a center() function in Window.

Closes godotengine/godot-proposals#5232.

Took inspiration (copied and modified) from the implementation of Window.popup_centered().

Edit: Confused myself from following the popup logic... every non-embedded window will now center itself on it's own screen, while embedded windows will center themselves on the parent window.

Quick and dirty test project (test with embed_subwindows on and off):
empty_project.zip

@Jordyfel Jordyfel requested review from a team as code owners August 26, 2023 07:38
@Jordyfel Jordyfel force-pushed the center-window branch 2 times, most recently from 7518c30 to ebd0567 Compare August 26, 2023 08:16
@akien-mga akien-mga added this to the 4.2 milestone Aug 26, 2023
@Jordyfel
Copy link
Contributor Author

This CI fail might be because I manually moved the doc in the XML after running --doctool. Should I not do that?

@akien-mga
Copy link
Member

Indeed you should keep it where it's generated, it's alphabetically ordered.

@raulsntos
Copy link
Member

I share the concerns from #65843 (comment) in the original PR. I think "center" for the method name is confusing since it can be interpreted as a noun. Can we consider calling it move_to_center or center_to_screen instead?

@Jordyfel
Copy link
Contributor Author

In my opinion it's clear enough, since it's a name of a method. Consistent with popup().

Copy link
Contributor

@Sauermann Sauermann left a comment

Choose a reason for hiding this comment

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

Implementation looks good to me.

The method has problems in the embedded case, when used within a window that has content_scale_stretch = Window.CONTENT_SCALE_STRETCH_INTEGER.
MRP: empty_project.zip

WindowCenter.mp4

I'm not sure, if this is a limitation of CONTENT_SCALE_STRETCH_INTEGER which was introduced in #75784 or if this can/should be worked around.

doc/classes/Window.xml Outdated Show resolved Hide resolved
scene/main/window.cpp Outdated Show resolved Hide resolved
@Jordyfel
Copy link
Contributor Author

This problem with integer scaling happens only when the screen is reduced in size. The code in that PR doesn't allow the scale factor to be less than 1, which makes sense. I'm not sure this case needs handling, since doing this would be almost always undesired anyway.

If anything, it would be sensible to set the minimum size of the screen to the original viewport size in integer scaling mode.

@Sauermann
Copy link
Contributor

If anything, it would be sensible to set the minimum size of the screen to the original viewport size in integer scaling mode.

I had the same thought. So the mentioned problematic case can be ignored here.

Also please squash your commits together as explained in the docs:
https://docs.godotengine.org/en/latest/contributing/workflow/pr_workflow.html#the-interactive-rebase

@Jordyfel
Copy link
Contributor Author

Done, also renamed to move_to_center().

Copy link
Contributor

@Sauermann Sauermann left a comment

Choose a reason for hiding this comment

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

Implementation looks good. Tested also functionality.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks good.

@akien-mga akien-mga merged commit 072ba70 into godotengine:master Aug 28, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@YuriSizov YuriSizov changed the title Implement center window function. Implement center window function Aug 28, 2023
@Jordyfel Jordyfel deleted the center-window branch August 28, 2023 11:22
@CsloudX
Copy link

CsloudX commented Oct 21, 2023

Why I can't found move_to_center in Window class(4.2 beta1)?

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.

Readd a method to center window (lost during DisplayServer refactoring)
6 participants