fix: Adds a check to confirm the component is not loaded

[munsterlander]

Description

This fixes #2563 by validating that a component is mounted and the child component is not mounted before adding it to the queue.

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

Closes #2563

[munsterlander]

@spydon I am going to need some help on this one. The code works fine except for its breaking the DoubleTapCallbacks,HasDraggableComponents, and HasTappableComponents tests. Some of their tests fail and others do not. None of these tests use onDetach, so I am at a bit of a loss as to why it would be doing that.

Edit: I have confirmed that they (for some reason) do hit the FlameGame onDetach method and I don't know why these three dispatchers would do that: DoubleTapDispatcher, MultiDragDispatcher, MultiTapDispatcher.

[spydon]

Like I wrote on discord (so others can discuss too):

The components shouldn't be removed, when the game is attached again onLoad shouldn't run. So when the game instance is used again it is in exactly the same state as when it was detached.

[munsterlander]

Like I wrote on discord (so others can discuss too):

The components shouldn't be removed, when the game is attached again onLoad shouldn't run. So when the game instance is used again it is in exactly the same state as when it was detached.

I realized late last night that I was being dumb in my implementation and forgot to focus on processLifecycleEvents onDetach. I actually think there is a larger, albeit different issue though and will look at it now. Regardless, it is very interesting that these tests call FlameGame onDetach - I don't think that is expected behavior.

[spydon]

This looks like it! Could you add some test for it? :)

[munsterlander]

Ok everyone, I finally have this solved. I have learned way more about the Flame engine internals than I had ever planned on, lol. To summarise:

The issue has nothing to do with what it seemed like on the surface. In short, the _reAddChildren() method is the culprit, because it recalls _addChild onto itself. No big deal, but after the game and all components have been loaded and mounted, the component class that is called from the FlameGame.mount, is already mounted, thus isMounted AND isLoaded are true, BUT because _reAddChildren() clears the children, child.isMounted is still true and should not be added back to the mounting queue. Thus we simply need to check for if (isMounted && !child.isMounted) { and everything works including the tests.

[munsterlander]

This looks like it! Could you add some test for it? :)

I don't even know how to begin testing these scenarios. I would think quite the contrary that the existing tests, caught when it was wrong. To test this, you would have to have access to the internals that aren't exposed and you can't assert because its a valid scenario, so honestly, I am at a loss as to where to start.

[spydon]

This looks like it! Could you add some test for it? :)

I don't even know how to begin testing these scenarios. I would think quite the contrary that the existing tests, caught when it was wrong. To test this, you would have to have access to the internals that aren't exposed and you can't assert because its a valid scenario, so honestly, I am at a loss as to where to start.

You should only have to make sure that onLoad only runs once when a component is re-added right, you could just create a component with a counter in onLoad for that?
Something like this:
https://github.com/flame-engine/flame/blob/main/packages/flame/test/components/component_test.dart#L1319

[munsterlander]

This looks like it! Could you add some test for it? :)

I don't even know how to begin testing these scenarios. I would think quite the contrary that the existing tests, caught when it was wrong. To test this, you would have to have access to the internals that aren't exposed and you can't assert because its a valid scenario, so honestly, I am at a loss as to where to start.

You should only have to make sure that onLoad only runs once when a component is re-added right, you could just create a component with a counter in onLoad for that? Something like this: https://github.com/flame-engine/flame/blob/main/packages/flame/test/components/component_test.dart#L1319

Test has been added. It's a pretty long test, but it was the only way I could really validate all the states and truly confirm that the game was detached, the child remained, and then when reloaded, onLoad is not happening again and the child is being restored to its state that it was previously as we discussed.

So for anyone else reading, the expectation is that when I detach a flame game from the flutter widget tree and then reattach it, everything is exactly as it was prior to being detached. When the game is detached, the gameLoop is disposed.

[munsterlander]

Ok, during the testing of the induced failing tests for Tap and Drag callbacks, I learned that when you remove the game from the Flutter widget tree, you aren't removing (unmounting / detaching) the FlameGame or Game class themselves, but you are detaching the GameRenderBox. Just like components, you would expect the FlameGame to remember its state, so it should not dismount itself as I previously committed and thought. It's a component, it should stay exactly like the other children. Thus the code was fine with my original fix, but my test was wrong. We can now confirm the right state by simply checking to see if the game is attached or not.