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

Make the TabContainer node use Tabs internally, instead of its own implementation #2514

Closed
YeldhamDev opened this issue Mar 27, 2021 · 2 comments · Fixed by godotengine/godot#58687
Milestone

Comments

@YeldhamDev
Copy link
Member

Describe the project you are working on

The Godot Engine™.

Describe the problem or limitation you are having in your project

Currently, TabContainer has its own tab system implementation, one which basically copies everything from Tabs, which means that there is two implementations of the exact same thing, and both need to be updated when adding functionality. This adds unnecessary extra work for their maintenance.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

The solution would be making TabContainer use Tabs internally, removing the need of two different implementations. That, however, is more complex than it seems, as written in the next section.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

Basically, it would happen by adding a Tabs node as a child of TabContainer, remove the existing code, and pass the commands to that instead. But this brings some problems:

  • When using TabContainers, the workflow used by people (and Godot's own codebase) is by using child manipulation functions (add_child(), get_index(), get_child_count(), etc) with it, since it's a Container derived node. The problem is that adding Tabs to it shoves a wrench in the machine, since those functions can't be relied upon anymore without a bit of extra hacking. This problem could be completely solved if Add support for internal nodes godot#30391 was to be merged.
  • As the current implementation is a copy of Tabs, TabContainer also has the same majority of functions and enumerations that the former has, including theme styling. What should be done about that? Just make those link to the Tabs node internally, which means two things still need to be updated, even if to a lesser extent? Or expose the node itself, which would open a complete new can of worms?
  • The act of dragging and dropping tabs. While I didn't do much digging on that, I assume things will be trick to manage there, since the dragging won't happen in the node itself.

There's the possibility that more problems could come up later, but you get the point. I'm trying to tackle this problem since a long time, but doubts on how to deal with those hurdles stops me in my tracks, so I opened this proposal as a form of discussion on how this could be done (and if the task is worth the trouble to begin with).

If this enhancement will not be used often, can it be worked around with a few lines of script?

It's something internal, so no.

Is there a reason why this should be core and not an add-on in the asset library?

It's about a core feature itself.

@YuriSizov
Copy link
Contributor

YuriSizov commented Mar 27, 2021

Currently, TabContainer has its own tab system implementation, one which basically copies everything from Tabs, which means that there is two implementations of the exact same thing, and both need to be updated when adding functionality. This adds unnecessary extra work for their maintenance.

I would actually call them annoyingly different, which is why we have issues like #2287, #2250, and probably some others in the same vein.

I think I've addressed all 3 of your problems in #2129 (comment), but to reiterate:

  1. TabContainer actually tracks tabs separately, partially because for its popup functionality nodes can be added to it while not representing any tabs. It can indeed cause problems in user code, as it does with other complex nodes. I think since we already have that problem for a bunch of existing nodes we shouldn't shy from relying on the system here, even though it can be broken. Add support for internal nodes godot#30391 is the proper fix and we can just assume that it's in place and move on with the implementation relying on internal nodes.

I would also add that if we are to rely on Tabs internally, we'd need to override add_child/remove_child anyway to create and remove tabs, so we can safeguard more cases from user mistakes along with it.

  1. I think we can feed as much as possible to Tabs (or rather TabBar as I would rename it), as long as it makes sense for an general purpose node, and we can expose it via a get_tabbar command. We do it for other nodes too, like OptionButton having a PopupMenu child. Otherwise I refer to Remove the Tabs node in favor of TabContainer #2129 (comment) again, because there I've listed precisely what can be done with each part of both classes.

  2. As mentioned in the link, the main problem would be syncing tabs being rearranged in Tabs with children in TabContainer. However, I think we actually have that problem unsolved right now, because Tabs give no indication to the user if their tabs are moved, or even dragged into another Tabs control. At least I don't see any way to react to it. So a couple of signals is needed here to give whoever is listening a nod if a tab is being moved from Tabs and if a tab is being added to Tabs. Listeners, such as TabContainer can then remove or add child nodes as necessary.

@Calinou Calinou changed the title Make TabContainer use Tabs internally, instead of its own implementation Make the TabContainer node use Tabs internally, instead of its own implementation Nov 13, 2021
@akien-mga akien-mga added this to the 4.0 milestone Mar 4, 2022
@akien-mga
Copy link
Member

Implemented by godotengine/godot#58687.

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

Successfully merging a pull request may close this issue.

3 participants