Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Partially remove setSystemUiVisibility() usages #31092

Merged
merged 10 commits into from
Feb 17, 2022
Merged

Partially remove setSystemUiVisibility() usages #31092

merged 10 commits into from
Feb 17, 2022

Conversation

@camsim99
Copy link
Contributor

@camsim99 camsim99 commented Jan 26, 2022 •
edited
Loading

This PR seeks to partially address Issue #91128 by using backwards compatible overlay/inset APIs provided by Android as of API 30.

Because Flutter still supports API 19 and these APIs leave a gap in compatibility for APIs 19-29, this cannot fully be accomplished, but this PR refactors code such that when this code can be removed, it will be straightforward to do so. See comment below for details.

CC: @wytesk133 (sorry, couldn't put as reviewer)

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
    writing and running engine tests.
  • I signed the CLA.
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@camsim99 camsim99 changed the title Remove setSystemUiVisibility Usages (on main) Partially remove setSystemUiVisibility Usages Jan 31, 2022
@camsim99 camsim99 changed the title Partially remove setSystemUiVisibility Usages Partially remove setSystemUiVisibility() usages Jan 31, 2022
@camsim99
Copy link
Contributor Author

camsim99 commented Jan 31, 2022 •
edited
Loading

Differences between this PR and the reverted original

Added backwards compatibility for API 19
The Android API used to set the system UI mode as of API 30 used in this PR (WindowInsetsControllerCompat) only provide backwards compatibility back through API 20, so the original code used to set the system UI mode is kept for API 19. I moved as much as it as I could to separate methods so that it can be more easily removed in the future.

Corrected default system UI mode
The method to set the layout of an app to full screen by using the WindowCompat API (WindowCompat.setDecorFitsSystemWindows(window, false)) also hides the navigation bar for API 19-29, which is not the desired default system UI mode. So, for these APIs, I made the window unset the flag that hides the navigation bar.

Corrected miscellaneous errors
One thing I missed in the first PR is that the system UI mode needs to be set after setting the system UI overlays whenever the immersive mode is not set when the overlays are set, i.e. when overlays to show have been specified (fix here).

Another thing I missed is that the currentSystemUiMode tracked in the PlatformPlugin should be null until updated rather than setting it as edge to edge by default, since this has different behavior than the default system UI mode.

@camsim99 camsim99 marked this pull request as ready for review February 1, 2022 17:42
@camsim99 camsim99 requested review from Piinks and blasten February 1, 2022 19:20
blasten
blasten reviewed Feb 3, 2022
View reviewed changes
shell/platform/android/io/flutter/embedding/android/FlutterActivity.java
window.setStatusBarColor(0x40000000);
window.getDecorView().setSystemUiVisibility(PlatformPlugin.DEFAULT_SYSTEM_UI);
WindowCompat.setDecorFitsSystemWindows(window, false);
if (Build.VERSION.SDK_INT < 30) {
Copy link

@blasten blasten Feb 3, 2022

Choose a reason for hiding this comment

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

does it need to also check for Build.VERSION.SDK_INT >= 19?

Copy link
Contributor Author

@camsim99 camsim99 Feb 3, 2022

Choose a reason for hiding this comment

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

No, because it works for SDK >= 16! You can see that in the source code at the bottom of this page.

Copy link

@blasten blasten Mar 14, 2022

Choose a reason for hiding this comment

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

it looks like this caused a regression for SDK_INT >= 30. See b/223628114

Copy link
Contributor Author

@camsim99 camsim99 Mar 14, 2022

Choose a reason for hiding this comment

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

Ok, I'm investigating a fix for this to re-land it.

blasten
blasten reviewed Feb 3, 2022
View reviewed changes
shell/platform/android/io/flutter/embedding/android/FlutterActivity.java Outdated
window.getDecorView().setSystemUiVisibility(PlatformPlugin.DEFAULT_SYSTEM_UI);
WindowCompat.setDecorFitsSystemWindows(window, false);
if (Build.VERSION.SDK_INT < 30) {
// This ensures that the navigation bar is not hidden for APIs 19-30,
Copy link

@blasten blasten Feb 3, 2022

Choose a reason for hiding this comment

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

is it 19-29 (inclusive) or 19-30 (inclusive)?

Copy link
Contributor Author

@camsim99 camsim99 Feb 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It is technically 16-29 inclusive (I put 19 because of what Flutter supports). I will update the comment to better reflect that!

blasten
blasten approved these changes Feb 3, 2022
View reviewed changes
Copy link

@blasten blasten left a comment

Choose a reason for hiding this comment

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

LGTM

@chinmaygarde
Copy link
Member

chinmaygarde commented Feb 10, 2022

This needs a rebase for the presubmits.

Piinks
Piinks approved these changes Feb 16, 2022
View reviewed changes
Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

Flutter_LGTM

@camsim99 camsim99 added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Feb 17, 2022
@fluttergithubbot fluttergithubbot merged commit 68473fe into flutter:main Feb 17, 2022
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 17, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 17, 2022
@engine-flutter-autoroll
68473fe Partially remove setSystemUiVisibility() usages (flutter/engi…
7b93070 
…ne#31092)
@camsim99 camsim99 mentioned this pull request Feb 24, 2022
Merged
8 tasks
fluttergithubbot pushed a commit that referenced this pull request Feb 28, 2022
@camsim99
Undo Deletion in #31092 (#31661)
1e5b26b
@naamapps naamapps mentioned this pull request Mar 6, 2022
Closed
blasten pushed a commit to blasten/engine that referenced this pull request Mar 14, 2022
Revert "Undo Deletion in flutter#31092 (flutter#31661)"
4e727ff 
This reverts commit 1e5b26b.
blasten pushed a commit to blasten/engine that referenced this pull request Mar 14, 2022
Revert "Partially remove setSystemUiVisibility() usages (flutter#31092)"
c9373e4 
This reverts commit 68473fe.
@blasten blasten mentioned this pull request Mar 14, 2022
Merged
@camsim99 camsim99 mentioned this pull request May 26, 2022
Closed
8 tasks
@JonathanPeterCole JonathanPeterCole mentioned this pull request Apr 17, 2023
Merged
8 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@blasten blasten blasten approved these changes

@Piinks Piinks Piinks approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

platform-android waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@camsim99 @chinmaygarde @blasten @Piinks @fluttergithubbot