Prepare for "Auto - follow OS" option for Theme setting #5527

BigHeadCreations · 2022-10-20T15:25:50Z

(Edited to include several prep commits, but not the final main commit: #5527 (comment) )

This adds an Auto setting to the Theme selector. When Auto is selected the theme will match the theme of the OS. The Dark Mode toggle was removed and replaced with a multi option select similar to "Mark messages as read on scroll".

~~Fixes~~ Related: #4009
~~Fixes~~ Related: #5169

gnprice

Thanks @BigHeadCreations! Comments below.

You also described in a chat thread an issue this version suffers from in the message list, which we should be sure to fix before merging. I'll continue that thread there.

gnprice · 2022-10-20T16:34:36Z

src/boot/ZulipSafeAreaProvider.js

+    let themeToUse = theme;
+    if (themeToUse === 'auto') {
+      themeToUse = osScheme === 'light' || osScheme == null ? 'light' : 'dark';
+    }


This is the sort of logic I'd definitely like to keep in just one place -- it's a bit subtle, and it'd be easy for different copies of it to wind up diverging subtly as future changes get made to the code.

For that purpose it'd work fine to put it in a function somewhere, and have each of these different components call that function. But in this case there's actually a cleaner mechanism: ThemeProvider is the only one that needs to compute the theme data, and then every other component that needs it can get it from there like so:

const themeData = useContext(ThemeContext);

just like many of our components already do.

Oh hm except for this component, ZulipSafeAreaProvider. It's unusual in that it has this additional logic using getIsHydrated -- and, relatedly, appears in our React component tree above ThemeProvider, rather than below. From ZulipMobile.js, which has our top-level React component:

return ( <RootErrorBoundary> <CompatibilityChecker> <StoreProvider> <ZulipSafeAreaProvider> <StoreHydratedGate> <AppEventHandlers> <TranslationProvider> <ThemeProvider> <ActionSheetProvider> <ZulipNavigationContainer /> // ← contains the rest of the app </ActionSheetProvider> // … then just more closing tags …

So I think this component will want to invoke this logic as a function, in addition to ThemeProvider doing so. Let's put the function in a new file src/settings/settingsSelectors.js, and invoke it in these two components like const themeToUse = getThemeToUse(theme, osScheme);. See src/chat/fetchingSelectors.js for a short example of a similar kind of file.

gnprice · 2022-10-20T22:08:58Z

src/webview/css/cssPygments.js

-/* Syntax Highlighting for night-mode */
+/* Syntax Highlighting for dark-mode */
 ${
-  isNightMode
+  isDarkMode


Thanks for taking on all this renaming in the code!

gnprice · 2022-10-20T22:14:57Z

src/reduxTypes.js

-export type ThemeName = 'default' | 'night';
+/**
+ * The theme of the app.
+ *
+ * * auto: follow the OS theme setting
+ * * light
+ * * dark
+ */
+export type ThemeName = 'auto' | 'light' | 'dark';


This type describes part of the data we store persistently on the user's device. So when changing it, it's important to add a migration so that data from existing installs gets updated to reflect the new type.

See this bit of the new-feature guide:
https://github.com/zulip/zulip-mobile/blob/main/docs/howto/new-feature.md#redux-state-migrations

Because this is in the user's local settings, not something we fetch from the server, the migration won't be simply dropCache like it often is. Happily I think it will still be pretty simple. An example existing migration that's quite similar is migration 32 (about zh-TW).

Added a migration for this setting in a new commit 👍

Added a migration for this setting in a new commit 👍

But I also just added a question about one of the lines in the commit. You'll see it below here.

src/styles/theme.js

gnprice · 2022-10-20T22:29:59Z

src/common/Popup.js

  // TODO(color/theme): find a cleaner way to express this
-  const isDarkTheme = useGlobalSelector(state => getGlobalSettings(state).theme !== 'default');
+  const isDarkTheme = themeToUse !== 'light';


Ah I see, in several of these components it won't suffice to say useContext(ThemeProvider) because the latter only gives the ThemeData (with the concrete colors) and not the identity of the theme as dark or light.

As the existing comment here says, it'd be good to clean that up at some point. But for purposes of this PR, I'll be happy to let that be.

Still, let's avoid duplicating all those conditionals involved in computing themeToUse. These components can each call a central function, like the getThemeToUse described above, and let that take care of that logic.

gnprice · 2022-10-20T23:07:33Z

src/reduxTypes.js

+/**
+ * The theme of the app.
+ *
+ * * auto: follow the OS theme setting
+ * * light
+ * * dark
+ */
+export type ThemeName = 'auto' | 'light' | 'dark';


It may be helpful to have two different types here:

ThemeName for the name of a concrete specific theme -- either "light", or "dark";

ThemeSetting, say, for the value the user has chosen -- so either a ThemeName, or "auto".

Before this feature, that distinction doesn't really exist (or if it does, it's pretty abstract) because the user's choices correspond 1:1 to specific themes. But with this PR, we have some places that want a value which could be "auto" but also a bunch of places that really want only a concrete "light" or "dark". Being able to express the difference will let the type-checker help us keep track and make sure we don't accidentally let the value "auto" flow to some code that assumes it's seeing only "light" or "dark".

Done. Thanks for this idea. I think it greatly helps to differentiate where in the app it is ok to have the setting (auto, light, dark), vs what we really want in some places (only light or dark).

BigHeadCreations · 2022-10-24T14:51:12Z

src/storage/migrations.js

+    settings: {
+      ...state.settings,
+      theme:
+        state.settings.theme === 'default' || state.settings.theme === 'light' ? 'light' : 'dark',


I think this line should just have to be
state.settings.theme === 'default' ? 'light' : 'dark',
however when it was like this the test with the description "check dropCache at 53" in migrations-test.js (around line 141) was failing. That test's before and after block both test the endBase. I wasn't sure if I should have fixed that test somehow or adjusted my code to what I submitted.

Hmm, yeah. The code should indeed look like that, so the test should be fixed to pass.

Looking back at that test, I think the issue is basically that when I wrote it (in 61c11dd) I constructed the "before" data in a way that didn't really say the same thing as what the test case wants conceptually, and as a result it was susceptible to breaking with future changes like this one.

I just pushed to the tip of your PR branch a small commit (4247b9a) which should fix the issue. The fix makes sense already on top of main, so just rebase that commit to the start of your PR branch, and then you should be able to write the expected logic here and have things work.

Done. I put your commit (now 16162d9) at the beginning of this PR in the commit history and rebased my commit (b848cb6) so it has the correct migration for the theme setting. Thanks Greg.

gnprice

Thanks again @BigHeadCreations for the revision! The changes look good.

Comments below. I think most all of them will be pretty small to take care of, so this is getting close to merge.

As I mention below, now that the MessageList component is getting the theme correctly to start with, I also have a thought on how to get it updating properly, which I'll discuss in the chat thread.

gnprice · 2022-10-26T03:11:10Z

src/storage/__tests__/migrations-test.js

+      'check 54 with old setting of default',
+      { ...base52, settings: { ...base52.settings, theme: 'default' } },
+      { ...endBase, settings: { ...endBase.settings, theme: 'light' } },


So there's actually a fun little product question here: what should we do for existing installs that have default as their theme setting? I.e., that haven't enabled "night mode", as the UI currently calls it.

I think the best choice is probably that we should give them auto, not light. We don't currently have a way for the user to tell us the difference between choosing a light theme and having not made a choice -- between saying "I always want a light theme" and "thinking about this stuff is your job as the developers, I don't want to be bothered, just give me the best experience you can". Both of those are expressed as default, which means probably the bulk of users with that setting really mean the latter (because in general for all kinds of settings, typically a small fraction of users make any active choice.) So we should continue to give them the default behavior, which now means auto.

This is a good point and I agree with you.

For someone who never touches the theme setting on their phone, then having 'auto' as the default option in Zulip will still mean that they only ever see the light theme, which is what they will be used to. And if they do ever change their phone theme setting then Zulip will follow suit.

gnprice · 2022-10-26T03:22:11Z

src/storage/migrations.js

+  // Change ThemeName from 'default' or 'night' to 'light' or 'dark'
+  '54': state => ({


This migration should go in the same commit as the changes that make it necessary. That way we don't have an intermediate step in the commit history where things are broken. General discussion of this principle is here:
https://zulip.readthedocs.io/en/latest/contributing/version-control.html
(linked from the Git section of the zulip-mobile style guide: https://github.com/zulip/zulip-mobile/blob/main/docs/style.md#git )

In addition to this migration commit:
51bc424 settings: Add migration for theme setting.

the same thing applies to the one that fixes MessageList:
2915169 theme: add ThemeSetting type and calc ThemeName in MessageList

This one should also be squashed:
630838f theme: add getThemeToUse function.
for a related reason, which is that it's simplifying some code that was added in a previous commit, and if the two changes are squashed into one, the resulting changes are simpler to read than the original commit.

OTOH I do appreciate having the rename across the codebase separated out from the other, more substantive changes, as it is in the distinction between your first commit:
b86c1cc theme: Update naming for night/dark, default/light
and the rest.

I thought a bit more about how I'd ideally like the changes in this branch separated for best readability. It'd go like this:

First, introduce getThemeToUse as a stub that just returns theme, and make all the changes to components to start using it.

Then, update ThemeName and make all the corresponding renames internal to the code; getThemeToUse now translates from the old names found in the setting to the new names used in ThemeName. This is an NFC commit: https://github.com/zulip/zulip-mobile/blob/main/docs/glossary.md#nfc aka a pure refactor.

Finally, update ThemeSetting, add the migration, and make the settings UI change.

This is helpful because the boring, widespread changes now happen each in their own commit, and those commits have very little else happening, while the interesting changes that really affect behavior are all concentrated in the now-much-shorter final commit. That makes it easier both to scan through the boring changes and confirm they're boring, and to study the interesting changes to fully understand what they do.

Getting from the current revision to that other way of sequencing the changes involves some slicing and dicing using Git. In the Zulip project we get quite efficient at that; we do it regularly, because we like to keep a clean Git history. But it isn't real common in the industry, so I don't expect someone new to the project to have picked up the relevant set of Git tricks and features. And it's a potentially pretty tedious task without them.

So to save you that tedium, I've gone and done that reordering and regrouping of the changes, and pushed the branch to my GitHub clone. You can fetch it like so:

$ git remote add greg [email protected]:gnprice/zulip-mobile $ git fetch greg pr5527-reordered:refs/remotes/greg/pr5527-reordered

In total it has all the same changes that are in your PR branch (including the one small commit I pushed earlier as mentioned above at #5527 (comment) ):

# this should produce an empty diff (where me/pr-theme-follow-os is your PR branch): $ git diff me/pr-theme-follow-os greg/pr5527-reordered

Here's the split:

expand for `git log --stat upstream..` output

$ git log --reverse --no-decorate --stat upstream.. commit 16162d96e4281bf7a706fd333d6dcc25b405e805 Author: Greg Price <[email protected]> Date: Tue Oct 25 19:50:01 2022 -0700 migration tests [nfc]: Clarify the dropCache test case This version is more direct about what we're really doing with this test case, and as a result should be more robust to adding later migrations. src/storage/__tests__/migrations-test.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) commit 3a39dbeec88b81368a28713e1065a542ced35ece Author: Timothy Pearson <[email protected]> Date: Mon Oct 24 11:29:34 2022 -0700 wip theme: split setting/actual; add getThemeToUse as nop This is an NFC change except for the calls to `useColorScheme` it adds. We don't yet do anything with the result -- that's coming later in this series of commits -- but it will sometimes introduce a re-render, namely when the OS-level theme changes. src/boot/ThemeProvider.js | 7 ++++++- src/boot/ZulipSafeAreaProvider.js | 8 +++++++- src/common/Popup.js | 9 +++++++-- src/common/ZulipStatusBar.js | 9 +++++++-- src/nav/ZulipNavigationContainer.js | 8 ++++++-- src/reduxTypes.js | 27 +++++++++++++++++++++++---- src/settings/settingsSelectors.js | 7 +++++++ src/start/IosCompliantAppleAuthButton/index.js | 10 +++++++--- src/webview/MessageList.js | 7 ++++++- src/webview/backgroundData.js | 4 ++-- 10 files changed, 78 insertions(+), 18 deletions(-) commit cd7d76aeb9d751280dd7ae7eb818eb6ce109151d Author: Timothy Pearson <[email protected]> Date: Thu Oct 20 08:48:48 2022 -0600 wip theme [nfc]: rename internally to light/dark src/common/Popup.js | 2 +- src/common/ZulipStatusBar.js | 2 +- src/common/__tests__/getStatusBarColor-test.js | 12 ++++++------ src/nav/ZulipNavigationContainer.js | 4 ++-- src/reduxTypes.js | 2 +- src/settings/settingsSelectors.js | 3 +-- src/start/IosCompliantAppleAuthButton/Custom.js | 14 +++++++------- src/start/IosCompliantAppleAuthButton/index.js | 2 +- src/styles/theme.js | 7 +++---- src/webview/css/css.js | 4 ++-- src/webview/css/cssPygments.js | 8 ++++---- 11 files changed, 29 insertions(+), 31 deletions(-) commit 4c138af27e2d684aea2c951659f0d3634374a166 Author: Timothy Pearson <[email protected]> Date: Thu Oct 20 09:12:09 2022 -0600 wip theme: follow OS setting by default src/reduxTypes.js | 2 +- src/settings/SettingsScreen.js | 20 ++++++++++++++------ src/settings/__tests__/settingsReducer-test.js | 4 ++-- src/settings/settingsReducer.js | 2 +- src/settings/settingsSelectors.js | 9 +++++++-- src/storage/__tests__/migrations-test.js | 25 +++++++++++++++++++++++-- src/storage/migrations.js | 9 +++++++++ static/translations/messages_en.json | 8 ++++++-- 8 files changed, 63 insertions(+), 16 deletions(-)

Except for the intermediate versions of getThemeToUse, it's the exact same changes as in your latest revision, just reordered and regrouped.

To take the splitting from my branch, the procedure I recommend is:

git reset --hard greg/pr5527-reordered to switch to this version;

fill in the commit messages (copying text as appropriate from your current revision's commit messages);

then address the other comments in the review, squashing the fixes into the commits that have the changes they're fixing.

Thanks Greg, it was a big help for me that you sliced up those commits. Done.

gnprice · 2022-10-26T03:36:20Z

src/boot/ZulipSafeAreaProvider.js

+  const theme = useGlobalSelector(state => getGlobalSettings(state).theme);
+  const osScheme = useColorScheme();
+
  const backgroundColor = useGlobalSelector(state => {
    if (!getIsHydrated(state)) {
      // The only screen we'll be showing at this point is the loading
      // screen.  Match that screen's background.
      return BRAND_COLOR;
    }
+    const themeToUse = getThemeToUse(theme, osScheme);

-    return themeData[getGlobalSettings(state).theme].backgroundColor;
+    return themeData[themeToUse].backgroundColor;


Ah, there's a distinct reason why this getGlobalSettings call was in the spot in the control flow that it is -- a reason which probably should have had a comment here, because this component is one of the very few spots in the app that needs to worry about it.

The issue is that this component goes live before we've even read the user's data from storage, including their settings. (Because it appears above StoreHydratedGate in the React component tree, rather than below; see #5527 (comment) above.) So anything getGlobalSettings returns can't be meaningful. Nothing will blow up -- it just returns the default settings (concretely, the initialState from src/settings/settingsReducer.js) -- but if we ever used such a value as if it were the user's actual setting, that'd be a bug. That's why this getIsHydrated check is here.

This version doesn't ever actually behave incorrectly -- there's no live bug -- because it doesn't actually consult the theme variable until after the check, where it's known to be meaningful after all. But better not to have the value lying around at all while it's still potentially meaningless, to help avoid bugs in future changes.

So let's move the getGlobalSettings call back behind that check:

Suggested change

const theme = useGlobalSelector(state => getGlobalSettings(state).theme);

const osScheme = useColorScheme();

const backgroundColor = useGlobalSelector(state => {

if (!getIsHydrated(state)) {

// The only screen we'll be showing at this point is the loading

// screen. Match that screen's background.

return BRAND_COLOR;

}

const themeToUse = getThemeToUse(theme, osScheme);

return themeData[getGlobalSettings(state).theme].backgroundColor;

return themeData[themeToUse].backgroundColor;

const osScheme = useColorScheme();

const backgroundColor = useGlobalSelector(state => {

if (!getIsHydrated(state)) {

// The only screen we'll be showing at this point is the loading

// screen. Match that screen's background.

return BRAND_COLOR;

}

const theme = getGlobalSettings(state).theme;

const themeToUse = getThemeToUse(theme, osScheme);

return themeData[themeToUse].backgroundColor;

Ah I understand now. That is a bit tricky. Done.

gnprice · 2022-10-26T03:42:28Z

src/reduxTypes.js

+ * To get a ThemeName from the ThemeSetting, first check the current
+ * OS theme by calling the React Native hook useColorScheme and pass
+ * that to the helper function getThemeToUse.


Cool, this is helpful information to include in jsdoc.

gnprice · 2022-10-26T03:43:10Z

src/reduxTypes.js

+ *
+ */


formatting nit: can skip the blank line at end of jsdoc:

Suggested change

*

*/

*/

That just makes the code slightly more compact to read.

gnprice · 2022-10-26T04:02:09Z

src/reduxTypes.js

+ * * light
+ * * dark


nit: can leave these out of jsdoc -- they just repeat information the reader has from the type itself

gnprice · 2022-10-26T04:18:31Z

src/settings/settingsSelectors.js

+export const getThemeToUse = (theme: ThemeSetting, osScheme: ?ColorSchemeName): ThemeName => {
+  let themeToUse = theme;
+  if (themeToUse === 'auto') {
+    themeToUse = osScheme === 'light' || osScheme == null ? 'light' : 'dark';


This is a spot where the JS ?? operator comes in handy:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Nullish_coalescing_operator
(It's a spiffy new feature from the 2020 edition of the JS standard. As is typical for JS projects in recent years, we prevent that from being an obstacle to us using this and many other modern features by having our source code transpiled to an older baseline variety of the language in order to run on all the JS implementations we care about.)

Together with the fact that we're using the same names light/dark as appear in ColorSchemeName, the RHS here can be simplified to just osScheme ?? 'light'. Which I think is a bit more direct about what we're really saying here, as well as shorter: we want the OS's scheme, and if that information is missing we fall back to light.

Ah yes, the ?? operator makes this a bit cleaner.

gnprice · 2022-10-26T20:49:32Z

src/webview/MessageList.js

+    const osScheme = getColorScheme();
+    const themeToUse = getThemeToUse(theme, osScheme);


Great, glad this is working!

I also have a thought on how we can get this component updating properly when the theme changes, too, like the rest of them do. I'll discuss over on that chat thread.

gnprice

Thanks @BigHeadCreations for the revision! Generally this all looks good -- it's getting close to merge. Comments below, all small.

One other bonus comment, on something that isn't an obstacle to merging but would make this code simpler:

Now that we have the new themeName property on ThemeData objects, that eliminates most of the use cases we'd had for components to refer directly to the theme setting, rather than getting the theme from React context.
Before the feature this PR adds, just getting the theme from settings was only a line or two of code, so using context instead would have made things cleaner only in a bit of an abstract way. But with these changes, it becomes a bigger deal, requiring useColorScheme and getThemeToUse as well as the pure settings value.
So where possible, it'd be good to replace all that with just calling useContext(ThemeContext), much like lots of our other components do. That won't apply to all the components that call useColorScheme etc. in this revision -- notably, ThemeProvider needs to do so in order to provide the data for all those context-consumers to consume -- but it'll apply to most of them.

Because that cleanup isn't required for merging, probably what makes most sense is to first take care of the other comments below (which will be smaller to do, I think), and then make a go at this further simplification.

gnprice · 2022-10-31T22:34:04Z

src/styles/theme.js

+  themeName: ThemeName,
 |};


nit: let's put this at the top of the list when describing these objects.

It's basically the high-order bit, the summary of everything else, so it makes sense to see it first.

gnprice · 2022-10-31T22:39:58Z

src/styles/theme.js

@@ -9,16 +9,18 @@ export type ThemeData = {|
  backgroundColor: string,
  cardColor: string,
  dividerColor: string,
+  themeName: ThemeName,


Adding and using this new property is a logically separate change from introducing getThemeToUse. Relatedly, it's not really covered by the commit message:

theme: split setting/actual; add getThemeToUse as no-op This is an NFC change except for the calls to `useColorScheme` it adds. We don't yet do anything with the result -- that's coming later in this series of commits -- but it will sometimes introduce a re-render, namely when the OS-level theme changes.

So in keeping with our "clear and coherent commits" style:
https://zulip.readthedocs.io/en/latest/contributing/version-control.html
let's put it in a separate commit, either before or after this one. That will also let it have its own commit message.

Done. It did feel like I was cramming that change into this commit. I actually rewrote this commit message while you were reviewing this, but I have now added this change into its own commit with a clear commit message.

gnprice · 2022-10-31T22:50:25Z

src/reduxTypes.js

+ * OS theme by calling the React Native hook useColorScheme and pass
+ * that to the helper function getThemeToUse.
+ */
+export type ThemeName = 'light' | 'dark';


commit-message nit:

theme [nfc]: rename internally from default/night to light/dark

The summary line should be capitalized after the colon-prefix, so like:

theme [nfc]: Rename internally from default/night to light/dark

(I think the branch I provided for splitting the changes may have been confusing on this, because I had summary lines like wip theme [nfc]: rename internally to light/dark. But the lack of capitalization was one of the things making the commits WIP.)

Thanks. I think I had this correct before but missed this detail after your slice and dice help.

gnprice · 2022-10-31T22:58:31Z

src/reduxTypes.js

+ * OS theme by calling the React Native hook useColorScheme and pass
+ * that to the helper function getThemeToUse.
+ */
+export type ThemeName = 'light' | 'dark';


Also in this commit message, a bit more substantive:

theme [nfc]: rename internally from default/night to light/dark As mentioned in issue #4009, this changes how the theme of the app is described. It changes all instances of 'default' to 'light' and 'night' to 'dark'. Fixes #5169.

This commit doesn't fix #5169, because that's about what the user sees and this commit is internal to the code. The next commit does, though, so the "fixes" line should just go there.

Also on the "fixes" line, a formatting nit:
https://github.com/zulip/zulip-mobile/blob/main/docs/style.md#fixes-format

gnprice · 2022-10-31T23:03:36Z

src/storage/__tests__/migrations-test.js

      'check dropCache at 55',
-      { ...endBase, migrations: { version: 54 }, mute: [], nonsense: [1, 2, 3] },
+      // Just before the `dropCache`, plus a `cacheKeys` property, plus junk.
+      { ...base52, mute: [], nonsense: [1, 2, 3] },


Hmm, I guess I should have left that migrations bit in for explicitness. To check the dropCache at 55, and live up to this comment, we'll want to start just before that dropCache -- so at version 54.

(Before rebasing, the dropCache was at 53 and so the migrations bit would have said version 52, redundant with base52. So I left it out, but that made things less explicit.)

gnprice · 2022-10-31T23:09:49Z

src/webview/MessageList.js

+  context: ThemeData;
  webviewRef = React.createRef<React$ElementRef<typeof WebView>>();


nit: blank line here, to visually group the two context-related lines apart from these next few lines

(The next few lines are declaring/initializing local state that our own code in this class manages, whereas .context is a special name known to React that the React engine manages. So one wants to think of them in separate categories.)

gnprice · 2022-10-31T23:13:03Z

src/webview/MessageList.js

-    const { auth, theme } = backgroundData;
+    const { auth } = backgroundData;
+
    const html: string = getHtml(


nit: no new blank line here, as it doesn't correspond to a boundary between logical groups

(If anything, one wants to think of contentHtml above and html here as grouped together, and auth destructured from backgroundData as grouped with the object-destructuring from props above.)

gnprice · 2022-10-31T23:13:58Z

static/translations/messages_en.json

@@ -141,7 +141,7 @@
  "Learn more": "Learn more",
  "Full profile": "Full profile",
  "Settings": "Settings",
-  "Night mode": "Night mode",
+  "Dark mode": "Dark mode",


It looks like this string doesn't end up appearing in the UI; so we can just delete it.

gnprice · 2022-10-31T23:21:43Z

src/webview/MessageList.js

@@ -176,10 +180,11 @@ class MessageListInner extends React.Component<Props> {
    const contentHtml = messageListElementsForShownMessages
      .map(element => messageListElementHtml({ backgroundData, element, _ }))
      .join('');
-    const { auth, theme } = backgroundData;


It looks like this is the one place where we were using the theme property on this backgroundData object. So, in either this same commit or a tiny followup commit, let's drop this no-longer-used property from there, too.

Done. I added it as a small separate commit.

gnprice · 2022-11-03T19:46:20Z

Thanks @BigHeadCreations for all your work on this! I'm going to see about integrating it now.

There are a couple of nits remaining, which I'll comment separately and then fix up as I merge the changes.

There's also one larger issue, which @chrisbobbe thankfully discovered while testing the PR. This issue doesn't mainly reflect on your work in writing these changes -- it's a bug in React Native.

That bug unfortunately undermines the useColorScheme/Appearance feature of RN, the one we pointed to in #4009 to rely on in writing this PR, so that it's essentially unusable on iOS. Here's a good bug report showing the symptom:

Apparence turns light (for an instant) to when back and forth on iOS with SDK39 expo/expo#10815

and here's what looks to be the best upstream issue thread for the bug:

Appearance addChangeListener handler is called when app goes to background with wrong color scheme facebook/react-native#28525

The effect is that with this PR, if you're on iOS and you have the theme set to auto (the default), then -- just as shown in that Expo report -- every time you open the app when it was already in the background, it flashes briefly to the opposite theme and then back. Pretty jarring.

So the immediate thing we'll do is to merge the first N-1 commits of this branch -- which make various useful cleanups and refactors -- and leave out the main commit at the end, the one that actually introduces the new behavior of following the OS setting.

Then I think the next steps after that will be:

We'll finish Rename "Night mode" to "Dark theme" #5169, renaming the setting in the UI, separately.
The right API for implementing Automatically follow user's OS-level dark/light preference, take 1 #4009 on iOS looks to be DynamicColorIOS: https://reactnative.dev/docs/dynamiccolorios
- That API aligns with how native iOS apps are meant to handle Dark Mode: a UIColor value can effectively contain multiple concrete colors, one for dark mode and one for light mode. So a DynamicColorIOS value parallels that.
- (FWIW I think this architecture has real advantages over the alternative -- the architecture with "theme" objects that we currently have, that the Android SDK provides, and that this PR extends. Effectively this architecture pushes the dark/light switch declaratively down to the lower layers, whereas our current architecture has that switch at the top of the app code, so that when the theme changes a whole bunch of app code needs to rerun in order to propagate the new choice. That need to rerun is precisely the root cause of this bug Appearance addChangeListener handler is called when app goes to background with wrong color scheme facebook/react-native#28525 . The difference is very much analogous to, on the web, doing layout or animations in CSS rather than in JS -- CSS is a way of pushing a richer description declaratively down to the lower layers, and saves you from the jank one gets by trying to have JS constantly play catch-up.)
- Unfortunately, that API is not available on Android. So we have one API that's nominally available on both platforms but broken on iOS, and one that's available only on iOS. We'll need to use both.
Further, there's the message-list webview, where a number of colors are provided in CSS. Currently we handle dark mode there by conditionally including a fragment of CSS for the dark-mode colors. That conditional fits well with the "theme" architecture that we have; but DynamicColorIOS doesn't provide an answer for how to handle such CSS.
- I think the answer here will be to use some CSS with a media query, specifically @media (prefers-color-scheme: dark). That will be governed by the system setting, so it'll behave a lot like a DynamicColorIOS value.
I think the basic structure will still involve a set of theme objects:
- We'll have a "dark" and a "light" theme much like in this PR, and much like we do now. And on Android, we'll use useColorScheme to pick either the dark or the light theme, just as in this PR.
- But then on iOS, we'll also have an "auto" theme. It'll contain DynamicColorIOS values that effectively encode both the dark and the light theme.
- The bulk of the places where we're currently consulting the theme setting by hand to pick a color, and where in this PR we switch to consulting the new theme.themeName, will need to handle the "auto" theme and provide a DynamicColorIOS in that case. Probably we should take this as a prompt to refactor those so that their colors live on the theme objects.
- In MessageList, we're consulting theme.themeName to decide whether to include CSS that specifies light or dark mode. This will also need to handle the "auto" theme -- which it will do with some CSS with a prefers-color-scheme media query as mentioned above.

More precisely, the very next steps will be that I'll update #5169 and #4009 (and/or perhaps file a new issue) to reflect the above. 🙂

gnprice

Here's those remaining nits as mentioned in the previous comment. Two in the code as pointed to below, and one in the commit messages:

    Fixes #4009
    Fixes #5169

This still has the formatting nit from here: #5527 (comment)

I'll just fix these now, in the course of merging.

gnprice · 2022-11-03T19:56:24Z

src/storage/__tests__/migrations-test.js

@@ -128,7 +139,9 @@ describe('migrations', () => {
    // whether any properties outside `storeKeys` are present or not.
    [
      'check dropCache at 55',
-      { ...endBase, migrations: { version: 54 }, mute: [], nonsense: [1, 2, 3] },
+      // Just before the `dropCache`, plus a `cacheKeys` property, plus junk.
+      { ...base52, migrations: { version: 54 }, mute: [], nonsense: [1, 2, 3] },


The fix to this issue:
#5527 (comment)
appears in the final, main commit. But it should instead get squashed into the commit that introduced the issue (which is the first one in the branch) so that the issue doesn't get introduced in the first place.

That also slightly simplifies the main commit, as that commit isn't otherwise touching this spot in the code, and the fix isn't logically part of the changes it's making.

gnprice · 2022-11-03T19:58:19Z

docs/architecture/react.md

-We also confirmed this behavior experimentally, in a 2020 version of
-`MessageList` which used `ThemeContext` to get the theme colors.
-(Since 1ba871910, `MessageList` uses a transparent background and so
-doesn't need the theme; since 4fa2418b8 it doesn't mention
-`ThemeContext`.)  That component re-`render`ed when the theme changed,
-*even though its `shouldComponentUpdate` always returned `false`*.
-This didn't cause a live problem because the UI doesn't
-allow changing the theme while a `MessageList` is in the navigation
-stack. If it were possible, it would be a concern: setting a short
-interval to automatically toggle the theme, we see that the message
-list's color scheme changes as we'd want it to, but we also see the
-bad effects that `shouldComponentUpdate` returning `false` is meant to
-prevent: losing the scroll position, mainly (but also, we expect,
-discarding the image cache, etc.).
+Concretely, this means that our `MessageList` component updates
+(re-`render`s) when the theme changes, since it's a `ThemeContext`
+consumer, *even though its `shouldComponentUpdate` always returns
+`false`*. This generally isn't a problem because the UI will only
+very rarely change the theme while a `MessageList` is in the navigation
+stack (only if `ThemeSetting` is set to 'auto' and the user's OS is
+set to change theme automatically at sunrise/sunset). When this does
+happen we see that the message list's color scheme changes as we want
+it to, but we also see the bad effects that `shouldComponentUpdate`
+returning `false` is meant to prevent: losing the scroll position,
+mainly (but also, we expect, discarding the image cache, etc.).


This docs change should happen in the same commit where the code it's describing changes:
https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Auto.20OS-theme.20implementation/near/1457502

That means this commit:
774b491 theme: MessageList get theme data from context instead of props

but it currently appears in this preceding commit:
3980dca theme: Split setting/actual; add getThemeToUse as no-op
where it doesn't match the commit message and is out of place.

Ah yes, you are correct. Thanks for fixing this up.

This version is more direct about what we're really doing with this test case, and as a result should be more robust to adding later migrations.

BigHeadCreations · 2022-11-03T20:12:42Z

Here's those remaining nits as mentioned in the previous comment. Two in the code as pointed to below, and one in the commit messages:
    Fixes #4009
    Fixes #5169
This still has the formatting nit from here: #5527 (comment)

I'll just fix these now, in the course of merging.

Ah sorry about that. I think I had tunnel vision as I didn't even notice the colon. My previous version had it as Fixes #4009. and I thought your nit was referring to the ending period.

BigHeadCreations · 2022-11-03T20:18:52Z

There's also one larger issue, which @chrisbobbe thankfully discovered while testing the PR. This issue doesn't mainly reflect on your work in writing these changes -- it's a bug in React Native.

Oh wow, I agree that that is definitely unacceptable. I didn't notice this testing in the iOS Simulator, perhaps it is just present on a hardware device? Good catch @chrisbobbe 👍

gnprice · 2022-11-03T20:44:04Z

Oh wow, I agree that that is definitely unacceptable. I didn't notice this testing in the iOS Simulator, perhaps it is just present on a hardware device?

Possible, but that'd be disappointing about the fidelity of the simulator -- it doesn't seem like something that'd involve quirks of the hardware.

My guess is that you just didn't happen to trigger the issue. The repro -- switching to another app, then switching back -- is something that in normal use of the app one would do all the time, but one wouldn't necessarily do when testing a change.

It's also not something that I'd have guessed would affect this API or this feature, so wouldn't think to explicitly test. The reason switching apps causes anything theme-related to happen at all is due to a clever hack Apple has, where just after the app goes to background it briefly switches themes back and forth:
expo/expo#10815 (comment)

if i remember correctly, the reason it switches between light and dark mode is to take a screenshot of your app in both states so that it can show the appropriate version in the multitasker if between color schemes on the os and then look in the multitasker.

gnprice · 2022-11-03T20:48:47Z

(A clever hack, and one that's there purely for the sake of a very Apple-style bit of polish -- if the sun sets and your device switches to dark mode, they want the app switcher to still show a visually cohesive set of screenshots, rather than a mix of dark mode for apps you've used since the switch and light mode for apps you used before that.)

Change the way `MessageList` updates its theme. It now gets its ThemeData information from the `ThemeContext` instead of its selector props. This is helpful if the theme ever changes, by causing the component to re-render rather than get the colors out of sync. That currently isn't possible while a `MessageList` is mounted, because the theme can only change by navigating to the settings screen; but it will become possible with #4009 via the OS-level theme changing.

Now that MessageList gets the theme from the ThemeContext, the `theme` property can be removed from BackgroundData.

This is an NFC change except for the calls to `useColorScheme` it adds. We don't yet do anything with the result -- that's coming later in this series of commits -- but it will sometimes introduce a re-render, namely when the OS-level theme changes.

This resolves part of #5169: we rename the values in the internal ThemeName type, and various local identifiers. Still open from #5169 is to rename what we actually show to the user in the settings UI, and the ThemeSetting values that reflect that.

gnprice · 2022-11-03T20:59:42Z

Pushed a revision:
382556f migration tests [nfc]: Clarify the dropCache test case
f6ff91a theme: Provide name in ThemeData; switch to context in MessageList
3d7c169 msglist [nfc]: Drop theme from BackgroundData
33750f8 theme: Split setting/actual; add getThemeToUse as no-op
43acc41 theme [nfc]: Rename internally from default/night to light/dark
61d88a1 theme: Follow OS setting by default

This fixes the nits I mentioned above, and also makes a few other small changes:

Reorder so we switch MessageList to use ThemeContext before introducing getThemeToUse; this simplifies both of those changes, eliminating MessageList.js from the latter
Tweak the jsdoc on ThemeSetting so that it's accurate from the beginning, without the later changes made at the end of the branch
Tweak the react.md text a bit: clarify the mention of the UI, and allow for the user changing the OS setting manually

Next I'll drop the final commit, adjust the PR metadata accordingly, and merge.

BigHeadCreations marked this pull request as draft October 20, 2022 19:51

gnprice reviewed Oct 20, 2022

View reviewed changes

BigHeadCreations commented Oct 24, 2022

View reviewed changes

BigHeadCreations marked this pull request as ready for review October 24, 2022 20:53

gnprice reviewed Oct 26, 2022

View reviewed changes

gnprice reviewed Oct 31, 2022

View reviewed changes

gnprice reviewed Nov 3, 2022

View reviewed changes

migration tests [nfc]: Clarify the dropCache test case

382556f

This version is more direct about what we're really doing with this test case, and as a result should be more robust to adding later migrations.

BigHeadCreations added 4 commits November 3, 2022 13:54

msglist [nfc]: Drop theme from BackgroundData

3d7c169

Now that MessageList gets the theme from the ThemeContext, the `theme` property can be removed from BackgroundData.

gnprice changed the title ~~Add "Auto - follow OS" option for Theme setting~~ Prepare for "Auto - follow OS" option for Theme setting Nov 3, 2022

gnprice merged commit 43acc41 into zulip:main Nov 3, 2022

		// Change ThemeName from 'default' or 'night' to 'light' or 'dark'
		'54': state => ({

		const osScheme = getColorScheme();
		const themeToUse = getThemeToUse(theme, osScheme);

		context: ThemeData;
		webviewRef = React.createRef<React$ElementRef<typeof WebView>>();

Prepare for "Auto - follow OS" option for Theme setting #5527

Prepare for "Auto - follow OS" option for Theme setting #5527

Conversation

BigHeadCreations commented Oct 20, 2022 • edited by gnprice Loading

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice commented Nov 3, 2022 • edited Loading

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BigHeadCreations commented Nov 3, 2022

BigHeadCreations commented Nov 3, 2022

gnprice commented Nov 3, 2022

gnprice commented Nov 3, 2022

gnprice commented Nov 3, 2022

BigHeadCreations commented Oct 20, 2022 •

edited by gnprice

Loading

gnprice commented Nov 3, 2022 •

edited

Loading