godot icon indicating copy to clipboard operation
godot copied to clipboard

Add `as_sortable_control()` to unify Container checks

Open KoBeWi opened this issue 9 months ago • 2 comments

There are lots of repetitive checks in each Container for node being a visible non-top-level Control. This PR adds a method to unify it. Now every such condition will check for is_visible_in_tree() and is_top_level() (previously it was inconsistent, like checking is_visible() instead, or omitting one of the checks). Maybe it's a bit awkward that it returns Control or null, but it makes the code simpler.

Right now the method is not exposed, but it would be useful for users making custom Containers, so it can be considered.

KoBeWi avatar May 06 '24 10:05 KoBeWi

There is also SplitContainer, GraphElement, GraphNode, GraphFrame, and TabContainer which could use this. Though TabContainer needs invisible children. So, it could be made virtual and overridden so it can include invisible children itself.

This fixes an issue with GridContainer where top level children are sorted when they shouldn't be.

Also, #91422 adds a complementary Control::is_managed_by_container(), which could make use of this.

kitbdev avatar May 06 '24 14:05 kitbdev

There is also SplitContainer, GraphElement, GraphNode, GraphFrame, and TabContainer which could use this.

Right, I'll add them too.

Though TabContainer needs invisible children. So, it could be made virtual and overridden so it can include invisible children itself.

I think it this case it could just do a manual check.

KoBeWi avatar May 06 '24 15:05 KoBeWi

Added changes to remaining classes except TabContainer. Also Graph nodes seem to use invisible children for calculating minimum size, so I left that too (not sure if it's intended though).

KoBeWi avatar May 08 '24 09:05 KoBeWi

Thanks!

akien-mga avatar May 08 '24 10:05 akien-mga