Skip to content

model: Add Reactions data structure, for efficient access in UI #286

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 2 commits into from
Sep 7, 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
52 changes: 16 additions & 36 deletions lib/api/model/model.dart
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
import 'package:json_annotation/json_annotation.dart';

import 'reaction.dart';

export 'reaction.dart';

part 'model.g.dart';

/// As in [InitialSnapshot.customProfileFields].
Expand Down Expand Up @@ -348,7 +352,9 @@ sealed class Message {
bool isMeMessage;
int? lastEditTimestamp;

final List<Reaction> reactions;
@JsonKey(fromJson: _reactionsFromJson, toJson: _reactionsToJson)
Reactions? reactions; // null is equivalent to an empty [Reactions]

final int recipientId;
final String senderEmail;
final String senderFullName;
Expand All @@ -366,6 +372,15 @@ sealed class Message {
final String? matchContent;
final String? matchSubject;

static Reactions? _reactionsFromJson(dynamic json) {
final list = (json as List<dynamic>);
return list.isNotEmpty ? Reactions.fromJson(list) : null;
}

static Object _reactionsToJson(Reactions? value) {
return value ?? [];
Comment on lines +380 to +381
Copy link
Member

Choose a reason for hiding this comment

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

Ah I see, so this is what fixed those test failures (#286 (review)).

}

static List<MessageFlag> _flagsFromJson(dynamic json) {
final list = json as List<dynamic>;
return list.map((raw) => MessageFlag.fromRawString(raw as String)).toList();
Expand Down Expand Up @@ -565,38 +580,3 @@ class DmMessage extends Message {
@override
Map<String, dynamic> toJson() => _$DmMessageToJson(this);
}

/// As in [Message.reactions].
@JsonSerializable(fieldRename: FieldRename.snake)
class Reaction {
final String emojiName;
final String emojiCode;
final ReactionType reactionType;
final int userId;
// final Map<String, dynamic> user; // deprecated; ignore

Reaction({
required this.emojiName,
required this.emojiCode,
required this.reactionType,
required this.userId,
});

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

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

@override
String toString() => 'Reaction(emojiName: $emojiName, emojiCode: $emojiCode, reactionType: $reactionType, userId: $userId)';
}

/// As in [Reaction.reactionType].
@JsonEnum(fieldRename: FieldRename.snake)
enum ReactionType {
unicodeEmoji,
realmEmoji,
zulipExtraEmoji;

String toJson() => _$ReactionTypeEnumMap[this]!;
}
32 changes: 4 additions & 28 deletions lib/api/model/model.g.dart

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

178 changes: 178 additions & 0 deletions lib/api/model/reaction.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,178 @@
import 'dart:collection';

import 'package:collection/collection.dart';
import 'package:json_annotation/json_annotation.dart';

part 'reaction.g.dart';

/// A message's reactions, in a convenient data structure.
class Reactions {
int get total => _total;
int _total;
Comment on lines +10 to +11
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 this doesn't get updated on add/remove.

Relatedly, it should get checked in the tests. 😉

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah indeed.

Should we check total in the message-list event handling tests too? Or would that be redundant with checking it in the Reactions model tests?

Copy link
Member

Choose a reason for hiding this comment

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

Just in the model tests seems fine — as long as the message-list tests check that the messages end up with the right list of reactions, I'm happy to leave to the Reactions model tests the verification that total (as well as the grouping and sorting of reactions in aggregated) is correctly kept in sync with the list of reactions.


/// A list of [ReactionWithVotes] objects.
///
/// There won't be two items with the same
/// [ReactionWithVotes.reactionType] and [ReactionWithVotes.emojiCode].
/// (We don't also key on [ReactionWithVotes.emojiName];
/// see [ReactionWithVotes].)
///
/// Sorted descending by the size of [ReactionWithVotes.userIds],
/// i.e., the number of votes.
late final List<ReactionWithVotes> aggregated;

Reactions._(this.aggregated, this._total);

factory Reactions(List<Reaction> unaggregated) {
final byReaction = LinkedHashMap<Reaction, ReactionWithVotes>(
equals: (a, b) => a.reactionType == b.reactionType && a.emojiCode == b.emojiCode,
hashCode: (r) => Object.hash(r.reactionType, r.emojiCode),
);
for (final reaction in unaggregated) {
final current = byReaction[reaction] ??= ReactionWithVotes.empty(reaction);
current.userIds.add(reaction.userId);
}

return Reactions._(
byReaction.values.sorted(
// Descending by number of votes
(a, b) => -a.userIds.length.compareTo(b.userIds.length),
),
unaggregated.length,
);
}

factory Reactions.fromJson(List<dynamic> json) {
return Reactions(
json.map((r) => Reaction.fromJson(r as Map<String, dynamic>)).toList(),
);
}

List<dynamic> toJson() {
final result = <Reaction>[];
for (final reactionWithVotes in aggregated) {
result.addAll(reactionWithVotes.userIds.map((userId) => Reaction(
reactionType: reactionWithVotes.reactionType,
emojiCode: reactionWithVotes.emojiCode,
emojiName: reactionWithVotes.emojiName,
userId: userId,
)));
}
return result;
}

void add(Reaction reaction) {
final currentIndex = aggregated.indexWhere((r) {
return r.reactionType == reaction.reactionType && r.emojiCode == reaction.emojiCode;
});
if (currentIndex == -1) {
final newItem = ReactionWithVotes.empty(reaction);
newItem.userIds.add(reaction.userId);
aggregated.add(newItem);
} else {
final current = aggregated[currentIndex];
current.userIds.add(reaction.userId);

// Reposition `current` in list to keep it sorted by number of votes
final newIndex = 1 + aggregated.lastIndexWhere(
(item) => item.userIds.length >= current.userIds.length,
currentIndex - 1,
);
if (newIndex < currentIndex) {
aggregated
..setRange(newIndex + 1, currentIndex + 1, aggregated, newIndex)
..[newIndex] = current;
}
}
_total++;
}

void remove({
required ReactionType reactionType,
required String emojiCode,
required int userId,
}) {
final currentIndex = aggregated.indexWhere((r) {
return r.reactionType == reactionType && r.emojiCode == emojiCode;
});
if (currentIndex == -1) { // TODO(log)
return;
}
final current = aggregated[currentIndex];
current.userIds.remove(userId);
if (current.userIds.isEmpty) {
aggregated.removeAt(currentIndex);
} else {
final lteIndex = aggregated.indexWhere(
(item) => item.userIds.length <= current.userIds.length,
currentIndex + 1,
);
final newIndex = lteIndex == -1 ? aggregated.length - 1 : lteIndex - 1;
if (newIndex > currentIndex) {
aggregated
..setRange(currentIndex, newIndex, aggregated, currentIndex + 1)
..[newIndex] = current;
}
}
_total--;
}
}

/// A data structure identifying a reaction and who has voted for it.
///
/// [emojiName] is not part of the key identifying the reaction.
/// Servers don't key on it (only user, message, reaction type, and emoji code),
/// and we mimic that behavior:
/// https://github.com/zulip/zulip-flutter/pull/256#discussion_r1284865099
/// It's included here so we can display it in UI.
class ReactionWithVotes {
Copy link
Member

Choose a reason for hiding this comment

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

nit: let's give this file a consistent ordering: either general to specific and root to leaf, or vice versa. In particular either Reaction and ReactionWithVotes both go before Reactions, or both go after.

In model.dart it's general to specific and root to leaf, so the most natural thing is probably to do the same here.

final ReactionType reactionType;
final String emojiCode;
final String emojiName;
final Set<int> userIds = {};

ReactionWithVotes.empty(Reaction reaction)
: reactionType = reaction.reactionType,
emojiCode = reaction.emojiCode,
emojiName = reaction.emojiName;

@override
String toString() => 'ReactionWithVotes(reactionType: $reactionType, emojiCode: $emojiCode, emojiName: $emojiName, userIds: $userIds)';
}

/// A reaction object found inside message objects in the Zulip API.
///
/// E.g., under "reactions:" in <https://zulip.com/api/get-message>.
@JsonSerializable(fieldRename: FieldRename.snake)
class Reaction {
final String emojiName;
final String emojiCode;
final ReactionType reactionType;
final int userId;
// final Map<String, dynamic> user; // deprecated; ignore

Reaction({
required this.emojiName,
required this.emojiCode,
required this.reactionType,
required this.userId,
});

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

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

@override
String toString() => 'Reaction(emojiName: $emojiName, emojiCode: $emojiCode, reactionType: $reactionType, userId: $userId)';
}

/// As in [Reaction.reactionType].
@JsonEnum(fieldRename: FieldRename.snake)
enum ReactionType {
unicodeEmoji,
realmEmoji,
zulipExtraEmoji;

String toJson() => _$ReactionTypeEnumMap[this]!;
}
29 changes: 29 additions & 0 deletions lib/api/model/reaction.g.dart

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

15 changes: 9 additions & 6 deletions lib/model/message_list.dart
Original file line number Diff line number Diff line change
Expand Up @@ -350,18 +350,21 @@ class MessageListView with ChangeNotifier, _MessageSequence {
final message = messages[index];
switch (event.op) {
case ReactionOp.add:
message.reactions.add(Reaction(
(message.reactions ??= Reactions([])).add(Reaction(
emojiName: event.emojiName,
emojiCode: event.emojiCode,
reactionType: event.reactionType,
userId: event.userId,
));
case ReactionOp.remove:
message.reactions.removeWhere((r) {
return r.emojiCode == event.emojiCode
&& r.reactionType == event.reactionType
&& r.userId == event.userId;
});
if (message.reactions == null) { // TODO(log)
return;
}
message.reactions!.remove(
reactionType: event.reactionType,
emojiCode: event.emojiCode,
userId: event.userId,
);
}

notifyListeners();
Expand Down
2 changes: 1 addition & 1 deletion test/api/model/events_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ void main() {
MessageEvent mkEvent(List<MessageFlag> flags) => Event.fromJson({
'type': 'message',
'id': 1,
'message': message.toJson()..remove('flags'),
'message': (deepToJson(message) as Map<String, dynamic>)..remove('flags'),
'flags': flags.map((f) => f.toJson()).toList(),
}) as MessageEvent;
check(mkEvent(message.flags)).message.jsonEquals(message);
Expand Down
Loading