Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/__tests__/lib/exampleData.js
Original file line number Diff line number Diff line change
Expand Up @@ -752,6 +752,7 @@ export const backgroundData: BackgroundData = deepFreeze({
allImageEmojiById: action.realm_init.data.realm_emoji,
auth: selfAuth,
debug: baseReduxState.session.debug,
doNotMarkMessagesAsRead: baseReduxState.settings.doNotMarkMessagesAsRead,
flags: baseReduxState.flags,
mute: [],
mutedUsers: Immutable.Map(),
Expand Down
3 changes: 3 additions & 0 deletions src/boot/store.js
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,9 @@ const migrations: {| [string]: (GlobalState) => GlobalState |} = {
outbox: state.outbox.filter(o => o.sender_id !== undefined),
}),

// Add `doNotMarkMessagesAsRead` in `SettingsState`.
// (Handled automatically by merging with the new initial state.)

// TIP: When adding a migration, consider just using `dropCache`.
};

Expand Down
1 change: 1 addition & 0 deletions src/reduxTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,7 @@ export type SettingsState = {|
experimentalFeaturesEnabled: boolean,
streamNotification: boolean,
browser: BrowserPreference,
doNotMarkMessagesAsRead: boolean,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're making a change to the Redux state type, so we should have a migration for it.

(More specifically, it's in a subtree that we actually store between runs -- see storeKeys and cacheKeys in src/boot/store.js. This didn't apply to the change to SessionState via changing Debug, because state.session isn't stored; it's in discardKeys.)

Actually, we had a recent discussion where the conclusion was that maybe we shouldn't quite have a migration in this case, because the way the persisted state gets merged with the reducer's initial state will have the right effect already. But still we should have a comment in the list of migrations, saying what changed and that we explicitly decided no migration was needed.

(I'm regretting now that that discussion was on GitHub instead of Zulip, though -- it may not be easy to find.)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a difference when using dropCache vs returning a modified state like some migrations are doing? example - '28'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

|};

export type StreamsState = Stream[];
Expand Down
2 changes: 1 addition & 1 deletion src/session/__tests__/sessionReducer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ describe('sessionReducer', () => {
const action = deepFreeze({ type: DEBUG_FLAG_TOGGLE, key: 'someKey', value: true });
expect(sessionReducer(baseState, action)).toEqual({
...baseState,
debug: { doNotMarkMessagesAsRead: false, someKey: true },
debug: { someKey: true },
});
});

Expand Down
4 changes: 1 addition & 3 deletions src/session/sessionReducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -85,9 +85,7 @@ const initialState: SessionState = {
orientation: 'PORTRAIT',
outboxSending: false,
pushToken: null,
debug: {
doNotMarkMessagesAsRead: false,
},
debug: Object.freeze({}),
hasDismissedServerCompatNotice: false,
};

Expand Down
24 changes: 4 additions & 20 deletions src/settings/DebugScreen.js
Original file line number Diff line number Diff line change
@@ -1,37 +1,21 @@
/* @flow strict-local */

import React, { useCallback } from 'react';
import React from 'react';
import { View } from 'react-native';

import type { RouteProp } from '../react-navigation';
import type { AppNavigationProp } from '../nav/AppNavigator';
import { useSelector, useDispatch } from '../react-redux';
import { getSession } from '../selectors';
import { SwitchRow, Screen } from '../common';
import { debugFlagToggle } from '../actions';
import { Screen } from '../common';

type Props = $ReadOnly<{|
navigation: AppNavigationProp<'debug'>,
route: RouteProp<'debug', void>,
|}>;

export default function DebugScreen(props: Props) {
const dispatch = useDispatch();
const debug = useSelector(state => getSession(state).debug);

const handleSettingToggle = useCallback(
(key: string) => {
dispatch(debugFlagToggle(key, !debug[key]));
},
[debug, dispatch],
);

return (
<Screen title="Debug">
<SwitchRow
label="Do not mark messages read on scroll"
value={debug.doNotMarkMessagesAsRead}
onValueChange={() => handleSettingToggle('doNotMarkMessagesAsRead')}
/>
<View />
</Screen>
);
}
8 changes: 8 additions & 0 deletions src/settings/SettingsScreen.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ type Props = $ReadOnly<{|
export default function SettingsScreen(props: Props) {
const theme = useSelector(state => getSettings(state).theme);
const browser = useSelector(state => getSettings(state).browser);
const doNotMarkMessagesAsRead = useSelector(state => getSettings(state).doNotMarkMessagesAsRead);
const dispatch = useDispatch();

const handleThemeChange = useCallback(() => {
Expand All @@ -55,6 +56,13 @@ export default function SettingsScreen(props: Props) {
dispatch(settingsChange({ browser: value ? 'embedded' : 'external' }));
}}
/>
<SwitchRow
label="Do not mark messages read on scroll"
value={doNotMarkMessagesAsRead}
onValueChange={value => {
dispatch(settingsChange({ doNotMarkMessagesAsRead: value }));
}}
/>
<NestedNavRow
Icon={IconNotifications}
label="Notifications"
Expand Down
1 change: 1 addition & 0 deletions src/settings/settingsReducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ const initialState: SettingsState = {
experimentalFeaturesEnabled: false,
streamNotification: false,
browser: 'default',
doNotMarkMessagesAsRead: false,
};

export default (state: SettingsState = initialState, action: Action): SettingsState => {
Expand Down
5 changes: 2 additions & 3 deletions src/types.js
Original file line number Diff line number Diff line change
Expand Up @@ -142,9 +142,8 @@ export type EditMessage = {|
topic: string,
|};

export type Debug = {|
doNotMarkMessagesAsRead: boolean,
|};
/** Add debug setting here. */
export type Debug = {||};

export type TopicExtended = {|
...$Exact<Topic>,
Expand Down
5 changes: 4 additions & 1 deletion src/webview/MessageList.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ export type BackgroundData = $ReadOnly<{|
allImageEmojiById: $ReadOnly<{| [id: string]: ImageEmojiType |}>,
auth: Auth,
debug: Debug,
doNotMarkMessagesAsRead: boolean,
flags: FlagsState,
mute: MuteState,
mutedUsers: MutedUsersState,
Expand Down Expand Up @@ -214,11 +215,12 @@ class MessageListInner extends Component<Props> {
htmlPieceDescriptors: htmlPieceDescriptorsForShownMessages,
_,
});
const { auth, theme } = backgroundData;
const { auth, theme, doNotMarkMessagesAsRead } = backgroundData;
const html: string = getHtml(contentHtml, theme, {
scrollMessageId: initialScrollMessageId,
auth,
showMessagePlaceholders,
doNotMarkMessagesAsRead,
});

/**
Expand Down Expand Up @@ -309,6 +311,7 @@ const MessageList: ComponentType<OuterProps> = connect<SelectorProps, _, _>(
allImageEmojiById: getAllImageEmojiById(state),
auth: getAuth(state),
debug: getDebug(state),
doNotMarkMessagesAsRead: getSettings(state).doNotMarkMessagesAsRead,
flags: getFlags(state),
mute: getMute(state),
mutedUsers: getMutedUsers(state),
Expand Down
4 changes: 2 additions & 2 deletions src/webview/handleOutboundEvents.js
Original file line number Diff line number Diff line change
Expand Up @@ -180,8 +180,8 @@ const fetchMore = (props: Props, event: WebViewOutboundEventScroll) => {
};

const markRead = (props: Props, event: WebViewOutboundEventScroll) => {
const { debug, flags, auth } = props.backgroundData;
if (debug.doNotMarkMessagesAsRead) {
const { doNotMarkMessagesAsRead, flags, auth } = props.backgroundData;
if (doNotMarkMessagesAsRead) {
return;
}
const unreadMessageIds = filterUnreadMessagesInRange(
Expand Down
3 changes: 2 additions & 1 deletion src/webview/html/html.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ type InitOptionsType = {|
scrollMessageId: number | null,
auth: Auth,
showMessagePlaceholders: boolean,
doNotMarkMessagesAsRead: boolean,
|};

/**
Expand Down Expand Up @@ -40,7 +41,7 @@ type InitOptionsType = {|
const webkitBugWorkaround: string = '<script> </script>';

export default (content: string, theme: ThemeName, initOptions: InitOptionsType) => template`
$!${script(initOptions.scrollMessageId, initOptions.auth)}
$!${script(initOptions.scrollMessageId, initOptions.auth, initOptions.doNotMarkMessagesAsRead)}
$!${css(theme)}

<meta name="viewport" content="width=device-width, initial-scale=1, user-scalable=no">
Expand Down
5 changes: 4 additions & 1 deletion src/webview/js/generatedEs3.js
Original file line number Diff line number Diff line change
Expand Up @@ -646,7 +646,10 @@ var compiledWebviewJs = (function (exports) {
startMessageId: rangeHull.first,
endMessageId: rangeHull.last
});
setMessagesReadAttributes(rangeHull);

if (!doNotMarkMessagesAsRead) {
setMessagesReadAttributes(rangeHull);
}

if (messageRange.first < messageRange.last) {
prevMessageRange = messageRange;
Expand Down
11 changes: 10 additions & 1 deletion src/webview/js/js.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,13 @@ import { toggleSpoiler } from './spoilers';
*/
declare var platformOS: string;

/**
* used to control behavior based on debug settings.
* defined in `handleInitialLoad`.
* declared globally so as to use across functions.
*/
declare var doNotMarkMessagesAsRead: boolean;

/* eslint-disable no-extend-native */

/* Polyfill Array.from. Native in Chrome 45 and at least Safari 13.
Expand Down Expand Up @@ -507,7 +514,9 @@ const sendScrollMessage = () => {
startMessageId: rangeHull.first,
endMessageId: rangeHull.last,
});
setMessagesReadAttributes(rangeHull);
if (!doNotMarkMessagesAsRead) {
setMessagesReadAttributes(rangeHull);
}
// If there are no visible + read messages (for instance, the entire screen
// is taken up by a single large message), then we don't want to update
// prevMessageRange. This way, if the user scrolled past some messages to
Expand Down
7 changes: 6 additions & 1 deletion src/webview/js/script.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,19 @@ import matchesPolyfill from './matchesPolyfill';
import compiledWebviewJs from './generatedEs3';
import config from '../../config';

export default (scrollMessageId: number | null, auth: Auth): string => `
export default (
scrollMessageId: number | null,
auth: Auth,
doNotMarkMessagesAsRead: boolean,
): string => `
<script>
window.__forceSmoothScrollPolyfill__ = true;
${smoothScroll}
${matchesPolyfill}
window.enableWebViewErrorDisplay = ${config.enableWebViewErrorDisplay.toString()};
document.addEventListener('DOMContentLoaded', function() {
var platformOS = ${JSON.stringify(Platform.OS)};
var doNotMarkMessagesAsRead = ${JSON.stringify(doNotMarkMessagesAsRead)};
${compiledWebviewJs}
compiledWebviewJs.handleInitialLoad(
${JSON.stringify(scrollMessageId)},
Expand Down