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: Loading builder for Route #3113

Merged
merged 15 commits into from
Jul 24, 2024
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 23 additions & 2 deletions packages/flame/lib/src/components/route.dart
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,11 @@ class Route extends PositionComponent
with ParentIsA<RouterComponent>, HasTimeScale {
Route(
Component Function()? builder, {
Component Function()? loadingBuilder,
this.transparent = false,
this.maintainState = true,
}) : _builder = builder,
_loadingBuilder = loadingBuilder,
_renderEffect = Decorator();

/// If true, then the route below this one will continue to be rendered when
Expand All @@ -53,6 +55,11 @@ class Route extends PositionComponent
/// in which case the user must override the [build] method.
final Component Function()? _builder;

/// The function that will be invoked that will build the loading page
/// component when this route first becomes active. Can be left `null`,
/// in which case it is not required.
spydon marked this conversation as resolved.
Show resolved Hide resolved
final Component Function()? _loadingBuilder;

/// This method is invoked when the route is pushed on top of the
/// [RouterComponent]'s stack.
///
Expand Down Expand Up @@ -118,17 +125,31 @@ class Route extends PositionComponent
/// also be added as a child component.
Component? _page;

/// The loadingPage that was built and is now owned by this route. This
/// loadingPage will also be added as a child component.
spydon marked this conversation as resolved.
Show resolved Hide resolved
Component? _loadingPage;

/// Additional visual effect that may be applied to the page during rendering.
final Decorator _renderEffect;

/// Invoked by the [RouterComponent] when this route is pushed to the top
/// of the navigation stack.
/// of the navigation stack
@internal
void didPush(Route? previousRoute) {
_page ??= build()..addToParent(this);
_page ??= build();
(_loadingBuilder != null) ? _addLoadingPage() : _page!.addToParent(this);
onPush(previousRoute);
}

/// Adds the [_loadingPage] to the parent and invoked by [didPush] , when
/// [_loadingBuilder] is specified
Future<void> _addLoadingPage() async {
_loadingPage ??= _loadingBuilder!()..addToParent(this);
await _page!.addToParent(this);
spydon marked this conversation as resolved.
Show resolved Hide resolved
await _page!.loaded;
_loadingPage!.removeFromParent();
}

/// Invoked by the [RouterComponent] when this route is popped off the top
/// of the navigation stack.
/// If [maintainState] is false, the page component rendered by this route
Expand Down
55 changes: 55 additions & 0 deletions packages/flame/test/components/route_test.dart
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import 'dart:async';
import 'dart:ui';

import 'package:flame/components.dart';
Expand Down Expand Up @@ -474,6 +475,51 @@ void main() {
);
},
);
testWithFlameGame('Route with loading', (game) async {
final loadingComponent = PositionComponent(size: Vector2.all(100));
final pageComponent = _HeavyComponent()..size = Vector2.all(100);
final router = RouterComponent(
initialRoute: 'start',
routes: {
'start': Route(
Component.new,
),
'new': Route(
() => pageComponent,
loadingBuilder: () => loadingComponent,
),
},
)..addToParent(game);
await game.ready();

router.pushNamed('new');

// I can't perform this following checking, probably because the way it
// was implemented, in assumption that line 512 actually logically works.
// If line 512, is doing nothing then I don't think, I can perform this
// following check either, that is to check state when loading was
// mounted before it was removed.

// expect(
// loadingComponent.isMounted,
// isTrue,
// );
// expect(
// pageComponent.isMounted,
// isFalse,
// );
Copy link
Member

Choose a reason for hiding this comment

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

I don't think I follow why this doesn't work, what are you getting here currently?

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'm sorry , False is being expected for 503, as for 507 it does pass

Copy link
Member

Choose a reason for hiding this comment

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

Why is false expected for 503? Since the route is added it should be showing the loadingComponent right?

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 just found, I made an error in the checking at line 514 is not accurate in this commit, it should be expecting True, but I checked there for False and it passed

That means my loading component was never mounted even before and after the main component was mounted?
Maybe, that mean my implementation may have some error ?

Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing that due to how Future.delayed works in tests the loadingComponent was added and removed in the same tick. You could pass in a future to the _HeavyComponent that you manually complete after you have checked that the loadingComponent is mounted.

Copy link
Member

Choose a reason for hiding this comment

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

Did you have any more time to look at this @PistonShot1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the delays got caught up with another project, but I am trying to implement the approach you mentioned , to pass a future that I can manually complete, I am not exactly sure how to implement this . I am assuming with Completer or sort ?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, Completer should work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the test for checking if the loadingComponent was mounted and remove, both doesn't pass (at line 499 and at line 509), as I have commented, it is some how implying that the loadingPage was never mounted. I am not sure why, but the changes I added at packages/flame/lib/src/components/route.dart (line 146-151), supposed have mounted and removed after awaiting _page!.loaded;


game.update(pageComponent.dummyTime.inSeconds.toDouble());
await game.ready();
expect(
loadingComponent.isRemoved,
isFalse,
);
expect(
pageComponent.isMounted,
isTrue,
);
});
});
}

Expand Down Expand Up @@ -527,3 +573,12 @@ class _ColoredComponent extends PositionComponent {
canvas.drawRect(size.toRect(), _paint);
}
}

class _HeavyComponent extends PositionComponent {
final dummyTime = const Duration(seconds: 3);
@override
FutureOr<void> onLoad() async {
await Future.delayed(dummyTime);
return super.onLoad();
}
}
Loading