msglist: Set unreads.oldUnreadsMissing to false on batched mark-all-as-read #378

chrisbobbe · 2023-11-11T20:46:09Z

No description provided.

sirpengi · 2023-11-15T19:06:53Z

lib/widgets/message_list.dart

+      // success themselves; the event from the last batch is unmarked.
+      // Discussion:
+      //   <https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/flutter.3A.20Mark-as-read/near/1680275>
+      PerAccountStoreWidget.of(context).unreads.oldUnreadsMissing = false;


notifyListeners isn't being called after mutating the unreads model here. I think there's an argument for not doing that here (as we aren't using this flag in the UI) but leaving a note to that effect might be good?

Oh right, thanks for catching that! I think it'd be good to call notifyListeners. We might in the future use this flag in the UI; here's from the doc on old_unreads_missing in the /register response:

[…] When true, we recommend that clients display a warning, as they are likely to produce erroneous results until reloaded with the user having fewer than MAX_UNREAD_MESSAGES unread messages.

I'll send a revision with one way of making that happen (edit: done).

chrisbobbe · 2023-11-15T20:19:00Z

Thanks for the review! Revision pushed.

gnprice

Thanks to you both! Sorry for the delayed review. This looks good, with a few nits below.

gnprice · 2024-02-06T00:23:43Z

test/widgets/message_list_test.dart

@@ -608,6 +626,19 @@ void main() {
          narrow: narrow, messages: [message], unreadMsgs: unreadMsgs);
        check(isMarkAsReadButtonVisible(tester)).isTrue();

+        int unreadsNotifiedCount = 0;
+        final unreadsModel = store.unreads;


nit: just inline this, it's more transparent and not materially longer

gnprice · 2024-02-06T00:27:02Z

test/widgets/message_list_test.dart

+        int unreadsNotifiedCount = 0;
+        final unreadsModel = store.unreads;
+        unreadsModel.addListener(() {
+          unreadsNotifiedCount++;
+        });
+        // Might as well test with oldUnreadsMissing: true.
+        //
+        // We didn't fill the model with 50k unreads, so this is questionably
+        // realistic… but the 50k cap isn't actually API-guaranteed, and this is
+        // plausibly realistic for a hypothetical server that decides based on
+        // message age rather than the 50k cap.
+        unreadsModel.oldUnreadsMissing = true;


I think this "might as well" reasoning works so long as this wrinkle isn't adding so much to read (in code and comments) that it comes to take over the test case.

One simplification is that I think this test is just as effective without the unreadsNotifiedCount part, just checking that oldUnreadsMissing is still true. So:

Suggested change

int unreadsNotifiedCount = 0;

final unreadsModel = store.unreads;

unreadsModel.addListener(() {

unreadsNotifiedCount++;

});

// Might as well test with oldUnreadsMissing: true.

//

// We didn't fill the model with 50k unreads, so this is questionably

// realistic… but the 50k cap isn't actually API-guaranteed, and this is

// plausibly realistic for a hypothetical server that decides based on

// message age rather than the 50k cap.

unreadsModel.oldUnreadsMissing = true;

// Might as well test with oldUnreadsMissing: true.

store.unreads.oldUnreadsMissing = true;

gnprice · 2024-02-06T00:30:13Z

test/widgets/message_list_test.dart

+        // Didn't redundantly call [Unreads.handleAllMessagesReadSuccess], which
+        // is meant only for the modern protocol and would be redundant in the
+        // legacy protocol. (In the legacy protocol, we set oldUnreadsMissing to
+        // false when an event comes, not when the mark-as-read request
+        // succeeds.)


In the same vein:

Suggested change

// Didn't redundantly call [Unreads.handleAllMessagesReadSuccess], which

// is meant only for the modern protocol and would be redundant in the

// legacy protocol. (In the legacy protocol, we set oldUnreadsMissing to

// false when an event comes, not when the mark-as-read request

// succeeds.)

// Check that [Unreads.handleAllMessagesReadSuccess] wasn't called;

// in the legacy protocol, that'd be redundant with the mark-read event.

gnprice · 2024-02-06T00:32:14Z

test/widgets/message_list_test.dart

+        check(isMarkAsReadButtonVisible(tester)).isTrue();
+        store.unreads.oldUnreadsMissing = true;
+
+        final connection = store.connection as FakeApiConnection;


nit: there's an outer connection that this is equivalent to, in the same way as store:

Suggested change

final connection = store.connection as FakeApiConnection;

(I see there's a bunch of existing cases of similar redundancy in this file. I'll send a quick cleanup for those. → #506)

chrisbobbe · 2024-02-08T03:21:25Z

Thanks for the review! Revision pushed.

gnprice

Thanks! One nit still open, though.

gnprice · 2024-02-08T21:16:03Z

test/widgets/message_list_test.dart

+        check(isMarkAsReadButtonVisible(tester)).isTrue();
+        store.unreads.oldUnreadsMissing = true;
+
+        final connection = store.connection as FakeApiConnection;


chrisbobbe · 2024-02-12T02:56:31Z

Ah indeed; thanks for catching that. Revision pushed!

We'll reuse this to handle the TODO in this _handlePress method.

…s-read

gnprice · 2024-02-12T20:06:17Z

Thanks! Looks good; merging.

chrisbobbe requested review from gnprice and sirpengi November 11, 2023 20:46

sirpengi reviewed Nov 15, 2023

View reviewed changes

chrisbobbe force-pushed the set-old-unreads-missing-when-batched branch from ed57148 to 4e62e7a Compare November 15, 2023 20:18

gnprice reviewed Feb 6, 2024

View reviewed changes

chrisbobbe force-pushed the set-old-unreads-missing-when-batched branch from 4e62e7a to 36f344d Compare February 8, 2024 03:21

gnprice reviewed Feb 8, 2024

View reviewed changes

chrisbobbe force-pushed the set-old-unreads-missing-when-batched branch from 36f344d to c7e894f Compare February 12, 2024 02:56

chrisbobbe added 3 commits February 12, 2024 12:05

msglist [nfc]: Pull out useLegacy variable in MarkAsReadWidget

d3adf04

We'll reuse this to handle the TODO in this _handlePress method.

unreads: Add handleAllMessagesReadSuccess method, to be used soon

379fc4d

msglist: Set unreads.oldUnreadsMissing to false on batched mark-all-a…

08a0827

…s-read

gnprice force-pushed the set-old-unreads-missing-when-batched branch from c7e894f to 08a0827 Compare February 12, 2024 20:06

gnprice merged commit 08a0827 into zulip:main Feb 12, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

msglist: Set unreads.oldUnreadsMissing to false on batched mark-all-as-read #378

msglist: Set unreads.oldUnreadsMissing to false on batched mark-all-as-read #378

chrisbobbe commented Nov 11, 2023

sirpengi Nov 15, 2023

chrisbobbe Nov 15, 2023 •

edited

Loading

chrisbobbe commented Nov 15, 2023

gnprice left a comment

gnprice Feb 6, 2024

gnprice Feb 6, 2024

gnprice Feb 6, 2024

gnprice Feb 6, 2024 •

edited

Loading

gnprice Feb 8, 2024

chrisbobbe commented Feb 8, 2024

gnprice left a comment

gnprice Feb 8, 2024

chrisbobbe commented Feb 12, 2024

gnprice commented Feb 12, 2024

msglist: Set unreads.oldUnreadsMissing to false on batched mark-all-as-read #378

msglist: Set unreads.oldUnreadsMissing to false on batched mark-all-as-read #378

Conversation

chrisbobbe commented Nov 11, 2023

sirpengi Nov 15, 2023

Choose a reason for hiding this comment

chrisbobbe Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

chrisbobbe commented Nov 15, 2023

gnprice left a comment

Choose a reason for hiding this comment

gnprice Feb 6, 2024

Choose a reason for hiding this comment

gnprice Feb 6, 2024

Choose a reason for hiding this comment

gnprice Feb 6, 2024

Choose a reason for hiding this comment

gnprice Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

gnprice Feb 8, 2024

Choose a reason for hiding this comment

chrisbobbe commented Feb 8, 2024

gnprice left a comment

Choose a reason for hiding this comment

gnprice Feb 8, 2024

Choose a reason for hiding this comment

chrisbobbe commented Feb 12, 2024

gnprice commented Feb 12, 2024

chrisbobbe Nov 15, 2023 •

edited

Loading

gnprice Feb 6, 2024 •

edited

Loading