Mark as read #364

sirpengi · 2023-11-02T15:09:27Z

This is proceeded by #361 and #363, which both provide api routes necessary for functionality here. This implements the manual "Mark as Read" button that allows the user to explicitly mark messages in their current narrow as read.

This series of PRs fixes: #130

Design made to match figma spec at https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?type=design&node-id=132-9684&mode=design&t=jJwHzloKJ0TMOG4M-0

gnprice

Thanks! Here's from a first pass, just on the commits that are new in this PR:
33c0f92 unreads: Add proposal for countInNarrow
0731b86 msglist: Add MarkAsRead widget

I believe the four previous commits:
53de4c2 api [nfc]: Factor out logic that supports serializing ApiNarrowDm
fc69d26 api [nfc]: Pull out a method Anchor.toJson
4d2c4bd api: Add updateMessageFlags and updateMessageFlagsForNarrow routes
f09b8d9 api: Add markAllAsRead, markStreamAsRead, and markTopicAsRead routes

are from other PRs. (The PR description should mention those — just say this one is stacked atop those.)

gnprice · 2023-11-02T18:06:14Z

lib/model/unreads.dart

+      case DmNarrow():
+        return dms[narrow]?.length ?? 0;


This should reuse countInDmNarrow.

Then let's also pull out the other cases as their own methods, along the line I described here:
#334 (comment)

Specifically probably like int countInTopicNarrow(int streamId, String topic) is most convenient. (The case of DM narrows is unusual in that even when you know you're talking about a DM narrow, the DmNarrow value itself is the most convenient way to identify it.)

That way other call sites that know what kind of count they want can be direct about it, just like the recent-DMs view is in #334.

gnprice · 2023-11-02T18:08:16Z

lib/model/unreads.dart

+        for (final streamData in streams.values) {
+          for (final model in streamData.values) {


Suggested change

for (final streamData in streams.values) {

for (final model in streamData.values) {

for (final topics in streams.values) {

for (final messageIds in topics.values) {

to match the naming used elsewhere in this class (look for use sites of streams to find examples)

gnprice · 2023-11-02T18:10:36Z

lib/model/unreads.dart

+        if (streamData == null) return 0;
+        final model = streamData[narrow.topic];
+        return model?.length ?? 0;


Can use ?[] similarly to ?.:

Suggested change

if (streamData == null) return 0;

final model = streamData[narrow.topic];

return model?.length ?? 0;

return streamData?[narrow.topic]?.length ?? 0;

(plus the renaming mentioned above)

gnprice · 2023-11-02T18:18:44Z

lib/widgets/message_list.dart

+Future<void> markNarrowAsRead(BuildContext context, ApiConnection connection, Narrow narrow) async {
+  final zulipLocalizations = ZulipLocalizations.of(context);


This is based on some code we have elsewhere, so links to that code will be helpful for review. We also may want to keep the UX aligned with the one in web, so the comparison may be useful for making future changes too — so the links will be helpful to put in the code itself, rather than the commit message. So like:

Suggested change

Future<void> markNarrowAsRead(BuildContext context, ApiConnection connection, Narrow narrow) async {

final zulipLocalizations = ZulipLocalizations.of(context);

Future<void> markNarrowAsRead(BuildContext context, ApiConnection connection, Narrow narrow) async {

// Compare web's `FUNCTION` in web/src/FILENAME.js

// and zulip-mobile's `FUNCTION` in src/action-sheets/index.js .

final zulipLocalizations = ZulipLocalizations.of(context);

gnprice · 2023-11-02T18:20:22Z

lib/widgets/message_list.dart

+  while (true) {
+    final result = await updateMessageFlagsForNarrow(connection,
+      anchor: anchor,
+      // anchor="oldest" is an anchor ID lower than any valid


"[AnchorCode.oldest]" — 'anchor="oldest"' doesn't mean much in a Dart context

(For that matter it doesn't mean much in a JS context — it looks like whoever wrote the JS comment you're copying here was thinking in Python.)

gnprice · 2023-11-02T18:23:15Z

lib/widgets/message_list.dart

+class MarkAsReadWidget extends StatelessWidget {
+  const MarkAsReadWidget({super.key, required this.model});
+
+  final MessageListView model;


Hmm, does this really want a MessageListView? I'd generally like to let that remain in the hands of the _MessageListState — that's the element for which the MessageListView is the view-model.

It looks like this widget only consults model.narrow and model.store. So let's instead just pass it the narrow, and it can get the store in the usual way.

gnprice · 2023-11-02T18:25:10Z

lib/widgets/message_list.dart

+    if (!context.mounted) return;
+    final narrow = model.narrow;
+    final connection = model.store.connection;
+    if (connection.zulipFeatureLevel! >= 155) {


should be marked with a todo-server comment

For background and motivation on those comments, see here: https://github.com/zulip/zulip-mobile/blob/main/docs/style.md#todo-server
though the formatting has evolved slightly since then; see examples in this codebase.

gnprice · 2023-11-02T18:34:01Z

assets/l10n/app_en.arb

+  },
+  "markAsReadInProgress": "Marking messages as read...",
+  "@markAsReadInProgress": {
+    "description": "SnackBar message when marking messages as read"


These descriptions are for translators, not people working in this codebase. So references to identifiers within our code, or Flutter's API or the like, will be mystifying and don't belong.

This one could be "Progress message while […]".

sirpengi · 2023-11-07T18:29:40Z

@gnprice ready for review now!

gnprice

Thanks! Generally this looks good. Comments below — notably we'll want tests on those countInFoo methods, and there's an important optimization web has that we'll need here.

gnprice · 2023-11-07T19:07:31Z

lib/model/unreads.dart

+  }
+
+  // TODO(#346): account for muted topics and streams
+  int countInStreamNarrow(StreamNarrow narrow) {


Suggested change

int countInStreamNarrow(StreamNarrow narrow) {

int countInStreamNarrow(int streamId) {

cf #364 (comment)

gnprice · 2023-11-07T19:22:42Z

lib/model/unreads.dart

+  // TODO(#346): account for muted topics and streams
+  int countInAllMessagesNarrow() {


Let's add another TODO comment for #370, which I just filed:

Suggested change

// TODO(#346): account for muted topics and streams

int countInAllMessagesNarrow() {

// TODO(#346): account for muted topics and streams

// TODO(#370): maintain this count incrementally, rather than recomputing from scratch

int countInAllMessagesNarrow() {

and same on countInStreamNarrow. That will answer a concern the reader may naturally think of, that this looks inefficient.

gnprice · 2023-11-07T19:24:43Z

lib/model/unreads.dart

+      case DmNarrow():
+        return countInDmNarrow(narrow);
+      case AllMessagesNarrow():
+        return countInAllMessagesNarrow();
+      case StreamNarrow():


Let's put these in logical order, so e.g. AllMessagesNarrow at the beginning or end. The same order as in their declarations would be good.

gnprice · 2023-11-07T19:31:52Z

lib/model/unreads.dart

+  int countInAllMessagesNarrow() {
+    int c = 0;
+    for (final messageIds in dms.values) {
+      c = c + messageIds.length;
+    }
+    for (final topics in streams.values) {
+      for (final messageIds in topics.values) {
+        c = c + messageIds.length;


These should get tests.

They can be pretty simple, using the existing helpers in unreads_test.dart: prepare, fillMessages, then check(model.countIn…).equals(3) or whatever.

In principle I guess we should have added a test for countInDmNarrow when we added that method, oops. That'll be easy to add now, though.

gnprice · 2023-11-07T19:33:37Z

lib/widgets/message_list.dart

@@ -286,7 +290,9 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
      // TODO on new message when scrolled up, anchor scroll to what's in view
      reverse: true,
      itemBuilder: (context, i) {
-        final data = model!.items[length - 1 - i];
+        if (i == 0) return MarkAsReadWidget(narrow: model!.narrow);


Suggested change

if (i == 0) return MarkAsReadWidget(narrow: model!.narrow);

if (i == 0) return MarkAsReadWidget(narrow: widget.narrow);

gnprice · 2023-11-07T19:56:44Z

lib/widgets/message_list.dart

+        // We previously showed an in-progress [SnackBar], so say we're done.
+        // There may be a backlog of [SnackBar]s accumulated in the queue
+        // so be sure to clear them out here.
+        ScaffoldMessenger.of(context)
+          ..clearSnackBars()


Hmm interesting.

That backlog would mean we're showing an out-of-date amount of progress, right? In that case, when we show a new snack bar here perhaps we should be removing the old one first. I.e. with removeCurrentSnackBar, or perhaps hideCurrentSnackBar.

(The "snack bar" system doesn't just automatically do that all the time, because the user may not be done reading the old snack bar and there's no guarantee the new one has anything to do with it. But here, the new subsumes the old.)

These snackbars don't have any progress information on them, so all their messages are the same ("Marking messages as read..."). I've added more documentation explaining why the queue isn't cleared when a new snack bar is shown, and refactored the messenger access to allow clearing when we lose the context. In short, the reason is clearing the snack bar on each pass results in quick flickering of the popups which obscures the message. It feels better to let the items run through their duration (less hectic energy) and ensure we clear them later.

The snackbar api isn't great, there's no easy way to track the state of each message. An alternative here is to only show one snackbar on the first pass and give it an outrageous duration - but we still have the same issue of needing to clear it when the context disappears.

Cool, makes sense. This seems good enough, then, until someday in the future we decide it's time to polish this with some UI that's more in the nature of a progress indicator.

gnprice · 2023-11-07T19:58:15Z

lib/widgets/message_list.dart

+      if (!context.mounted) return;
+      final zulipLocalizations = ZulipLocalizations.of(context);
+      showErrorDialog(context: context,
+        title: zulipLocalizations.errorDialogTitle,


Suggested change

title: zulipLocalizations.errorDialogTitle,

title: zulipLocalizations.errorMarkAsReadFailedTitle,

gnprice · 2023-11-07T20:08:07Z

lib/widgets/message_list.dart

+      // for more responsive feedback. See zulip@f0d87fcf6.
+      numBefore: 0,
+      numAfter: 1000,
+      narrow: narrow.apiEncode(),


Web has the following optimization, for the mark-all-as-read case:

// Since there's a database index on is:unread, it's a fast // search query and thus worth including here as an optimization. narrow: JSON.stringify([{operator: "is", operand: "unread", negated: false}]),

That's important when there's a ton of already-read history — as there may very often be for the "all" case or the stream case. So we should do the same.

The way to express that is with another ApiNarrowElement subclass. Say simply ApiNarrowIsUnread. Can go right above ApiNarrowMessageId, after all the other ApiNarrowElement subclasses.

(Later we can later generalize that to an ApiNarrowIs where the operand is some appropriate enum. But that's a part of the API that's only sort of documented; so that'll involve some reverse-engineering, not a pure matter of transcribing API docs, and we can leave it for another time.)

gnprice · 2023-11-07T20:13:37Z

lib/widgets/message_list.dart

+    //   around continuously until the task ends.  But we don't have an
+    //   off-the-shelf way to wire up such a thing, and marking a giant
+    //   number of messages unread isn't a common enough flow to be worth
+    //   substantial effort on UI polish.  So for now, we use a SnackBar,
+    //   even though they may feel a bit janky.


Suggested change

// around continuously until the task ends. But we don't have an

// off-the-shelf way to wire up such a thing, and marking a giant

// number of messages unread isn't a common enough flow to be worth

// substantial effort on UI polish. So for now, we use a SnackBar,

// even though they may feel a bit janky.

// around continuously until the task ends. For now we use

// a series of [SnackBar]s, which may feel a bit janky.

The justification here is true of marking unread but I think it's less convincing for marking as read, which is what this function is doing.

gnprice · 2023-11-07T20:20:05Z

lib/widgets/message_list.dart

+    responseCount++;
+    updatedCount += result.updatedCount;
+
+    if (result.foundNewest) {


Hmm, web has an interesting wrinkle here:

if (unread.old_unreads_missing) { // In the rare case that the user had more than // 50K total unreads on the server, the client // won't have known about all of them; this was // communicated to the client via // unread.old_unreads_missing. // // However, since we know we just marked // **everything** as read, we know that we now // have a correct data set of unreads. unread.clear_old_unreads_missing();

Seems right. I'd be fine with just leaving a TODO for it, though.

gnprice · 2023-11-09T22:40:03Z

lib/widgets/message_list.dart

+    case StreamNarrow(:final streamId):
+      await markStreamAsRead(connection,
+      streamId: streamId);
+    case TopicNarrow(:final streamId, :final topic):
+      await markTopicAsRead(connection,
+      streamId: streamId, topicName: topic);


Suggested change

case StreamNarrow(:final streamId):

await markStreamAsRead(connection,

streamId: streamId);

case TopicNarrow(:final streamId, :final topic):

await markTopicAsRead(connection,

streamId: streamId, topicName: topic);

case StreamNarrow(:final streamId):

await markStreamAsRead(connection, streamId: streamId);

case TopicNarrow(:final streamId, :final topic):

await markTopicAsRead(connection, streamId: streamId, topicName: topic);

gnprice · 2023-11-09T22:47:42Z

lib/widgets/message_list.dart

+        // We previously showed an in-progress [SnackBar], so say we're done.
+        // There may be a backlog of [SnackBar]s accumulated in the queue
+        // so be sure to clear them out here.
+        ScaffoldMessenger.of(context)
+          ..clearSnackBars()


Cool, makes sense. This seems good enough, then, until someday in the future we decide it's time to polish this with some UI that's more in the nature of a progress indicator.

gnprice · 2023-11-09T22:57:45Z

Thanks @sirpengi for the revision! All looks good except one nit above. I'll just fix that and merge.

These helpers do not account for muted topics and streams, those are left for zulip#346.

gnprice · 2023-11-09T23:01:23Z

And merged. Fixed one other nit:

78adb13 narrow: Add ApiNarrowIsUnread

The change this is making is within the API bindings (lib/api/), so I prefer the prefix "api:".

gnprice reviewed Nov 2, 2023

View reviewed changes

sirpengi force-pushed the pr-mark-as-read branch 5 times, most recently from 3c763fa to 6074c2a Compare November 7, 2023 11:20

sirpengi marked this pull request as ready for review November 7, 2023 18:28

sirpengi changed the title ~~Draft: Mark as read~~ Mark as read Nov 7, 2023

gnprice mentioned this pull request Nov 7, 2023

Maintain total unread counts efficiently #370

Open

gnprice reviewed Nov 7, 2023

View reviewed changes

api [nfc]: Use resolveDmElements in narrowLink

6cce020

sirpengi force-pushed the pr-mark-as-read branch from 6074c2a to d207b65 Compare November 9, 2023 14:09

gnprice reviewed Nov 9, 2023

View reviewed changes

sirpengi added 3 commits November 9, 2023 14:58

api: Add ApiNarrowIsUnread

2b4019d

unreads: Add helpers to provide unread counts for narrows

1eebcb6

These helpers do not account for muted topics and streams, those are left for zulip#346.

msglist: Add MarkAsRead widget

e7fe06c

gnprice force-pushed the pr-mark-as-read branch from d207b65 to e7fe06c Compare November 9, 2023 23:01

gnprice merged commit e7fe06c into zulip:main Nov 9, 2023

This was referenced Nov 11, 2023

msglist: Mark-as-read button takes a long time (batch-processing already-read messages) #377

Closed

Sticky header hit-tests at bottom of message list, though paints at top #327

Closed

sirpengi deleted the pr-mark-as-read branch January 23, 2024 14:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mark as read #364

Mark as read #364

sirpengi commented Nov 2, 2023 •

edited

Loading

gnprice left a comment

gnprice Nov 2, 2023

gnprice Nov 2, 2023

gnprice Nov 2, 2023

gnprice Nov 2, 2023

gnprice Nov 2, 2023

gnprice Nov 2, 2023

gnprice Nov 2, 2023

gnprice Nov 2, 2023

sirpengi commented Nov 7, 2023

gnprice left a comment

gnprice Nov 7, 2023

gnprice Nov 7, 2023

gnprice Nov 7, 2023

gnprice Nov 7, 2023

gnprice Nov 7, 2023

gnprice Nov 7, 2023

sirpengi Nov 9, 2023

gnprice Nov 9, 2023

gnprice Nov 7, 2023

gnprice Nov 7, 2023

gnprice Nov 7, 2023

gnprice Nov 7, 2023

gnprice Nov 9, 2023

gnprice Nov 9, 2023

gnprice commented Nov 9, 2023

gnprice commented Nov 9, 2023

		for (final streamData in streams.values) {
		for (final model in streamData.values) {

		Future<void> markNarrowAsRead(BuildContext context, ApiConnection connection, Narrow narrow) async {
		final zulipLocalizations = ZulipLocalizations.of(context);

	int countInStreamNarrow(StreamNarrow narrow) {
	int countInStreamNarrow(int streamId) {

		// TODO(#346): account for muted topics and streams
		int countInAllMessagesNarrow() {

	if (i == 0) return MarkAsReadWidget(narrow: model!.narrow);
	if (i == 0) return MarkAsReadWidget(narrow: widget.narrow);

	title: zulipLocalizations.errorDialogTitle,
	title: zulipLocalizations.errorMarkAsReadFailedTitle,

Mark as read #364

Mark as read #364

Conversation

sirpengi commented Nov 2, 2023 • 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

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

sirpengi commented Nov 7, 2023

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

gnprice commented Nov 9, 2023

gnprice commented Nov 9, 2023

sirpengi commented Nov 2, 2023 •

edited

Loading