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 an option to always have the tabs visible, if a panel has more than one tab #27

Merged
merged 3 commits into from
Feb 18, 2024

Conversation

OverloadedOrama
Copy link
Contributor

@OverloadedOrama OverloadedOrama commented Dec 4, 2023

This new option ensures than a dockable panel will have its tabs visible, even when tabs_visible = false, when it has more than one tab.

image

Above is a panel with just one tab, and below is a panel with two tabs. This happens when tabs_visible = false and tabs_visible_if_more_than_one = true.

This is something we did in Pixelorama 0.x, because we wanted to avoid tabs being visible when unnecessary, but I never ported the change here so I thought I might as well do that.

This PR also removes set_tab_alignment() and get_tab_align(), as they were unused because the tab_alignment variable already has a getter and setter.

One thing I'm not too sure about is the variable name tabs_visible_if_more_than_one. I feel like this might be a bit too long (although use_hidden_tabs_for_min_size is also about the same length), but I'm not sure how to make it shorter while also ensuring that its purpose is clear.

And I'm also not sure about calling set_tabs_visible() in _resort(). There may be a more optimal way to do this, but without this, tab visibility isn't being updated when a panel gets added/removed/hidden.

OverloadedOrama added a commit to Orama-Interactive/Pixelorama that referenced this pull request Jan 12, 2024
Bringing this feature back from 0.x. Not very happy that we're once again not synced with upstream dockable_container, so hopefully gilzoide/godot-dockable-container#27 is merged soon.
@gilzoide
Copy link
Owner

Hey @OverloadedOrama , nice stuff, thanks for the addition! Sorry it took me so long to check this PR.

To be honest, for me it makes more sense to hide a single tab, even if tabs_visible == true, instead of showing multiple tabs even if tabs_visible == false. I mean, instead of tabs_visible_if_more_than_one, maybe something like single_tab_visible or hide_single_tab would be better.
What do you think?

This PR also removes set_tab_alignment() and get_tab_align(), as they were unused because the tab_alignment variable already has a getter and setter.

Awesome, thank you!

And I'm also not sure about calling set_tabs_visible() in _resort(). There may be a more optimal way to do this, but without this, tab visibility isn't being updated when a panel gets added/removed/hidden.

Hmm, I think the logic over hiding/showing tabs should be done in DockablePanel instead of the DockableContainer: the container simply forwards the options to the panels, which are responsible for their own tabs. This logic would likely live in the track_nodes method and the options' setter methods.
I wouldn't say it's more optimal, but it seems more organized/clean code-ish this way (either way the flag will be set for every panel during a resort).

@OverloadedOrama
Copy link
Contributor Author

To be honest, for me it makes more sense to hide a single tab, even if tabs_visible == true, instead of showing multiple tabs even if tabs_visible == false. I mean, instead of tabs_visible_if_more_than_one, maybe something like single_tab_visible or hide_single_tab would be better. What do you think?

That sounds good to me, I changed it to follow this logic and named the variable hide_single_tab.

Hmm, I think the logic over hiding/showing tabs should be done in DockablePanel instead of the DockableContainer: the container simply forwards the options to the panels, which are responsible for their own tabs. This logic would likely live in the track_nodes method and the options' setter methods. I wouldn't say it's more optimal, but it seems more organized/clean code-ish this way (either way the flag will be set for every panel during a resort).

Yeah that makes sense, I agree. I moved the logic to DockablePanel and added a new _handle_tab_visiblility method which gets called in the setter methods and in track_nodes, and changes in the DockableContainer tab visibility related variables has been limited to just update the DockablePanel variables.

A slight issue I encountered was that, while the DockableContainer variable is named tabs_visible, the DockablePanel variable can't be named the same way, as it is a built-in variable of TabContainer. So I went ahead and named it tabs_hidden, which has the opposite value of DockableContainer's tabs_visible. I'm not too happy about that but I couldn't think of a better name (I thought of should_tabs_be_visible but that seems too long), so ideas are welcome. Alternatively, if we want the DockableContainer and DockablePanel variables to have the same value, we could change DockableContainer's variable to also be tabs_hidden and negate it, at the cost of a small compatibility breakage. I don't have a strong opinion personally and I'm fine either way.

@gilzoide
Copy link
Owner

So I went ahead and named it tabs_hidden, which has the opposite value of DockableContainer's tabs_visible. I'm not too happy about that but I couldn't think of a better name (I thought of should_tabs_be_visible but that seems too long), so ideas are welcome.

Yeah, tabs_hidden seem a little weird. I mean, it could be that, but it's simpler and clearer to stick to the meaning of tabs_visible, even if we create a new variable for it. I have no problem with long property/method names, but maybe something like "should_show_tabs" or "show_tabs" reads better. Anyway, the DockablePanel, SplitHandle and DragNDropPanel are internal to the library, so it doesn't matter much, there's no reason for users to interface with them anyway.

we could change DockableContainer's variable to also be tabs_hidden and negate it, at the cost of a small compatibility breakage

My original idea was to have the options in DockableContainer be the same as TabContainers, so people would sort of already know what they do. But this is debatable, of course, maybe it shouldn't be that way 🤔

I don't have a strong opinion personally and I'm fine either way.

Yeah, me neither. I'd say keeping tabs_visible is simpler, there's no strong reason to change this right now.

@OverloadedOrama
Copy link
Contributor Author

OverloadedOrama commented Feb 15, 2024

I renamed tabs_hidden to show_tabs. I think it is better now indeed, and since the logic between DockableContainer and DockablePanel's variables isn't being negated anymore, there is no reason to change anything else now.

@gilzoide
Copy link
Owner

I think it is better now indeed

I agree! I've tested your branch here and it seems to be working fine, so I guess we can merge it now =]
Thanks again for the addition!

@gilzoide gilzoide merged commit ddff84a into gilzoide:main Feb 18, 2024
2 checks passed
@OverloadedOrama OverloadedOrama deleted the show-more-than-one-tab branch February 18, 2024 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants