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

chrisbobbe · chrisbobbe · commit f2a1b1f427ac · 2023-08-24T17:16:33.000-07:00
TODO more tests (see TODOs in model_test.dart)
diff --git a/lib/api/model/model.dart b/lib/api/model/model.dart
@@ -1,3 +1,5 @@
+import 'package:collection/collection.dart';
+import 'package:flutter/foundation.dart';
 import 'package:json_annotation/json_annotation.dart';
 
 part 'model.g.dart';
@@ -258,7 +260,7 @@ sealed class Message {
   bool isMeMessage;
   int? lastEditTimestamp;
 
-  final List<Reaction> reactions;
+  final Reactions reactions;
   final int recipientId;
   final String senderEmail;
   final String senderFullName;
@@ -305,6 +307,121 @@ sealed class Message {
   Map<String, dynamic> toJson();
 }
 
+/// A message's reactions, in a convenient data structure.
+class Reactions {
+  final List<Reaction> unaggregated;
+
+  /// 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].)
+  late final List<ReactionWithVotes> aggregated;
+
+  Reactions(this.unaggregated) {
+    final byReaction = <int, ReactionWithVotes>{};
+    for (final reaction in unaggregated) {
+      final key = Object.hash(reaction.reactionType, reaction.emojiCode);
+      final current = byReaction[key] ??= ReactionWithVotes(
+        reactionType: reaction.reactionType,
+        emojiCode: reaction.emojiCode,
+        emojiName: reaction.emojiName,
+      );
+      current.addVote(reaction.userId);
+    }
+
+    aggregated = byReaction.values.sorted(
+      // Descending by number of votes
+      (a, b) => -a._userIds.length.compareTo(b._userIds.length),
+    );
+  }
+
+  void add(Reaction reaction) {
+    unaggregated.add(reaction);
+    final current = aggregated.where((r) {
+      return r.reactionType == reaction.reactionType && r.emojiCode == reaction.emojiCode;
+    }).firstOrNull;
+    if (current == null) {
+      final newThing = ReactionWithVotes(
+        reactionType: reaction.reactionType,
+        emojiCode: reaction.emojiCode,
+        emojiName: reaction.emojiName,
+      );
+      newThing.addVote(reaction.userId);
+      aggregated.add(newThing);
+    } else {
+      current.addVote(reaction.userId);
+    }
+    // TODO could reposition `current` in list to keep it sorted by userIds size
+  }
+
+  void remove({
+    required ReactionType reactionType,
+    required String emojiCode,
+    required int userId,
+  }) {
+    unaggregated.removeWhere((r) {
+      return r.reactionType == reactionType
+        && r.emojiCode == emojiCode
+        && r.userId == 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);
+    }
+    // TODO could reposition `current` in list to keep it sorted by userIds size
+  }
+
+  factory Reactions.fromJson(List<dynamic> json) {
+    return Reactions(
+      json.map((r) => Reaction.fromJson(r as Map<String, dynamic>)).toList(),
+    );
+  }
+
+  List<dynamic> toJson() => unaggregated;
+}
+
+/// 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 {
+  final ReactionType reactionType;
+  final String emojiCode;
+  final String emojiName;
+
+  Set<int> get userIds => _userIds;
+  final Set<int> _userIds = {};
+
+  void addVote(int userId) {
+    _userIds.add(userId);
+  }
+
+  void removeVote(int userId) {
+    _userIds.remove(userId);
+  }
+
+  ReactionWithVotes({
+    required this.reactionType,
+    required this.emojiCode,
+    required this.emojiName,
+  });
+
+  @override
+  String toString() => 'ReactionWithVotes(reactionType: $reactionType, emojiCode: $emojiCode, emojiName: $emojiName, userIds: $userIds)';
+}
+
 @JsonSerializable(fieldRename: FieldRename.snake)
 class StreamMessage extends Message {
   @override
diff --git a/lib/api/model/model.g.dart b/lib/api/model/model.g.dart
diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart
@@ -357,11 +357,11 @@ class MessageListView with ChangeNotifier, _MessageSequence {
           userId: event.userId,
         ));
       case ReactionOp.remove:
-        message.reactions.removeWhere((r) {
-          return r.emojiCode == event.emojiCode
-            && r.reactionType == event.reactionType
-            && r.userId == event.userId;
-        });
+        message.reactions.remove(
+          reactionType: event.reactionType,
+          emojiCode: event.emojiCode,
+          userId: event.userId,
+        );
     }
 
     notifyListeners();
diff --git a/test/api/model/events_test.dart b/test/api/model/events_test.dart
@@ -14,7 +14,7 @@ void main() {
     MessageEvent mkEvent(List<String> flags) => Event.fromJson({
       'type': 'message',
       'id': 1,
-      'message': message.toJson()..remove('flags'),
+      'message': (deepToJson(message) as Map<String, dynamic>)..remove('flags'),
       'flags': flags,
     }) as MessageEvent;
     check(mkEvent(message.flags)).message.jsonEquals(message);
diff --git a/test/api/model/model_checks.dart b/test/api/model/model_checks.dart
@@ -5,18 +5,47 @@ extension MessageChecks on Subject<Message> {
   Subject<String> get content => has((e) => e.content, 'content');
   Subject<bool> get isMeMessage => has((e) => e.isMeMessage, 'isMeMessage');
   Subject<int?> get lastEditTimestamp => has((e) => e.lastEditTimestamp, 'lastEditTimestamp');
-  Subject<List<Reaction>> get reactions => has((e) => e.reactions, 'reactions');
+  Subject<Reactions> get reactions => has((e) => e.reactions, 'reactions');
   Subject<List<String>> get flags => has((e) => e.flags, 'flags');
 
   // TODO accessors for other fields
 }
 
-extension ReactionsChecks on Subject<List<Reaction>> {
+
+extension ReactionsListChecks on Subject<List<Reaction>> {
   void deepEquals(_) {
     throw UnimplementedError('Tried to call [Subject<List<Reaction>>.deepEquals]. Use jsonEquals instead.');
   }
 }
 
+extension ReactionsChecks on Subject<Reactions> {
+  void deepEquals(_) {
+    throw UnimplementedError('Tried to call [Subject<Reactions>.deepEquals]. Use jsonEquals instead.');
+  }
+
+  Subject<List<Reaction>> get unaggregated => has((e) => e.unaggregated, 'unaggregated');
+  Subject<List<ReactionWithVotes>> get aggregated => has((e) => e.aggregated, 'aggregated');
+}
+
+extension ReactionWithVotesChecks on Subject<ReactionWithVotes> {
+  Subject<ReactionType> get reactionType => has((r) => r.reactionType, 'reactionType');
+  Subject<String> get emojiCode => has((r) => r.emojiCode, 'emojiCode');
+  Subject<String> get emojiName => has((r) => r.emojiName, 'emojiName');
+  Subject<Set<int>> get userIds => has((e) => e.userIds, 'userIds');
+
+  /// Whether this [ReactionWithVotes] corresponds to the given same-emoji [reactions].
+  void matchesReactions(List<Reaction> reactions) {
+    assert(reactions.isNotEmpty);
+    final first = reactions.first;
+    assert(reactions.every((r) => r.reactionType == first.reactionType && r.emojiCode == first.emojiCode));
+    return which(it()
+      ..reactionType.equals(first.reactionType)
+      ..emojiCode.equals(first.emojiCode)
+      ..userIds.deepEquals(reactions.map((r) => r.userId).toSet())
+    );
+  }
+}
+
 extension ReactionChecks on Subject<Reaction> {
   Subject<String> get emojiName => has((r) => r.emojiName, 'emojiName');
   Subject<String> get emojiCode => has((r) => r.emojiCode, 'emojiCode');
diff --git a/test/api/model/model_test.dart b/test/api/model/model_test.dart
@@ -1,8 +1,11 @@
 import 'package:checks/checks.dart';
+import 'package:checks/context.dart';
 import 'package:test/scaffolding.dart';
 import 'package:zulip/api/model/model.dart';
 
 import '../../example_data.dart' as eg;
+import '../../stdlib_checks.dart';
+import 'model_checks.dart';
 
 void main() {
   group('User', () {
@@ -48,8 +51,9 @@ void main() {
   });
 
   group('DmMessage', () {
-    final Map<String, dynamic> baseJson = Map.unmodifiable(
-      eg.dmMessage(from: eg.otherUser, to: [eg.selfUser]).toJson());
+    final Map<String, dynamic> baseJson = Map.unmodifiable(deepToJson(
+      eg.dmMessage(from: eg.otherUser, to: [eg.selfUser]),
+    ) as Map<String, dynamic>);
 
     DmMessage parse(Map<String, dynamic> specialJson) {
       return DmMessage.fromJson({ ...baseJson, ...specialJson });
@@ -108,4 +112,118 @@ void main() {
         .deepEquals([2, 3, 11]);
     });
   });
+
+  group('Reactions', () {
+    // helper to cut out "it()..isA<ReactionWithVotes>()" goo for callers
+    ConditionSubject matchesReactions(List<Reaction> reactions) {
+      return it()..isA<ReactionWithVotes>().matchesReactions(reactions);
+    }
+
+    test('fromJson', () {
+      final reaction1Json = {'emoji_name': 'thumbs_up', 'emoji_code': '1f44d', 'reaction_type': 'unicode_emoji',     'user_id': 1};
+      final reaction2Json = {'emoji_name': 'thumbs_up', 'emoji_code': '1f44d', 'reaction_type': 'unicode_emoji',     'user_id': 2};
+      final reaction3Json = {'emoji_name': 'thumbs_up', 'emoji_code': '1f44d', 'reaction_type': 'unicode_emoji',     'user_id': 3};
+
+      final reaction4Json = {'emoji_name': 'twocents',  'emoji_code': '181',   'reaction_type': 'realm_emoji',       'user_id': 1};
+      final reaction5Json = {'emoji_name': 'twocents',  'emoji_code': '181',   'reaction_type': 'realm_emoji',       'user_id': 2};
+
+      final reaction6Json = {'emoji_name': 'zulip',     'emoji_code': 'zulip', 'reaction_type': 'zulip_extra_emoji', 'user_id': 4};
+      final reaction7Json = {'emoji_name': 'zulip',     'emoji_code': 'zulip', 'reaction_type': 'zulip_extra_emoji', 'user_id': 5};
+
+      final reaction1 = Reaction.fromJson(reaction1Json);
+      final reaction2 = Reaction.fromJson(reaction2Json);
+      final reaction3 = Reaction.fromJson(reaction3Json);
+      final reaction4 = Reaction.fromJson(reaction4Json);
+      final reaction5 = Reaction.fromJson(reaction5Json);
+      final reaction6 = Reaction.fromJson(reaction6Json);
+      final reaction7 = Reaction.fromJson(reaction7Json);
+
+      check(Reactions.fromJson([
+        reaction1Json, reaction2Json, reaction3Json, reaction4Json, reaction5Json, reaction6Json, reaction7Json
+      ]))
+        ..unaggregated.jsonEquals([
+            reaction1, reaction2, reaction3, reaction4, reaction5, reaction6, reaction7,
+          ])
+        ..aggregated.deepEquals([
+            matchesReactions([reaction1, reaction2, reaction3]),
+            matchesReactions([reaction4, reaction5]),
+            matchesReactions([reaction6, reaction7]),
+          ]);
+
+      // TODO test sorting of `aggregated`
+    });
+
+    test('add', () {
+      final reaction0 = Reaction(
+        reactionType: ReactionType.unicodeEmoji, emojiCode: '1f44d', emojiName: 'thumbs_up', userId: 1);
+      final reactions = Reactions([reaction0]);
+      check(reactions)
+        ..unaggregated.jsonEquals([reaction0])
+        ..aggregated.deepEquals([
+            matchesReactions([reaction0])
+          ]);
+
+      // …Different reactionType
+      final reaction1 = Reaction(
+        reactionType: ReactionType.realmEmoji, emojiCode: '181', emojiName: 'twocents', userId: 1);
+      reactions.add(reaction1);
+      check(reactions)
+        ..unaggregated.jsonEquals([reaction0, reaction1])
+        ..aggregated.deepEquals([
+            matchesReactions([reaction0]),
+            matchesReactions([reaction1]),
+          ]);
+
+      // …Same reactionType, different emojiCode
+      final reaction2 = Reaction(
+        reactionType: ReactionType.realmEmoji, emojiCode: '113', emojiName: 'slackzulip', userId: 1);
+      reactions.add(reaction2);
+      check(reactions)
+        ..unaggregated.jsonEquals([reaction0, reaction1, reaction2])
+        ..aggregated.deepEquals([
+            matchesReactions([reaction0]),
+            matchesReactions([reaction1]),
+            matchesReactions([reaction2]),
+          ]);
+
+      // …Same reaction, different user
+      final reaction3 = Reaction(
+        reactionType: ReactionType.realmEmoji, emojiCode: '113', emojiName: 'slackzulip', userId: 2);
+      reactions.add(reaction3);
+      check(reactions)
+        ..unaggregated.jsonEquals([reaction0, reaction1, reaction2, reaction3])
+        ..aggregated.deepEquals([
+            matchesReactions([reaction0]),
+            matchesReactions([reaction1]),
+            matchesReactions([reaction2, reaction3]),
+          ]);
+
+      final reaction4 = Reaction(
+        reactionType: ReactionType.unicodeEmoji, emojiCode: '1f6e0', emojiName: 'working_on_it', userId: 2);
+      reactions.add(reaction4);
+      check(reactions)
+        ..unaggregated.jsonEquals([reaction0, reaction1, reaction2, reaction3, reaction4])
+        ..aggregated.deepEquals([
+            matchesReactions([reaction0]),
+            matchesReactions([reaction1]),
+            matchesReactions([reaction2, reaction3]),
+            matchesReactions([reaction4]),
+          ]);
+
+      // …Same reactionType and emojiCode, different emojiName
+      final reaction5 = Reaction(
+        reactionType: ReactionType.unicodeEmoji, emojiCode: '1f6e0', emojiName: 'tools', userId: 2);
+      reactions.add(reaction5);
+      check(reactions)
+        ..unaggregated.jsonEquals([reaction0, reaction1, reaction2, reaction3, reaction4, reaction5])
+        ..aggregated.deepEquals([
+            matchesReactions([reaction0]),
+            matchesReactions([reaction1]),
+            matchesReactions([reaction2, reaction3]),
+            matchesReactions([reaction4, reaction5]),
+          ]);
+    });
+
+    // TODO test `remove`
+  });
 }
diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart
@@ -406,10 +406,8 @@ void main() async {
         final reaction3 = Reaction(reactionType: ReactionType.unicodeEmoji,
           emojiName: 'working_on_it', emojiCode: '1f6e0', userId: 1);
 
-        // Same user, same emojiCode, different emojiName. To be removed: servers
-        // key on user, message, reaction type, and emoji code, but not emoji name.
-        // So we mimic that behavior; see discussion:
-        //   https://github.com/zulip/zulip-flutter/pull/256#discussion_r1284865099
+        // Same user, same emojiCode, different emojiName. To be removed:
+        // we don't key on emoji name (see [ReactionWithVotes]).
         final reaction4 = Reaction(reactionType: ReactionType.unicodeEmoji,
           emojiName: 'hello',         emojiCode: '1f44b', userId: 1);
 
@@ -424,7 +422,7 @@ void main() async {
         checkNotifiedOnce();
         check(model).messages.single
           ..identicalTo(message)
-          ..reactions.jsonEquals([reaction2, reaction3]);
+          ..reactions.jsonEquals(Reactions([reaction2, reaction3]));
       });
 
       test('remove reaction; message is not in list', () async {
