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

Determine lifecycle by looking at window focus also #41094

Merged
merged 1 commit into from Apr 28, 2023

Conversation

gspencergoog
Copy link
Contributor

@gspencergoog gspencergoog commented Apr 11, 2023

Description

This incorporates additional signal from Activity.onWindowFocusChanged to help decide if the application is resumed or inactive.

When the user pulls down the notification shade or opens the app switcher in iOS, then iOS sends a notification to the application that it no longer has input focus (is no longer "active" in Apple terminology).

However, Android (at least on a Pixel) doesn't send onPause and onResume events for these things, as one might expect. Instead, this PR changes things so that we listen to Activity.onWindowFocusChanged and see if any of the windows still have focus.

If it doesn't have focus, then the lifecycle switches to inactive (even if onPause hasn't been called), and if it does have focus (and onResume hasn't been called) then we should go to resumed.

State changes are determined and deduped in the LifecycleChannel class.

Here's the old state table:

Android State Flutter state
Resumed resumed
Paused inactive
Stopped paused
Detached detached

Here's the new state table:

Android State Window focused Flutter state
Resumed true resumed
Resumed false inactive *
Paused true inactive
Paused false inactive
Stopped true paused
Stopped false paused
Detached true detached
Detached false detached
  • = This is the relevant change in this PR.

("Window focused" means one or more windows managed by Flutter are focused)

The inactive state is for when the application is running and visible, but doesn't have the input focus. An example where this currently happens are when a phone call is in progress on top of the app, or on some OEMs when going into the app switcher (I've tested on Realme and it does that, at least). With the PR, it will also go into inactive when the app has lost input focus, but is still in the Android onResume state. This means that on phones that don't pause the app when they go into the app switcher or the notification window shade (Pixel, others), the app will go into inactive when it didn't before. If developers weren't doing anything special in the inactive state before, then this PR will have no change for them. If they were, they will go into that state more often (but more consistently across OEMs).

Related Issues

Tests

  • Added unit tests for handling onWindowFocusChanged.

@gspencergoog gspencergoog force-pushed the android_focus_lifecycle branch 3 times, most recently from e29c0ce to cd16c9f Compare April 14, 2023 15:08
@gspencergoog gspencergoog marked this pull request as ready for review April 18, 2023 22:30
@gspencergoog gspencergoog marked this pull request as draft April 18, 2023 22:32
Copy link
Contributor

@reidbaker reidbaker left a comment

Choose a reason for hiding this comment

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

This code looks great but it will need some tests. Specifically tests to make sure that we are forwarding the onWindowFocusChanged events. I pinged another contributor for a second opinion about the potential risks of having a second item set inactive.

My final thought is about ensuring that android activities forward the onWindowFocusChanged call along to flutter fragments. Do you know if we have the ability to write custom linters to warn the caller if they dont forward events to us?

Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

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

Seems good to me. Comments below.

  • That the onWindowFocusChanged will report false, even when one of the windows in the activity is focused (I'm not sure yet what it does when you switch between one window and another in a multi-window app).

Yeah, I'd expect that if you have two activities visible in multi-window mode and you switch focus from one to the other, then the system would call onWindowFocusChanged(false) on the one activity and onWindowFocusChanged(true) on the other.

That should be pretty similar to what happens if you switch between activities in the app switcher, which I believe will call onStop on one activity and onResume et al. on the other. So if that causes confusion in Flutter, then it's probably a confusion that already exists.

@gspencergoog
Copy link
Contributor Author

I removed the state in the LifecycleChannel (the private String lastState variable), since I think it's too easy to get out of sync with the real state of the Android Activity, or with the state in the framework, since an add-to-app activity could start handling the lifecycle states, or could send messages to the framework to change the state on its own. Sure, not common things, but they could happen.

Sending extra state updates in the case where we move from inactive to resumed or vice versa are unfortunate, but I'd rather have one source of truth for the state of the app and just deduplicate them.

Off to writing tests now.

@gspencergoog gspencergoog force-pushed the android_focus_lifecycle branch 3 times, most recently from f311b92 to 523bb42 Compare April 21, 2023 20:36
@gspencergoog
Copy link
Contributor Author

Specifically tests to make sure that we are forwarding the onWindowFocusChanged events. I pinged another contributor for a second opinion about the potential risks of having a second item set inactive.

What should those tests look like? I've added a unit test to verify that we do the right thing when we receive onWindowFocusChanged, but I don't know how to write something to test that a fragment receives the events when we also handle them. Are there already tests like these for fragments that I could look at as an example?

Do you know if we have the ability to write custom linters to warn the caller if they don't forward events to us?

No, I don' t know the answer to that, maybe @gnprice knows?

@gspencergoog gspencergoog marked this pull request as ready for review April 21, 2023 20:56
/// The application is visible and responding to user input.
/// The application is visible and responsive to user input.
///
/// On Android, this state corresponds to an app or the Flutter host view
Copy link
Member

Choose a reason for hiding this comment

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

not sure the or is apt here. If you consider the first part of the or, 'an app having focus while in Android's "resumed" state' could be true most of the time while this enum isn't resumed in hybrid apps

Copy link
Contributor

Choose a reason for hiding this comment

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

It's also worth noting that "focus" here refers to the window, and a single application may have more than one window, thus implying part of the application has focus while other parts don't. I think even something as simple as displaying a native Android dialog (which a plugin could do) would trigger onWindowFocusChanged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What are the possible values it could be set to in the hybrid case where the app has focus and is in the Android resumed state?

I am assuming that if "onResume" has been called by Android on the activity, and the Flutter activity has focus, then the only Flutter state this should be in is "resumed". If the Flutter activity doesn't have focus, then it should be in "inactive".

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 picking on the doc wording, not on the implementation. It's not "the app OR the Flutter host view having focus", it's just "the Flutter host view having focus". In hybrid apps where 95% of the app isn't Flutter, you're not going to be AppLifecycleState.resumed if "the app has focused while in Android's resumed state". The Flutter view might not even be attached to the FlutterEngine

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 picking on the doc wording, not on the implementation.

Sure, I got that, and a totally fine thing to do! I want to get it right.

In hybrid apps where 95% of the app isn't Flutter, you're not going to be AppLifecycleState.resumed if "the app has focused while in Android's resumed state". The Flutter view might not even be attached to the FlutterEngine.

If the Flutter view isn't attached to the engine, wouldn't it be in Android's "Detached" state? In that state, it doesn't matter what the focus is, the Flutter state will be AppLifecycleState.detached. Only in Android's "Resumed" state am I making any changes to how things work. Take a look at the state table I put in the PR description and tell me if that helps explain it.

@gspencergoog gspencergoog force-pushed the android_focus_lifecycle branch 2 times, most recently from e0c89cc to e6bdea3 Compare April 25, 2023 00:16
@gspencergoog
Copy link
Contributor Author

Okay, I've updated it so that LifecycleChannel keeps track of the focus state and emits the appropriate state to the framework for the combination of Android lifecycle state and focus state.

Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

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

I like this state-table-based approach!

@gspencergoog
Copy link
Contributor Author

Okay, I've added tests for the LifecycleChannel, and I've done manual testing on some devices. Are there any other tests I should add?

Otherwise, I think this is ready for another look.

@math1man
Copy link
Contributor

Overall LGTM

Copy link
Contributor

Choose a reason for hiding this comment

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

Great documentation!

@gspencergoog gspencergoog added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 28, 2023
@auto-submit auto-submit bot merged commit eb68df6 into flutter:main Apr 28, 2023
31 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 28, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 28, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 28, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 28, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Apr 28, 2023
…125714)

flutter/engine@98b6fab...2a84ea5

2023-04-28 zanderso@users.noreply.github.com Revert "Replace deprecated and internal Skia EncodedImage calls with public versions" (flutter/engine#41595)
2023-04-28 maruel@gmail.com Roll buildroot to c96c9d4641714301fab450a5f3b0f3c42712e1e3 (flutter/engine#41589)
2023-04-28 ychris@google.com [platform_view] Only dispose view when it is removed from the composition order (flutter/engine#41521)
2023-04-28 gspencergoog@users.noreply.github.com Determine lifecycle by looking at window focus also (flutter/engine#41094)
2023-04-28 zanderso@users.noreply.github.com Increase timeout for clang-tidy on mac (flutter/engine#41588)
2023-04-28 bdero@google.com [Impeller] Always enable validation for goldens (flutter/engine#41574)
2023-04-28 maruel@gmail.com fuchsia: do not read version_history.json (flutter/engine#41585)
2023-04-28 skia-flutter-autoroll@skia.org Roll Dart SDK from c7160d4ea0b7 to 8e9bf2bb9878 (5 revisions) (flutter/engine#41586)
2023-04-28 kjlubick@users.noreply.github.com Replace deprecated and internal Skia EncodedImage calls with public versions (flutter/engine#41368)
2023-04-28 skia-flutter-autoroll@skia.org Roll Skia from 05d09f28250a to 9867fa253064 (18 revisions) (flutter/engine#41584)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC jsimmons@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
XilaiZhang added a commit that referenced this pull request May 1, 2023
auto-submit bot pushed a commit that referenced this pull request May 1, 2023
… also" (#41626)

Reverts #41094.

context: updated the java/android timeouts and details in b/280204906
dnfield pushed a commit to timmaffett/engine that referenced this pull request May 1, 2023
… also" (flutter#41626)

Reverts flutter#41094.

context: updated the java/android timeouts and details in b/280204906
auto-submit bot pushed a commit that referenced this pull request May 3, 2023
#41702)

## Description

This reverts commit 9183bff to re-land #41094 because the Google test failures have been fixed. There are no changes to the original PR, since the fixes were in the Google code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App platform-android
Projects
None yet
5 participants