fix: Prevent onRemove/onDetach being called for initial Gesture Detector addition

[munsterlander]

Description

This fixes: #2647
Reverts: #2602
OBE: #2650

To be clear, the memory leak issue is not addressed because it is not a Flame issue. Flame properly calls the onRemove event and if it is needed to pass that on to children, users should do as documented: https://docs.flame-engine.org/latest/flame/game.html#lifecycle.

So if you are using GameWidget.controlled, add a tappable component and then remove the game, the lifecycle looks like:

flutter: onLoad
flutter: mount
flutter: onMount
flutter: attach
flutter: add has_tappable_components
flutter: refreshWidget 
// Remove Game
flutter: detach
flutter: removeGameStateListener
flutter: onRemove
flutter: remove

If you do not use GameWidget.controlled, add a tappable component and then remove the game, the lifecycle looks like:

flutter: onLoad
flutter: mount
flutter: onMount
flutter: attach
flutter: add has_tappable_components
flutter: refreshWidget
// Remove Game
flutter: detach
flutter: removeGameStateListener
flutter: onRemove
flutter: remove

If you do not use GameWidget.controlled, do not add a tappable component and then remove the game, the lifecycle looks like:

flutter: onLoadFuture
flutter: mount
flutter: onMount
flutter: attach
// Remove Game
flutter: detach
flutter: removeGameStateListener
flutter: onRemove
flutter: remove

Previously, the lifecycle would look like:

flutter: onLoad
flutter: mount
flutter: onMount
flutter: attach
flutter: add has_tappable_components
flutter: refreshWidget 
flutter: detach // These unnecessary calls have been eliminated
flutter: removeGameStateListener // These unnecessary calls have been eliminated
flutter: onRemove // These unnecessary calls have been eliminated
flutter: attach
// Remove Game
flutter: detach
flutter: removeGameStateListener
flutter: onRemove
flutter: remove

I have updated the below diagram for those who may need it in the future:

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

[spydon]

I think this can be done in a simpler manner by using the refreshWidget method that is passed in as a callback to GestureDetectorBuilder in Game and add a flag that says that it is an internal refresh and then when that flag is set just skip calling the methods that you are skipping now too:

  @internal
  bool isInternalRefresh = false;

  @internal
  void refreshWidget() {
    isInternalRefresh = true;
    gameStateListeners.forEach((callback) => callback());
    isInternalRefresh = false;
  }

What do you think?

flutter: onLoad
flutter: mount
flutter: onMount
flutter: attach
flutter: add has_tappable_components
flutter: refreshWidget
flutter: detach // These unnecessary calls have been eliminated
flutter: removeGameStateListener // These unnecessary calls have been eliminated
flutter: onRemove // These unnecessary calls have been eliminated
flutter: attach

Shouldn't the last attach call here also be removed?

[munsterlander]

@spydon This was my original plan of attack and I even went a step further by trying to identify the callback to check and see if it was a gesture detector, but the callback name is an internal id to Flutter (or maybe I don't know enough to get a simpleName or such).

refreshWidget is also called by the overlay manager and the mouseCursor, so not knowing the full lifecycle of those items, I didn't want to do a blanket override. I also wanted to do refreshWidget with an optional named parameter, but didn't know how to pass true for an invocation such as: late final GestureDetectorBuilder gestureDetectors = GestureDetectorBuilder(refreshWidget)..initializeGestures(this);

I am open to this way so any pointers are greatly appreciated!

As far as the last attach, I thought about that but with what I implemented, GRB was being set to null, so then it wouldn't be available to read the flag in onAttach. Having said that, if we can set the flag in Game and base it off of the refreshWidget, this can be overcome.

Edit: And I would say, I was originally looking at stopping the events in the GRB, but learned that is not a smart approach, so technically, there should be no reason why the flag cannot exist in Game as all detectors have a game reference.

Edit2: I have implemented a version of your suggestion and tested everything to confirm it still works as anticipated. All tests pass and the event lifecycle now removes the extra onAttach call. Its marked internal and can be overridden by passing a parameter in future additions to Flame. I have updated the description and the diagram.

[munsterlander]

Analyzer failed after direct committing your changes from GitHub. When pulled, its passing local. Let me update branch and see what happens.

[spydon]

Analyzer failed after direct committing your changes from GitHub. When pulled, its passing local. Let me update branch and see what happens.

How strange, the warning wasn't even about any code that you have touched. 🤷‍♂️

[munsterlander]

Analyzer failed after direct committing your changes from GitHub. When pulled, its passing local. Let me update branch and see what happens.

How strange, the warning wasn't even about any code that you have touched. 🤷‍♂️

Yeah, I pulled the last updates down, but I can't figure out what is going on. I can't see the logs or I don't know how to see the Analyzer logs in GitHub. What am I missing on this?

OH! The little triangle. Ok, I see the issue. So with your suggested name change, it just needed to be updated in the other file.

[spydon]

OH! The little triangle. Ok, I see the issue. So with your suggested name change, it just needed to be updated in the other file.

Yeah that was one thing, but there also was a warning about a file in Jenny. Let's see if that warning pops up again now.

[munsterlander]

class _RecordingDialogueViewAsMixin extends _SomeOtherBaseClass
    with DialogueView {
  _RecordingDialogueViewAsMixin([this.waitDuration = Duration.zero]);
  final List<String> events = [];
  final Duration waitDuration;

This is the warning.