Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 6 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a7f7fff
unreads: Be more precise in dartdoc's description of information gaps
chrisbobbe Oct 26, 2023
cc76f2a
unreads [nfc]: Use for/in style instead of forEach
chrisbobbe Oct 26, 2023
0ebaef1
unreads: Track whole-model unread count
chrisbobbe Oct 26, 2023
aeb570f
test: Fix jsonEquals for int-keyed Maps
chrisbobbe Oct 27, 2023
ad074b2
unreads [nfc]: Add StreamUnreads class, to store per-stream unread count
chrisbobbe Oct 27, 2023
848d233
unreads: Track per-stream unread counts
chrisbobbe Oct 27, 2023
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
155 changes: 120 additions & 35 deletions lib/model/unreads.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,13 @@ import 'narrow.dart';
/// Callers should do their own filtering based on other state, like muting,
/// as desired.
///
/// In each component of this model ([streams], [dms], [mentions]),
/// In each component of this model ([_totalCount], [streams], [dms], [mentions]),
/// if a message is not represented, its status is either read
/// or unknown to the component. In all components,
/// a message's status will be unknown if, at /register time,
/// it was very old by the server's reckoning. See [oldUnreadsMissing].
/// It may also be unknown for other reasons that differ by component; see each.
/// In [mentions], there's another more complex reason
/// the state might be unknown; see there.
Comment on lines -25 to +26
Copy link
Member

Choose a reason for hiding this comment

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

nit: commit message can mark as NFC

///
/// Messages in unsubscribed streams, and messages sent by muted users,
/// are generally deemed read by the server and shouldn't be expected to appear.
Expand All @@ -39,28 +40,37 @@ import 'narrow.dart';
// messages and refresh [mentions] (see [mentions] dartdoc).
class Unreads extends ChangeNotifier {
factory Unreads({required UnreadMessagesSnapshot initial, required selfUserId}) {
final streams = <int, Map<String, QueueList<int>>>{};
int totalCount = 0;
final streams = <int, StreamUnreads>{};
final dms = <DmNarrow, QueueList<int>>{};
final mentions = Set.of(initial.mentions);

for (final unreadStreamSnapshot in initial.streams) {
final streamId = unreadStreamSnapshot.streamId;
final topic = unreadStreamSnapshot.topic;
(streams[streamId] ??= {})[topic] = QueueList.from(unreadStreamSnapshot.unreadMessageIds);
final streamUnreads = streams[streamId] ??= StreamUnreads.empty();
final unreadInTopic = unreadStreamSnapshot.unreadMessageIds.length;
streamUnreads
..topics[topic] = QueueList.from(unreadStreamSnapshot.unreadMessageIds)
..count += unreadInTopic;
totalCount += unreadInTopic;
}

for (final unreadDmSnapshot in initial.dms) {
final otherUserId = unreadDmSnapshot.otherUserId;
final narrow = DmNarrow.withUser(otherUserId, selfUserId: selfUserId);
dms[narrow] = QueueList.from(unreadDmSnapshot.unreadMessageIds);
totalCount += unreadDmSnapshot.unreadMessageIds.length;
}

for (final unreadHuddleSnapshot in initial.huddles) {
final narrow = DmNarrow.ofUnreadHuddleSnapshot(unreadHuddleSnapshot, selfUserId: selfUserId);
dms[narrow] = QueueList.from(unreadHuddleSnapshot.unreadMessageIds);
totalCount += unreadHuddleSnapshot.unreadMessageIds.length;
}

return Unreads._(
totalCount: totalCount,
streams: streams,
dms: dms,
mentions: mentions,
Expand All @@ -70,18 +80,27 @@ class Unreads extends ChangeNotifier {
}

Unreads._({
required totalCount,
required this.streams,
required this.dms,
required this.mentions,
required this.oldUnreadsMissing,
required this.selfUserId,
});
}) : _totalCount = totalCount;

// TODO excluded for now; would need to handle nuances around muting etc.
// int count;
/// Total unread messages.
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, but do we have a use case for this?

I don't immediately see one — I feel like any place that we might want a count like this one, we'd actually want the version that accounts for muting.

If we don't have a use case where it'd be valid to use, then given that it looks appealingly like something one might want in situations where in fact it'd be buggy to use it, it's probably best not to have it around.

(If we did have reason to keep it around, I'd want the doc to clearly warn that it's not the "total number of unreads" one wants in the UI.)

///
/// Prefer this when possible over traversing the model to make a sum.
///
/// We initialize and maintain this ourselves,
/// ignoring [UnreadMessagesSnapshot.count] because it has information gaps
/// beyond the ones explained in [oldUnreadsMissing]:
/// https://chat.zulip.org/#narrow/stream/378-api-design/topic/register.3A.20maintaining.20.60unread_msgs.2Ementions.60.20correctly/near/1668449
int get totalCount => _totalCount;
int _totalCount;

/// Unread stream messages, as: stream ID → topic → message ID.
final Map<int, Map<String, QueueList<int>>> streams;
final Map<int, StreamUnreads> streams;

/// Unread DM messages, as: DM narrow → message ID.
final Map<DmNarrow, QueueList<int>> dms;
Expand Down Expand Up @@ -250,6 +269,7 @@ class Unreads extends ChangeNotifier {
switch (event) {
case UpdateMessageFlagsAddEvent():
if (event.all) {
_totalCount = 0;
streams.clear();
dms.clear();
mentions.clear();
Expand Down Expand Up @@ -286,14 +306,23 @@ class Unreads extends ChangeNotifier {
.add(messageId);
}
}
newlyUnreadInStreams.forEach((incomingStreamId, incomingTopics) {
incomingTopics.forEach((incomingTopic, incomingMessageIds) {
for (
final MapEntry(key: incomingStreamId, value: incomingTopics)
in newlyUnreadInStreams.entries
) {
for (
final MapEntry(key: incomingTopic, value: incomingMessageIds)
in incomingTopics.entries
) {
_addAllInStreamTopic(incomingMessageIds..sort(), incomingStreamId, incomingTopic);
Comment on lines +309 to 317
Copy link
Member

Choose a reason for hiding this comment

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

I wish I had a good idea for making these more concise, but I don't. :-/ Short of, like, an extension entryPairs on Map to return 2-tuples instead of MapEntry — which is probably what Map.entries would have done in the first place if tuples/records had existed in Dart from early on.

Can at least make them more compact vertically, though:

Suggested change
for (
final MapEntry(key: incomingStreamId, value: incomingTopics)
in newlyUnreadInStreams.entries
) {
for (
final MapEntry(key: incomingTopic, value: incomingMessageIds)
in incomingTopics.entries
) {
_addAllInStreamTopic(incomingMessageIds..sort(), incomingStreamId, incomingTopic);
for (final MapEntry(key: incomingStreamId, value: incomingTopics)
in newlyUnreadInStreams.entries) {
for (final MapEntry(key: incomingTopic, value: incomingMessageIds)
in incomingTopics.entries) {
_addAllInStreamTopic(incomingMessageIds..sort(), incomingStreamId, incomingTopic);

});
});
newlyUnreadInDms.forEach((incomingDmNarrow, incomingMessageIds) {
}
}
for (
final MapEntry(key: incomingDmNarrow, value: incomingMessageIds)
in newlyUnreadInDms.entries
) {
_addAllInDm(incomingMessageIds..sort(), incomingDmNarrow);
});
}
}
}
notifyListeners();
Expand All @@ -302,62 +331,87 @@ class Unreads extends ChangeNotifier {
// TODO use efficient lookups
bool _slowIsPresentInStreams(int messageId) {
return streams.values.any(
(topics) => topics.values.any(
(streamUnreads) => streamUnreads.topics.values.any(
(messageIds) => messageIds.contains(messageId),
),
);
}

void _addLastInStreamTopic(int messageId, int streamId, String topic) {
((streams[streamId] ??= {})[topic] ??= QueueList()).addLast(messageId);
final streamUnreads = streams[streamId] ??= StreamUnreads.empty();
(streamUnreads.topics[topic] ??= QueueList()).addLast(messageId);
streamUnreads.count++;
_totalCount += 1;
}

// [messageIds] must be sorted ascending and without duplicates.
void _addAllInStreamTopic(QueueList<int> messageIds, int streamId, String topic) {
final topics = streams[streamId] ??= {};
topics.update(topic,
ifAbsent: () => messageIds,
int numAdded = 0;
final streamUnreads = streams[streamId] ??= StreamUnreads.empty();
streamUnreads.topics.update(topic,
ifAbsent: () {
numAdded = messageIds.length;
return 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),
(existing) {
final result = setUnion(existing, messageIds);
numAdded = result.length - messageIds.length;
return result;
},
);
streamUnreads.count += numAdded;
_totalCount += numAdded;
}

// TODO use efficient model lookups
void _slowRemoveAllInStreams(Set<int> idsToRemove) {
int totalRemoved = 0;
final newlyEmptyStreams = [];
streams.forEach((streamId, topics) {
for (final MapEntry(key: streamId, value: streamUnreads) in streams.entries) {
int removedInStream = 0;
final newlyEmptyTopics = [];
topics.forEach((topic, messageIds) {
for (final MapEntry(key: topic, value: messageIds) in streamUnreads.topics.entries) {
final lengthBefore = messageIds.length;
messageIds.removeWhere((id) => idsToRemove.contains(id));
final removedInTopic = lengthBefore - messageIds.length;
removedInStream += removedInTopic;
totalRemoved += removedInTopic;
if (messageIds.isEmpty) {
newlyEmptyTopics.add(topic);
}
});
}
for (final topic in newlyEmptyTopics) {
topics.remove(topic);
streamUnreads.topics.remove(topic);
}
if (topics.isEmpty) {
if (streamUnreads.topics.isEmpty) {
newlyEmptyStreams.add(streamId);
}
});
streamUnreads.count -= removedInStream;
}
for (final streamId in newlyEmptyStreams) {
streams.remove(streamId);
}
_totalCount -= totalRemoved;
}

void _removeAllInStreamTopic(Set<int> incomingMessageIds, int streamId, String topic) {
final topics = streams[streamId];
if (topics == null) return;
final messageIds = topics[topic];
final streamUnreads = streams[streamId];
if (streamUnreads == null) return;
final messageIds = streamUnreads.topics[topic];
if (messageIds == null) return;

// ([QueueList] doesn't have a `removeAll`)
final lengthBefore = messageIds.length;
messageIds.removeWhere((id) => incomingMessageIds.contains(id));
final numRemoved = lengthBefore - messageIds.length;
streamUnreads.count -= numRemoved;
_totalCount -= numRemoved;
if (messageIds.isEmpty) {
topics.remove(topic);
if (topics.isEmpty) {
streamUnreads.topics.remove(topic);
if (streamUnreads.topics.isEmpty) {
streams.remove(streamId);
}
}
Expand All @@ -370,30 +424,61 @@ class Unreads extends ChangeNotifier {

void _addLastInDm(int messageId, DmNarrow narrow) {
(dms[narrow] ??= QueueList()).addLast(messageId);
_totalCount += 1;
}

// [messageIds] must be sorted ascending and without duplicates.
void _addAllInDm(QueueList<int> messageIds, DmNarrow dmNarrow) {
int numAdded = 0;
dms.update(dmNarrow,
ifAbsent: () => messageIds,
ifAbsent: () {
numAdded = messageIds.length;
return 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),
(existing) {
final result = setUnion(existing, messageIds);
numAdded = result.length - messageIds.length;
return result;
},
);
_totalCount += numAdded;
}

// TODO use efficient model lookups
void _slowRemoveAllInDms(Set<int> idsToRemove) {
int numRemoved = 0;
final newlyEmptyDms = [];
dms.forEach((dmNarrow, messageIds) {
for (final MapEntry(key: dmNarrow, value: messageIds) in dms.entries) {
final lengthBefore = messageIds.length;
messageIds.removeWhere((id) => idsToRemove.contains(id));
numRemoved += lengthBefore - messageIds.length;
if (messageIds.isEmpty) {
newlyEmptyDms.add(dmNarrow);
}
});
}
for (final dmNarrow in newlyEmptyDms) {
dms.remove(dmNarrow);
}
_totalCount -= numRemoved;
}
}

class StreamUnreads {
StreamUnreads({required this.count, required this.topics});
StreamUnreads.empty()
: count = 0,
topics = {};

/// Total unread messages in this stream.
///
/// Prefer this when possible over traversing [topics] to make a sum.
int count;

Map<String, QueueList<int>> topics;

@visibleForTesting
Map<String, dynamic> toJson() => {'count': count, 'topics': topics};
}
3 changes: 2 additions & 1 deletion test/model/unreads_checks.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ import 'package:zulip/model/narrow.dart';
import 'package:zulip/model/unreads.dart';

extension UnreadsChecks on Subject<Unreads> {
Subject<Map<int, Map<String, QueueList<int>>>> get streams => has((u) => u.streams, 'streams');
Subject<int> get count => has((u) => u.totalCount, 'count');
Subject<Map<int, StreamUnreads>> get streams => has((u) => u.streams, 'streams');
Subject<Map<DmNarrow, QueueList<int>>> get dms => has((u) => u.dms, 'dms');
Subject<Set<int>> get mentions => has((u) => u.mentions, 'mentions');
Subject<bool> get oldUnreadsMissing => has((u) => u.oldUnreadsMissing, 'oldUnreadsMissing');
Expand Down
15 changes: 9 additions & 6 deletions test/model/unreads_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import 'package:zulip/model/narrow.dart';
import 'package:zulip/model/unreads.dart';

import '../example_data.dart' as eg;
import '../stdlib_checks.dart';
import 'unreads_checks.dart';

void main() {
Expand Down Expand Up @@ -53,7 +54,7 @@ void main() {
assert(Set.of(messages.map((m) => m.id)).length == messages.length,
'checkMatchesMessages: duplicate messages in test input');

final Map<int, Map<String, QueueList<int>>> expectedStreams = {};
final Map<int, StreamUnreads> expectedStreams = {};
final Map<DmNarrow, QueueList<int>> expectedDms = {};
final Set<int> expectedMentions = {};
for (final message in messages) {
Expand All @@ -62,9 +63,10 @@ void main() {
}
switch (message) {
case StreamMessage():
final perTopic = expectedStreams[message.streamId] ??= {};
final messageIds = perTopic[message.subject] ??= QueueList();
final streamUnreads = expectedStreams[message.streamId] ??= StreamUnreads.empty();
final messageIds = streamUnreads.topics[message.subject] ??= QueueList();
messageIds.add(message.id);
streamUnreads.count++;
case DmMessage():
final narrow = DmNarrow.ofMessage(message, selfUserId: eg.selfUser.userId);
final messageIds = expectedDms[narrow] ??= QueueList();
Expand All @@ -77,8 +79,8 @@ void main() {
expectedMentions.add(message.id);
}
}
for (final perTopic in expectedStreams.values) {
for (final messageIds in perTopic.values) {
for (final streamUnreads in expectedStreams.values) {
for (final messageIds in streamUnreads.topics.values) {
messageIds.sort();
}
}
Expand All @@ -87,7 +89,8 @@ void main() {
}

check(model)
..streams.deepEquals(expectedStreams)
..count.equals(messages.where((m) => !m.flags.contains(MessageFlag.read)).length)
..streams.jsonEquals(expectedStreams)
..dms.deepEquals(expectedDms)
..mentions.unorderedEquals(expectedMentions);
}
Expand Down
2 changes: 2 additions & 0 deletions test/stdlib_checks.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,8 @@ Object? deepToJson(Object? object) {
result = object.map((x) => deepToJson(x)).toList();
case Map() when object.keys.every((k) => k is String):
result = object.map((k, v) => MapEntry<String, dynamic>(k, deepToJson(v)));
case Map() when object.keys.every((k) => k is int):
result = object.map((k, v) => MapEntry<String, dynamic>(k.toString(), deepToJson(v)));
Comment on lines +66 to +67
Copy link
Member

Choose a reason for hiding this comment

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

Hmm interesting.

The idea in the existing code here is that if you have a Map whose keys aren't strings, that is not a valid JSON-like value — not a value that a toJson method should be emitting. Keys in actual JSON objects are strings (just like in JS objects).

E.g. see the deepToJson doc:

/// All JSON atoms (numbers, booleans, null, and strings) are used directly.
/// All JSON containers (lists, and maps with string keys) are copied
/// as their elements are converted recursively.
/// For any other value, a dynamic call `.toJson()` is made and
/// should return either a JSON atom or a JSON container.
Object? deepToJson(Object? object) {

Does jsonEncode accept a value like this? If it does, we should probably accommodate it here too.

If not, but jsonEncode gives a more helpful error message about this, then it'd be good to try to give an equally helpful error.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In DartPad, this fails:

import 'dart:convert';

main() {
  print(jsonEncode({ 1: 123 }));
}

with

Uncaught Error: Converting object to an encodable object failed: Instance of 'JsLinkedHashMap<int, int>'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Compare that with the output of this test on main (256be9a):

    test('', () {
      check({1: 1}).jsonEquals({1: 1});
    });
$ flutter test test/model/unreads_test.dart 
00:02 +0 -1: constructor  [E]                                                      
  Converting object to an encodable object failed: _Map len:1
  test/stdlib_checks.dart 49:5        deepToJson
  test/stdlib_checks.dart 79:26       JsonChecks.jsonEquals
  test/model/unreads_test.dart 98:21  main.<fn>.<fn>

Copy link
Collaborator Author

@chrisbobbe chrisbobbe Oct 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

That's a JsonUnsupportedObjectError that we throw in deepToJson:

  final Object? shallowlyConverted;
  try {
    shallowlyConverted = (object as dynamic).toJson();
  } catch (e) {
    throw JsonUnsupportedObjectError(object, cause: e);
  }

Inspecting its cause with print(e) gives:

NoSuchMethodError: Class '_Map<int, int>' has no instance method 'toJson'.
Receiver: _Map len:1
Tried calling: toJson()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe it would help to make that cause appear in the test failure output? If the test writer already knows not to expect deepToJson to convert an int-keyed map to a string-keyed map (as JS's JSON.stringify does), then that output is likely to be a helpful clue, because it points to an int-keyed map.

Copy link
Member

Choose a reason for hiding this comment

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

It looks like we give the same error as jsonEncode does here. It's different in DartPad vs. flutter test because the former is running Dart by compiling to JS; but in flutter test, the jsonEncode version gives the same "_Map len:1" stringification.

Which isn't a coincidence, because as the implementation comment says:

Object? deepToJson(Object? object) {
  // Implementation is based on the recursion underlying [jsonEncode],
  // at [_JsonStringifier.writeObject] in the stdlib's convert/json.dart .
  // (We leave out the cycle-checking, for simplicity / out of laziness.)

and that includes largely throwing the same exceptions.

I'm inclined to not try to optimize the error messages beyond what jsonEncode gives. For example it's not clear that this would be helpful:

If the test writer already knows not to expect deepToJson to convert an int-keyed map to a string-keyed map (as JS's JSON.stringify does), then that output is likely to be a helpful clue,

because that doesn't seem to have been the scenario.

default:
return (null, false);
}
Expand Down