settings_page: Add marks as read on scroll options#5294
settings_page: Add marks as read on scroll options#5294SilentCruzer wants to merge 1 commit intozulip:mainfrom
Conversation
SilentCruzer
changed the title
chrisbobbe
left a comment
chrisbobbe
left a comment
There was a problem hiding this comment.
Thanks! See some comments below, and also a substantive comment about what options to offer, at #5241 (comment).
| import React, { useCallback } from 'react'; | ||
| import type { Node } from 'react'; | ||
|
|
||
| import { View, Text, Picker } from 'react-native'; |
There was a problem hiding this comment.
I see a deprecation notice at the top of the doc:
Deprecated. Use one of the community packages instead.
From the list at the handy link they give, I think @react-native-picker/picker looks like the best choice.
| auth: getAuth(state), | ||
| debug, | ||
| doNotMarkMessagesAsRead: | ||
| !marksMessagesAsRead(props.narrow) || globalSettings.doNotMarkMessagesAsRead, |
There was a problem hiding this comment.
We shouldn't remove the first part, !marksMessagesAsRead(props.narrow). It makes it so that some views never automatically mark as read; see the comments in marksMessagesAsRead for why.
| style={[ | ||
| styles.listItem, | ||
| { | ||
| height: 56, |
There was a problem hiding this comment.
Sorry about that, I should have mentioned it earlier. I just made a rough UI for testing the feature. I was planning to make a proper code for that after I made the correct implementation.
| }, | ||
| ]} | ||
| > | ||
| <Text style={{ fontSize: 15 }}>Mark messages read on scroll</Text> |
There was a problem hiding this comment.
Let's use ZulipTextIntl for this.
| <View> | ||
| <Picker | ||
| selectedValue={doNotMarkMessagesAsRead} | ||
| style={{ width: 150 }} |
| }); | ||
| if (!doNotMarkMessagesAsRead) { | ||
| setMessagesReadAttributes(rangeHull); | ||
| } |
There was a problem hiding this comment.
This looks like the place where the setting actually takes effect; is that right? It seems like this bit should be replaced with something different, instead of removed altogether.
gnprice
left a comment
gnprice
left a comment
There was a problem hiding this comment.
Thanks @SilentCruzer! A couple more comments, in addition to the things @chrisbobbe mentioned.
| if (doNotMarkMessagesAsRead === 'stream' && !isStreamOrTopicNarrow(props.narrow)) { | ||
| return; | ||
| } | ||
| if (doNotMarkMessagesAsRead === 'topic' && !isTopicNarrow(props.narrow)) { | ||
| return; | ||
| } | ||
| if (doNotMarkMessagesAsRead === 'never') { |
There was a problem hiding this comment.
From this implementation, it looks like the meaning of this value is more like "do mark messages as read", rather than "do not mark messages as read".
In that case, let's rename it to match its meaning. One good name would be shouldMarkAsReadOnScroll.
| // 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, | ||
| doNotMarkMessagesAsRead: 'all' | 'stream' | 'topic' | 'never', |
There was a problem hiding this comment.
Whenever changing a type that appears in the Redux state, we'll want a migration to update the user's existing data.
See discussion here:
https://github.com/zulip/zulip-mobile/blob/main/docs/howto/new-feature.md#redux-state-migrations
| const marksMessagesAsRead = ( | ||
| narrow: Narrow, | ||
| shouldMarkAsReadOnScroll: 'never' | 'always' | 'conversation', | ||
| ): 'never' | 'always' | 'conversation' => |
There was a problem hiding this comment.
Except for the search, starred, and mentioned narrows, every other narrows should depend on the currently chosen setting. That's why I added this extra parameter
| endMessageId: rangeHull.last, | ||
| }); | ||
| if (!doNotMarkMessagesAsRead) { | ||
| if (shouldMarkAsReadOnScroll === 'always' || shouldMarkAsReadOnScroll === 'conversation') { |
There was a problem hiding this comment.
This conditional is not entirely correct.
setMessagesReadAttributes(rangeHull), should only work if the chosen setting is 'always' or the settings is 'conversation' and the current narrow is a topic or pm.
Is there a way to check which narrow we are currently in the js.js file?
|
Thanks again @SilentCruzer for working on this! Closing this PR in favor of #5469. |
3 participants
Closes: #5241
Making a draft pull request since I am not sure about the implementation.
The main change is basically changing the type of the variable
doNotMarkMessagesAsReadfrom boolean to a string union.Added a dropdown menu on the settings page to choose the options from the string union.
So the messages will mark as read based on the option.
Also, the tests for
doNotMarkMessagesAsReadare written on migrations-test.js. If the implementation is correct can I get a few pointers for the new tests.Screenshots: