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

Implemented Dark Mode for Android (#25525) #26605

Merged

Conversation

Projects
None yet
7 participants
@matthew-carroll
Copy link
Contributor

commented Jan 15, 2019

Starting in Android Pie, when battery saver is on, or when the developer option for "Night Mode" is enabled, Android apps should show a dark styled UI. This PR introduces this behavior to Flutter.

This is the framework half of the change. The engine PR here is here:
flutter/engine#7488

On the framework side, this PR integrates the new platformBrightness property into MediaQuery and then binds WidgetsApp to that property so that the widget hierarchy rebuilds on change to brightness.

Supporting Flutter changes for brightness required adding uiMode to the Android manifest under overridden configuration changes for FlutterActivity.

Show resolved Hide resolved packages/flutter/lib/src/material/app.dart Outdated
Show resolved Hide resolved packages/flutter/lib/src/material/app.dart
Show resolved Hide resolved packages/flutter/lib/src/widgets/app.dart Outdated
Show resolved Hide resolved packages/flutter/lib/src/widgets/media_query.dart Outdated

@matthew-carroll matthew-carroll requested a review from goderbauer Jan 30, 2019

@xster
Copy link
Contributor

left a comment

LG. Please add tests

@xster

This comment has been minimized.

Copy link
Contributor

commented Jan 30, 2019

specifically re tests:

  • check that the darkTheme is used when a mock window or the platform sends Brightness.dark
  • check that the theme and darkTheme toggles automatically via a descendent Material widget when the platform toggles
  • check that the theme is indeed used when darkTheme is undefined.

In general, I think it's a good idea to run flutter test --coverage and making sure all the new lines are covered until we have an infra to display it in GitHub. You can see the lcov with a vscode extension or something (or you can just read the output file).

@HansMuller
Copy link
Contributor

left a comment

This looks good, just some small requests.

Show resolved Hide resolved packages/flutter/lib/src/material/app.dart Outdated
Show resolved Hide resolved packages/flutter/lib/src/material/app.dart Outdated
Show resolved Hide resolved packages/flutter/lib/src/material/app.dart
Show resolved Hide resolved packages/flutter/lib/src/rendering/binding.dart Outdated
Show resolved Hide resolved packages/flutter/lib/src/widgets/binding.dart Outdated
Show resolved Hide resolved packages/flutter/lib/src/widgets/media_query.dart Outdated
Show resolved Hide resolved packages/flutter/test/widgets/media_query_test.dart Outdated

Matt Carroll added some commits Feb 5, 2019

@matthew-carroll

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2019

@HansMuller @goderbauer @xster - I just pushed a number of updates based on comments. Can you all do another pass?

Matt Carroll
@goderbauer
Copy link
Member

left a comment

LGTM if it looks good to @xster and @HansMuller as well.

Show resolved Hide resolved packages/flutter/lib/src/widgets/binding.dart Outdated
@goderbauer

This comment has been minimized.

Copy link
Member

commented Feb 6, 2019

Looks like cirrus is not too happy, though.

@xster
Copy link
Contributor

left a comment

LGTM. Thanks for adding tests.

Show resolved Hide resolved packages/flutter/lib/src/material/app.dart Outdated
Show resolved Hide resolved packages/flutter/test/material/app_test.dart
Matt Carroll
Removed semantics test on TestWindow because it required disposal of …
…a SemanticsHandle and it was unclear how to do that.
@HansMuller
Copy link
Contributor

left a comment

LGTM with a few suggestions

Show resolved Hide resolved packages/flutter/lib/src/material/app.dart Outdated
Show resolved Hide resolved packages/flutter/lib/src/material/app.dart Outdated
Show resolved Hide resolved packages/flutter/lib/src/material/app.dart Outdated
Show resolved Hide resolved packages/flutter/lib/src/material/app.dart Outdated
Show resolved Hide resolved packages/flutter/lib/src/material/app.dart
Matt Carroll

@matthew-carroll matthew-carroll merged commit a13fdbc into flutter:master Feb 7, 2019

26 checks passed

WIP Ready for review
Details
analyze Task Summary
Details
analyze
Details
build_tests-linux Task Summary
Details
build_tests-linux
Details
build_tests-macos Task Summary
Details
build_tests-macos
Details
build_tests-windows Task Summary
Details
build_tests-windows
Details
cla/google All necessary CLAs are signed
codelabs-build-test Task Summary
Details
codelabs-build-test
Details
docs Task Summary
Details
docs
Details
tests-linux Task Summary
Details
tests-linux
Details
tests-macos Task Summary
Details
tests-macos
Details
tests-windows Task Summary
Details
tests-windows
Details
tool_tests-linux Task Summary
Details
tool_tests-linux
Details
tool_tests-macos Task Summary
Details
tool_tests-macos
Details
tool_tests-windows Task Summary
Details
tool_tests-windows
Details

agrapine added a commit to agrapine/flutter that referenced this pull request Feb 9, 2019

kangwang1988 added a commit to alibaba-flutter/flutter that referenced this pull request Feb 12, 2019

hyochan added a commit to hyochan/flutter that referenced this pull request Feb 12, 2019

goderbauer added a commit to goderbauer/flutter that referenced this pull request Feb 15, 2019

@creativecreatorormaybenot

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2019

@matthew-carroll How can I change the platformBrightness value and also affect the MaterialApp theme? I implemented adding a MediaQuery with my custom value for platformBrightness similar to how it was done here. I even tried setting state of the widget containing the MaterialApp via WidgetBindingsObserver's didChangePlatformBrightness. The MediaQuery has the correct value down the tree, but the theme does not change accordingly. However, when I toggle the device to dark mode it works fine. What part am I missing?

The MaterialApp has both theme and darkTheme populated.

@matthew-carroll

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2019

@creativecreatorormaybenot can you please file this question as an issue instead of a PR comment? Also, can you please include as much of your actual code as possible? I need to see exactly what you're doing to be able to comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.