settings: Add option to mark read on scroll only in conversation views

src/common/InputRowRadioButtons.js

@@ -1,5 +1,5 @@
 /* @flow strict-local */
-import React, { useCallback, useRef, useMemo, useEffect } from 'react';
+import React, { useCallback, useRef, useMemo, useEffect, useContext } from 'react';
 import type { Node } from 'react';
 import { View } from 'react-native';
 import invariant from 'invariant';
@@ -8,11 +8,12 @@ import { CommonActions } from '@react-navigation/native';
 import type { LocalizableText } from '../types';
 import type { GlobalParamList } from '../nav/globalTypes';
 import { randString } from '../utils/misc';
-import { BRAND_COLOR, createStyleSheet } from '../styles';
+import { createStyleSheet } from '../styles';
 import Touchable from './Touchable';
 import ZulipTextIntl from './ZulipTextIntl';
 import { IconRight } from './Icons';
 import type { AppNavigationMethods } from '../nav/AppNavigator';
+import { ThemeContext } from '../styles/theme';
 
 type Item<TKey> = $ReadOnly<{|
   key: TKey,
@@ -57,7 +58,7 @@ type Props<TItemKey> = $ReadOnly<{|
 |}>;
 
 /**
- * A form-input row for the user to make a choice, radio-button style.
+ * An input row for the user to make a choice, radio-button style.
  *
  * Shows the current value (the selected item), represented as the item's
  * `.title`. When tapped, opens the selectable-options screen, where the
@@ -67,7 +68,9 @@ type Props<TItemKey> = $ReadOnly<{|
 // represented by IconRight. NestedNavRow would probably be the wrong
 // abstraction, though, because it isn't an imput component; it doesn't have
 // a value to display.
-export default function InputRowRadioButtons<TItemKey: string>(props: Props<TItemKey>): Node {
+export default function InputRowRadioButtons<TItemKey: string | number>(
+  props: Props<TItemKey>,
+): Node {
   const { navigation, label, description, valueKey, items, onValueChange } = props;
 
   const screenKey: string = useRef(`selectable-options-${randString()}`).current;
@@ -128,6 +131,7 @@ export default function InputRowRadioButtons<TItemKey: string>(props: Props<TIte
   // It'll also be its width.
   const kRightArrowIconSize = 24;
 
+  const themeData = useContext(ThemeContext);
   const styles = useMemo(
     () =>
       createStyleSheet({
@@ -136,6 +140,10 @@ export default function InputRowRadioButtons<TItemKey: string>(props: Props<TIte
           alignItems: 'center',
           paddingVertical: 8,
           paddingHorizontal: 16,
+
+          // Minimum touch target height (and width):
+          //   https://material.io/design/usability/accessibility.html#layout-and-typography
+          minHeight: 48,
         },
         textWrapper: {
           flex: 1,
@@ -164,7 +172,7 @@ export default function InputRowRadioButtons<TItemKey: string>(props: Props<TIte
           <ZulipTextIntl text={selectedItem.title} style={styles.valueTitle} />
         </View>
         <View style={styles.iconRightWrapper}>
-          <IconRight size={kRightArrowIconSize} color={BRAND_COLOR} />
+          <IconRight size={kRightArrowIconSize} color={themeData.color} />
         </View>
       </View>
     </Touchable>

src/common/NestedNavRow.js

@@ -37,6 +37,11 @@ export default function NestedNavRow(props: Props): Node {
     () =>
       createStyleSheet({
         container: {
+          flexDirection: 'row',
+          alignItems: 'center',
+          paddingVertical: 8,
+          paddingHorizontal: 16,
+
           // Minimum touch target height (and width):
           //   https://material.io/design/usability/accessibility.html#layout-and-typography
           minHeight: 48,
@@ -60,7 +65,7 @@ export default function NestedNavRow(props: Props): Node {
 
   return (
     <Touchable onPress={onPress}>
-      <View style={[styles.container, globalStyles.listItem]}>
+      <View style={styles.container}>
         {!!Icon && <Icon size={24} style={styles.iconFromProps} />}
         <ZulipTextIntl style={styles.label} text={label} />
         <View style={globalStyles.rightItem}>

src/common/SelectableOptionsScreen.js

@@ -78,7 +78,9 @@ LogBox.ignoreLogs([/selectable-options > params\.onRequestSelectionChange \(Func
 // If we need separate components dedicated to checkboxes and radio buttons,
 // we can split this. Currently it's up to the caller to enforce the
 // radio-button invariant (exactly one item selected) if they want to.
-export default function SelectableOptionsScreen<TItemKey: string>(props: Props<TItemKey>): Node {
+export default function SelectableOptionsScreen<TItemKey: string | number>(
+  props: Props<TItemKey>,
+): Node {
   const { route } = props;
   const { title, description, items, onRequestSelectionChange } = route.params;
 

src/common/SwitchRow.js

@@ -17,9 +17,14 @@ type Props = $ReadOnly<{|
 
 const componentStyles = createStyleSheet({
   container: {
+    flexDirection: 'row',
+    alignItems: 'center',
+    paddingVertical: 8,
+    paddingHorizontal: 16,
+
     // For uniformity with other rows this might share a screen with, e.g.,
-    // NestedNavRow on the settings screen. See height-related attributes on
-    // those rows.
+    // NestedNavRow and InputRowRadioButtons on the settings screen. See
+    // height-related attributes on those rows.
     minHeight: 48,
   },
   icon: {
@@ -37,7 +42,7 @@ export default function SwitchRow(props: Props): Node {
   const themeContext = useContext(ThemeContext);
 
   return (
-    <View style={[componentStyles.container, styles.listItem]}>
+    <View style={componentStyles.container}>
       {!!Icon && <Icon size={24} style={[componentStyles.icon, { color: themeContext.color }]} />}
       <ZulipTextIntl text={label} style={styles.flexed} />
       <View style={styles.rightItem}>

src/reduxTypes.js

@@ -397,7 +397,7 @@ export type GlobalSettingsState = $ReadOnly<{
 
   // Possibly this should be per-account.  If so it should probably be put
   // on the server, so it can also be cross-device for the account.
-  doNotMarkMessagesAsRead: boolean,
+  markMessagesReadOnScroll: 'always' | 'never' | 'conversation-views-only',
 
   ...
 }>;

src/settings/SettingsScreen.js

@@ -8,6 +8,7 @@ import type { AppNavigationProp } from '../nav/AppNavigator';
 import { useGlobalSelector, useDispatch } from '../react-redux';
 import { getGlobalSettings } from '../selectors';
 import NestedNavRow from '../common/NestedNavRow';
+import InputRowRadioButtons from '../common/InputRowRadioButtons';
 import SwitchRow from '../common/SwitchRow';
 import Screen from '../common/Screen';
 import {
@@ -27,8 +28,8 @@ type Props = $ReadOnly<{|
 export default function SettingsScreen(props: Props): Node {
   const theme = useGlobalSelector(state => getGlobalSettings(state).theme);
   const browser = useGlobalSelector(state => getGlobalSettings(state).browser);
-  const doNotMarkMessagesAsRead = useGlobalSelector(
-    state => getGlobalSettings(state).doNotMarkMessagesAsRead,
+  const markMessagesReadOnScroll = useGlobalSelector(
+    state => getGlobalSettings(state).markMessagesReadOnScroll,
   );
   const dispatch = useDispatch();
   const { navigation } = props;
@@ -47,11 +48,23 @@ export default function SettingsScreen(props: Props): Node {
           dispatch(setGlobalSettings({ browser: value ? 'embedded' : 'external' }));
         }}
       />
-      <SwitchRow
-        label="Do not mark messages read on scroll"
-        value={doNotMarkMessagesAsRead}
+      <InputRowRadioButtons
+        navigation={navigation}
+        label="Mark messages as read on scroll"
+        description="When scrolling through messages, should they automatically be marked as read?"
+        valueKey={markMessagesReadOnScroll}
+        items={[
+          { key: 'always', title: 'Always' },
+          { key: 'never', title: 'Never' },
+          {
+            key: 'conversation-views-only',
+            title: 'Only in conversation views',
+            subtitle:
+              'Messages will be marked as read in single-topic or private-message views, but not in interleaved views, such as whole-stream views.',
+          },
+        ]}
         onValueChange={value => {
-          dispatch(setGlobalSettings({ doNotMarkMessagesAsRead: value }));
+          dispatch(setGlobalSettings({ markMessagesReadOnScroll: value }));
         }}
       />
       <NestedNavRow

src/settings/settingsReducer.js

@@ -18,7 +18,7 @@ const initialState: SettingsState = {
   theme: 'default',
   browser: 'default',
   experimentalFeaturesEnabled: false,
-  doNotMarkMessagesAsRead: false,
+  markMessagesReadOnScroll: 'always',
 
   //
   // PerAccountSettingsState

src/storage/__tests__/migrations-test.js

@@ -84,10 +84,27 @@ describe('migrations', () => {
     },
   };
 
+  // What `base` becomes after migrations up through 52.
+  const base52 = {
+    ...base37,
+    migrations: { version: 52 },
+    settings: {
+      language: 'en',
+      theme: 'default',
+      offlineNotification: true,
+      onlineNotification: true,
+      experimentalFeaturesEnabled: false,
+      streamNotification: false,
+      browser: 'default',
+      // renamed/retyped from boolean doNotMarkMessagesAsRead
+      markMessagesReadOnScroll: 'always',
+    },
+  };
+
   // What `base` becomes after all migrations.
   const endBase = {
-    ...base37,
-    migrations: { version: 51 },
+    ...base52,
+    migrations: { version: 52 },
   };
 
   for (const [desc, before, after] of [
@@ -206,12 +223,12 @@ describe('migrations', () => {
     [
       'check 37 with setting already false',
       { ...base15, settings: { ...base15.settings, doNotMarkMessagesAsRead: false } },
-      { ...endBase, settings: { ...endBase.settings, doNotMarkMessagesAsRead: false } },
+      { ...endBase, settings: { ...endBase.settings, markMessagesReadOnScroll: 'always' } },
     ],
     [
       'check 37 with setting already true',
       { ...base15, settings: { ...base15.settings, doNotMarkMessagesAsRead: true } },
-      { ...endBase, settings: { ...endBase.settings, doNotMarkMessagesAsRead: true } },
+      { ...endBase, settings: { ...endBase.settings, markMessagesReadOnScroll: 'never' } },
     ],
     [
       'check 38',
@@ -241,6 +258,24 @@ describe('migrations', () => {
         drafts: { 'topic:8:stuff': 'text', 'stream:8': 'more text', 'pm:12': 'pm text' },
       },
     ],
+    [
+      'check 52 with old setting false',
+      {
+        ...base37,
+        migrations: { version: 51 },
+        settings: { ...base37.settings, doNotMarkMessagesAsRead: false },
+      },
+      { ...endBase, settings: { ...endBase.settings, markMessagesReadOnScroll: 'always' } },
+    ],
+    [
+      'check 52 with old setting true',
+      {
+        ...base37,
+        migrations: { version: 51 },
+        settings: { ...base37.settings, doNotMarkMessagesAsRead: true },
+      },
+      { ...endBase, settings: { ...endBase.settings, markMessagesReadOnScroll: 'never' } },
+    ],
   ]) {
     /* eslint-disable no-loop-func */
     test(desc, async () => {

src/storage/migrations.js

@@ -371,12 +371,16 @@ const migrationsInner: {| [string]: (LessPartialState) => LessPartialState |} =
   // Handle explicitly the migrations above (before 30, 34, 36, and this
   // one) that were done implicitly by the behavior of `autoRehydrate` on a
   // REHYDRATE action.
+  /* $FlowIgnore[prop-missing]: `doNotMarkMessagesAsRead` renamed to
+     `markMessagesReadOnScroll` in 52 */
   '37': state => ({
     // This handles the migrations affecting RealmState, before 34, 36, and here.
     ...dropCache(state),
     // Handle the migration before 30.
     settings: {
       ...state.settings,
+      /* $FlowIgnore[prop-missing]: `doNotMarkMessagesAsRead` renamed to
+         `markMessagesReadOnScroll` in 52 */
       doNotMarkMessagesAsRead: state.settings.doNotMarkMessagesAsRead ?? false,
     },
   }),
@@ -443,6 +447,19 @@ const migrationsInner: {| [string]: (LessPartialState) => LessPartialState |} =
   // Add serverEmojiData to state.realm.
   '51': dropCache,
 
+  // Change boolean doNotMarkMessagesAsRead to enum markMessagesReadOnScroll
+  '52': state => {
+    // $FlowIgnore[prop-missing]
+    const { doNotMarkMessagesAsRead, ...restSettings } = state.settings;
+    return {
+      ...state,
+      settings: {
+        ...restSettings,
+        markMessagesReadOnScroll: (doNotMarkMessagesAsRead: boolean) ? 'never' : 'always',
+      },
+    };
+  },
+
   // TIP: When adding a migration, consider just using `dropCache`.
   //   (See its jsdoc for guidance on when that's the right answer.)
 };

src/styles/miscStyles.js

@@ -12,10 +12,8 @@ export const statics = {
   listItem: {
     flexDirection: 'row',
     alignItems: 'center',
-    paddingTop: 8,
-    paddingBottom: 8,
-    paddingLeft: 16,
-    paddingRight: 16,
+    paddingVertical: 8,
+    paddingHorizontal: 16,
   },
   flexed: {
     flex: 1,

src/webview/MessageList.js

@@ -35,8 +35,9 @@ import { handleWebViewOutboundEvent } from './handleOutboundEvents';
 import { base64Utf8Encode } from '../utils/encoding';
 import * as logging from '../utils/logging';
 import { tryParseUrl } from '../utils/url';
-import { caseNarrow } from '../utils/narrow';
+import { caseNarrow, isConversationNarrow } from '../utils/narrow';
 import { type BackgroundData, getBackgroundData } from './backgroundData';
+import { ensureUnreachable } from '../generics';
 
 type OuterProps = $ReadOnly<{|
   narrow: Narrow,
@@ -300,7 +301,20 @@ const MessageList: ComponentType<OuterProps> = connect<SelectorProps, _, _>(
       ),
       typingUsers: getCurrentTypingUsers(state, props.narrow),
       doNotMarkMessagesAsRead:
-        !marksMessagesAsRead(props.narrow) || globalSettings.doNotMarkMessagesAsRead,
+        !marksMessagesAsRead(props.narrow)
+        || (() => {
+          switch (globalSettings.markMessagesReadOnScroll) {
+            case 'always':
+              return false;
+            case 'never':
+              return true;
+            case 'conversation-views-only':
+              return !isConversationNarrow(props.narrow);
+            default:
+              ensureUnreachable(globalSettings.markMessagesReadOnScroll);
+              return false;
+          }
+        })(),
     };
   },
 )(connectActionSheet(withGetText(MessageListInner)));

src/webview/__tests__/generateInboundEvents-test.js

@@ -15,7 +15,7 @@ describe('generateInboundEvents', () => {
     messages: [],
     messageListElementsForShownMessages: [],
     typingUsers: [],
-    doNotMarkMessagesAsRead: eg.baseReduxState.settings.doNotMarkMessagesAsRead,
+    doNotMarkMessagesAsRead: false,
   });
 
   type FudgedProps = {|

static/translations/messages_en.json

@@ -283,7 +283,12 @@
   "Active": "Active",
   "Idle": "Idle",
   "Offline": "Offline",
-  "Do not mark messages read on scroll": "Do not mark messages read on scroll",
+  "Mark messages as read on scroll": "Mark messages as read on scroll",
+  "When scrolling through messages, should they automatically be marked as read?": "When scrolling through messages, should they automatically be marked as read?",
+  "Always": "Always",
+  "Never": "Never",
+  "Only in conversation views": "Only in conversation views",
+  "Messages will be marked as read in single-topic or private-message views, but not in interleaved views, such as whole-stream views.": "Messages will be marked as read in single-topic or private-message views, but not in interleaved views, such as whole-stream views.",
   "Topics": "Topics",
   "Add subscribers": "Add subscribers",
   "Subscribe": "Subscribe",