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

fix: Check for removing state while adding a child #3050

Merged
merged 3 commits into from
Feb 25, 2024

Conversation

ufrshubham
Copy link
Member

@ufrshubham ufrshubham commented Feb 25, 2024

Description

Adding a child component to a parent which is in removing state, caused the lifecycle processing to go into a cyclic dependency when the parent is re-added. It happens because, while processing the lifecycle events, child's add event causes itself and the parent to get added to the blocked list. As a result of this, when the parent's add event is processed next, it gets skipped due to being in the blocked list.

This PR makes sure that the child does not get enqueued when parent is about to be removed.

Checklist

  • I have followed the Contributor Guide when preparing my PR.
  • I have updated/added tests for ALL new/updated/fixed functionality.
  • I have updated/added relevant documentation in docs and added dartdoc comments with ///.
  • I have updated/added relevant examples in examples or docs.

Breaking Change?

  • Yes, this PR is a breaking change.
  • No, this PR is not a breaking change.

Related Issues

NA

@denisgl7
Copy link
Contributor

Thanks!

@ufrshubham ufrshubham marked this pull request as ready for review February 25, 2024 10:36
Copy link
Member

@spydon spydon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you describe in the PR description what the PR solves?

What happens when enqueeing an add when the parent is removing? Shouldn't it be reconciled at mounting still? 🤔

@ufrshubham
Copy link
Member Author

What happens when enqueeing an add when the parent is removing? Shouldn't it be reconciled at mounting still? 🤔

Due to the sequence of the queue, the child's handleLifecycleEventAdd caused itself and its parent, both to get added to the blocked set here

_blocked.add(identityHashCode(child));
_blocked.add(identityHashCode(parent));

So in the next iteration when the parent's re-add event was encountered, it was getting skipped here:

if (_blocked.contains(identityHashCode(child)) ||
_blocked.contains(identityHashCode(parent))) {
continue;
}

Copy link
Member

@spydon spydon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm!

@spydon spydon merged commit 3a24a51 into main Feb 25, 2024
9 checks passed
@spydon spydon deleted the devkage/removing-parent-add-child branch February 25, 2024 11:02
@ufrshubham
Copy link
Member Author

Posting the MRE just in case anyone is interested:

class RemoveAddGame extends FlameGame with HasKeyboardHandlerComponents {
  final RectangleComponent rectangle = RectangleComponent(
    size: Vector2.all(100),
    position: Vector2.all(100),
  );

  @override
  Future<void> onLoad() async {
    await add(rectangle);
  }

  @override
  KeyEventResult onKeyEvent(
    KeyEvent event,
    Set<LogicalKeyboardKey> keysPressed,
  ) {
    if (event is! KeyDownEvent) {
      return KeyEventResult.handled;
    }

    if (event.logicalKey == LogicalKeyboardKey.keyA) {
      add(rectangle);
    } else if (event.logicalKey == LogicalKeyboardKey.keyR) {
      rectangle.removeFromParent();
    }

    rectangle.add(Component());

    return super.onKeyEvent(event, keysPressed);
  }
}

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.

3 participants