[BEEEP] [PS-940] Support for dark theme selection while using Default (System) theme by mpbw2 · Pull Request #1959 · bitwarden/android

mpbw2 · 2022-06-17T19:00:29Z

Type of change

Objective

Now you can select which dark theme to use when using the Default (System) (automatic light/dark) theme. (Previously only the "Dark" theme was used).

When Default (System) is the selected theme, a new Default Dark Theme option appears immediately below the Theme option. It defaults to "Dark" but allows "Black" and "Nord" to be selected as well. When the device/OS is in dark mode, the selected dark theme becomes active. This section is only visible while the Default (System) theme is selected.

Code changes

OptionsPage.xaml/cs/ViewModel: Added the UI & selection control logic for the Options screen within Settings
ThemeManager.cs: Added autoDarkTheme arg & logic
StateService.cs: Added get/save methods for AutoDarkTheme
Constants.cs: Added AutoDarkThemeKey for storage

Screenshots

System/OS in light mode with `Default (System)` theme & custom default dark themes selected:

System/OS in dark mode with `Default (System)` theme & custom default dark themes selected:

Light & dark themes selected, `Default Dark Theme` section not visible

Before you submit

I have checked for formatting errors (dotnet tool run dotnet-format --check) (required)
I have added unit tests where it makes sense to do so (encouraged but not required)
This change requires a documentation update (notify the documentation team)
This change has particular deployment requirements (notify the DevOps team)

fedemkr · 2022-06-21T14:24:49Z

src/App/Pages/Settings/OptionsPage.xaml.cs

            _vm = BindingContext as OptionsPageViewModel;
            _vm.Page = this;
            _themePicker.ItemDisplayBinding = new Binding("Value");
+            _autoDarkThemePicker.ItemDisplayBinding = new Binding("Value");


Maybe I'm unaware of some issue but wouldn't it be possible to put the binding on the xaml page? (as well as the others)

Tbh I'm not sure - I'm following the established pattern since we're going to rebuild Settings at some point anyway and I didn't want to create new issues before then.

fedemkr · 2022-06-21T14:35:14Z

src/App/Pages/Settings/OptionsPage.xaml.cs

            else
            {
                _themePicker.On<iOS>().SetUpdateMode(UpdateMode.WhenFinished);
+                _autoDarkThemePicker.On<iOS>().SetUpdateMode(UpdateMode.WhenFinished);


The On<iOS> here is redundant to put it on the else (given that we only have Android and iOS as platforms).
We could just remove the else and let the code be there as is, or remove the On<iOS>. (I'd just remove the else). BTW, this can also be done on xaml if we'd want to here the how to.
Maybe we should look into all the places where it's used; but we could just add a custom control CustomPicker or something like that, that we can use to set this update mode by default and just use it like that. I see that there are a lot of places where this is being set up. Also, we could you the CustomPickerRenderer of iOS to set this up as well globally. But we need to be careful maybe there are some specific places where we don't want update mode when finished.

Same comment here (following existing pattern for safety/sanity until we rebuild Settings)

fedemkr · 2022-06-21T15:01:29Z

src/App/Pages/Settings/OptionsPageViewModel.cs

+            AutoDarkThemeOptions = new List<KeyValuePair<string, string>>
+            {
+                new KeyValuePair<string, string>("dark", AppResources.Dark),
+                new KeyValuePair<string, string>("black", AppResources.Black),
+                new KeyValuePair<string, string>("nord", "Nord"),
+            };


I think we should use Tuple instead of KeyValuePair here you have a gist with performance comparison. There are some caveats regarding allocation where KeyValuePair is better but in this case we don't care much given that there are just a few items.
It doesn't matter much with these few items but I also think it's more readable with Tuples.
What do you think?

KeyValuePair seems more appropriate to me here since that's what the values are, vs Tuple which doesn't suggest a relation among the values. If the data set was larger maybe it would be worth the change, but as is I don't see the benefit.

fedemkr · 2022-06-21T15:03:58Z

src/App/Pages/Settings/OptionsPageViewModel.cs

+                        additionalPropertyNames: new[] { nameof(ShowAutoDarkThemeOptions) })
+                   )
+                {
+                    var task = SaveThemeAsync();


here there's no need to assign the returned Task to a variable. And I've checked there's no try catch on SaveThemeAsync so if any exceptions are raised are going to be swollen by the Task. So either we add the try catch or just call fire and forget here: SaveThemeAsync().FireAndForget() (just be careful if it switches to a background thread and maybe when calling it and there are things on SaveThemeAsync that may need to be marshalled to the Main thread, so test it a few times just in case)

fedemkr · 2022-06-21T15:12:51Z

src/App/Pages/Settings/OptionsPageViewModel.cs

            DisableFavicon = (await _stateService.GetDisableFaviconAsync()).GetValueOrDefault();
            var theme = await _stateService.GetThemeAsync();
            ThemeSelectedIndex = ThemeOptions.FindIndex(k => k.Key == theme);
+            var autoDarkTheme = await _stateService.GetAutoDarkThemeAsync() ?? "dark";


I'd really like if theme strings (like "dark", "nord", and so on) would be consts (or an Enum even better) in ThemeManager that can be referenced. So that there is less chance of misspelling and if for some reason we change any of them it's easier to replace.
Maybe not for this PR, but we should keep this in mind.
What do you think?

fedemkr · 2022-06-21T15:17:37Z

src/App/Pages/Settings/OptionsPageViewModel.cs

+                var autoDarkTheme = AutoDarkThemeOptions[AutoDarkThemeSelectedIndex].Key;
+                await _stateService.SetAutoDarkThemeAsync(autoDarkTheme);


Could we inline these?

await _stateService.SetAutoDarkThemeAsync(AutoDarkThemeOptions[AutoDarkThemeSelectedIndex].Key);

Or is it too long?

fedemkr · 2022-06-21T15:19:41Z

src/App/Resources/AppResources.resx

+  <data name="AutoDarkTheme" xml:space="preserve">
+    <value>Default Dark Theme</value>
+  </data>


IMO, resource strings keys should be the same (when not too long) as the value to be easier to read in the code without the need to go to the resx file to check what is the value. So here I'd use DefaultDarkTheme as the key.
Furthermore if for some reason in the future we have a value "Auto Dark Theme" we couldn't use the key as it'd be used here.

fedemkr · 2022-06-21T15:20:34Z

src/App/Resources/AppResources.resx

+  <data name="AutoDarkThemeDescription" xml:space="preserve">
+    <value>Choose the dark theme to use when using Default (System) theme while your device's dark mode is enabled</value>
+  </data>


Even though, here I think it's ok to have a key like this, if we change the above this should change to DefaultDarkThemeDescription

support for dark theme selection while using Default (System) theme

3c9ea1a

mpbw2 requested review from a team, andrebispo5, fedemkr and vvolkgang June 17, 2022 19:00

mpbw2 changed the title ~~[BEEEP] Support for dark theme selection while using Default (System) theme~~ [BEEEP] [PS-940] Support for dark theme selection while using Default (System) theme Jun 17, 2022

fedemkr requested changes Jun 21, 2022

View reviewed changes

refinements

04bceda

mpbw2 requested a review from fedemkr June 21, 2022 18:38

fedemkr approved these changes Jun 21, 2022

View reviewed changes

mpbw2 merged commit 109aeb4 into master Jun 21, 2022

mpbw2 deleted the feature-autodarkthemeselect branch June 21, 2022 18:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BEEEP] [PS-940] Support for dark theme selection while using Default (System) theme#1959

[BEEEP] [PS-940] Support for dark theme selection while using Default (System) theme#1959
mpbw2 merged 2 commits intomasterfrom
feature-autodarkthemeselect

mpbw2 commented Jun 17, 2022

Uh oh!

fedemkr Jun 21, 2022

Uh oh!

mpbw2 Jun 21, 2022

Uh oh!

fedemkr Jun 21, 2022

Uh oh!

mpbw2 Jun 21, 2022

Uh oh!

fedemkr Jun 21, 2022

Uh oh!

mpbw2 Jun 21, 2022

Uh oh!

fedemkr Jun 21, 2022

Uh oh!

fedemkr Jun 21, 2022

Uh oh!

fedemkr Jun 21, 2022

Uh oh!

fedemkr Jun 21, 2022

Uh oh!

fedemkr Jun 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		var autoDarkTheme = AutoDarkThemeOptions[AutoDarkThemeSelectedIndex].Key;
		await _stateService.SetAutoDarkThemeAsync(autoDarkTheme);

Conversation

mpbw2 commented Jun 17, 2022

Type of change

Objective

Code changes

Screenshots

System/OS in light mode with Default (System) theme & custom default dark themes selected:

System/OS in dark mode with Default (System) theme & custom default dark themes selected:

Light & dark themes selected, Default Dark Theme section not visible

Before you submit

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

System/OS in light mode with `Default (System)` theme & custom default dark themes selected:

System/OS in dark mode with `Default (System)` theme & custom default dark themes selected:

Light & dark themes selected, `Default Dark Theme` section not visible