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

Add missing window flags for styling system bars #32167

Merged
merged 10 commits into from
Apr 13, 2022

Conversation

camsim99
Copy link
Contributor

@camsim99 camsim99 commented Mar 21, 2022

Addresses the inability to set the icon brightness on system bars on API 30, part of issue #10027.

Using the reproduction included in the issue noted above, I was able to produce the following on a Pixel 2 emulator running API 30 with this change (see before here):

I confirmed these results on Pixel devices running API 28 (with this revert included), 29, and 31.

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 updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

@camsim99 camsim99 marked this pull request as ready for review March 22, 2022 00:04
@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

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.

Can you add a test that verifies these flag settings independent of the divider color being set?

@camsim99 camsim99 requested review from Piinks March 24, 2022 21:46
@zanderso
Copy link
Member

From PR review triage: This looks ready for another review pass @blasten @Piinks

Comment on lines 379 to 382
window.clearFlags(
WindowManager.LayoutParams.FLAG_TRANSLUCENT_STATUS
| WindowManager.LayoutParams.FLAG_TRANSLUCENT_NAVIGATION);

Copy link

Choose a reason for hiding this comment

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

These flags are deprecated in API 30

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are needed to fix the problem for API < 30, so I added a comment noting the deprecation and suppressed the warning

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.

What does the status bar look like on API 27?

@camsim99
Copy link
Contributor Author

camsim99 commented Apr 8, 2022

What does the status bar look like on API 27?

It looks the same as the pictures above! I also tested this on API 21 to see what happens when none of the operations can be performed (API 23 is needed at least), and it looks the same with and without setting/clearing the flags. I can also move the setting/clearing of flags after the SDK versions are checked for each operation (like here) to rule out side effects, but I was just trying to avoid redundant code.

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

@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 Apr 13, 2022
@fluttergithubbot fluttergithubbot merged commit 6c62681 into flutter:main Apr 13, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs tests 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
5 participants