feat: adding has mounted to component #1418

erickzanardo · 2022-03-04T18:48:40Z

Description

Solves #1417

Checklist

The title of my PR starts with a [Conventional Commit] prefix (fix:, feat:, docs: etc).
I have read the [Contributor Guide] and followed the process outlined for submitting PRs.
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.

Breaking Change

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

packages/flame/lib/src/components/component.dart

packages/flame/test/components/component_lifecycle_test.dart

st-pasha · 2022-03-04T21:22:39Z

packages/flame/test/components/component_lifecycle_test.dart

+    flameGame.test('component hasMounted completes', (game) async {
+      final component = _MyComponent();
+      await game.add(component);
+      final hasMounted = component.hasMounted;


I presume it is not possible to await for this future, since it cannot complete without the game loop running its course -- hence this would become an infinite cycle.

Perhaps it would be useful to have at least some test where we await component.hasMounted, a test case that would mimic the intended use case for this feature.

I double checked and the future does completes, otherwise the test would fail. I changed to expectLater to be sure, but even the original code was working.

But can you do await component.hasMounted;? I mean, isn't that how this future is supposed to be used?

Yeah, you can await it, the completes matcher makes sure that it finishes.

I could change my test to make use of the await keyword, but would make the test uglier IMO

The point is, right now we have no tests to check whether this future is actually awaitable.

How come? This test is doing exactly that 🤔

packages/flame/lib/src/components/component.dart

spydon · 2022-03-07T18:45:00Z

packages/flame/lib/src/components/component.dart

+  /// A future that will complete once the component is mounted on its parent
+  Future<void> get hasMounted {
+    if (isMounted) {
+      return Future.value();


Nice solution!

st-pasha · 2022-03-07T19:01:34Z

packages/flame/lib/src/components/component.dart

@@ -403,6 +418,7 @@ class Component {
    if (existingChild || _state == LifecycleState.removed) {
      onGameResize(findGame()!.canvasSize);
    }
+    _mountCompleter?.complete();


Set it to null here?

alestiago · 2022-03-07T20:26:59Z

packages/flame/lib/src/components/component.dart

@@ -203,6 +206,17 @@ class Component {
  /// ```
  void onMount() {}

+  /// A future that will complete once the component is mounted on its parent
+  Future<void> get hasMounted {


I don't know why, but hasMounted doesn't convince me as the method name. If I ever read this I'll expect to get a boolean value from it not a Future<void>. @erickzanardo suggested mounted which I somewhat preferred over hasMounted.

In addition, I have a question on why do we think that using a Completer is better than registering callbacks (for example, whenMounted(callback))? Is this to avoid callback hell? Do we highly recommend always awaiting for hasMounted, hence the Future type? or do we also suggest to unawait if necessary?

I like the mounted name! @spydon what do you think?

About the future vs callback, I think it have a nicer API, and actually, by using a future, we can let the user use a callback if they prefer, by doing component.mounted.wheComplete(fn), so we can have the best of both worlds.

@erickzanardo true. Missed that API!

I think it's nice if we have it as mounted. I love how it reads:

component.mounted.whenComplete
and
await component.mounted

component.mounted still sounds alot like a boolean property, similar to .isMounted. Maybe a better name would be .whenMounted:

await component.whenMounted()

…o erick.has_mounted

* feat: adding has mounted to component * feat: pr suggestions * feat: improving hasMounted * feat: renaming hasMounted to mounted * feat: pr suggestion

* ProcessQueues() method * Added deadQueue in LifecycleManager * refactor removeFromParent() * make shouldRemove non-overridable * Added lifecycle state "removing" * prevent double-removal * shouldRemove can only be set to true * eliminate _shouldRemove * rename _dead -> _dying * added adoption queue * removed nextParent * deprecate ComponentSet.clear and .removeAll * ComponentSet no longer handles removal * remove usages of shouldRemove * doc-comments * onRemove refactor * cleanup * remove obsolete paragraph in docs * rename queue dying -> removals * feat: update examples dashbook (#1398) * docs: Added tutorial for creating a bare Flame project (#1376) * feat: Add missing optional priority to SpriteBodyComponent (#1404) * Add missing optional priority to SpriteBodyComponent Gives you the option to set the priority directly when creating the component. * Add optional parameter priority Adds priority as an optional parameter * removed wrong trailing comma Added a comma at the wrong position. * feat: Added getImageLayer to flame_tiled (#1405) * feat: Create sphinx extension for integrating Flutter apps into the documentation site (#1393) * feat: adding FlameBloc mixin to allow its usage with enhanced FlameGame classes (#1399) * feat: adding FlameBloc mixin to allow its usage with enhanced FlameGame classes * fixing tests * Apply suggestions from code review Co-authored-by: Lukas Klingsbo <me@lukas.fyi> Co-authored-by: Pasha Stetsenko <stpasha@google.com> Co-authored-by: Luan Nico <luanpotter27@gmail.com> Co-authored-by: Lukas Klingsbo <me@lukas.fyi> Co-authored-by: Pasha Stetsenko <stpasha@google.com> Co-authored-by: Luan Nico <luanpotter27@gmail.com> * refactor: Organize tests in the game/ folder (#1403) * position_type_test * detectors_test * reformat projections_test * Reorganize tests in projector_test * review flame_game_test * move some cameratests * created viewport_test file * reformat camera tests * feat: Add missing optional priority to SpriteBodyComponent (#1404) * Add missing optional priority to SpriteBodyComponent Gives you the option to set the priority directly when creating the component. * Add optional parameter priority Adds priority as an optional parameter * removed wrong trailing comma Added a comma at the wrong position. * feat: Added getImageLayer to flame_tiled (#1405) * feat: Create sphinx extension for integrating Flutter apps into the documentation site (#1393) Co-authored-by: KurtLa <KurtLa@users.noreply.github.com> Co-authored-by: Munsterlander <munsterlander@users.noreply.github.com> Co-authored-by: Erick <erickzanardoo@gmail.com> * feat: improving generics on position body component (#1397) * chore(release): publish packages (#1407) - flame@1.1.0-releasecandidate.1 - flame_bloc@1.2.0-releasecandidate.1 - flame_rive@1.1.0-releasecandidate.1 - flame_test@1.2.0-releasecandidate.1 - flame_tiled@1.3.0-releasecandidate.1 * chore: fixing pub deps to allow publish (#1408) * chore(release): publish packages - flame@1.1.0-releasecandidate.1 - flame_bloc@1.2.0-releasecandidate.1 - flame_rive@1.1.0-releasecandidate.1 - flame_test@1.2.0-releasecandidate.1 - flame_tiled@1.3.0-releasecandidate.1 * chore: fixing deps to enable pub publish * fixing vector math version * chore(flame_forge2d): export all files in barrel file (#1409) * chore(release): publish packages (#1410) - flame_forge2d@0.9.0-releasecandidate.1 * chore: commented out PR template sections (#1412) * feat: Make ContactCallback begin end methods optional overrides (#1415) * feat: made begin and end optional overrides * chore: removed unecessary end override * feat: Camera as a component (#1355) * feat(collision detection)!: Use a broadphase to make collision detection more efficient (#1252) * fix: PositionBodyComponent had an async onMount, without needing (#1424) * fix: Fix collision detection comments and typo (#1422) * Fix collision detection comments and typo * Update packages/flame/lib/src/collisions/collision_callbacks.dart Co-authored-by: Pasha Stetsenko <stpasha@google.com> * Update doc/flame/collision_detection.md Co-authored-by: Pasha Stetsenko <stpasha@google.com> Co-authored-by: Pasha Stetsenko <stpasha@google.com> * feat: adding has mounted to component (#1418) * feat: adding has mounted to component * feat: pr suggestions * feat: improving hasMounted * feat: renaming hasMounted to mounted * feat: pr suggestion * chore(release): publish packages (#1427) - flame_svg@1.1.0-releasecandidate.1 - flame@1.1.0-releasecandidate.2 - flame_bloc@1.2.0-releasecandidate.2 - flame_forge2d@0.9.0-releasecandidate.2 - flame_rive@1.1.0-releasecandidate.2 - flame_test@1.2.0-releasecandidate.2 - flame_tiled@1.3.0-releasecandidate.2 * fix a test * fix broken merge * rename parent->owner in LifecycleManager * update doc-comment Co-authored-by: Erick <erickzanardoo@gmail.com> Co-authored-by: KurtLa <KurtLa@users.noreply.github.com> Co-authored-by: Munsterlander <munsterlander@users.noreply.github.com> Co-authored-by: Lukas Klingsbo <me@lukas.fyi> Co-authored-by: Luan Nico <luanpotter27@gmail.com> Co-authored-by: Allison Ryan <77211884+allisonryan0002@users.noreply.github.com> Co-authored-by: Alejandro Santiago <dev@alestiago.com>

feat: adding has mounted to component

09d2ce8

erickzanardo requested review from spydon, luanpotter, renancaraujo and wolfenrain March 4, 2022 18:48

spydon reviewed Mar 4, 2022

View reviewed changes

packages/flame/lib/src/components/component.dart Outdated Show resolved Hide resolved

feat: pr suggestions

c750a56

erickzanardo requested a review from spydon March 4, 2022 19:47

spydon approved these changes Mar 4, 2022

View reviewed changes

st-pasha reviewed Mar 4, 2022

View reviewed changes

packages/flame/test/components/component_lifecycle_test.dart Outdated Show resolved Hide resolved

st-pasha reviewed Mar 4, 2022

View reviewed changes

alestiago reviewed Mar 6, 2022

View reviewed changes

packages/flame/lib/src/components/component.dart Outdated Show resolved Hide resolved

erickzanardo added 2 commits March 7, 2022 12:40

Merge branch 'main' into erick.has_mounted

a070192

feat: improving hasMounted

10a91f7

spydon reviewed Mar 7, 2022

View reviewed changes

st-pasha reviewed Mar 7, 2022

View reviewed changes

alestiago reviewed Mar 7, 2022

View reviewed changes

erickzanardo added 6 commits March 8, 2022 08:04

Merge branch 'main' into erick.has_mounted

bc50a77

feat: renaming hasMounted to mounted

b995d88

Merge branch 'erick.has_mounted' of github.com:flame-engine/flame int…

ba92621

…o erick.has_mounted

Merge branch 'main' into erick.has_mounted

d383ebd

feat: pr suggestion

b94828b

Merge branch 'erick.has_mounted' of github.com:flame-engine/flame int…

63d8515

…o erick.has_mounted

erickzanardo merged commit f8f9e04 into main Mar 8, 2022

erickzanardo deleted the erick.has_mounted branch March 8, 2022 17:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: adding has mounted to component #1418

feat: adding has mounted to component #1418

erickzanardo commented Mar 4, 2022

st-pasha Mar 4, 2022

erickzanardo Mar 7, 2022

st-pasha Mar 7, 2022

erickzanardo Mar 7, 2022

st-pasha Mar 8, 2022

erickzanardo Mar 8, 2022

spydon Mar 7, 2022

st-pasha Mar 7, 2022

alestiago Mar 7, 2022 •

edited

Loading

erickzanardo Mar 7, 2022

alestiago Mar 8, 2022

st-pasha Mar 8, 2022

feat: adding has mounted to component #1418

feat: adding has mounted to component #1418

Conversation

erickzanardo commented Mar 4, 2022

Description

Checklist

Breaking Change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alestiago Mar 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alestiago Mar 7, 2022 •

edited

Loading