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 new FoldableContainer node. #101487

Closed

Conversation

WhalesState
Copy link
Contributor

@WhalesState WhalesState commented Jan 13, 2025

Screencast.from.01-15-2025.02.49.58.PM.webm

@WhalesState

This comment was marked as outdated.

@WhalesState WhalesState marked this pull request as draft January 13, 2025 12:22
Copy link
Contributor

@RedMser RedMser left a comment

Choose a reason for hiding this comment

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

Looking forward to this! I only reviewed the API and docs.

Once the PR leaves draft, it would be nice to have a screenshot/video not only of the node in game (with default theme), but also how it looks styled in the editor (e.g. when used in an EditorPlugin).

doc/classes/FoldableContainer.xml Outdated Show resolved Hide resolved
doc/classes/FoldableContainer.xml Outdated Show resolved Hide resolved
scene/gui/foldable_container.h Outdated Show resolved Hide resolved
doc/classes/FoldableContainer.xml Outdated Show resolved Hide resolved
doc/classes/FoldableContainer.xml Outdated Show resolved Hide resolved
doc/classes/FoldableContainer.xml Outdated Show resolved Hide resolved
doc/classes/FoldableContainer.xml Outdated Show resolved Hide resolved
doc/classes/FoldableContainer.xml Outdated Show resolved Hide resolved
@WhalesState

This comment was marked as outdated.

@WhalesState WhalesState force-pushed the foldable-container branch 2 times, most recently from 1375758 to d9a288c Compare January 13, 2025 21:48
@WhalesState

This comment was marked as outdated.

@WhalesState WhalesState force-pushed the foldable-container branch 4 times, most recently from 5a151b7 to 1c95209 Compare January 14, 2025 00:54
@WhalesState WhalesState marked this pull request as ready for review January 14, 2025 00:54
@WhalesState

This comment was marked as resolved.

@WhalesState WhalesState force-pushed the foldable-container branch 3 times, most recently from cc834a4 to 50fc4a0 Compare January 14, 2025 01:41
@WhalesState

This comment was marked as resolved.

@raulsntos
Copy link
Member

The property FoldableContainer::text_direction conflicts with the enum Container::TextDirection because in C# property names are converted to PascalCase. It seems we already have similar cases for other Control nodes, and we just suppress the warning. See:

const Vector<String> prop_allowed_inherited_member_hiding = {
"ArrayMesh.BlendShapeMode",
"Button.TextDirection",
"Label.TextDirection",
"LineEdit.TextDirection",
"LinkButton.TextDirection",
"MenuBar.TextDirection",
"RichTextLabel.TextDirection",
"TextEdit.TextDirection",

@ckaiser
Copy link
Contributor

ckaiser commented Jan 14, 2025

Did some testing, I'm liking the direction it's going, some thoughts and notes as I was trying it out:

There's no simple way to adjust the sizing of the buttons, which means you have to control that with the icon, which could be cumbersome, IMO. A theme override for icon_max_width like Button has would suffice.

"Toggle On" is inconsistent with how BaseButton refers to toggle buttons: "Button Pressed"

API wise, the fact that expanded is the property you set/get and folding_changed(is_folded) is the signal seems inconsistent.
There's also the problem that the Node type already contains references to folding, so I can totally see a user trying to use node.is_displayed_folded() with autocomplete or something like that.
It might be better to just use expanded in all cases, problem with that is that it leaves the name of the node as the only use of the word "fold". I usually see these kinds of widgets also be referred to as "accordion" widgets (and indeed, having functionality to link multiple of these together like ButtonGroup would be nice eventually).

Found a weird bug when testing it in an EditorPlugin, when inside the bottom panel the contents of the foldable container expand to include the title, see here:
image

If I fold the container and then expand it, it behaves normally afterwards, and running the scene makes it behave normally, so it's got to be a weird quirk of either the editor theme or the plugin initialization, I'm attaching an MRP so you can test it: foldable-mrp.zip.

Haven't looked at the code or documentation, but good work so far! This will be very useful for editor UI, plugins, configuration, etc.

@WhalesState WhalesState requested a review from a team as a code owner January 15, 2025 04:31
@WhalesState
Copy link
Contributor Author

WhalesState commented Jan 15, 2025

Updated the code and added FoldableGroup resource which will allow expanding only one container at a time.
Added fold() and expand() helpers that will emit folding_changed
Added set_expanded and is_expanded as helpers for readability because !folded is a bit confusing, we also have set_folded and is_folded as the main setter/getter.

Thanks for all the suggestions.

Screencast.from.01-15-2025.06.32.20.AM.webm

Also updated the testing project foldable-test.zip.

@WhalesState
Copy link
Contributor Author

WhalesState commented Jan 15, 2025

There's no simple way to adjust the sizing of the buttons, which means you have to control that with the icon, which could be cumbersome, IMO. A theme override for icon_max_width like Button has would suffice.

I don't know how this should work, maybe we can note that all icons must have the same size.

"Toggle On" is inconsistent with how BaseButton refers to toggle buttons: "Button Pressed"

Buttons groups will be tricky to implement, currently it can be managed from code when the pressed/toggled signal emits.

I usually see these kinds of widgets also be referred to as "accordion" widgets (and indeed, having functionality to link multiple of these together like ButtonGroup would be nice eventually).

Added FoldableGroup resource.

Found a weird bug when testing it in an EditorPlugin, when inside the bottom panel the contents of the foldable container expand to include the title

Fixed and updated the test project to include more examples.

doc/classes/FoldableGroup.xml Outdated Show resolved Hide resolved
doc/classes/FoldableGroup.xml Outdated Show resolved Hide resolved
@WhalesState
Copy link
Contributor Author

I also want to allow popups support for buttons, I can add button_get_popup_position, which returns the correct screen position for the popup and will not store the popups for each button, users will be able to show/hide them freely when button_pressed or button_toggled emits. Because buttons are not nodes and can't easily get the screen position or transform for them.

@WhalesState WhalesState force-pushed the foldable-container branch 3 times, most recently from d37fe3b to a93bad9 Compare January 15, 2025 12:23
@WhalesState
Copy link
Contributor Author

Updated.

Screencast.from.01-15-2025.02.49.58.PM.webm

@WhalesState
Copy link
Contributor Author

WhalesState commented Jan 17, 2025

This is how the ColorPicker looks now in Blazium Engine, because they have merged FoldableContainer and ColorButton new nodes and also BaseButton.SizeMode that i have used for most of the ColorPicker buttons.

I'm looking forward to improve the Godot's Control nodes too but most of my PRs either ignored or marked as 4.x even if they do/can fix bugs.

Screencast.from.01-17-2025.12.24.07.PM.webm

@WhalesState WhalesState force-pushed the foldable-container branch 2 times, most recently from 674b3f3 to 1c3eec6 Compare January 17, 2025 11:22
@WhalesState WhalesState marked this pull request as draft January 28, 2025 15:13
@AThousandShips AThousandShips removed this from the 4.x milestone Jan 28, 2025
@RedMser
Copy link
Contributor

RedMser commented Jan 28, 2025

I'm looking forward to improve the Godot's Control nodes too but most of my PRs either ignored

There's way more PRs being opened than there are reviewers and maintainers. I'm just patient and let my changes sit, while I work with the feature in a personal fork.

marked as 4.x even if they do/can fix bugs.

You can ask in chat to have it re-triaged (a PR of mine recently converted from enhancement to bug for example, which moves its milestone to the next minor version). If you can split your PR into one part bugfix another part enhancement, it's the best way to get it moving forward quicker.

@WhalesState
Copy link
Contributor Author

I'm patient, but a year is too much for a bug fix that exists since 3.x.

Does Godot really needs a reminder for a PR that fixes a bug?

Is it written anywhere that I should go to a chat platform to wish for a bug that i have fixed to be merged in the next release?

@WhalesState
Copy link
Contributor Author

I did my best to work on another community driven fork and Godot at same time, fixing bugs here and adding features there because i know Godot will ignore my PRs if they add a feature and the 4.x milestone will be added by ATS.

You can See what i could have done to the Godot community. This is editing the fallback theme directly from project settings and works in real time even inside game, This could have been even better if the godot devs have helped me, great thanks to KoBeWi, Akien, kitbdev and many other devs that have helped me a lot, even AThousandShips have helped me a lot in the past and many thanks to her too.

2025-01-20.11-37-51.online-video-cutter.com.mp4

This is a new collapse mode and is similar to VSCode split.

2025-01-25.13-47-39.online-video-cutter.com.mp4

Hope Godot survives all of this because I believe-in and respect all your hard-work, but i also have wished for some respect for my work too.

@Repiteo
Copy link
Contributor

Repiteo commented Jan 28, 2025

I can't help but feel there's been quite a few fundamental misunderstandings here.


I did my best to work on another community driven fork and Godot at same time, fixing bugs here and adding features there because i know Godot will ignore my PRs if they add a feature and the 4.x milestone will be added by ATS.

Being put in the 4.x Milestone only means that it's not an immediate priority for a given version. All non-bugfix PRs are going to be in this milestone because we're entered the beta period for 4.4; that is, we've entered a feature freeze. It is quite literally not a priority to review any PRs that aren't addressing bugs, it'd be a complete waste of finite resources/energy.


This could have been even better if the godot devs have helped me, great thanks to KoBeWi, Akien, kitbdev and many other devs that have helped me a lot, even AThousandShips have helped me a lot in the past and many thanks to her too.

It certainly reads as if quite a lot of the godot devs did indeed help you. In this PR we see exactly that as well, with no real detraction to speak of. If this is annoyance at not being prioritized for a merge, then I must re-emphasize the earlier point of this type of PR not being the team's priority atm. Hell, I have ~70 open PRs as we speak! But I also have over double that amount merged, because even though the due-process takes time, it will get done with time.


Is it written anywhere that I should go to a chat platform to wish for a bug that i have fixed to be merged in the next release?

As a matter of fact, yes! Here's the explicit exerpt touching on this very concern:

Contribute Engine Code (mainly C++) The engine development is mainly coordinated on our Contributor RocketChat, so if you are serious about making PRs you should join us there!

You're free to believe that this isn't elevated/visible enough — if so, you can always make an enhancement request in the docs repo — but it's not something we're deliberately obfuscating to make the PR process more tedious.


Hope Godot survives all of this because I believe-in and respect all your hard-work, but i also have wished for some respect for my work too.

If you have any examples of being disrespected/harassed in this repo, do not hesitate to reach out to our Code of Conduct team with those concerns. If this is about this PR not being reviewed in what feels like a timely manner, then I'll state as unambiguously as possible: that's not disrespect. I get being personally attached to your work & wanting to see it reach as many people as possible, but a review of your work — or lack thereof — is not, and never has been, a personal statement on the PR itself. Reviews are most often a direct consequence of contributors putting their finite time/resources into analyzing areas they're passionate/skilled in, and those are not a guarantee — much less something owed.

@KoBeWi
Copy link
Member

KoBeWi commented Jan 28, 2025

I was in the middle of checking this PR when you closed it, so nice timing. If you don't plan on working on it anymore I might take over. The implementation looks solid and the container has potential to be used in editor (inspector) and other controls (ColorPicker). Although I find the button part too complex; what I'd do instead is allow adding Button nodes to the title bar, similar to another proposal for TabContainer.

@WhalesState
Copy link
Contributor Author

Please feel free to cherry-pick any of my commits and i know you can make the code much better.

@WhalesState
Copy link
Contributor Author

I was in the middle of checking this PR when you closed it, so nice timing. If you don't plan on working on it anymore I might take over. The implementation looks solid and the container has potential to be used in editor (inspector) and other controls (ColorPicker). Although I find the button part too complex; what I'd do instead is allow adding Button nodes to the title bar, similar to another proposal for TabContainer.

it's just missing a way to make a popup appears when a title button is pressed and i don't know an easy way to implement it.

@WhalesState
Copy link
Contributor Author

WhalesState commented Jan 29, 2025

@Repiteo

Sorry about the late reply, I don't say anyone is wrong, because i know the team is busy but at least we can give those PRs more attention while adding their milestone. Few lines fixing clear bugs should be prioritized since it will allow faster progress and will not cause much conflict, shouldn't take about 10 months.

It makes me feel unwelcome when my PRs are ignored, similar to many other devs who also hates being ignored, most of the PRs are just few lines and fixing Interface bugs, Its ok for a feature requests to be ignored like This PR, but I just wanted more attention and help on fixing those issues in the best possible way instead of killing them and cause many conflicts over time.

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.

Add FoldableContainer Node
7 participants