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

master #34

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ThinkDigitalSoftware
Copy link

Add PlatformBrightness widget to allow automatic updating of the
app's theme when the platform changes it's brightness.

of the app's theme when the platform changes it's brightness.
@Norbert515
Copy link
Owner

How does the app get notified when the system brightness changes? And how is the PlatformBrightness integrated with the DynamicTheme? I can't seem to find a connection.

@ThinkDigitalSoftware
Copy link
Author

ThinkDigitalSoftware commented Nov 12, 2019

@Norbert515 , There was an API to be notified via callback, however, it required platform integrations and it only worked on Android Q. This was a complex solution, so I found out that there's a way to check for brightness. It's a simple builder that always rebuilds with the current platform Brightness setting. There's no notification, it just stays current anytime the widget tree rebuilds. I couldn't find anything to listen to unless I created my own stream, but it would only be updated during a build.

I tested it by putting a breakpoint in the builder of the PlatformBrightness widget and then went to settings and changed the theme. The builder runs immediately after opening the app again and it contains the new value.
The only other way to test this without switching screens is if you did split-screen and changed it, but I still believe that this widget is called.

@ThinkDigitalSoftware
Copy link
Author

And how is the PlatformBrightness integrated with the DynamicTheme? I can't seem to find a connection.

What do you mean by that? I didn't make it a part of the existing widget because of separation of concerns. DynamicTheme is for controlling the app's theme, and PlatformBrightness is for having awareness of the system setting, which is different from the app's theme

@Norbert515
Copy link
Owner

Sounds good! But should the DynamicTheme also adapt to the platform brightness (maybe with a flag to toggle that behavior)?

@ThinkDigitalSoftware
Copy link
Author

ThinkDigitalSoftware commented Dec 10, 2019

That should be up to the user. I believe it's only job should be to provide the current value, like orientation builders

@Reprevise
Copy link
Contributor

@ThinkDigitalSoftware
Copy link
Author

ThinkDigitalSoftware commented Dec 24, 2019

didPlatformChangeBrightness does look like the official way to do this actually

Oh, Nevermind. These are all methods. We're trying to provide a widget with the most recent value

@Reprevise
Copy link
Contributor

Sure, you can use onPlatformBrightnessChanged to get the most recent value.

@ThinkDigitalSoftware
Copy link
Author

I don't see how it's better than my suggestion solution. When the platformBrightness changes, MediaQuery rebuilds its descendants with that, which my widget depends on, so it works. No callbacks needed

@Reprevise
Copy link
Contributor

Okay so if I understand this correctly, the PlatformBrightness widget gets placed somewhere and provides the brightness. Right now, I'm thinking of apps like Gmail that let you choose between the system theme and dark and light mode. Would that be possible with this change?

@ThinkDigitalSoftware
Copy link
Author

Okay so if I understand this correctly, the PlatformBrightness widget gets placed somewhere and provides the brightness. Right now, I'm thinking of apps like Gmail that let you choose between the system theme and dark and light mode. Would that be possible with this change?

That's what the original DynamicTheme offers already. The ability to change the apps's theme manually and dynamically. The new widget I'm proposing taps into the system's brightness mode and returns that. You can use this to make your app switch themes based on the system brightness instead of manually. There are some apps, like Google search, that automatically turn to dark mode if the system UI is dark. or they have manual modes too.

@Reprevise
Copy link
Contributor

I forked the repo and added your changes to it. I also added to the example. I think I'm doing it wrong but your builder always returned Brightness.light. It would be great if you could check it out and run it when you get time :)

@ThinkDigitalSoftware
Copy link
Author

ThinkDigitalSoftware commented Dec 24, 2019

What are you doing to view the changes? You have to go into system settings on the phone and change the theme to dark mode . This is an android pie or q thing

@Reprevise
Copy link
Contributor

I'm running an emulator w/ Android 10 and changing the theme to dark mode.

@ThinkDigitalSoftware
Copy link
Author

I'm running an emulator w/ Android 10 and changing the theme to dark mode.

OK, let me retest

@ThinkDigitalSoftware
Copy link
Author

Here's my test on a pixel physical device on Q.
20191224_122425

@Reprevise
Copy link
Contributor

Can I see your code?

@ThinkDigitalSoftware
Copy link
Author

ThinkDigitalSoftware commented Dec 24, 2019

Sure, I just pushed another commit with the example added
https://github.com/Norbert515/dynamic_theme/blob/0cedc2233014ca9f74aa7b1a37ce1fb0c0bb5fe0/example/lib/main.dart is the example file

@Reprevise
Copy link
Contributor

Alright, I see. Only issue I see is that it doesn't work above MaterialApp so users are going to have to wrap all of their routes with a consumer or the PlatformBrightness widget

@ThinkDigitalSoftware
Copy link
Author

It works for me ...
Screen Shot 2019-12-24 at 1 49 02 PM

@Reprevise
Copy link
Contributor

Not for me:
image

@ThinkDigitalSoftware
Copy link
Author

OK, I was expecting to see an error. It does show Light for me as well. One sec

@ThinkDigitalSoftware
Copy link
Author

I don't know how to resolve this cause it requires a WidgetsApp class above it.

@Reprevise
Copy link
Contributor

How does Flutter do it with ThemeMode (property of MaterialApp) then?

@ThinkDigitalSoftware
Copy link
Author

ThinkDigitalSoftware commented Dec 24, 2019

Because they access the variable you set there or default it to ThemeMode.system. Inside the widget, it uses MediaQuery.platformBrightnessOf(context); which works because this is separated by a builder and the MediaQuery is already in the widget tree by that time.

  builder: (BuildContext context, Widget child) {
        // Use a light theme, dark theme, or fallback theme.
        final ThemeMode mode = widget.themeMode ?? ThemeMode.system;
        ThemeData theme;
        if (widget.darkTheme != null) {
          final ui.Brightness platformBrightness = MediaQuery.platformBrightnessOf(context);
          if (mode == ThemeMode.dark ||
              (mode == ThemeMode.system && platformBrightness == ui.Brightness.dark)) {
            theme = widget.darkTheme;
          }
        }

@ThinkDigitalSoftware
Copy link
Author

@Reprevise best thing I can do is make a warning or throw an error if it's put above the MaterialApp, mentioning that there's no ancestor found, since MediaQuery by default returns Brightness.light if it can't find the value

@Reprevise
Copy link
Contributor

Alright, sounds like the best option

@shinriyo
Copy link

shinriyo commented Oct 7, 2020

@ThinkDigitalSoftware Conflicting files

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants