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

Conversation

PistonShot1
Copy link
Contributor

@PistonShot1 PistonShot1 commented Apr 3, 2024

Description

This change is mainly to make it easier for specifying loading screen along when routing between component using the RouterComponent. Currently the change is only made on Route, that now have optional loadingBuilder to Route constructor which would expect a Component, which would be added as the loading page/screen while onLoad of the builder (the main routing component) to be completed.

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.

Copy link
Member

@spydon spydon left a comment

Choose a reason for hiding this comment

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

Lgtm, very clean implementation.
Could you add a simple test that just tests that both the loadingPage and then the page is shown?
It would be in here:
https://github.com/flame-engine/flame/blob/main/packages/flame/test/components/route_test.dart

packages/flame/lib/src/components/route.dart Outdated Show resolved Hide resolved
@spydon spydon changed the title feat: addition of loading builder to route feat: Loading builder for Route Apr 3, 2024
@PistonShot1
Copy link
Contributor Author

wait ya , dont merge yet, adding test for this

@PistonShot1
Copy link
Contributor Author

Don't merge yet.

hi @spydon , sorry I was away for awhile. Just pushed new one with test. Anyways, I just have problem it making a full test to check if the loading component was mounted before it was removed. I was only able to check for loading component was removed and the builder component ( page component) was mounted right after it was removed.

If you look at the file route_test.dart , I added test and also did add few comments , to explain the situation for the test coverage. Sorry I am not that good at coming with test, but I did manually check it by linking my version of flame repo to one of my old flame game, I mean my flame in pubspec refer to my git repo instead of pub.dev and test this feature I added, according to my observation it did work to what I expected.

@PistonShot1
Copy link
Contributor Author

I mean does checking the loading component was removed technically means it also verifies that it was mounted before right . So if that is the case then the test I added already suffice ?

Comment on lines 503 to 510
// 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;

@spydon
Copy link
Member

spydon commented Jul 21, 2024

@PistonShot1 have you made any progress on this? :)

@PistonShot1
Copy link
Contributor Author

@spydon . I have fixed it, sorry took so long for such a small feature addition, I gave up after the test keep on fail, but as you suspected it was the way I wrote the test, now it works fine and should be working and passed the expected tests

Copy link
Member

@spydon spydon left a comment

Choose a reason for hiding this comment

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

Lgtm, thanks for your contribution!

packages/flame/lib/src/components/route.dart Outdated Show resolved Hide resolved
packages/flame/lib/src/components/route.dart Outdated Show resolved Hide resolved
packages/flame/lib/src/components/route.dart Outdated Show resolved Hide resolved
@spydon spydon enabled auto-merge (squash) July 24, 2024 13:25
@spydon spydon merged commit 1e62b34 into flame-engine:main Jul 24, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants