-
-
Notifications
You must be signed in to change notification settings - Fork 105
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
Follow the system theme by default #140
base: master
Are you sure you want to change the base?
Conversation
f92e177
to
c11d2f0
Compare
c11d2f0
to
78669d0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @mrkrash,
Overall, great stuff. It generally works, but because you are still effectively returning a boolean from the themeMode getter, the settings page breaks when you restart Anytime.
It would also be more consistent with other parts of the UI it the theme was not a drop down box, but a modal selector in the same style as the 'Auto update episodes' and 'Search provider' options.
This change also needs translations. I can help with this, but generally for short phrases I use a combination of Google Translate & Bing Translator. This change will need translations for Italian & German.
|
||
return theme == 'dark'; | ||
String get themeMode { | ||
var value = _sharedPreferences.getString('theme') ?? ThemeMode.system.name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You fetch the stored preference and default to system if not set, which is good.
if (value == ThemeMode.system.name) { | ||
var brightness = SchedulerBinding.instance.platformDispatcher | ||
.platformBrightness; | ||
return brightness == Brightness.dark ? ThemeMode.dark.name : ThemeMode.light.name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But here, you effectively overwrite system with dark or light depending upon the current system theme. Whilst you have changed the return type from bool to String, you are effectively still returning a boolean of either light or dark. This works fine for setting the overall theme, but breaks the setting page.
lib/ui/settings/settings.dart
Outdated
onSelected: (value) { | ||
settingsBloc.themeMode(value!); | ||
}, | ||
dropdownMenuEntries: [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you set the theme to Auto and restart Anytime, when you go back into settings rather than Auto being selected either Light or Dark will be selected depending upon the current theme.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, as behaviour is abnormal. I fix it.
80ba5aa
to
e3b7b8d
Compare
38ef476
to
23eeeb9
Compare
I think that now work correctly. Probably need to update the labels and translate the German |
Thanks @mrkrash - I will try and review this over the weekend. |
Add settings and behaviors to choose between dark light or follow system theme
Close #87