Skip to content

Added MaterialApp.themeMode to control which theme is used. #35499

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.

Sign up for GitHub

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

Jump to bottom
Merged
darrenaustin merged 2 commits into from
Jul 9, 2019
Merged

Added MaterialApp.themeMode to control which theme is used. #35499

darrenaustin merged 2 commits into from
Jul 9, 2019

Conversation

@darrenaustin
Copy link
Contributor

@darrenaustin darrenaustin commented Jul 3, 2019

Description

In order to make is easier for applications to provide support for switching between light and dark themes, or just letting the system do it for them, this PR adds a ThemeMode themeMode property to the MaterialApp. ThemeMode is a new enum that has three values:

  • system: use either theme or darkTheme based on the platform's "dark mode" settings.
  • light: always use theme, regardless of the system setting.
  • dark: always use darkTheme, regardless of the system setting.

It defaults to system, which maintains the current behavior.

Without this, writing an app that wanted to override the system setting would require them to use only the theme property even if they had to set it to their 'dark' theme. This way they can just set theme and darkTheme appropriately and just switch the mode as needed.

Tests

I added several tests to verify that the various combinations of themeMode and platform brightness were handled correctly.

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

  • No, this is not a breaking change.

@darrenaustin darrenaustin added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Jul 3, 2019
matthew-carroll
matthew-carroll reviewed Jul 8, 2019
View reviewed changes
packages/flutter/test/material/app_test.dart Outdated
expect(appliedTheme.brightness, Brightness.dark);
});

testWidgets('MaterialApp uses regular theme when themeMode is system & platformBrightness is light', (WidgetTester tester) async {
Copy link
Contributor

@matthew-carroll matthew-carroll Jul 8, 2019

Choose a reason for hiding this comment

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

Is it possible that use of symbols like "&" in test names could cause complications for devs that try to execute specific tests via command line? Seems like there could be cases where this might require some kind of encoding, e.g., URL encoding, which would be an unnecessary burden...

Copy link
Contributor Author

@darrenaustin darrenaustin Jul 9, 2019

Choose a reason for hiding this comment

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

Good point, I will fix this.

@matthew-carroll
Copy link
Contributor

matthew-carroll commented Jul 8, 2019

Generally LGTM. One case where things end up conflicting is if I set the ThemeMode to system, don't bother providing a darkTheme, and then turn my device to dark mode. This will work without error, but that combination of properties would seem to be contradictory. So the one place where this conceptually breaks is that we can request a ThemeMode that can't physically be honored. I don't know if there is a good way to resolve that.

@darrenaustin
Copy link
Contributor Author

darrenaustin commented Jul 9, 2019

One case where things end up conflicting is if I set the ThemeMode to system, don't bother providing a darkTheme, and then turn my device to dark mode. This will work without error, but that combination of properties would seem to be contradictory.

I get the issue, but don't see a way to address it other than through documentation. One possibility might be to have a default value for darkTheme (say to something based on ColorScheme.dark()). However, that would potentially break apps that weren't designed for that or were purposely not supporting a dark theme.

I view the themeMode as a way for the app to tell the framework what it should do if it needs to choose between themes (which right now is always trying to honor the system preference). If the app only provides one theme, then there is nothing to choose so it doesn't apply.

@darrenaustin
Clarified some of the documentation based on PR feedback.
4548a27 
Removed '&' from test names to avoid shell quoting issues.
@matthew-carroll
Copy link
Contributor

matthew-carroll commented Jul 9, 2019

Yeah, I think this is probably the best we can do WRT to the platform vs provided themes issue. So LGTM.

@darrenaustin darrenaustin merged commit af1bd09 into flutter:master Jul 9, 2019
@darrenaustin darrenaustin deleted the theme_mode branch July 9, 2019 22:26
@darrenaustin darrenaustin mentioned this pull request Jul 17, 2019
Merged
8 tasks
johnsonmh pushed a commit to johnsonmh/flutter that referenced this pull request Jul 30, 2019
@darrenaustin @johnsonmh
Added MaterialApp.themeMode to control which theme is used. (flutter#…
00317d9 
…35499)

Added support for a themeMode property to the MaterialApp to control
how the light or dark theme is selected.
@HansMuller HansMuller mentioned this pull request Sep 6, 2019
Closed
15 tasks
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@matthew-carroll matthew-carroll matthew-carroll approved these changes

@HansMuller HansMuller Awaiting requested review from HansMuller

Assignees

No one assigned

Labels

f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@darrenaustin @matthew-carroll @googlebot