settings_page: Add marks as read on scroll options

package.json

@@ -27,6 +27,7 @@
     "@react-native-community/masked-view": "^0.1.10",
     "@react-native-community/netinfo": "6.0.0",
     "@react-native-community/push-notification-ios": "^1.5.0",
+    "@react-native-picker/picker": "^2.4.1",
     "@react-navigation/bottom-tabs": "npm:@zulip/react-navigation-bottom-tabs@5.11.16-0.zulip.1",
     "@react-navigation/drawer": "^5.9.3",
     "@react-navigation/material-top-tabs": "^5.2.19",

src/reduxTypes.js

@@ -344,7 +344,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,
+  shouldMarkAsReadOnScroll: 'never' | 'always' | 'conversation',
 
   ...
 }>;

src/settings/SettingsScreen.js

@@ -2,14 +2,17 @@
 
 import React, { useCallback } from 'react';
 import type { Node } from 'react';
-
+import { View } from 'react-native';
+// $FlowFixMe[untyped-import]
+import { Picker } from '@react-native-picker/picker';
 import type { RouteProp } from '../react-navigation';
 import type { MainTabsNavigationProp } from '../main/MainTabsScreen';
 import * as NavigationService from '../nav/NavigationService';
 import { useGlobalSelector, useDispatch } from '../react-redux';
 import { getGlobalSettings } from '../selectors';
 import NestedNavRow from '../common/NestedNavRow';
 import SwitchRow from '../common/SwitchRow';
+import ZulipTextIntl from '../common/ZulipTextIntl';
 import Screen from '../common/Screen';
 import {
   IconDiagnostics,
@@ -34,8 +37,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 shouldMarkAsReadOnScroll = useGlobalSelector(
+    state => getGlobalSettings(state).shouldMarkAsReadOnScroll,
   );
   const dispatch = useDispatch();
 
@@ -53,13 +56,32 @@ export default function SettingsScreen(props: Props): Node {
           dispatch(setGlobalSettings({ browser: value ? 'embedded' : 'external' }));
         }}
       />
-      <SwitchRow
-        label="Do not mark messages read on scroll"
-        value={doNotMarkMessagesAsRead}
-        onValueChange={value => {
-          dispatch(setGlobalSettings({ doNotMarkMessagesAsRead: value }));
+      <View
+        style={{
+          flex: 1,
+          flexDirection: 'row',
+          alignContent: 'space-around',
+          flexWrap: 'wrap',
+          paddingLeft: 15,
         }}
-      />
+      >
+        <ZulipTextIntl
+          style={{ fontSize: 15, alignSelf: 'center' }}
+          text="Mark as read on scroll"
+        />
+        <Picker
+          selectedValue={shouldMarkAsReadOnScroll}
+          onValueChange={(itemValue, itemIndex) => {
+            dispatch(setGlobalSettings({ shouldMarkAsReadOnScroll: itemValue }));
+          }}
+          style={{ height: '100%', width: '50%' }}
+        >
+          <Picker.Item label="Always" value="always" />
+          <Picker.Item label="Conversation" value="conversation" />
+          <Picker.Item label="Never" value="never" />
+        </Picker>
+      </View>
+
       <NestedNavRow
         Icon={IconNotifications}
         label="Notifications"

src/settings/settingsReducer.js

@@ -15,7 +15,7 @@ const initialState: SettingsState = {
   experimentalFeaturesEnabled: false,
   streamNotification: false,
   browser: 'default',
-  doNotMarkMessagesAsRead: false,
+  shouldMarkAsReadOnScroll: 'always',
 };
 
 export default (state: SettingsState = initialState, action: Action): SettingsState => {

src/storage/__tests__/migrations-test.js

@@ -80,7 +80,7 @@ describe('migrations', () => {
       streamNotification: false,
       // added:
       browser: 'default',
-      doNotMarkMessagesAsRead: false,
+      shouldMarkAsReadOnScroll: 'always',
     },
   };
 
@@ -204,14 +204,19 @@ describe('migrations', () => {
     ],
     // 36 covered by whole
     [
-      'check 37 with setting already false',
-      { ...base15, settings: { ...base15.settings, doNotMarkMessagesAsRead: false } },
-      { ...endBase, settings: { ...endBase.settings, doNotMarkMessagesAsRead: false } },
+      'check 37 with setting - always',
+      { ...base15, settings: { ...base15.settings, shouldMarkAsReadOnScroll: 'always' } },
+      { ...endBase, settings: { ...endBase.settings, shouldMarkAsReadOnScroll: 'always' } },
     ],
     [
-      'check 37 with setting already true',
-      { ...base15, settings: { ...base15.settings, doNotMarkMessagesAsRead: true } },
-      { ...endBase, settings: { ...endBase.settings, doNotMarkMessagesAsRead: true } },
+      'check 37 with setting - conversation',
+      { ...base15, settings: { ...base15.settings, shouldMarkAsReadOnScroll: 'conversation' } },
+      { ...endBase, settings: { ...endBase.settings, shouldMarkAsReadOnScroll: 'conversation' } },
+    ],
+    [
+      'check 37 with setting - never',
+      { ...base15, settings: { ...base15.settings, shouldMarkAsReadOnScroll: 'never' } },
+      { ...endBase, settings: { ...endBase.settings, shouldMarkAsReadOnScroll: 'never' } },
     ],
     [
       'check 38',

src/storage/migrations.js

@@ -377,7 +377,7 @@ const migrationsInner: {| [string]: (LessPartialState) => LessPartialState |} =
     // Handle the migration before 30.
     settings: {
       ...state.settings,
-      doNotMarkMessagesAsRead: state.settings.doNotMarkMessagesAsRead ?? false,
+      shouldMarkAsReadOnScroll: state.settings.shouldMarkAsReadOnScroll ?? 'always',
     },
   }),
 

src/webview/MessageList.js

@@ -62,7 +62,7 @@ type SelectorProps = {|
   fetching: Fetching,
   messageListElementsForShownMessages: $ReadOnlyArray<MessageListElement>,
   typingUsers: $ReadOnlyArray<UserOrBot>,
-  doNotMarkMessagesAsRead: boolean,
+  shouldMarkAsReadOnScroll: 'never' | 'always' | 'conversation',
 |};
 
 export type Props = $ReadOnly<{|
@@ -158,7 +158,7 @@ class MessageListInner extends Component<Props> {
       messageListElementsForShownMessages,
       initialScrollMessageId,
       showMessagePlaceholders,
-      doNotMarkMessagesAsRead,
+      shouldMarkAsReadOnScroll,
       _,
     } = this.props;
     const contentHtml = messageListElementsForShownMessages
@@ -175,7 +175,7 @@ class MessageListInner extends Component<Props> {
       scrollMessageId: initialScrollMessageId,
       auth,
       showMessagePlaceholders,
-      doNotMarkMessagesAsRead,
+      shouldMarkAsReadOnScroll,
     });
 
     /**
@@ -261,29 +261,32 @@ class MessageListInner extends Component<Props> {
  * "Can", not "will": other conditions can mean we don't want to mark
  * messages as read even when in a narrow where this is true.
  */
-const marksMessagesAsRead = (narrow: Narrow): boolean =>
+const marksMessagesAsRead = (
+  narrow: Narrow,
+  shouldMarkAsReadOnScroll: 'never' | 'always' | 'conversation',
+): 'never' | 'always' | 'conversation' =>
   // Generally we want these to agree with the web/desktop app, so that user
   // expectations transfer between the different apps.
   caseNarrow(narrow, {
     // These narrows show one conversation in full.  Each message appears
     // in its full context, so it makes sense to say the user's read it
     // and doesn't need to be shown it as unread again.
-    topic: () => true,
-    pm: () => true,
+    topic: () => shouldMarkAsReadOnScroll,
+    pm: () => shouldMarkAsReadOnScroll,
 
     // These narrows show several conversations interleaved.  They always
     // show entire conversations, so each message still appears in its
     // full context and it still makes sense to mark it as read.
-    stream: () => true,
-    home: () => true,
-    allPrivate: () => true,
+    stream: () => shouldMarkAsReadOnScroll,
+    home: () => shouldMarkAsReadOnScroll,
+    allPrivate: () => shouldMarkAsReadOnScroll,
 
     // These narrows show selected messages out of context.  The user will
     // typically still want to see them another way, in context, before
     // letting them disappear from their list of unread messages.
-    search: () => false,
-    starred: () => false,
-    mentioned: () => false,
+    search: () => 'never',
+    starred: () => 'never',
+    mentioned: () => 'never',
   });
 
 const MessageList: ComponentType<OuterProps> = connect<SelectorProps, _, _>(
@@ -303,8 +306,9 @@ const MessageList: ComponentType<OuterProps> = connect<SelectorProps, _, _>(
         props.narrow,
       ),
       typingUsers: getCurrentTypingUsers(state, props.narrow),
-      doNotMarkMessagesAsRead:
-        !marksMessagesAsRead(props.narrow) || globalSettings.doNotMarkMessagesAsRead,
+      shouldMarkAsReadOnScroll:
+        marksMessagesAsRead(props.narrow, globalSettings.shouldMarkAsReadOnScroll)
+        || globalSettings.shouldMarkAsReadOnScroll,
     };
   },
 )(connectActionSheet(withGetText(MessageListInner)));

src/webview/__tests__/generateInboundEvents-test.js

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

src/webview/handleOutboundEvents.js

@@ -13,7 +13,7 @@ import { pmKeyRecipientsFromMessage } from '../utils/recipient';
 import { isUrlAnImage } from '../utils/url';
 import * as logging from '../utils/logging';
 import { filterUnreadMessagesInRange } from '../utils/unread';
-import { parseNarrow } from '../utils/narrow';
+import { parseNarrow, isTopicNarrow, isPmNarrow } from '../utils/narrow';
 import {
   fetchOlder,
   fetchNewer,
@@ -166,7 +166,7 @@ type Props = $ReadOnly<{
   dispatch: Dispatch,
   messages: $ReadOnlyArray<Message | Outbox>,
   narrow: Narrow,
-  doNotMarkMessagesAsRead: boolean,
+  shouldMarkAsReadOnScroll: 'never' | 'always' | 'conversation',
   showActionSheetWithOptions: ShowActionSheetWithOptions,
   startEditMessage: (editMessage: EditMessage) => void,
   ...
@@ -185,7 +185,14 @@ const fetchMore = (props: Props, event: WebViewOutboundEventScroll) => {
 
 const markRead = (props: Props, event: WebViewOutboundEventScroll) => {
   const { flags, auth } = props.backgroundData;
-  if (props.doNotMarkMessagesAsRead) {
+  if (props.shouldMarkAsReadOnScroll === 'never') {
+    return;
+  }
+  if (
+    props.shouldMarkAsReadOnScroll === 'conversation'
+    && !isTopicNarrow(props.narrow)
+    && !isPmNarrow(props.narrow)
+  ) {
     return;
   }
   const unreadMessageIds = filterUnreadMessagesInRange(

src/webview/html/html.js

@@ -9,7 +9,7 @@ type InitOptionsType = {|
   scrollMessageId: number | null,
   auth: Auth,
   showMessagePlaceholders: boolean,
-  doNotMarkMessagesAsRead: boolean,
+  shouldMarkAsReadOnScroll: 'never' | 'always' | 'conversation',
 |};
 
 /**
@@ -45,7 +45,7 @@ export default (
   theme: ThemeName,
   initOptions: InitOptionsType,
 ): string => template`
-$!${script(initOptions.scrollMessageId, initOptions.auth, initOptions.doNotMarkMessagesAsRead)}
+$!${script(initOptions.scrollMessageId, initOptions.auth, initOptions.shouldMarkAsReadOnScroll)}
 $!${css(theme)}
 
 <meta name="viewport" content="width=device-width, initial-scale=1, user-scalable=no">

src/webview/js/generatedEs3.js

@@ -554,7 +554,7 @@ var compiledWebviewJs = (function (exports) {
       endMessageId: rangeHull.last
     });
 
-    if (!doNotMarkMessagesAsRead) {
+    if (shouldMarkAsReadOnScroll === 'always' || shouldMarkAsReadOnScroll === 'conversation') {
       setMessagesReadAttributes(rangeHull);
     }
 

src/webview/js/js.js

@@ -81,7 +81,7 @@ declare var isDevelopment: boolean;
  * defined in `handleInitialLoad`.
  * declared globally so as to use across functions.
  */
-declare var doNotMarkMessagesAsRead: boolean;
+declare var shouldMarkAsReadOnScroll: 'never' | 'always' | 'conversation';
 
 // We pull out document.body in one place, and check it's not null, in order
 // to provide that assertion to the type-checker.
@@ -471,7 +471,7 @@ const sendScrollMessage = () => {
     startMessageId: rangeHull.first,
     endMessageId: rangeHull.last,
   });
-  if (!doNotMarkMessagesAsRead) {
+  if (shouldMarkAsReadOnScroll === 'always' || shouldMarkAsReadOnScroll === 'conversation') {
     setMessagesReadAttributes(rangeHull);
   }
   // If there are no visible + read messages (for instance, the entire screen

src/webview/js/script.js

@@ -15,7 +15,7 @@ import compiledWebviewJs from './generatedEs3';
 export default (
   scrollMessageId: number | null,
   auth: Auth,
-  doNotMarkMessagesAsRead: boolean,
+  shouldMarkAsReadOnScroll: 'never' | 'always' | 'conversation',
 ): string => `
 <script>
 window.__forceSmoothScrollPolyfill__ = true;
@@ -24,7 +24,7 @@ ${matchesPolyfill}
 document.addEventListener('DOMContentLoaded', function() {
   var platformOS = ${JSON.stringify(Platform.OS)};
   var isDevelopment = ${JSON.stringify(process.env.NODE_ENV === 'development')};
-  var doNotMarkMessagesAsRead = ${JSON.stringify(doNotMarkMessagesAsRead)};
+  var shouldMarkAsReadOnScroll = ${JSON.stringify(shouldMarkAsReadOnScroll)};
   ${compiledWebviewJs}
   compiledWebviewJs.handleInitialLoad(
     ${JSON.stringify(scrollMessageId)},

static/translations/messages_en.json

@@ -102,6 +102,7 @@
   "Settings": "Settings",
   "Night mode": "Night mode",
   "Open links with in-app browser": "Open links with in-app browser",
+  "Mark as read on scroll": "Mark as read on scroll",
   "Language": "Language",
   "Arabic": "Arabic",
   "Bokmål": "Bokmål",

yarn.lock

@@ -1967,6 +1967,11 @@
   dependencies:
     invariant "^2.2.4"
 
+"@react-native-picker/picker@^2.4.1":
+  version "2.4.1"
+  resolved "https://registry.yarnpkg.com/@react-native-picker/picker/-/picker-2.4.1.tgz#92feb7e0672d739624517dae04bf4de1452dfcdc"
+  integrity sha512-1XWy3IQgwr7MWd30KdY1iUh2gQZD+JiotN1ifj/ptFUYKon/0UFwngKQaWCO/CP/FdLl20/huSSLwKedYrdMMA==
+
 "@react-native/assets@1.0.0":
   version "1.0.0"
   resolved "https://registry.yarnpkg.com/@react-native/assets/-/assets-1.0.0.tgz#c6f9bf63d274bafc8e970628de24986b30a55c8e"