Skip to content

Add DM narrows; clean up DM recipients some more in API #160

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 7 commits into from
Jun 8, 2023
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
48 changes: 48 additions & 0 deletions lib/api/model/model.dart
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,32 @@ class DmRecipient {
_$DmRecipientFromJson(json);

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

@override
String toString() => 'DmRecipient(id: $id, email: $email, fullName: $fullName)';

@override
bool operator ==(Object other) {
if (other is! DmRecipient) return false;
return other.id == id && other.email == email && other.fullName == fullName;
}

@override
int get hashCode => Object.hash('DmRecipient', id, email, fullName);
}

class DmRecipientListConverter extends JsonConverter<List<DmRecipient>, List<dynamic>> {
const DmRecipientListConverter();

@override
List<DmRecipient> fromJson(List json) {
return json.map((e) => DmRecipient.fromJson(e as Map<String, dynamic>))
.toList(growable: false)
..sort((a, b) => a.id.compareTo(b.id));
}

@override
List toJson(List<DmRecipient> object) => object;
}

@JsonSerializable(fieldRename: FieldRename.snake)
Expand All @@ -366,8 +392,30 @@ class DmMessage extends Message {
@JsonKey(includeToJson: true)
String get type => 'private';

/// The `display_recipient` from the server, sorted by user ID numerically.
///
/// This lists the sender as well as all (other) recipients, and it
/// lists each user just once. In particular the self-user is always
/// included.
///
/// Note the data here is not updated on changes to the users, so everything
/// other than the user IDs may be stale.
/// Consider using [allRecipientIds] instead, and getting user details
/// from the store.
// TODO(server): Document that it's all users. That statement is based on
// reverse-engineering notes in zulip-mobile:src/api/modelTypes.js at PmMessage.
@DmRecipientListConverter()
final List<DmRecipient> displayRecipient;

/// The user IDs of all users in the thread, sorted numerically.
///
/// This lists the sender as well as all (other) recipients, and it
/// lists each user just once. In particular the self-user is always
/// included.
///
/// This is a result of [List.map], so it has an efficient `length`.
Iterable<int> get allRecipientIds => displayRecipient.map((e) => e.id);

DmMessage({
super.avatarUrl,
required super.client,
Expand Down
8 changes: 4 additions & 4 deletions lib/api/model/model.g.dart

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

98 changes: 97 additions & 1 deletion lib/model/narrow.dart
Original file line number Diff line number Diff line change
Expand Up @@ -90,4 +90,100 @@ class TopicNarrow extends Narrow {
int get hashCode => Object.hash('TopicNarrow', streamId, topic);
}

// TODO other narrow types: DMs; starred, mentioned; searches; arbitrary
bool _isSortedWithoutDuplicates(List<int> items) {
final length = items.length;
if (length == 0) {
return true;
}
int lastItem = items[0];
for (int i = 1; i < length; i++) {
final item = items[i];
if (item <= lastItem) {
return false;
}
lastItem = item;
}
return true;
}

/// The narrow for a direct-message conversation.
// Zulip has many ways of representing a DM conversation; for example code
// handling many of them, see zulip-mobile:src/utils/recipient.js .
// Please add more constructors and getters here to handle any of those
// as we turn out to need them.
class DmNarrow extends Narrow {
DmNarrow({required this.allRecipientIds, required int selfUserId})
: assert(_isSortedWithoutDuplicates(allRecipientIds)),
assert(allRecipientIds.contains(selfUserId)),
_selfUserId = selfUserId;

/// The user IDs of everyone in the conversation, sorted.
///
/// Each message in the conversation is sent by one of these users
/// and received by all the other users.
///
/// The self-user is always a member of this list.
/// It has one element for the self-1:1 thread,
/// two elements for other 1:1 threads,
/// and three or more elements for a group DM thread.
///
/// See also:
/// * [otherRecipientIds], an alternate way of identifying the conversation.
/// * [DmMessage.allRecipientIds], which provides this same format.
final List<int> allRecipientIds;

/// The user ID of the self-user.
///
/// The [DmNarrow] implementation needs this information
/// for converting between different forms of referring to the narrow,
/// such as [allRecipientIds] vs. [otherRecipientIds].
final int _selfUserId;

/// The user IDs of everyone in the conversation except self, sorted.
///
/// This is empty for the self-1:1 thread,
/// has one element for other 1:1 threads,
/// and has two or more elements for a group DM thread.
///
/// See also:
/// * [allRecipientIds], an alternate way of identifying the conversation.
late final List<int> otherRecipientIds = allRecipientIds
.where((userId) => userId != _selfUserId)
.toList(growable: false);

/// A string that uniquely identifies the DM conversation (within the account).
late final String _key = otherRecipientIds.join(',');

@override
bool containsMessage(Message message) {
if (message is! DmMessage) return false;
if (message.allRecipientIds.length != allRecipientIds.length) return false;
int i = 0;
for (final userId in message.allRecipientIds) {
if (userId != allRecipientIds[i]) return false;
i++;
}
return true;
}

// Not [otherRecipientIds], because for the self-1:1 thread that triggers
// a server bug as of Zulip Server 7 (2023-05): an empty list here
// causes a 5xx response from the server.
@override
ApiNarrow apiEncode() => [ApiNarrowDm(allRecipientIds)];

@override
bool operator ==(Object other) {
if (other is! DmNarrow) return false;
assert(other._selfUserId == _selfUserId,
'Two [Narrow]s belonging to different accounts were compared with `==`. '
'This is a bug, because a [Narrow] does not contain information to '
'reliably detect such a comparison, so it may produce false positives.');
return other._key == _key;
}

@override
int get hashCode => Object.hash('DmNarrow', _key);
}

// TODO other narrow types: starred, mentioned; searches; arbitrary
2 changes: 2 additions & 0 deletions lib/widgets/compose_box.dart
Original file line number Diff line number Diff line change
Expand Up @@ -695,6 +695,8 @@ class ComposeBox extends StatelessWidget {
return StreamComposeBox(narrow: narrow, streamId: narrow.streamId);
} else if (narrow is TopicNarrow) {
return const SizedBox.shrink(); // TODO(#144): add a single-topic compose box
} else if (narrow is DmNarrow) {
return const SizedBox.shrink(); // TODO(#144): add a DM compose box
} else if (narrow is AllMessagesNarrow) {
return const SizedBox.shrink();
} else {
Expand Down
31 changes: 25 additions & 6 deletions lib/widgets/message_list.dart
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,15 @@ class MessageListAppBarTitle extends StatelessWidget {
final store = PerAccountStoreWidget.of(context);
final streamName = store.streams[streamId]?.name ?? '(unknown stream)';
return Text("#$streamName > $topic"); // TODO show stream privacy icon; format on two lines

case DmNarrow(:var otherRecipientIds):
final store = PerAccountStoreWidget.of(context);
if (otherRecipientIds.isEmpty) {
return const Text("DMs with yourself");
} else {
final names = otherRecipientIds.map((id) => store.users[id]?.fullName ?? '(unknown user)');
return Text("DMs with ${names.join(", ")}"); // TODO show avatars
}
}
}
}
Expand Down Expand Up @@ -306,12 +315,22 @@ class DmRecipientHeader extends StatelessWidget {

@override
Widget build(BuildContext context) {
return Align(
alignment: Alignment.centerLeft,
child: RecipientHeaderChevronContainer(
color: _kDmRecipientHeaderColor,
child: const Text("Direct message", // TODO DM recipient headers
style: TextStyle(color: Colors.white))));
return GestureDetector(
behavior: HitTestBehavior.translucent,
onTap: () {
final store = PerAccountStoreWidget.of(context);
final narrow = DmNarrow(
allRecipientIds: message.allRecipientIds.toList(growable: false),
selfUserId: store.account.userId);
Navigator.push(context,
MessageListPage.buildRoute(context: context, narrow: narrow));
},
child: Align(
alignment: Alignment.centerLeft,
child: RecipientHeaderChevronContainer(
color: _kDmRecipientHeaderColor,
child: const Text("Direct message", // TODO DM recipient headers
style: TextStyle(color: Colors.white)))));
}
}

Expand Down
66 changes: 65 additions & 1 deletion test/api/model/model_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ import 'package:checks/checks.dart';
import 'package:test/scaffolding.dart';
import 'package:zulip/api/model/model.dart';

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

void main() {
group('User', () {
final Map<String, dynamic> baseJson = Map.unmodifiable({
Expand All @@ -22,7 +24,7 @@ void main() {
'profile_data': <String, dynamic>{},
});

User mkUser (Map<String, dynamic> specialJson) {
User mkUser(Map<String, dynamic> specialJson) {
return User.fromJson({ ...baseJson, ...specialJson });
}

Expand All @@ -44,4 +46,66 @@ void main() {
check(mkUser({'is_system_bot': true}).isSystemBot).equals(true);
});
});

group('DmMessage', () {
final Map<String, dynamic> baseJson = Map.unmodifiable(
eg.dmMessage(from: eg.otherUser, to: [eg.selfUser]).toJson());

DmMessage parse(Map<String, dynamic> specialJson) {
return DmMessage.fromJson({ ...baseJson, ...specialJson });
}

Iterable<DmRecipient> asRecipients(Iterable<User> users) {
return users.map((u) =>
DmRecipient(id: u.userId, email: u.email, fullName: u.fullName));
}

Map<String, dynamic> withRecipients(Iterable<User> recipients) {
final from = recipients.first;
return {
'sender_id': from.userId,
'sender_email': from.email,
'sender_full_name': from.fullName,
'display_recipient': asRecipients(recipients).map((r) => r.toJson()).toList(),
};
}

User user2 = eg.user(userId: 2);
User user3 = eg.user(userId: 3);
User user11 = eg.user(userId: 11);

test('displayRecipient', () {
check(parse(withRecipients([user2])).displayRecipient)
.deepEquals(asRecipients([user2]));

check(parse(withRecipients([user2, user3])).displayRecipient)
.deepEquals(asRecipients([user2, user3]));
check(parse(withRecipients([user3, user2])).displayRecipient)
.deepEquals(asRecipients([user2, user3]));

check(parse(withRecipients([user2, user3, user11])).displayRecipient)
.deepEquals(asRecipients([user2, user3, user11]));
check(parse(withRecipients([user3, user11, user2])).displayRecipient)
.deepEquals(asRecipients([user2, user3, user11]));
check(parse(withRecipients([user11, user2, user3])).displayRecipient)
.deepEquals(asRecipients([user2, user3, user11]));
});

test('allRecipientIds', () {
check(parse(withRecipients([user2])).allRecipientIds)
.deepEquals([2]);

check(parse(withRecipients([user2, user3])).allRecipientIds)
.deepEquals([2, 3]);
check(parse(withRecipients([user3, user2])).allRecipientIds)
.deepEquals([2, 3]);

check(parse(withRecipients([user2, user3, user11])).allRecipientIds)
.deepEquals([2, 3, 11]);
check(parse(withRecipients([user3, user11, user2])).allRecipientIds)
.deepEquals([2, 3, 11]);
check(parse(withRecipients([user11, user2, user3])).allRecipientIds)
.deepEquals([2, 3, 11]);
});
});
}
36 changes: 29 additions & 7 deletions test/example_data.dart
Original file line number Diff line number Diff line change
Expand Up @@ -63,29 +63,28 @@ final _messagePropertiesBase = {
'recipient_id': 32, // obsolescent in API, and ignored
};

// When we have a User object, this can take that as an argument.
Map<String, dynamic> _messagePropertiesFromSender() {
Map<String, dynamic> _messagePropertiesFromSender(User? sender) {
return {
'client': 'ExampleClient',
'sender_email': 'a-person@example',
'sender_full_name': 'A Person',
'sender_id': 12345, // TODO generate example IDs
'sender_email': sender?.email ?? 'a-person@example',
'sender_full_name': sender?.fullName ?? 'A Person',
'sender_id': sender?.userId ?? 12345, // TODO generate example IDs
'sender_realm_str': 'zulip',
};
}

// When we have a Stream object, this can take that as an argument.
// Also it can default explicitly to an example stream.
StreamMessage streamMessage(
{String? streamName, int? streamId}) {
{User? sender, String? streamName, int? streamId}) {
// The use of JSON here is convenient in order to delegate parts of the data
// to helper functions. The main downside is that it loses static typing
// of the properties as we're constructing the data. That's probably OK
// because (a) this is only for tests; (b) the types do get checked
// dynamically in the constructor, so any ill-typing won't propagate further.
return StreamMessage.fromJson({
..._messagePropertiesBase,
..._messagePropertiesFromSender(),
..._messagePropertiesFromSender(sender),
'display_recipient': streamName ?? 'a stream',
'stream_id': streamId ?? 123, // TODO generate example IDs

Expand All @@ -99,6 +98,29 @@ StreamMessage streamMessage(
});
}

/// Construct an example direct message.
///
/// See also:
/// * [streamMessage], to construct an example stream message.
DmMessage dmMessage({required User from, required List<User> to}) {
assert(!to.any((user) => user.userId == from.userId));
return DmMessage.fromJson({
..._messagePropertiesBase,
..._messagePropertiesFromSender(from),
'display_recipient': [from, ...to]
.map((u) => {'id': u.userId, 'email': u.email, 'full_name': u.fullName})
.toList(growable: false),

'content': '<p>This is an example DM.</p>',
'content_type': 'text/html',
'flags': [],
'id': 1234567, // TODO generate example IDs
'subject': '',
'timestamp': 1678139636,
'type': 'private',
});
}

// TODO example data for many more types

final InitialSnapshot initialSnapshot = InitialSnapshot(
Expand Down
Loading