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

Save current tab in TabBar and TabContainer #83893

Merged
merged 1 commit into from
Oct 27, 2023

Conversation

kitbdev
Copy link
Contributor

@kitbdev kitbdev commented Oct 24, 2023

fixes #50930

Removed PROPERTY_USAGE_EDITOR from current_tab.
For TabBar, moved tabs array above other properties (like how OptionButton does it).
For TabContainer, delayed setting current_tab until ready so children are added first.

@kitbdev kitbdev requested a review from a team as a code owner October 24, 2023 16:06
@AThousandShips
Copy link
Member

I'd say the rearranging of the tabs is a separate thing and not relevant to the specific change of exposing the property for storage, this should be split into multiple PRs IMO

@kitbdev
Copy link
Contributor Author

kitbdev commented Oct 24, 2023

@AThousandShips In TabBar moving the tabs array to be above is necessary for this pr so it loads in the correct order, or we would get an issue like #10213. If wanted I can add an explanatory comment like was done for that fix (#16739 (comment)), something like // "current_tab" property must come after "tab_count", otherwise the property isn't loaded correctly.

For the TabContainer's tab_alignment, that didn't need to be moved in this pr so I could split it into a separate pr or just not do it at all.

@AThousandShips
Copy link
Member

That would indeed be required, as you didn't mention it in the PR either

The rest should be separate to focus the review on just the issue

@kitbdev
Copy link
Contributor Author

kitbdev commented Oct 24, 2023

added the comment and undid the tab_alignment move.

@AThousandShips
Copy link
Member

AThousandShips commented Oct 24, 2023

Does it only have to be above the current tab? If so I'd say to move it instead, like the other PR did

I'd prefer as little changes as possible to solve the issue, generally the idea

@kitbdev
Copy link
Contributor Author

kitbdev commented Oct 24, 2023

In OptionButton the array is at the top of all properties, so that's what I did here. I didn't want to have the array in between the properties since it's cleaner when they're together, but I could since it only needs to be above current_tab.

@AThousandShips
Copy link
Member

AThousandShips commented Oct 24, 2023

Just make sure it doesn't affect any other properties (hence why moving only the relevant one is safer, and preferred)

@kitbdev
Copy link
Contributor Author

kitbdev commented Oct 24, 2023

It looks like no properties are affected by tab count and tab count is not affected by any other properties. _update_cache() and similar are either only called when not in the tree or or called everywhere they are affected, so the order doesn't matter for them.

@kitbdev
Copy link
Contributor Author

kitbdev commented Oct 24, 2023

made ensure_tab_visible run out of the tree so that it properly handles the saved current_tab.

scene/gui/tab_container.cpp Outdated Show resolved Hide resolved
scene/gui/tab_bar.cpp Outdated Show resolved Hide resolved
scene/gui/tab_container.h Outdated Show resolved Hide resolved
@akien-mga akien-mga merged commit ad5ebd7 into godotengine:master Oct 27, 2023
15 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
Development

Successfully merging this pull request may close these issues.

TabContainer resets visibility to first tab in editor
4 participants