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

Defer tab_changed signal when removing children from TabContainer nodes #63176

Closed

Conversation

YeldhamDev
Copy link
Member

@YeldhamDev YeldhamDev commented Jul 19, 2022

Reverts #63145 by resolving the problem with a different approach.

While it still removes the tab immediately, the signal emission itself is deferred. This means functions that do operations which modify the tab bar still work correctly when called right after remove_child(child), while stuff that relies on the signal also works as well.

This implementation also avoids a bug from the older version when doing a operation that affect the tab bar right when the tab_changed signal is called.

I tested it with the bugs closed by the previous PR and they seemed to remain fixed, but I remember also not being able to reproduce the bug even before the old fix, so more testing is required.

@YeldhamDev YeldhamDev added this to the 4.0 milestone Jul 19, 2022
@YeldhamDev YeldhamDev requested a review from a team as a code owner July 19, 2022 01:49
@YeldhamDev YeldhamDev changed the title Defer tab_changed signal when removing children from TabContainernodes Defer tab_changed signal when removing children from TabContainer nodes Jul 19, 2022
scene/gui/tab_container.cpp Outdated Show resolved Hide resolved
@KoBeWi
Copy link
Member

KoBeWi commented Oct 24, 2022

When you remove multiple tabs, the signal is queued for each removal and then fired all at once. But idk if it has undesired effect 🤔

@YeldhamDev
Copy link
Member Author

When you remove multiple tabs, the signal is queued for each removal and then fired all at once. But idk if it has undesired effect

Really can't think of a better solution. The only thing that would truly take care of all corner cases is if there was a callback for just after the child was removed.

@YeldhamDev
Copy link
Member Author

@KoBeWi One thing that could be done is go back to the current solution, but use an array instead of a single-node variable. Feels hacky, but I think that would solve the previous problem without firing the signals all at once.

@KoBeWi
Copy link
Member

KoBeWi commented Oct 25, 2022

Or you can add queue_tab_changed() function that ensures the signal is emitted once.

@YeldhamDev
Copy link
Member Author

That would still make the signal always fire at the last moment of the frame, and this may be undesirable when doing further modifications to the tabs all at once.

@akien-mga
Copy link
Member

CC @Rindbee.

So which PR do we favor between this one and #67928?

@Rindbee
Copy link
Contributor

Rindbee commented Oct 31, 2022

I prefer #67928, there are multiple signals emitted during the remove_tab call, I'm not sure if they can be blocked from being emitted.

@akien-mga
Copy link
Member

Superseded by #67928.

@akien-mga akien-mga closed this Nov 2, 2022
@YeldhamDev YeldhamDev deleted the tab_change_removal_defer branch November 2, 2022 21:31
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.

4 participants