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

Unfocus children in Group#clearChildren() #6423

Merged
merged 5 commits into from Feb 21, 2021

Conversation

petoncle
Copy link
Contributor

Group.removeActor(Actor child, boolean unfocus) gives the option to unfocus the removed child.
Group.removeActor(Actor child) unfocuses the removed child (calls removeActor(child, true)).

For some reason, Group.clearChildren() currently does not unfocus the children. This looks wrong.
This PR adds a clearChildren(boolean unfocus) method.

Now the question is: do you want clearChildren() (no arguments) to unfocus like removeActor(child) does, or do you want clearChildren() to not unfocus to avoid potentially breaking apps which may rely on the fact that clearChildren() never unfocused until now? I'm all for breaking apps, but 🤷.

@petoncle
Copy link
Contributor Author

petoncle commented Feb 18, 2021

After thinking about it some more, I believe not making clearChildren() (no arguments) unfocus its children is not good for me personally as I'll have to change all my clearChildren() calls to clearChildren(true), and most of the times I don't even call clearChildren() but clear().

@NathanSweet
Copy link
Member

Good find. I think clearChildren without specifying unfocus should match removeActor without specifying unfocus: it should unfocus. As @petoncle mentioned, this makes it a breaking change but the API consistency is likely worth it.

Some notes: Having removed actors retain focus can cause errors and surprises. We don't always remove focus though, because sometimes removal is temporary and keeping the focus is desired. Adding an unfocus boolean to a bunch of methods isn't great, but gives the flexibility needed and helps people consider the issue.

@NathanSweet NathanSweet merged commit 909fd8a into libgdx:master Feb 21, 2021
@NathanSweet
Copy link
Member

I wonder, should we have a Group clear(boolean unfocus)?

@petoncle
Copy link
Contributor Author

petoncle commented Feb 21, 2021

I wonder, should we have a Group clear(boolean unfocus)?

We might as well for consistency with the clearChildren and removeActor methods.

@petoncle
Copy link
Contributor Author

@NathanSweet I've added a Group#clear(boolean unfocus) method if you're interested. I think you'll have to re-open the PR to be able to merge it in.

NathanSweet added a commit that referenced this pull request Feb 21, 2021
@NathanSweet
Copy link
Member

No worries, I've made the change.

NathanSweet added a commit that referenced this pull request Feb 21, 2021
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.

None yet

2 participants