Skip to content

store: Add RecentDmConversationsView view-model #210

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
merged 8 commits into from
Jul 5, 2023
23 changes: 23 additions & 0 deletions lib/api/model/initial_snapshot.dart
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ class InitialSnapshot {

// TODO etc., etc.

final List<RecentDmConversation> recentPrivateConversations;

final List<Subscription> subscriptions;

final List<ZulipStream> streams;
Expand Down Expand Up @@ -66,6 +68,7 @@ class InitialSnapshot {
this.zulipMergeBase,
required this.alertWords,
required this.customProfileFields,
required this.recentPrivateConversations,
required this.subscriptions,
required this.streams,
required this.maxFileUploadSizeMib,
Expand All @@ -79,3 +82,23 @@ class InitialSnapshot {

Map<String, dynamic> toJson() => _$InitialSnapshotToJson(this);
}

/// An item in `recent_private_conversations`.
///
/// For docs, search for "recent_private_conversations:"
/// in <https://zulip.com/api/register-queue>.
@JsonSerializable(fieldRename: FieldRename.snake)
class RecentDmConversation {
final int maxMessageId;
final List<int> userIds;

RecentDmConversation({
required this.maxMessageId,
required this.userIds,
});

factory RecentDmConversation.fromJson(Map<String, dynamic> json) =>
_$RecentDmConversationFromJson(json);

Map<String, dynamic> toJson() => _$RecentDmConversationToJson(this);
}
20 changes: 20 additions & 0 deletions lib/api/model/initial_snapshot.g.dart

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

18 changes: 18 additions & 0 deletions lib/model/narrow.dart
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@

import '../api/model/initial_snapshot.dart';
import '../api/model/model.dart';
import '../api/model/narrow.dart';
import '../api/route/messages.dart';
Expand Down Expand Up @@ -70,6 +71,9 @@ class StreamNarrow extends Narrow {
@override
ApiNarrow apiEncode() => [ApiNarrowStream(streamId)];

@override
String toString() => 'StreamNarrow($streamId)';

@override
bool operator ==(Object other) {
if (other is! StreamNarrow) return false;
Expand Down Expand Up @@ -102,6 +106,9 @@ class TopicNarrow extends Narrow implements SendableNarrow {
@override
StreamDestination get destination => StreamDestination(streamId, topic);

@override
String toString() => 'TopicNarrow($streamId, $topic)';

@override
bool operator ==(Object other) {
if (other is! TopicNarrow) return false;
Expand Down Expand Up @@ -146,6 +153,14 @@ class DmNarrow extends Narrow implements SendableNarrow {
);
}

/// A [DmNarrow] from an item in [InitialSnapshot.recentPrivateConversations].
factory DmNarrow.ofRecentDmConversation(RecentDmConversation conversation, {required int selfUserId}) {
return DmNarrow(
allRecipientIds: [...conversation.userIds, selfUserId]..sort(),
selfUserId: selfUserId,
);
}

/// The user IDs of everyone in the conversation, sorted.
///
/// Each message in the conversation is sent by one of these users
Expand Down Expand Up @@ -207,6 +222,9 @@ class DmNarrow extends Narrow implements SendableNarrow {
@override
ApiNarrow apiEncode() => [ApiNarrowDm(allRecipientIds)];

@override
String toString() => 'DmNarrow(allRecipientIds: $allRecipientIds)';

@override
bool operator ==(Object other) {
if (other is! DmNarrow) return false;
Expand Down
126 changes: 126 additions & 0 deletions lib/model/recent_dm_conversations.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
import 'package:collection/collection.dart';
import 'package:flutter/foundation.dart';

import '../api/model/initial_snapshot.dart';
import '../api/model/model.dart';
import '../api/model/events.dart';
import 'narrow.dart';

/// A view-model for the recent-DM-conversations UI.
///
/// This maintains the list of recent DM conversations,
/// plus additional data in order to efficiently maintain the list.
class RecentDmConversationsView extends ChangeNotifier {
factory RecentDmConversationsView({
required List<RecentDmConversation> initial,
required selfUserId,
}) {
final entries = initial.map((conversation) => MapEntry(
DmNarrow.ofRecentDmConversation(conversation, selfUserId: selfUserId),
conversation.maxMessageId,
)).toList()..sort((a, b) => -a.value.compareTo(b.value));
return RecentDmConversationsView._(
map: Map.fromEntries(entries),
sorted: QueueList.from(entries.map((e) => e.key)),
selfUserId: selfUserId,
);
}

RecentDmConversationsView._({
required this.map,
required this.sorted,
required this.selfUserId,
});

/// The latest message ID in each conversation.
final Map<DmNarrow, int> map;

/// The [DmNarrow] keys of the map, sorted by latest message descending.
final QueueList<DmNarrow> sorted;

final int selfUserId;

/// Insert the key at the proper place in [sorted].
///
/// Optimized, taking O(1) time, for the case where that place is the start,
/// because that's the common case for a new message.
/// May take O(n) time in general.
void _insertSorted(DmNarrow key, int msgId) {
Copy link
Member

Choose a reason for hiding this comment

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

I think the comment on the corresponding function in zulip-mobile is useful to propagate:

Suggested change
void _insertSorted(DmNarrow key, int msgId) {
/// Insert the key at the proper place in [sorted].
///
/// Optimized, taking O(1) time, for the case where that place is the start,
/// because that's the common case for a new message.
/// May take O(n) time in general.
void _insertSorted(DmNarrow key, int msgId) {

In particular the point about when it's O(1) and why.

final i = sorted.indexWhere((k) => map[k]! < msgId);
// QueueList is a deque, with O(1) to add at start or end.
switch (i) {
case == 0:
sorted.addFirst(key);
case < 0:
sorted.addLast(key);
default:
sorted.insert(i, key);
}
}

/// Handle [MessageEvent], updating [map] and [sorted].
///
/// Can take linear time in general. That sounds inefficient...
/// but it's what the webapp does, so must not be catastrophic. 🤷
/// (In fact the webapp calls `Array#sort`,
/// which takes at *least* linear time, and may be 𝛳(N log N).)
///
/// The point of the event is that we're learning about the message
/// in real time immediately after it was sent --
/// so the overwhelmingly common case is that the message
/// is newer than any existing message we know about. (*)
Copy link
Member

Choose a reason for hiding this comment

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

dangling asterisk

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I'm guessing the reason you've omitted the footnote is because it's about a race with FETCH_MESSAGES_COMPLETE, and we don't yet have an analogue of that in zulip-flutter; but the same reason means that the case where this is the newest message is also not just the overwhelmingly common case, but the only possible case.

I think the logic is probably actually cleanest to read and understand if that caveat is left in and the wording just adjusted to reflect that it's not yet present. I'll suggest a version.

/// That's therefore the case we optimize for,
/// particularly in the helper _insertSorted.
///
/// (*) In fact at present that's the only possible case.
/// The alternative will become possible when we have the analogue of
/// zulip-mobile's FETCH_MESSAGES_COMPLETE, with fetches done for a
/// [MessageListView] reporting their messages back via the [PerAccountStore]
/// to all our central data structures.
/// Then that can race with a new-message event: for example,
/// say we get a fetch-messages result that includes the just-sent
/// message 1002, and only after that get the event about message 1001,
/// sent moments earlier. The event queue always delivers events in order, so
/// even the race is possible only because we fetch messages outside of the
/// event queue.
void handleMessageEvent(MessageEvent event) {
final message = event.message;
if (message is! DmMessage) {
return;
}
final key = DmNarrow.ofMessage(message, selfUserId: selfUserId);
final prev = map[key];
if (prev == null) {
// The conversation is new. Add to both `map` and `sorted`.
map[key] = message.id;
_insertSorted(key, message.id);
} else if (prev >= message.id) {
// The conversation already has a newer message.
// This should be impossible as long as we only listen for messages coming
// through the event system, which sends events in order.
// Anyway, do nothing.
} else {
// The conversation needs to be (a) updated in `map`...
map[key] = message.id;

// ... and (b) possibly moved around in `sorted` to keep the list sorted.
final i = sorted.indexOf(key);
assert(i >= 0, 'key in map should be in sorted');
if (i == 0) {
// The conversation was already the latest, so no reordering needed.
// (This is likely a common case in practice -- happens every time
// the user gets several DMs in a row in the same thread -- so good to
// optimize.)
} else {
// It wasn't the latest. Just handle the general case.
sorted.removeAt(i); // linear time, ouch
_insertSorted(key, message.id);
}
}
notifyListeners();
}

// TODO update from messages loaded in message lists. When doing so,
// review handleMessageEvent so it acknowledges the subtle races that can
// happen when taking data from outside the event system.
}
17 changes: 15 additions & 2 deletions lib/model/store.dart
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import '../log.dart';
import 'autocomplete.dart';
import 'database.dart';
import 'message_list.dart';
import 'recent_dm_conversations.dart';

export 'package:drift/drift.dart' show Value;
export 'database.dart' show Account, AccountsCompanion;
Expand Down Expand Up @@ -149,6 +150,7 @@ class PerAccountStore extends ChangeNotifier {
required this.connection,
required InitialSnapshot initialSnapshot,
}) : zulipVersion = initialSnapshot.zulipVersion,
maxFileUploadSizeMib = initialSnapshot.maxFileUploadSizeMib,
users = Map.fromEntries(
initialSnapshot.realmUsers
.followedBy(initialSnapshot.realmNonActiveUsers)
Expand All @@ -158,20 +160,30 @@ class PerAccountStore extends ChangeNotifier {
(stream) => MapEntry(stream.streamId, stream))),
subscriptions = Map.fromEntries(initialSnapshot.subscriptions.map(
(subscription) => MapEntry(subscription.streamId, subscription))),
maxFileUploadSizeMib = initialSnapshot.maxFileUploadSizeMib;
recentDmConversationsView = RecentDmConversationsView(
initial: initialSnapshot.recentPrivateConversations, selfUserId: account.userId);

final Account account;
final ApiConnection connection; // TODO(#135): update zulipFeatureLevel with events

// TODO(#135): Keep all this data updated by handling Zulip events from the server.

// Data attached to the realm or the server.
final String zulipVersion; // TODO get from account; update there on initial snapshot
final int maxFileUploadSizeMib; // No event for this.

// Users and data about them.
final Map<int, User> users;

// Streams, topics, and stuff about them.
final Map<int, ZulipStream> streams;
final Map<int, Subscription> subscriptions;
final int maxFileUploadSizeMib; // No event for this.

// TODO lots more data. When adding, be sure to update handleEvent too.

// TODO call [RecentDmConversationsView.dispose] in [dispose]
final RecentDmConversationsView recentDmConversationsView;

final Set<MessageListView> _messageListViews = {};

void registerMessageList(MessageListView view) {
Expand Down Expand Up @@ -254,6 +266,7 @@ class PerAccountStore extends ChangeNotifier {
notifyListeners();
} else if (event is MessageEvent) {
assert(debugLog("server event: message ${jsonEncode(event.message.toJson())}"));
recentDmConversationsView.handleMessageEvent(event);
for (final view in _messageListViews) {
view.maybeAddMessage(event.message);
}
Expand Down
2 changes: 1 addition & 1 deletion pubspec.lock
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ packages:
source: hosted
version: "4.5.0"
collection:
dependency: transitive
dependency: "direct main"
description:
name: collection
sha256: f092b211a4319e98e5ff58223576de6c2803db36221657b46c82574721240687
Expand Down
1 change: 1 addition & 0 deletions pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ dependencies:
app_settings: ^4.2.0
image_picker: ^0.8.7+3
package_info_plus: ^4.0.1
collection: ^1.17.2

dev_dependencies:
flutter_test:
Expand Down
Loading