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

feat: adding has mounted to component #1418

Merged
merged 10 commits into from
Mar 8, 2022
16 changes: 16 additions & 0 deletions packages/flame/lib/src/components/component.dart
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import 'dart:async';
import 'dart:collection';

import 'package:flutter/painting.dart';
Expand Down Expand Up @@ -53,6 +54,8 @@ class Component {
bool get hasChildren => _children?.isNotEmpty ?? false;
ComponentSet? _children;

Completer<void>? _mountCompleter;

@protected
_LifecycleManager get lifecycle => _lifecycleManager ??= _LifecycleManager();
_LifecycleManager? _lifecycleManager;
Expand Down Expand Up @@ -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 {
Copy link
Contributor

@alestiago alestiago Mar 7, 2022

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

await component.whenMounted()

if (isMounted) {
return Future.value();
Copy link
Member

Choose a reason for hiding this comment

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

Nice solution!

}

_mountCompleter ??= Completer<void>();

return _mountCompleter!.future;
}

/// This method is called periodically by the game engine to request that your
/// component updates itself.
///
Expand Down Expand Up @@ -258,6 +272,7 @@ class Component {
/// Changes the current parent for another parent and prepares the tree under
/// the new root.
void changeParent(Component component) {
_mountCompleter = null;
parent?.remove(this);
nextParent = component;
}
Expand Down Expand Up @@ -403,6 +418,7 @@ class Component {
if (existingChild || _state == LifecycleState.removed) {
onGameResize(findGame()!.canvasSize);
}
_mountCompleter?.complete();
Copy link
Contributor

Choose a reason for hiding this comment

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

Set it to null here?

onMount();
_state = LifecycleState.mounted;
if (!existingChild) {
Expand Down
46 changes: 46 additions & 0 deletions packages/flame/test/components/component_lifecycle_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,52 @@ void main() {
);
});

flameGame.test('component hasMounted completes', (game) async {
erickzanardo marked this conversation as resolved.
Show resolved Hide resolved
final component = _MyComponent();
await game.add(component);
final hasMounted = component.hasMounted;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

How come? This test is doing exactly that 🤔


await game.ready();

return expectLater(hasMounted, completes);
});

flameGame.test(
'component hasMounted completes even after the '
'component is already mounted',
(game) async {
final component = _MyComponent();
await game.add(component);
await game.ready();

final hasMounted = component.hasMounted;

return expectLater(hasMounted, completes);
},
);

flameGame.test(
'component hasMounted completes when changing parent',
(game) async {
final parent = _MyComponent('parent');
final child = _MyComponent('child');
parent.add(child);
game.add(parent);

var hasMounted = child.hasMounted;
await game.ready();

await expectLater(hasMounted, completes);

child.changeParent(game);
hasMounted = child.hasMounted;
game.update(0);
await game.ready();

await expectLater(hasMounted, completes);
},
);

// Obsolete scenario, when we used to have a separate "prepare" stage
flameGame.test('parent prepares the component', (game) async {
final parent = _MyComponent('parent');
Expand Down