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 visibility mode to as_sortable_control() #92664

Merged

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Jun 1, 2024

as_sortable_control() introduced more problems than it's worth it tbh, most of which come from unifying what's considered a sortable child to always use is_visible_tree(). This PR breaks the unification by introducing visibility mode to this method. By default it checks for is_visible_in_tree(), but it can be is_visible() or no visibility check, depending on needs. This allows to apply it in some classes that couldn't use it before.

Fixes #92618
Fixes https://chat.godotengine.org/channel/gui?msg=dmJWbcbv5amvaLMi9
Fixes #92812 (not tested)
(this time it was PanelContainer)
EDIT:
Also ScrollContainer: Fixes #92614 Supersedes #92714

Alternatives are making sure that containers don't break when using is_visible_in_tree() (unlikely, given how Control sizing is unsolved Godot problem since forever), or reverting #91613.

@KoBeWi KoBeWi added this to the 4.3 milestone Jun 1, 2024
@KoBeWi KoBeWi requested a review from a team as a code owner June 1, 2024 22:51
scene/gui/tab_container.cpp Outdated Show resolved Hide resolved
@kitbdev
Copy link
Contributor

kitbdev commented Jun 2, 2024

I was thinking instead of having modes, have an is_visible_in_tree() guard in all Container's sort children notifications, since no logic needs to run in sort children if the Container itself is not visible. Then there's no need to check if each child is visible in tree, they can just check is_visible() and the Container handles the is_visible_in_tree() part.

Also, missing some Containers like ScrollContainer, AspectRatioContainer, CenterContainer, GraphFrame, FlowContainer, and GridContainer.
GraphNode should also be set to visible mode for it's minimum size.
I think all minimum size checks should use is_visible() instead of is_visible_in_tree().

@KoBeWi KoBeWi force-pushed the ultimate_final_solution_for_containers branch from 3274d1e to a15564f Compare June 2, 2024 19:59
@KoBeWi
Copy link
Member Author

KoBeWi commented Jun 2, 2024

For now I only fixed PanelContainer issue, other changes are bonus refactoring. I tried to keep the current behavior (TabContainer change was an oversight), any other improvement can be done later.

@AThousandShips
Copy link
Member

@KoBeWi KoBeWi force-pushed the ultimate_final_solution_for_containers branch from a15564f to 02e1e6d Compare June 3, 2024 18:01
@KoBeWi
Copy link
Member Author

KoBeWi commented Jun 3, 2024

Now it does.

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

LGTM, all the get_child cases match cleanly and code looks good

@akien-mga akien-mga merged commit a6bb8b0 into godotengine:master Jun 7, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

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