model: Add Unreads model, for tracking unread-message counts #304

chrisbobbe · 2023-09-14T17:31:53Z

Fixes #253.

Contains my current revision of #299.

This is still in progress, but thought I'd check in with what I have so far, and see if it's going in the right direction. 🙂

gnprice

Thanks! I haven't yet read the methods, but here's a few comments from reading the other files and down through the field definitions.

This is an area where it'll be great to do some explicit timing measurements; see detailed tips below. In particular, apart from using measurements to optimize specific areas of this code, it'll be good to have just a top-line number saying how long it takes at startup/load and at each key operation like getting a new message or marking some messages as read. That will guide how much further effort we spend on optimization.

For comparisons when I converted the corresponding data structures in zulip-mobile, see:
zulip/zulip-mobile@89cff7c
zulip/zulip-mobile@8dc3fe6
zulip/zulip-mobile@5725e7d
and the timings discussed in those commit messages.

(And it's a good thing I split those commits up, because the second one was buggy and had to be reverted: zulip/zulip-mobile@7c7f245.)

gnprice · 2023-09-15T00:21:46Z

lib/model/narrow.dart

+  factory DmNarrow.ofUnreadHuddleSnapshot(UnreadHuddleSnapshot snapshot, {required int selfUserId}) {
+    final userIds = snapshot.userIdsString.split(',').map((id) => int.parse(id));
+    return DmNarrow(selfUserId: selfUserId,
+      allRecipientIds: userIds.toList(growable: false)..sort());


In zulip-mobile our notes say this list comes sorted:

/** All users, including self; numerically sorted, comma-separated. */ user_ids_string: string,

So that'd be a good thing to try to confirm in #api documentation, and then we can skip sorting it ourselves.

gnprice · 2023-09-15T00:28:37Z

lib/model/narrow.dart

+    return DmNarrow(selfUserId: selfUserId,
+      allRecipientIds: [...detail.userIds!, selfUserId]..sort());
+  }
+
  factory DmNarrow.withUser(int userId, {required int selfUserId}) {


Maybe this should get moved to right below the plain DmNarrow constructor. It and ofMessage and the plain one are the really general-purpose constructors; as the number of special-purpose constructors grows, it starts getting pushed down where it's harder to spot.

gnprice · 2023-09-15T00:32:02Z

lib/model/unreads.dart

+  ///
+  /// Includes messages with
+  /// [MessageFlag.mentioned] or [MessageFlag.wildcardMentioned] or both.
+  final Set<int> mentions;


(This one perhaps leave as a Set; I don't think it shares the properties the others have, where removing from the set is almost all going to happen at the start of the list.)

gnprice · 2023-09-15T00:35:56Z

lib/model/unreads.dart

+  });
+
+  /// Unread stream messages, as: stream ID → topic → message ID.
+  final Map<int, Map<String, Set<int>>> streams;


- Change Sets of message IDs to sorted Lists, like in zulip-mobile, for space efficiency (needs attention to make sure they stay sorted and that operations are as time-efficient as possible)

Specifically they should be QueueList, because that's a deque (as is the Immutable.List we use in zulip-mobile). That way removing elements from the beginning, which is what typically happens when marking as read, is O(1).

gnprice · 2023-09-15T00:51:34Z

lib/model/unreads.dart

+    for (final unreadStreamSnapshot in initial.streams) {
+      final streamId = unreadStreamSnapshot.streamId;
+      final topic = unreadStreamSnapshot.topic;
+
+      (streams[streamId] ??= {})[topic] = Set.of(unreadStreamSnapshot.unreadMessageIds);
+    }


In zulip-mobile we have:

// First, collect together all the data for a given stream, just in a // plain old Array. // […] // Then, for each of those plain Arrays build an Immutable.Map from it // all in one shot. This is quite a bit faster than building the Maps // incrementally. For a user with lots of unreads in a busy org, we // can be handling 50k message IDs here, across perhaps 2-5k threads // in dozens of streams, so the effect is significant.

I'm not sure the same thing would apply when building a Dart Map, but it might.

This is a good target for doing some empirical timing. Take a test user who has 50k+ unreads across chat.zulip.org; sprinkle some timing around key bits of the code; see how long it takes with different versions.

For example timing code, see the use of Stopwatch in lib/model/store.dart, around the registerQueue call.

In order to get representative results, run in release mode or profile mode, and do so on a physical phone. Probably informative to do so, at least once, both on the now-low-end Android test device you have and on a more modern iPhone.

The absolute times will probably be longer on the lower-end device. The ratios between times for different versions of the code might be similar… or might be significantly different between devices. Such differences can definitely happen in principle (and be big), and I don't have a good sense of how likely they are in practice.

Once you have the general lay of the land for comparing the two devices, it's probably most expedient to do further exploration just on the more modern device. Definitely always in either release mode or profile mode, though. And always on a physical phone; I think between a physical phone and your laptop is probably more likely to have surprisingly sharp differences in how different versions of the code compare to each other than between different phone hardware.

chrisbobbe · 2023-09-15T01:43:55Z

lib/model/unreads.dart

+/// - unknown, because we're missing data on old unread messages
+///   (see [oldUnreadsMissing]).
+///
+/// Messages in unsubscribed streams are generally considered unread by the


oops: meant to say "read" instead of "unread" in this line

…ctor As Greg points out: zulip#304 (comment) > It and `ofMessage` and the plain one are the really > general-purpose constructors; as the number of special-purpose > constructors grows, it starts getting pushed down where it's > harder to spot.

chrisbobbe · 2023-09-27T10:01:16Z

Thanks for the review! I've pushed my latest work. I still need to do systematic timing and write tests, but I did an unscientific handful of trials, and I think probably for further efficiency work we might want to prioritize the event-handling code over the work done in the constructor from initial data.

On CZO and Recurse, where I have many thousands of unreads, I did a few initial loads and didn't see more than 3ms spent in the constructor. But on CZO, scrolling on desktop through a few screenfuls of unreads (fast enough to produce an event with more than just a few unreads in it), I saw the mark-as-read handler take up to 21ms for an event.

gnprice · 2023-09-28T00:43:06Z

On CZO and Recurse, where I have many thousands of unreads, I did a few initial loads and didn't see more than 3ms spent in the constructor.

Great!

I'd want to stress-test that by using a user that has the full 50k unreads, if you didn't already. One good way to get that is to take a test user on chat.zulip.org — either an old test user that already has tons of unreads, or you can make a fresh test user and then go use "mark as unread" in the all-messages narrow to gain a ton of unreads. That should be a good representative distribution, too (as would your own unreads in those two realms), which would otherwise be nontrivial to get on e.g. a test server.

If it's 3ms, or even up to like 50ms, in that situation, then I think we don't need to worry any further about performance at construction time.

But on CZO, scrolling on desktop through a few screenfuls of unreads (fast enough to produce an event with more than just a few unreads in it), I saw the mark-as-read handler take up to 21ms for an event.

Cool, so that's telling us this case would be the one to think about optimizing.

Looking back at those zulip-mobile commits I linked above, it looks like the times I saw were 30-50ms for this algorithm / these data structures. So the combination of Dart instead of JS/JSC, and whatever device you were timing on instead of the Pixel 2 XL I was using then, plus any small differences in your transcription, makes it roughly 2x faster. That may be about as good as one should expect.

The much faster timings, of <1ms up to an occasional 7ms, were observed in zulip/zulip-mobile@8dc3fe6 which used state.messages to avoid having to search through the whole data structure. That's the part that was buggy, got reverted, and is tracked as zulip/zulip-mobile#4684 ; a correct version will require more complexity. It'd be good to do that optimization in the Flutter app, but I think we should consider that out of scope for #253 and save it for post-launch.

Still, worth looking to see if there are any relatively easy ways we can optimize this pending that. I'm reading through the code now, and will look out for possibilities.

gnprice

OK, and on reading the code for marking as read, I do see what looks like an opportunity to optimize! Let's try it out.

Still reading the other code.

gnprice · 2023-09-28T00:52:04Z

lib/model/unreads.dart

+  void _slowRemoveAllInStreams(Iterable<int> messageIds) {
+    for (final messageId in messageIds) {
+      for (final stream in streams.values) {
+        for (final topic in stream.values) {
+          topic.remove(messageId);


Try this:

Suggested change

void _slowRemoveAllInStreams(Iterable<int> messageIds) {

for (final messageId in messageIds) {

for (final stream in streams.values) {

for (final topic in stream.values) {

topic.remove(messageId);

void _slowRemoveAllInStreams(Iterable<int> messageIds) {

final messageIdSet = Set.of(messageIds);

for (final stream in streams.values) {

for (final topic in stream.values) {

topic.removeWhere((id) => messageIdSet.contains(id));

I'm curious how that compares in your timing of mark-as-read events.

That's a more direct transcription of the zulip-mobile version:

function deleteMessages( state: UnreadStreamsState, ids: $ReadOnlyArray<number>, ): UnreadStreamsState { const idSet = new Set(ids); const toDelete = id => idSet.has(id); const emptyList: Immutable.List<number> = Immutable.List(); return updateAllAndPrune(state, Immutable.Map(), perStream => updateAllAndPrune(perStream, emptyList, (perTopic: Immutable.List<number>) => perTopic.find(toDelete) ? perTopic.filterNot(toDelete) : perTopic, ), ); }

(The find ? … : condition is an optimization to keep the resulting Immutable.List identical when it's going to have the same contents; here where we're mutating the same object anyway, that isn't needed.)

If there are N unreads known, and we get an event to remove k of them, then the zulip-mobile version will spend $O(N + k)$ time — $k$ to construct the set, and then $N$ to search through everything and remove matches. But the version in the PR will spend $O(k * N)$ time, which may be substantially more.

Could also improve a bit further by having this function take the Set<int> as an argument — the caller can reuse it for going through DMs.

gnprice · 2023-09-28T00:53:43Z

lib/model/unreads.dart

+      for (final stream in streams.values) {
+        for (final topic in stream.values) {
+          topic.remove(messageId);
+        }
+      }


This should gain logic to remove per-topic lists that have become empty, and then to remove per-stream maps that have become empty. That corresponds to the "prune" part of updateAllAndPrune in the zulip-mobile version (quoted in the preceding comment).

Makes sense. I'll add pruning to all the "remove" methods.

gnprice

OK, I've read through the three prep commits, and all the handle-event methods (and their private helpers) in the main commit. Generally all looks good! Comments below; and I see also there's a TODO to handle message moves, and one for adding tests.

Left for a future round is the rest of the main commit: so the changes in other files, and model/unreads.dart up to before handleMessageEvent.

(Also see short review above focused on the mark-as-read algorithm, and the comment above that about measuring performance.)

gnprice · 2023-09-28T01:04:17Z

lib/model/unreads.dart

+      case MessageFlag.mentioned:
+      case MessageFlag.wildcardMentioned:
+        // Empirically, we don't seem to get these events when a message is edited
+        // to add/remove an @-mention, even though @-mention state is represented
+        // as flags. Instead, we just get the [UpdateMessageEvent], and that
+        // contains the new set of flags, which we'll use to update [mentions].
+        // (See our handling of [UpdateMessageEvent].)
+        //
+        // Handle the event anyway, using the meaning on the tin.
+        // It might be used in a valid case we haven't thought of yet.


Hmm, interesting. Sure.

gnprice · 2023-09-28T01:08:46Z

lib/model/unreads.dart

+              if (detail == null) { // TODO(log) if on Zulip 6.0+
+                // Happens as a bug in some cases before fixed in Zulip 6.0:
+                //   https://chat.zulip.org/#narrow/stream/378-api-design/topic/unreads.20in.20unsubscribed.20streams/near/1458467
+                // TODO(server-6) remove Zulip 6.0 comment
+                return;


Perhaps better just continue — if we run into a bug like this, seems best to carry on with any messages we do have the details for.

gnprice · 2023-09-28T01:11:02Z

lib/model/unreads.dart

+                  ((newlyUnreadInStreams[detail.streamId!] ??= {})[detail.topic!] ??= QueueList())
+                    .add(messageId);


nit:

Suggested change

((newlyUnreadInStreams[detail.streamId!] ??= {})[detail.topic!] ??= QueueList())

.add(messageId);

final perStream = (newlyUnreadInStreams[detail.streamId!] ??= {});

final perTopic = (perStream[detail.topic!] ??= QueueList());

perTopic.add(messageId);

Only one line longer, and I think unpacking the two ??= onto separate lines is helpful for seeing everything that's happening.

gnprice · 2023-09-28T01:16:48Z

lib/model/unreads.dart

+  /// [messageIds] must be sorted ascending and without duplicates.
+  void _addAllInStreamTopic(QueueList<int> messageIds, int streamId, String topic) {


nit:

Suggested change

/// [messageIds] must be sorted ascending and without duplicates.

void _addAllInStreamTopic(QueueList<int> messageIds, int streamId, String topic) {

// [messageIds] must be sorted ascending and without duplicates.

void _addAllInStreamTopic(QueueList<int> messageIds, int streamId, String topic) {

because it doesn't have the form of a complete piece of documentation

gnprice · 2023-09-28T01:24:15Z

lib/model/unreads.dart

+    QueueList<int> updatedMessageIds = messageIds;
+    if (topics[topic] != null) {
+      // setUnion dedupes existing and incoming unread IDs,
+      // so we tolerate zulip/zulip#22164, fixed in 6.0
+      // TODO(server-6) remove 6.0 comment
+      updatedMessageIds = setUnion(topics[topic]!, messageIds);
+    }
+    topics[topic] = updatedMessageIds;


Suggested change

QueueList<int> updatedMessageIds = messageIds;

if (topics[topic] != null) {

// setUnion dedupes existing and incoming unread IDs,

// so we tolerate zulip/zulip#22164, fixed in 6.0

// TODO(server-6) remove 6.0 comment

updatedMessageIds = setUnion(topics[topic]!, messageIds);

}

topics[topic] = updatedMessageIds;

topics.update(topic,

ifAbsent: () => messageIds,

// setUnion dedupes existing and incoming unread IDs,

// so we tolerate zulip/zulip#22164, fixed in 6.0

// TODO(server-6) remove 6.0 comment

(existing) => setUnion(existing, messageIds),

);

This avoids repeated lookups of topic in the map topics. (I'd expect that the built-in Map implementation makes just one such lookup this way; pure Dart code using operators [] and []= can manage two; the current revision makes up to three.) I think it's also a bit easier to read.

gnprice · 2023-09-28T01:35:27Z

lib/model/unreads.dart

+  // TODO (?) instead of making a one-off [Set], write and use `setDifference`
+  //   that operates on our kinds of sets (implemented as sorted lists)
+  void _removeAllInStreamTopic(Set<int> messageIds, int streamId, String topic) {


It looks like we're only using this for DeleteMessageEvent. Those aren't common, I think, so no need to do anything fancy to optimize. (Especially as we're already, thankfully, doing this only for a single conversation's list of messages.)

gnprice · 2023-09-28T01:37:23Z

lib/model/unreads.dart

+        if (event.messageIds.length == 1) {
+          _removeInStreamTopic(event.messageIds.single, streamId, topic);


We can skip this optimization and just have the simplicity of always making a Set and passing to the remove-all method. That's because this is something we're doing only once for this event, not e.g. once per stream and topic in the data structure.

chrisbobbe · 2023-09-28T22:38:37Z

I'd want to stress-test that by using a user that has the full 50k unreads, if you didn't already. One good way to get that is to take a test user on chat.zulip.org — either an old test user that already has tons of unreads, or you can make a fresh test user and then go use "mark as unread" in the all-messages narrow to gain a ton of unreads. That should be a good representative distribution, too (as would your own unreads in those two realms), which would otherwise be nontrivial to get on e.g. a test server.

If it's 3ms, or even up to like 50ms, in that situation, then I think we don't need to worry any further about performance at construction time.

logging code

 class Unreads extends ChangeNotifier {
   factory Unreads({required UnreadMessagesSnapshot initial, required selfUserId}) {
+    final stopwatch = Stopwatch()..start();
+
     final streams = <int, Map<String, QueueList<int>>>{};
     final dms = <DmNarrow, QueueList<int>>{};
     final mentions = Set.of(initial.mentions);
@@ -60,13 +62,18 @@ class Unreads extends ChangeNotifier {
       dms[narrow] = QueueList.from(unreadHuddleSnapshot.unreadMessageIds);
     }
 
-    return Unreads._(
+    final result = Unreads._(
       streams: streams,
       dms: dms,
       mentions: mentions,
       oldUnreadsMissing: initial.oldUnreadsMissing,
       selfUserId: selfUserId,
     );
+
+    final t = (stopwatch..stop()).elapsed;
+    print('Unreads constructor time: ${t.inMilliseconds}ms');
+
+    return result;
   }

OK! I've just done this. With a test user who already had lots of unreads (unread_msgs had count: 50000 and old_unreads_missing: true), I started the app and tapped the test user to kick off a /register request, then watched the logs for how much time was spent in the Unreads constructor.

I did ten trials of this; here are the results in milliseconds: 6, 4, 4, 4, 5, 4, 6, 4, 4, 4

So, not a large distribution, and definitely well below 50ms.

…ctor As Greg points out: zulip#304 (comment) > It and `ofMessage` and the plain one are the really > general-purpose constructors; as the number of special-purpose > constructors grows, it starts getting pushed down where it's > harder to spot.

chrisbobbe · 2023-10-14T01:46:38Z

Just added a bunch of tests that are ready for an initial review pass 🙂 (I'm sure this is an area we'll want to iterate on).

gnprice

Thanks for the revision! Just looked through the code covered in my last review — all looks good except a few small comments below.

And I am still curious how the timing works out for mark-as-read events after the algorithms change discussed above 🙂 : #304 (comment)

I'll look at the tests next.

gnprice · 2023-10-16T18:25:52Z

lib/model/unreads.dart

+    }
+
+    // (Moved messages will be handled here;
+    // the TODO for that is just above the class declaration.)


(Hmm, I don't see that TODO.)

Indeed! Thanks! 😄

gnprice · 2023-10-16T18:30:50Z

lib/model/unreads.dart

+            newlyUnreadInStreams.forEach((incomingStreamId, incomingPerTopic) {
+              incomingPerTopic.forEach((incomingTopic, incomingMessageIds) {
+                _addAllInStreamTopic(incomingMessageIds..sort(), incomingStreamId, incomingTopic);


Hmm, this uses "per topic" with a different meaning from in the code above:

final perStream = (newlyUnreadInStreams[detail.streamId!] ??= {}); final perTopic = (perStream[detail.topic!] ??= QueueList()); perTopic.add(messageId);

The incomingPerTopic here corresponds to the perStream there.

Perhaps that means that "per topic" is too ambiguous — so possibly best to avoid it on both sides. But in any case should avoid using it with two contrasting meanings. 🙂

gnprice · 2023-10-16T18:42:59Z

lib/model/unreads.dart

+    final perTopic = streams[streamId];
+    if (perTopic == null) return;
+    final messageIds = perTopic[topic];
+    if (messageIds == null) return;


I think calling the intermediate variable here topics (like you used in the previous revision) instead of perTopic would work well. It's parallel to streams for the outer data structure, and messageIds for the inner.

gnprice · 2023-10-16T18:44:31Z

lib/model/unreads.dart

+  /// [messageIds] must be sorted ascending and without duplicates.
+  void _addAllInDm(QueueList<int> messageIds, DmNarrow dmNarrow) {


nit:

Suggested change

/// [messageIds] must be sorted ascending and without duplicates.

void _addAllInDm(QueueList<int> messageIds, DmNarrow dmNarrow) {

// [messageIds] must be sorted ascending and without duplicates.

void _addAllInDm(QueueList<int> messageIds, DmNarrow dmNarrow) {

gnprice

OK, and I've read through the tests. Generally they look good! Very thorough.

Comments below, including a few places where the implementation has some logic where it'd be easy for there to be a bug and so it'd be good to have tests targeting those specifically.

  - several TODOs are scattered through the tests; can postpone most
    of those?

Yeah, I think those can all be let be, except the "should not crash" cases. (And those don't need to be a lot of code — in particular it's fine to not cover all the possible combinations in areas where it seems like any reasonable implementation wouldn't look at a given field at all.)

  - seems like an issue when the test passes a Message
    object to the model, but the integrity of the test depends on
    the model not mutating it? could be a problem but I've found it
    makes certain things convenient

Hmm, yeah. I'm inclined to let this slide in these tests — partly because this data structure isn't expected to keep Message objects around, and so it's less likely it would mutate them.

If or when in some other test we find we do want to be careful about that, then probably one helpful tool would be to add a Message.copyWith method. Could annotate it @visibleForTesting, or even define it in test code as an extension method, to avoid absent-mindedly using it in the app code (where using it would probably be a bug).

gnprice · 2023-10-16T18:53:54Z

test/model/unreads_test.dart

+    assert((() {
+      final messageIds = messages.map((m) => m.id);
+      return Set.of(messageIds).length == messageIds.length;
+    })(), 'checkMatchesMessages: duplicate messages in test input');


nit: can tighten a bit:

Suggested change

assert((() {

final messageIds = messages.map((m) => m.id);

return Set.of(messageIds).length == messageIds.length;

})(), 'checkMatchesMessages: duplicate messages in test input');

assert(Set.of(messages.map((m) => m.id)).length == messages.length,

'checkMatchesMessages: duplicate messages in test input');

gnprice · 2023-10-16T18:54:39Z

test/model/unreads_test.dart

+    for (final message in messages) {
+      if (message.flags.contains(MessageFlag.read)) {
+        return;


continue, right?

Uh oh! Yep! Looks like two test failures after fixing this; investigating.

gnprice · 2023-10-16T18:58:32Z

test/model/unreads_test.dart

+        isUnread ? 'unread' : 'read',
+        isStream ? 'stream' : 'dm',
+        isDirectMentioned ? 'direct mentioned' : 'not direct mentioned',
+        isWildcardMentioned ? 'wildcard mentioned' : 'not wildcard mentioned',
+      ].join(' / ');
+      test(description, () {


gnprice · 2023-10-16T18:59:08Z

test/model/unreads_test.dart

+          if (!isUnread)   MessageFlag.read,
+          if (isDirectMentioned) MessageFlag.mentioned,
+          if (isWildcardMentioned) MessageFlag.wildcardMentioned,


nit:

Suggested change

if (!isUnread) MessageFlag.read,

if (isDirectMentioned) MessageFlag.mentioned,

if (isWildcardMentioned) MessageFlag.wildcardMentioned,

if (!isUnread) MessageFlag.read,

if (isDirectMentioned) MessageFlag.mentioned,

if (isWildcardMentioned) MessageFlag.wildcardMentioned,

or else aligned

gnprice · 2023-10-16T19:02:52Z

test/model/unreads_test.dart

+        for (final [
+          oldStream as ZulipStream, newStream as ZulipStream,
+          oldTopic as String,       newTopic as String,
+        ] in [
+          [stream1, stream1, 'a', 'a'],


Can avoid the downcasts by using tuples instead of lists:

Suggested change

for (final [

oldStream as ZulipStream, newStream as ZulipStream,

oldTopic as String, newTopic as String,

] in [

[stream1, stream1, 'a', 'a'],

for (final (oldStream, newStream, oldTopic, newTopic) in [

(stream1, stream1, 'a', 'a'),

Cool; I've done this here and elsewhere.

gnprice · 2023-10-16T19:43:48Z

test/model/unreads_test.dart

+            userIds: DmNarrow.ofMessage(message, selfUserId: eg.selfUser.userId)
+              .allRecipientIds
+              .where((id) => id != eg.selfUser.userId)
+              .toList(),


This is something we often need on a DmNarrow, so there's a property for it:

Suggested change

userIds: DmNarrow.ofMessage(message, selfUserId: eg.selfUser.userId)

.allRecipientIds

.where((id) => id != eg.selfUser.userId)

.toList(),

userIds: DmNarrow.ofMessage(message, selfUserId: eg.selfUser.userId)

.otherRecipientIds,

(and it's late final, so it's lazily computed but memoized).

gnprice · 2023-10-16T19:50:35Z

test/example_data.dart

-  return DmMessage.fromJson({
+  return DmMessage.fromJson(deepToJson({


Huh, was this just not working before? Surely we were using it…

aha, I guess the flags argument probably stopped working here when we converted them from strings to an enum.

Anyway, good for a small prep commit so it can get its own quick commit message.

gnprice · 2023-10-16T19:51:30Z

lib/model/unreads.dart

+      // setUnion dedupes existing and incoming unread IDs,
+      // so we tolerate zulip/zulip#22164, fixed in 6.0
+      // TODO(server-6) remove 6.0 comment
+      (existing) => setUnion(existing, messageIds),


This would be good to test. Similarly its streams counterpart.

gnprice · 2023-10-16T19:54:12Z

lib/model/unreads.dart

+                _addAllInStreamTopic(incomingMessageIds..sort(), incomingStreamId, incomingTopic);
+              });
+            });
+            newlyUnreadInDms.forEach((incomingDmNarrow, incomingMessageIds) {
+              _addAllInDm(incomingMessageIds..sort(), incomingDmNarrow);


These sort calls would be good to test.

gnprice · 2023-10-16T19:54:31Z

lib/model/unreads.dart

+              if (detail == null) { // TODO(log) if on Zulip 6.0+
+                // Happens as a bug in some cases before fixed in Zulip 6.0:
+                //   https://chat.zulip.org/#narrow/stream/378-api-design/topic/unreads.20in.20unsubscribed.20streams/near/1458467
+                // TODO(server-6) remove Zulip 6.0 comment
+                continue;


This would be good to test.

gnprice

OK, and finished reading everything! (In particular the parts I left out in the previous round of review: #304 (review) .) Comments below, mostly on comments.

gnprice · 2023-10-16T20:20:38Z

lib/api/model/initial_snapshot.dart

@@ -230,7 +230,7 @@ class UnreadMessagesSnapshot {
  final List<int> mentions;


Rereading the zulip-mobile type definitions here, I see we have:

// Unlike other lists of message IDs here, `mentions` is *not* sorted.

That warning is probably a good one to have in a comment here, too.

(It looks like the PR's data structure already has no trouble with that, as it promptly converts it to a Set and stops caring about the order.)

gnprice · 2023-10-16T20:27:47Z

lib/api/model/initial_snapshot.dart

@@ -259,7 +259,7 @@ class UnreadDmSnapshot {

  UnreadDmSnapshot({
    required this.otherUserId,
-    required this.unreadMessageIds,
+    required this.unreadMessageIds, // TODO assert sorted?


For the "TODO assert sorted?" comments in initial_snapshot.dart, see: https://chat.zulip.org/#narrow/stream/412-api-documentation/topic/register.3A.20.60unread_message_ids.60.20sorted.3F/near/1647932 (before merging we should make sure the doc will guarantee sorting)

I think from the state of that thread we can already rely on this (note Tim thumbs-up'd Alex's comment), though it'd be good to bump the thread to try to get the guarantee into the docs proper.

It'd be reasonable to have an assert that these are sorted; that'd just mean that if there is someday a server bug that breaks that invariant, and we happen to run across it while using a debug build, then we'll learn about it.

Could do it as an assert in the constructor, like at DmNarrow. Maybe take that same helper _isSortedWithoutDuplicates and move it somewhere like algorithms.dart to be shared.

I think I was originally thinking a check that would actually throw in production, since it'll always be a problem if it happens. Like we throw in production when e.g. mark-as-unread events are missing a message_details property:

factory UpdateMessageFlagsRemoveEvent.fromJson(Map<String, dynamic> json) { final result = _$UpdateMessageFlagsRemoveEventFromJson(json); // Crunchy-shell validation if ( result.flag == MessageFlag.read && true // (we assume `event_types` has `message` and `update_message_flags`) ) { result.messageDetails as Map<int, UpdateMessageFlagsMessageDetail>; } return result; }

Unless I hear otherwise I'll make it a regular assert in my next revision, but I'll add tests that the assert is there, which we can easily adapt to logic that throws in production (in which case tests will be necessary)

I think I was originally thinking a check that would actually throw in production, since it'll always be a problem if it happens.

Yeah, the reason I hesitate to add a check like that is that it's not free — it's a linear-time scan through each of these lists, so scanning through 50k elements at initial-fetch time in the case of a user with lots of unreads. That's something we could certainly afford if we had a good reason we needed it (we're unavoidably doing linear work on this data to process it, and this would be cheaper than other things we do); but since the server already gives us a guarantee, better not to spend time cross-checking it.

gnprice · 2023-10-16T20:33:17Z

lib/model/unreads.dart

+/// In each component of this model ([streams], [dms], [mentions]),
+/// if a message is not represented, its status is either read
+/// or unknown to the component. Absence implies no other meaning, like muted.


I don't follow this last sentence. Perhaps unpack it into more words?

Discussion: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/flutter.3A.20unreads.20model/near/1666124

gnprice · 2023-10-16T20:35:55Z

lib/model/unreads.dart

+  /// Unread DM messages, as: DM narrow → message ID.
+  final Map<DmNarrow, QueueList<int>> dms;
+
+  /// Messages with the self-user @-mentioned, directly or by wildcard.


Suggested change

/// Messages with the self-user @-mentioned, directly or by wildcard.

/// Unread messages with the self-user @-mentioned, directly or by wildcard.

gnprice · 2023-10-16T20:36:22Z

lib/model/unreads.dart

+  /// Implemented to track actual unread state as faithfully as possible.
+  /// Callers should do their own filtering based on other state, like muting,
+  /// as desired.


This remark applies to the whole class, right?

gnprice · 2023-10-16T20:39:20Z

lib/model/unreads.dart

+  /// If its state is actually unread, [mentions] recovers that knowledge when:
+  ///   a) the message is edited at all ([UpdateMessageEvent]), or
+  ///   b) the message gains a direct @-mention ([UpdateMessageFlagsEvent]), or
+  ///   c) (unimplemented) the user loads the message in the message list


How about:

Suggested change

/// c) (unimplemented) the user loads the message in the message list

/// c) TODO unimplemented: the user loads the message in the message list

That way when looking through TODO comments, there's one fewer layer of indirection to learn about the content of this one.

gnprice · 2023-10-16T20:50:51Z

lib/model/unreads.dart

+  /// Initialized to the value of [UnreadMessagesSnapshot.oldUnreadsMissing].
+  /// Is set to false when the user clears out all unreads at once
+  /// (signaled by a [UpdateMessageFlagsAddEvent] with [MessageFlag.read] and
+  /// `true` for [UpdateMessageFlagsAddEvent.all]).
+  bool oldUnreadsMissing;


Suggested change

/// Initialized to the value of [UnreadMessagesSnapshot.oldUnreadsMissing].

/// Is set to false when the user clears out all unreads at once

/// (signaled by a [UpdateMessageFlagsAddEvent] with [MessageFlag.read] and

/// `true` for [UpdateMessageFlagsAddEvent.all]).

bool oldUnreadsMissing;

/// Initialized to the value of [UnreadMessagesSnapshot.oldUnreadsMissing],

/// and set to false if the user clears out all unreads at once.

bool oldUnreadsMissing;

I think the details of how the latter is signaled are probably best read in the implementation, by just looking for references to this field.

(This is perhaps a difference from the style one wants in the upstream Flutter docs — those are meant to be read on the web, without consulting the implementation.)

(Or I guess a different angle: both this class, and its callers, are on the same side of the API this is describing — they're really both consumers of it. If at some point there's a reason for this logic to change, then this code will just unapologetically make the change; any other code that cares will change too, but it won't be because Unreads changed, rather it'll be because of the same Zulip API change that caused Unreads to change.)

gnprice · 2023-10-16T20:52:00Z

lib/model/unreads.dart

+    for (final unreadStreamSnapshot in initial.streams) {
+      final streamId = unreadStreamSnapshot.streamId;
+      final topic = unreadStreamSnapshot.topic;
+
+      (streams[streamId] ??= {})[topic] = QueueList.from(unreadStreamSnapshot.unreadMessageIds);
+    }
+
+    for (final unreadDmSnapshot in initial.dms) {
+      final otherUserId = unreadDmSnapshot.otherUserId;
+
+      final narrow = DmNarrow.withUser(otherUserId, selfUserId: selfUserId);
+      dms[narrow] = QueueList.from(unreadDmSnapshot.unreadMessageIds);
+    }


nit: each of these loops makes a nice single stanza:

Suggested change

for (final unreadStreamSnapshot in initial.streams) {

final streamId = unreadStreamSnapshot.streamId;

final topic = unreadStreamSnapshot.topic;

(streams[streamId] ??= {})[topic] = QueueList.from(unreadStreamSnapshot.unreadMessageIds);

}

for (final unreadDmSnapshot in initial.dms) {

final otherUserId = unreadDmSnapshot.otherUserId;

final narrow = DmNarrow.withUser(otherUserId, selfUserId: selfUserId);

dms[narrow] = QueueList.from(unreadDmSnapshot.unreadMessageIds);

}

for (final unreadStreamSnapshot in initial.streams) {

final streamId = unreadStreamSnapshot.streamId;

final topic = unreadStreamSnapshot.topic;

(streams[streamId] ??= {})[topic] = QueueList.from(unreadStreamSnapshot.unreadMessageIds);

}

for (final unreadDmSnapshot in initial.dms) {

final otherUserId = unreadDmSnapshot.otherUserId;

final narrow = DmNarrow.withUser(otherUserId, selfUserId: selfUserId);

dms[narrow] = QueueList.from(unreadDmSnapshot.unreadMessageIds);

}

gnprice · 2023-10-16T20:53:37Z

lib/model/unreads.dart

+    // TODO explore a different strategy with help from timing measurements:
+    //   https://github.com/zulip/zulip-flutter/pull/304#discussion_r1326660466


I think you've now done the measurements to conclude this strategy is fine:
#304 (comment)

As Greg suggests: zulip#304 (comment)

…ctor As Greg points out (before withUsers was added, but it should apply to that too): zulip#304 (comment) > [withUser] and `ofMessage` and the plain one are the really > general-purpose constructors; as the number of special-purpose > constructors grows, it starts getting pushed down where it's > harder to spot.

chrisbobbe · 2023-10-23T19:41:52Z

Thanks for the review! Revision pushed, so you can look at the code, and I'll do some stopwatching next.

chrisbobbe · 2023-10-23T20:36:37Z

Here's with a profile build running on my iPhone 13 Pro (iOS 16.6.1). This is with a test account on CZO with lots of unreads (the web app says 52,424 for "Inbox" after this experiment.) We see the time spent in the Unreads constructor (<0.5ms, I guess, rounded down to 0). Followed by me opening various streams with lots of unreads, in the web app, scrolling through to mark messages as read. Some of the events are from slow scrolling (so like one message at a time); some are from fast scrolling, which causes many message IDs to be lumped into one event.

flutter: Unreads constructor time: 0ms (`count` is 44399)
flutter: handleUpdateMessageFlagsEvent time: 13ms
flutter: handleUpdateMessageFlagsEvent time: 15ms
flutter: handleUpdateMessageFlagsEvent time: 14ms
flutter: handleUpdateMessageFlagsEvent time: 13ms
flutter: handleUpdateMessageFlagsEvent time: 14ms
flutter: handleUpdateMessageFlagsEvent time: 15ms
flutter: handleUpdateMessageFlagsEvent time: 21ms
flutter: handleUpdateMessageFlagsEvent time: 13ms
flutter: handleUpdateMessageFlagsEvent time: 13ms
flutter: handleUpdateMessageFlagsEvent time: 13ms
flutter: handleUpdateMessageFlagsEvent time: 14ms
flutter: handleUpdateMessageFlagsEvent time: 14ms
flutter: handleUpdateMessageFlagsEvent time: 15ms
flutter: handleUpdateMessageFlagsEvent time: 14ms
flutter: handleUpdateMessageFlagsEvent time: 13ms
flutter: handleUpdateMessageFlagsEvent time: 14ms
flutter: handleUpdateMessageFlagsEvent time: 14ms
flutter: handleUpdateMessageFlagsEvent time: 14ms
flutter: handleUpdateMessageFlagsEvent time: 12ms
flutter: handleUpdateMessageFlagsEvent time: 13ms

chrisbobbe · 2023-10-23T20:42:48Z

OK, and now after marking all 526 of this test user's DMs unread, here's going through the #narrow/is/private narrow. Again, sometimes going slowly to just mark one unread, sometimes many:

flutter: Unreads constructor time: 0ms (`count` is 44033)
flutter: handleUpdateMessageFlagsEvent time: 14ms
flutter: handleUpdateMessageFlagsEvent time: 15ms
flutter: handleUpdateMessageFlagsEvent time: 14ms
flutter: handleUpdateMessageFlagsEvent time: 14ms
flutter: handleUpdateMessageFlagsEvent time: 14ms
flutter: handleUpdateMessageFlagsEvent time: 14ms
flutter: handleUpdateMessageFlagsEvent time: 22ms
flutter: handleUpdateMessageFlagsEvent time: 13ms
flutter: handleUpdateMessageFlagsEvent time: 13ms
flutter: handleUpdateMessageFlagsEvent time: 14ms
flutter: handleUpdateMessageFlagsEvent time: 13ms
flutter: handleUpdateMessageFlagsEvent time: 12ms
flutter: handleUpdateMessageFlagsEvent time: 14ms

chrisbobbe · 2023-10-23T20:44:47Z

marking all 526 of this test user's DMs unread

In two trials of this, the mark-as-unread operation took 2ms and 1ms to handle.

chrisbobbe · 2023-10-23T20:47:26Z

Here's alternately marking the 728 latest messages in CZO's #api documentation as read and unread:

flutter: handleUpdateMessageFlagsEvent time: 14ms
flutter: handleUpdateMessageFlagsEvent time: 0ms
flutter: handleUpdateMessageFlagsEvent time: 14ms
flutter: handleUpdateMessageFlagsEvent time: 0ms
flutter: handleUpdateMessageFlagsEvent time: 14ms
flutter: handleUpdateMessageFlagsEvent time: 0ms
flutter: handleUpdateMessageFlagsEvent time: 14ms
flutter: handleUpdateMessageFlagsEvent time: 0ms
flutter: handleUpdateMessageFlagsEvent time: 13ms
flutter: handleUpdateMessageFlagsEvent time: 0ms
flutter: handleUpdateMessageFlagsEvent time: 14ms

(The 0ms is the mark as unread.)

chrisbobbe · 2023-10-23T20:53:01Z

Here's sending messages as the self-user to #test here > Chris, so they're read when they come in:

flutter: handleMessageEvent time: 0ms
flutter: handleMessageEvent time: 0ms
flutter: handleMessageEvent time: 0ms
flutter: handleMessageEvent time: 0ms
flutter: handleMessageEvent time: 0ms
flutter: handleMessageEvent time: 0ms
flutter: handleMessageEvent time: 0ms

(Quick and easy, as expected; the model sees that these new incoming messages are read and early-returns.)

And sending messages as a different user in the same topic, so that they're unread when they come in:

flutter: handleMessageEvent time: 0ms
flutter: handleMessageEvent time: 0ms
flutter: handleMessageEvent time: 0ms
flutter: handleMessageEvent time: 0ms
flutter: handleMessageEvent time: 0ms
flutter: handleMessageEvent time: 0ms
flutter: handleMessageEvent time: 0ms
flutter: handleMessageEvent time: 0ms

The code has more to do in this case, but nothing we've marked as being "slow" with a perf TODO, so it's unsurprising that those timings are acceptable. (In these trials, the topic had no unreads in it, but reading the implementation, I don't expect that to make a difference.)

(Just tried another send-message-as-different user, this time mentioning the self-user test account. Also 0ms; we don't expect handling a new message with a mention to be expensive.)

gnprice

Thanks! Quick comments first on one commit:
66ac06e tests [nfc]: Pull out helper eg.updateMessageFlagsRemoveEvent

because I'd like to merge that early for use in #317.

gnprice · 2023-10-23T20:12:02Z

test/example_data.dart

+}) {
+  return UpdateMessageFlagsRemoveEvent(
+    id: 0,
+    flag: MessageFlag.read,


This should be flag: flag, right?

Oops! Yep, thanks for the catch.

gnprice · 2023-10-23T20:52:16Z

test/example_data.dart

+      final mentioned = message.flags.contains(MessageFlag.mentioned)
+        || message.flags.contains(MessageFlag.wildcardMentioned);
+
+      return MapEntry(


nit:

Suggested change

final mentioned = message.flags.contains(MessageFlag.mentioned)

|| message.flags.contains(MessageFlag.wildcardMentioned);

return MapEntry(

final mentioned = message.flags.contains(MessageFlag.mentioned)

|| message.flags.contains(MessageFlag.wildcardMentioned);

return MapEntry(

chrisbobbe · 2023-10-23T21:04:20Z

Thanks for those comments! Revision pushed, addressing them.

gnprice · 2023-10-23T21:15:53Z

Cool, thanks for those timings.

It sounds like the mark-as-read handler takes about the same amount of time as you found in an earlier revision: #304 (comment)

I saw the mark-as-read handler take up to 21ms for an event.

in that it's now typically about 14ms, and you observed one at 21ms and one at 22ms.

I'm a bit surprised that the change here: #304 (comment) didn't help; but that's how it goes sometimes when trying to optimize. (Especially when doing so without first looking at a profile of where the time is going.)

Still not worse than zulip-mobile, though, as discussed here:
#304 (comment)

I'll be fine with merging it with the current performance, but will definitely be interested in trying to optimize it later. Given how very fast the constructor is in particular, I feel like it should be possible to get mark-as-read to be much faster — in particular I think it fundamentally shouldn't be any slower than the constructor. And taking 14ms is enough that it'll often cause a dropped frame, so that's worth avoiding.

chrisbobbe · 2023-10-23T21:18:15Z

Editing an unread message (as a non-self-user) in #test here > Chris so it flaps between having an @-mention and not:

flutter: handleUpdateMessageEvent time: 0ms
flutter: handleUpdateMessageEvent time: 0ms
flutter: handleUpdateMessageEvent time: 0ms
flutter: handleUpdateMessageEvent time: 0ms
flutter: handleUpdateMessageEvent time: 0ms

Also expected to be quick, so no surprise here. (I assume that, as expected, the @-mention changes were signaled just by update-message events, not update-message-flags events.)

chrisbobbe · 2023-10-23T21:22:04Z

A few delete-message events (of unread messages), for stream messages and DMs, just showed up as 0ms too. (That's not a really common case but we don't want it to be catastrophically slow.)

gnprice · 2023-10-23T21:31:59Z

Thanks for the quick revision! Merging that commit and one other like it:
e546b28 tests [nfc]: Pull out helper eg.updateMessageFlagsRemoveEvent
528651b tests: Fix eg.dmMessage's flags argument after converting to enum

as:
9dafc79 test [nfc]: Pull out helper eg.updateMessageFlagsRemoveEvent
6b17ec5 test: Fix eg.dmMessage's flags argument after converting to enum
92b4d3f test [nfc]: Add section headings in example_data.dart

I tweaked the commit-message prefixes to match what we've been using, and added a commit on top with some further organization for that file example_data.dart as it's grown. I have a further small NFC commit for it which I'll send as a PR. → #330

chrisbobbe · 2023-10-23T21:41:55Z

For the mark-as-read handler (with event.all: false), with this same test user on CZO, some experiments adjusting the stopwatch code suggest that almost all of the time is being spent in _slowRemoveAllInStreams. Which makes sense; we know it's inefficient. It can't yet directly look up message IDs in the model, so it has to traverse all streams/topics with tracked unreads. And there are a lot of those in this case; much more than unread DMs, and probably that's typical.

As Greg suggests: zulip#304 (comment)

…here

See discussion, which should produce a docs change soon: https://chat.zulip.org/#narrow/stream/412-api-documentation/topic/register.3A.20.60unread_message_ids.60.20sorted.3F/near/1647932

…essage

…ctor As Greg points out (before withUsers was added, but it should apply to that too): zulip#304 (comment) > [withUser] and `ofMessage` and the plain one are the really > general-purpose constructors; as the number of special-purpose > constructors grows, it starts getting pushed down where it's > harder to spot.

Transcribed from the same-named function in zulip-mobile: see src/immutableUtils.js.

chrisbobbe · 2023-10-23T21:47:03Z

Thanks! Just rebased atop that and #330, removed the stopwatching commit, and unmarked as draft! 🎉

gnprice

Thanks @chrisbobbe for all your work on this! All looks great modulo one small comment below.

gnprice · 2023-10-23T21:42:44Z

test/model/unreads_test.dart

+      ];
+
+      group('DM narrow subtypes', () {
+        for (final [desc as String, from as User, to as List<User>] in variousDms) {


This and the next group have a handful of stragglers from #304 (comment) .

chrisbobbe · 2023-10-23T22:11:53Z

Thanks for the review! Revision pushed, fixing those stragglers.

gnprice · 2023-10-23T22:47:23Z

Thanks! Looks good; merging.

Sometimes we get feedback from a lint rule -- https://dart.dev/tools/linter-rules/avoid_function_literals_in_foreach_calls -- whose documentation recommends using for loops instead of forEach. That rule didn't fire on these instances; shrug; but this file has had a mix of for loops and forEach loops, and it seems nicer to consistently use one or the other. Just in case there was a performance bug in a Map.forEach implementation, I tested mark-as-read (by scrolling through the web app) to see if we might get a free performance benefit there, where the norm on my device (with a profile build) has been to take about 14ms per event: zulip#304 (comment) But, unsurprisingly, it stayed at around 14ms as before.

Sometimes we get feedback from a lint rule -- https://dart.dev/tools/linter-rules/avoid_function_literals_in_foreach_calls -- whose documentation recommends using for loops instead of forEach. That rule didn't fire on these instances; not sure why. But anyway this file has had a mix of for loops and forEach loops, and it seems nicer to consistently use one or the other. Just in case there was a performance bug in a Map.forEach implementation, I tested mark-as-read (by scrolling through the web app) to see if we might get a free performance benefit there, where the norm on my device (with a profile build) has been to take about 14ms per event: #304 (comment) But, unsurprisingly, it stayed at around 14ms as before.

Sometimes we get feedback from a lint rule -- https://dart.dev/tools/linter-rules/avoid_function_literals_in_foreach_calls -- whose documentation recommends using for loops instead of forEach. That rule didn't fire on these instances; not sure why. But anyway this file has had a mix of for loops and forEach loops, and it seems nicer to consistently use one or the other. Just in case there was a performance bug in a Map.forEach implementation, I tested mark-as-read (by scrolling through the web app) to see if we might get a free performance benefit there, where the norm on my device (with a profile build) has been to take about 14ms per event: zulip#304 (comment) But, unsurprisingly, it stayed at around 14ms as before.

chrisbobbe added the a-model Implementing our data model (PerAccountStore, etc.) label Sep 14, 2023

chrisbobbe requested a review from gnprice September 14, 2023 17:31

gnprice reviewed Sep 15, 2023

View reviewed changes

chrisbobbe commented Sep 15, 2023

View reviewed changes

chrisbobbe force-pushed the wip-unread-model branch from 69e2a9d to 28b5f99 Compare September 27, 2023 09:56

chrisbobbe force-pushed the wip-unread-model branch from 28b5f99 to e127c5d Compare September 27, 2023 19:32

gnprice reviewed Sep 28, 2023

View reviewed changes

chrisbobbe force-pushed the wip-unread-model branch from e127c5d to 8f30bfe Compare October 14, 2023 01:45

gnprice reviewed Oct 16, 2023

View reviewed changes

chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this pull request Oct 23, 2023

api [nfc]: Add warning about a list not being sorted

7ff3ac2

As Greg suggests: zulip#304 (comment)

chrisbobbe force-pushed the wip-unread-model branch from 8f30bfe to af399c6 Compare October 23, 2023 19:41

gnprice reviewed Oct 23, 2023

View reviewed changes

chrisbobbe force-pushed the wip-unread-model branch from af399c6 to 04e199b Compare October 23, 2023 21:04

gnprice mentioned this pull request Oct 23, 2023

test [nfc]: Reorder example data and group a bit more finely #330

Merged

chrisbobbe added 6 commits October 23, 2023 17:43

api [nfc]: Add warning about a list not being sorted

e52d3f6

As Greg suggests: zulip#304 (comment)

algorithms [nfc]: Pull local helper isSortedWithoutDuplicates out to …

d40e5ff

…here

api: Assert that unread dm and stream snapshots have sorted message IDs

18806be

See discussion, which should produce a docs change soon: https://chat.zulip.org/#narrow/stream/412-api-documentation/topic/register.3A.20.60unread_message_ids.60.20sorted.3F/near/1647932

api: Ensure DeleteMessageEvent.{streamId,topic} present when stream m…

667d191

…essage

algorithms: Add setUnion, operating on sets implemented as sorted lists

1fde1da

Transcribed from the same-named function in zulip-mobile: see src/immutableUtils.js.

chrisbobbe force-pushed the wip-unread-model branch from 04e199b to 468d7fb Compare October 23, 2023 21:46

chrisbobbe marked this pull request as ready for review October 23, 2023 21:46

gnprice reviewed Oct 23, 2023

View reviewed changes

gnprice mentioned this pull request Oct 23, 2023

Unread markers #317

Merged

gnprice changed the title ~~WIP model: Add Unreads model, for tracking unread-message counts~~ model: Add Unreads model, for tracking unread-message counts Oct 23, 2023

model: Add Unreads model, for tracking unread-message counts

0266ff4

chrisbobbe force-pushed the wip-unread-model branch from 468d7fb to 0266ff4 Compare October 23, 2023 22:11

gnprice merged commit 0266ff4 into zulip:main Oct 23, 2023
1 check passed

This was referenced Oct 23, 2023

narrow [nfc]: Small tweaks to DmNarrow constructors #331

Merged

Optimize unreads model on mark-as-read #332

Open

chrisbobbe mentioned this pull request Oct 27, 2023

unreads: Track total and per-stream unread counts, in addition to IDs #345

Closed

gnprice mentioned this pull request Oct 31, 2023

Track unread counts #253

Closed

		((newlyUnreadInStreams[detail.streamId!] ??= {})[detail.topic!] ??= QueueList())
		.add(messageId);

-                  ((newlyUnreadInStreams[detail.streamId!] ??= {})[detail.topic!] ??= QueueList())
-                    .add(messageId);
+                  final perStream = (newlyUnreadInStreams[detail.streamId!] ??= {});
+                  final perTopic = (perStream[detail.topic!] ??= QueueList());
+                  perTopic.add(messageId);

		/// [messageIds] must be sorted ascending and without duplicates.
		void _addAllInStreamTopic(QueueList<int> messageIds, int streamId, String topic) {

		if (event.messageIds.length == 1) {
		_removeInStreamTopic(event.messageIds.single, streamId, topic);

		/// [messageIds] must be sorted ascending and without duplicates.
		void _addAllInDm(QueueList<int> messageIds, DmNarrow dmNarrow) {

		return DmMessage.fromJson({
		return DmMessage.fromJson(deepToJson({

		@@ -230,7 +230,7 @@ class UnreadMessagesSnapshot {
		final List<int> mentions;

	/// Messages with the self-user @-mentioned, directly or by wildcard.
	/// Unread messages with the self-user @-mentioned, directly or by wildcard.

	/// c) (unimplemented) the user loads the message in the message list
	/// c) TODO unimplemented: the user loads the message in the message list

		// TODO explore a different strategy with help from timing measurements:
		// https://github.com/zulip/zulip-flutter/pull/304#discussion_r1326660466

model: Add Unreads model, for tracking unread-message counts #304

model: Add Unreads model, for tracking unread-message counts #304

Conversation

chrisbobbe commented Sep 14, 2023 • edited by gnprice 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

chrisbobbe commented Sep 27, 2023 • edited Loading

gnprice commented Sep 28, 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

gnprice left a comment • edited Loading

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

chrisbobbe commented Sep 28, 2023

chrisbobbe commented Oct 14, 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

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

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

chrisbobbe commented Oct 23, 2023

chrisbobbe commented Oct 23, 2023

chrisbobbe commented Oct 23, 2023

chrisbobbe commented Oct 23, 2023

chrisbobbe commented Oct 23, 2023

chrisbobbe commented Oct 23, 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

chrisbobbe commented Oct 23, 2023

gnprice commented Oct 23, 2023

chrisbobbe commented Sep 14, 2023 •

edited by gnprice

Loading

chrisbobbe commented Sep 27, 2023 •

edited

Loading

gnprice left a comment •

edited

Loading

chrisbobbe commented Oct 23, 2023 •

edited

Loading

gnprice commented Oct 23, 2023 •

edited

Loading