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: AlignComponent set child (remove compare) #2774

Merged
merged 3 commits into from Oct 2, 2023

Conversation

denisgl7
Copy link
Contributor

At the moment, I have removed the additional check, because an object deleted outside its parent may not be checked and the reference is not reset in the onChildrenChanged method

@@ -70,9 +70,6 @@ class AlignComponent extends PositionComponent {
PositionComponent? get child => _child;

set child(PositionComponent? value) {
if (_child == value) {
Copy link
Member

@spydon spydon Sep 26, 2023

Choose a reason for hiding this comment

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

Can you explain more why this needs to be removed? Even if a child is removed with removeFromParent it should go through this lifecycle method.

Copy link
Contributor Author

@denisgl7 denisgl7 Sep 26, 2023

Choose a reason for hiding this comment

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

  @mustCallSuper
  @override
  void onChildrenChanged(Component child, ChildrenChangeType type) {
    print('onChildrenChanged: $child, $type');
    if (_child?.parent != this) {
      this.child = null;
    }
  }

I have never seen a print of this method

Of course you need to understand the problem. But problem is definitely not on the side of this component. Unfortunately, I don’t yet know all the intricacies of the framework. Perhaps the method is not called because we can add a child to the same parent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would also like to add that these changes to the class will not affect performance, but will make this component work well.

Copy link
Member

Choose a reason for hiding this comment

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

We should open a separate issue about this if it doesn't work, I'll merge this meanwhile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree

@spydon spydon enabled auto-merge (squash) October 2, 2023 11:09
@spydon spydon merged commit 20aaf65 into flame-engine:main Oct 2, 2023
7 checks passed
@denisgl7 denisgl7 deleted the align_component_small_fix branch October 31, 2023 07:38
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