Skip to content

model: Introduce data structures for "recent senders criterion" of user-mention autocomplete #692

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.

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

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions lib/model/message_list.dart
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,7 @@ class MessageListView with ChangeNotifier, _MessageSequence {
numAfter: 0,
);
store.reconcileMessages(result.messages);
store.recentSenders.handleMessages(result.messages); // TODO(#824)
for (final message in result.messages) {
if (_messageVisible(message)) {
_addMessage(message);
Expand Down Expand Up @@ -439,6 +440,7 @@ class MessageListView with ChangeNotifier, _MessageSequence {
}

store.reconcileMessages(result.messages);
store.recentSenders.handleMessages(result.messages); // TODO(#824)

final fetchedMessages = _allMessagesVisible
? result.messages // Avoid unnecessarily copying the list.
Expand Down
149 changes: 149 additions & 0 deletions lib/model/recent_senders.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,149 @@
import 'package:collection/collection.dart';
Copy link
Member

Choose a reason for hiding this comment

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

nit in commit message:

Much of this code is transcribed from Zulip web; in particular,
from:
  web\src\recent_senders.ts

Use forward slashes to separate path components; that's the Internet standard (e.g. in URLs).

Also helpful is to give a GitHub permalink to the file. (Once you're looking at a GitHub page like …/blob/main/web/src/recent_senders.ts, you can hit the y keyboard shortcut to turn it into a permalink.) That encodes the commit ID, so that a reader in the future can quickly and unambiguously find the file you were talking about even if in the repo it's been renamed since then, or its contents substantially changed.

import 'package:flutter/foundation.dart';

import '../api/model/events.dart';
import '../api/model/model.dart';
import 'algorithms.dart';

/// Tracks the latest messages sent by each user, in each stream and topic.
///
/// Use [latestMessageIdOfSenderInStream] and [latestMessageIdOfSenderInTopic]
/// for queries.
class RecentSenders {
// streamSenders[streamId][senderId] = MessageIdTracker
@visibleForTesting
final Map<int, Map<int, MessageIdTracker>> streamSenders = {};

// topicSenders[streamId][topic][senderId] = MessageIdTracker
@visibleForTesting
final Map<int, Map<String, Map<int, MessageIdTracker>>> topicSenders = {};

/// The latest message the given user sent to the given stream,
/// or null if no such message is known.
int? latestMessageIdOfSenderInStream({
required int streamId,
required int senderId,
}) => streamSenders[streamId]?[senderId]?.maxId;

/// The latest message the given user sent to the given topic,
/// or null if no such message is known.
int? latestMessageIdOfSenderInTopic({
required int streamId,
required String topic,
required int senderId,
}) => topicSenders[streamId]?[topic]?[senderId]?.maxId;

/// Records the necessary data from a batch of just-fetched messages.
///
/// The messages must be sorted by [Message.id] ascending.
void handleMessages(List<Message> messages) {
final messagesByUserInStream = <(int, int), QueueList<int>>{};
final messagesByUserInTopic = <(int, String, int), QueueList<int>>{};
for (final message in messages) {
if (message is! StreamMessage) continue;
final StreamMessage(:streamId, :topic, :senderId, id: int messageId) = message;
(messagesByUserInStream[(streamId, senderId)] ??= QueueList()).add(messageId);
(messagesByUserInTopic[(streamId, topic, senderId)] ??= QueueList()).add(messageId);
}

for (final entry in messagesByUserInStream.entries) {
final (streamId, senderId) = entry.key;
((streamSenders[streamId] ??= {})
[senderId] ??= MessageIdTracker()).addAll(entry.value);
}
for (final entry in messagesByUserInTopic.entries) {
final (streamId, topic, senderId) = entry.key;
(((topicSenders[streamId] ??= {})[topic] ??= {})
[senderId] ??= MessageIdTracker()).addAll(entry.value);
}
}

/// Records the necessary data from a new message.
void handleMessage(Message message) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
void handleMessage(Message message) {
void handleMessageEvent(MessageEvent event) {
final message = event.message;

Matches RecentDmConversationsView.handleMessageEvent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This change would match it with other somewhat similar parts of the codebase, but there are places this method is called where there is no MessageEvent available, such as in model/message_list.dart file. In contrast, RecentDmConversationsView.handleMessageEvent is only called in PerAccountStore.handleEvent where the MessageEvent is available.

Copy link
Member

Choose a reason for hiding this comment

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

That makes sense.

if (message is! StreamMessage) return;
final StreamMessage(:streamId, :topic, :senderId, id: int messageId) = message;
((streamSenders[streamId] ??= {})
[senderId] ??= MessageIdTracker()).add(messageId);
(((topicSenders[streamId] ??= {})[topic] ??= {})
[senderId] ??= MessageIdTracker()).add(messageId);
}

void handleDeleteMessageEvent(DeleteMessageEvent event, Map<int, Message> cachedMessages) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Zulip Web seems not to delete the recently deleted message id from the tracker list, but I am unsure whether to follow them exactly!

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, good question. Let's go ahead and do it, since we have all the infrastructure handy for it and it makes the behavior more correct.

Similar to the handleMessages case, though, let's do it with just one method call to each affected MessageIdTracker per handleDeleteMessageEvent call. After you do that for handleMessages, I think most of the code can be copied to use for this.

if (event.messageType != MessageType.stream) return;
Copy link
Member

Choose a reason for hiding this comment

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

So as this field on DeleteMessageEvent suggests, the set of messages in the event can't be totally arbitary — they're either all stream/channel messages or all DMs.

Then in fact there are more fields on the event and they specify things more precisely: if it's stream messages, they all have the same stream and topic, and those are given on the event too.

So that can be used to simplify this function quite a bit.

Copy link
Member

Choose a reason for hiding this comment

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

Also started an #api documentation thread to confirm and clarify the fact I stated above:
https://chat.zulip.org/#narrow/stream/19-documentation/topic/delete_message.20events/near/1895069


final messagesByUser = <int, List<int>>{};
for (final id in event.messageIds) {
final message = cachedMessages[id] as StreamMessage?;
if (message == null) continue;
(messagesByUser[message.senderId] ??= []).add(id);
}

final DeleteMessageEvent(:streamId!, :topic!) = event;
final sendersInStream = streamSenders[streamId];
final topicsInStream = topicSenders[streamId];
final sendersInTopic = topicsInStream?[topic];
for (final entry in messagesByUser.entries) {
final MapEntry(key: senderId, value: messages) = entry;

final streamTracker = sendersInStream?[senderId];
streamTracker?.removeAll(messages);
if (streamTracker?.maxId == null) sendersInStream?.remove(senderId);

final topicTracker = sendersInTopic?[senderId];
topicTracker?.removeAll(messages);
if (topicTracker?.maxId == null) sendersInTopic?.remove(senderId);
}
if (sendersInStream?.isEmpty ?? false) streamSenders.remove(streamId);
if (sendersInTopic?.isEmpty ?? false) topicsInStream?.remove(topic);
if (topicsInStream?.isEmpty ?? false) topicSenders.remove(streamId);
}
}

@visibleForTesting
class MessageIdTracker {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Making this class private would cause the following lint message, thus failing the CI:

Invalid use of a private type in a public API.
Try making the private type public, or making the API that uses the private type also be private.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, because the type appears in RecentSenders.streamSenders and RecentSenders.topicSenders.

Still we can keep a @visibleForTesting annotation on this, right? It looks like that went away in the latest revision.

/// A list of distinct message IDs, sorted ascending.
@visibleForTesting
QueueList<int> ids = QueueList();

/// The maximum id in the tracker list, or `null` if the list is empty.
int? get maxId => ids.lastOrNull;

/// Add the message ID to the tracker list at the proper place, if not present.
///
/// Optimized, taking O(1) time for the case where that place is the end,
/// because that's the common case for a message that is received through
/// [PerAccountStore.handleEvent]. May take O(n) time in some rare cases.
void add(int id) {
if (ids.isEmpty || id > ids.last) {
ids.addLast(id);
return;
}
final i = lowerBound(ids, id);
if (i < ids.length && ids[i] == id) {
// The ID is already present. Nothing to do.
return;
}
ids.insert(i, id);
}

/// Add the messages IDs to the tracker list at the proper place, if not present.
///
/// [newIds] should be sorted ascending.
void addAll(QueueList<int> newIds) {
if (ids.isEmpty) {
ids = newIds;
return;
}
ids = setUnion(ids, newIds);
}

void removeAll(List<int> idsToRemove) {
ids.removeWhere((id) {
final i = lowerBound(idsToRemove, id);
return i < idsToRemove.length && idsToRemove[i] == id;
});
}

@override
String toString() => ids.toString();
}
11 changes: 11 additions & 0 deletions lib/model/store.dart
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import 'database.dart';
import 'message.dart';
import 'message_list.dart';
import 'recent_dm_conversations.dart';
import 'recent_senders.dart';
import 'channel.dart';
import 'typing_status.dart';
import 'unreads.dart';
Expand Down Expand Up @@ -256,6 +257,7 @@ class PerAccountStore extends ChangeNotifier with ChannelStore, MessageStore {
),
recentDmConversationsView: RecentDmConversationsView(
initial: initialSnapshot.recentPrivateConversations, selfUserId: account.userId),
recentSenders: RecentSenders(),
);
}

Expand All @@ -276,6 +278,7 @@ class PerAccountStore extends ChangeNotifier with ChannelStore, MessageStore {
required MessageStoreImpl messages,
required this.unreads,
required this.recentDmConversationsView,
required this.recentSenders,
}) : assert(selfUserId == globalStore.getAccount(accountId)!.userId),
assert(realmUrl == globalStore.getAccount(accountId)!.realmUrl),
assert(realmUrl == connection.realmUrl),
Expand Down Expand Up @@ -369,6 +372,8 @@ class PerAccountStore extends ChangeNotifier with ChannelStore, MessageStore {

final RecentDmConversationsView recentDmConversationsView;

final RecentSenders recentSenders;

////////////////////////////////
// Other digests of data.

Expand Down Expand Up @@ -492,6 +497,7 @@ class PerAccountStore extends ChangeNotifier with ChannelStore, MessageStore {
_messages.handleMessageEvent(event);
unreads.handleMessageEvent(event);
recentDmConversationsView.handleMessageEvent(event);
recentSenders.handleMessage(event.message); // TODO(#824)
// When adding anything here (to handle [MessageEvent]),
// it probably belongs in [reconcileMessages] too.

Expand All @@ -502,6 +508,11 @@ class PerAccountStore extends ChangeNotifier with ChannelStore, MessageStore {

case DeleteMessageEvent():
assert(debugLog("server event: delete_message ${event.messageIds}"));
// This should be called before [_messages.handleDeleteMessageEvent(event)],
// as we need to know about each message for [event.messageIds],
// specifically, their `senderId`s. By calling this after the
// aforementioned line, we'll lose reference to those messages.
recentSenders.handleDeleteMessageEvent(event, messages);
_messages.handleDeleteMessageEvent(event);
unreads.handleDeleteMessageEvent(event);

Expand Down
1 change: 1 addition & 0 deletions test/model/autocomplete_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,7 @@ void main() {
check(done).isFalse();
for (int i = 0; i < 3; i++) {
await Future(() {});
if (done) break;
}
check(done).isTrue();
final results = view.results
Expand Down
41 changes: 41 additions & 0 deletions test/model/message_list_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import '../api/model/model_checks.dart';
import '../example_data.dart' as eg;
import '../stdlib_checks.dart';
import 'content_checks.dart';
import 'recent_senders_test.dart' as recent_senders_test;
import 'test_store.dart';

void main() {
Expand Down Expand Up @@ -141,6 +142,25 @@ void main() {
..haveOldest.isTrue();
});

// TODO(#824): move this test
test('fetchInitial, recent senders track all the messages', () async {
Copy link
Member

Choose a reason for hiding this comment

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

The need for importing from another test file is a sign that the layers here are getting a little tangled. But I think that's OK for merging this PR — it's really the same thing that #824 is about. So let's just add a todo-comment, to help find this to clean it up when we take care of that issue:

Suggested change
test('fetchInitial, recent senders track all the messages', () async {
test('fetchInitial, recent senders track all the messages', () async { // TODO(#824) move this test

Similarly the fetchOlder test below.

const narrow = CombinedFeedNarrow();
await prepare(narrow: narrow);
final messages = [
eg.streamMessage(),
// Not subscribed to the stream with id 10.
eg.streamMessage(stream: eg.stream(streamId: 10)),
];
connection.prepare(json: newestResult(
foundOldest: false,
messages: messages,
).toJson());
await model.fetchInitial();

check(model).messages.length.equals(1);
recent_senders_test.checkMatchesMessages(store.recentSenders, messages);
});

test('fetchOlder', () async {
const narrow = CombinedFeedNarrow();
await prepare(narrow: narrow);
Expand Down Expand Up @@ -233,6 +253,27 @@ void main() {
..messages.length.equals(200);
});

// TODO(#824): move this test
test('fetchOlder, recent senders track all the messages', () async {
const narrow = CombinedFeedNarrow();
await prepare(narrow: narrow);
final initialMessages = List.generate(10, (i) => eg.streamMessage(id: 100 + i));
await prepareMessages(foundOldest: false, messages: initialMessages);

final oldMessages = List.generate(10, (i) => eg.streamMessage(id: 89 + i))
// Not subscribed to the stream with id 10.
..add(eg.streamMessage(id: 99, stream: eg.stream(streamId: 10)));
connection.prepare(json: olderResult(
anchor: 100, foundOldest: false,
messages: oldMessages,
).toJson());
await model.fetchOlder();

check(model).messages.length.equals(20);
recent_senders_test.checkMatchesMessages(store.recentSenders,
[...initialMessages, ...oldMessages]);
});

test('MessageEvent', () async {
final stream = eg.stream();
await prepare(narrow: StreamNarrow(stream.streamId));
Expand Down
Loading