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

fix: Hardcode initCurrentGame lifecycle state as resumed #2775

Merged
merged 6 commits into from Oct 1, 2023

Conversation

adil192
Copy link
Contributor

@adil192 adil192 commented Sep 26, 2023

Description

When the init game method is called straight after the app has launched, the lifecycle state from WidgetsBinding.instance.lifecycleState often returns disposed or inactive which causes the engine to stay paused even though the app is foregrounded.
This PR fixes the lifecycle state as resumed when the init method is called.

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

Fixes #2771

@adil192 adil192 changed the title fix: hardcode initGame lifecycle state as resumed fix: hardcode initCurrentGame lifecycle state as resumed Sep 26, 2023
@adil192 adil192 changed the title fix: hardcode initCurrentGame lifecycle state as resumed Fix: hardcode initCurrentGame lifecycle state as resumed Sep 26, 2023
@adil192 adil192 changed the title Fix: hardcode initCurrentGame lifecycle state as resumed fix: Hardcode initCurrentGame lifecycle state as resumed Sep 26, 2023
@adil192
Copy link
Contributor Author

adil192 commented Sep 26, 2023

@spydon Do I need to add/edit tests for this?

@spydon
Copy link
Member

spydon commented Sep 26, 2023

@spydon Do I need to add/edit tests for this?

Yeah, we try to add tests whenever we fix a bug to make sure we don't have regressions later.

@adil192
Copy link
Contributor Author

adil192 commented Sep 26, 2023

I'm finding this pretty hard to discriminate with tests.
If the lifecycle state is set to e.g. detached then the tester doesn't call initState on the game widget (or even create a state at all).

Update: I ended up adding some @internal initLifecycleState() and disposeLifecycleState() methods in the Game class and using those in the test. Let me know what you think. I also don't know if @internal is the right annotation here

@spydon
Copy link
Member

spydon commented Sep 29, 2023

Update: I ended up adding some @internal initLifecycleState() and disposeLifecycleState() methods in the Game class and using those in the test. Let me know what you think. I also don't know if @internal is the right annotation here

Since the method that those methods are using isn't internal or private, I don't see the point in creating them?

The correct annotation would be @visibleForTesting.

@adil192
Copy link
Contributor Author

adil192 commented Sep 29, 2023

Since the method that those methods are using isn't internal or private, I don't see the point in creating them?

The correct annotation would be @visibleForTesting.

We can't use State objects in tests when we have backgrounded lifecycle states.
Having them as separate methods makes them testable.

@visibleForTesting wouldn't work since they're in Game so GameWidget wouldn't be able to access them (unless we added an ignore lint comment)

@spydon
Copy link
Member

spydon commented Sep 30, 2023

We can't use State objects in tests when we have backgrounded lifecycle states.

Why not? That was how it was done previously right?

@adil192
Copy link
Contributor Author

adil192 commented Sep 30, 2023

The existing tests change the lifecycle state after creating the widget, e.g.

(tester) async {
  await tester.pumpWidget(GameWidget(game: game));

  expect(game.paused, isFalse);
  WidgetsBinding.instance.handleAppLifecycleStateChanged(
    AppLifecycleState.paused,
  );
  expect(game.paused, isTrue);
},

If you set it to AppLifecycleState.paused before creating the GameWidget,
its GameWidgetState object is never created (which is what contains the initCurrentGame() method).

I tried manually instantiating the State object but it threw a bunch of errors because some of its private fields weren't populated.

I'm going to try another way to test this which I'll commit in a second

@adil192
Copy link
Contributor Author

adil192 commented Sep 30, 2023

This new test also correctly succeeds and fails with and without this patch

@spydon spydon enabled auto-merge (squash) October 1, 2023 16:19
@spydon spydon merged commit 0cd5037 into flame-engine:main Oct 1, 2023
7 checks passed
@adil192 adil192 deleted the patch-1 branch October 1, 2023 16:51
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.

Flame isn't Rendering Sprites
2 participants