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

chrisbobbe · 2023-08-25T00:24:20Z

I thought I'd check in with my progress toward keeping an efficient data structure to support the list of reaction buttons on a message in the message list. 🙂

As discussed in chat, this goes directly on Message instances.

In this revision, the data structure retains a raw, "unaggregated" list, like the list it gets from the server. That doesn't have to stay, since it takes up space, but it does make it efficient to count reactions for UserSettings.displayEmojiReactionUsers, and it supports a nice and efficient Reactions.toJson in case the app starts requiring that.

There are TODOs in the tests, but I'll get to those. 🙂

Related: #121

gnprice

Thanks! Comments below.

I haven't yet looked at the tests; LMK if that's also an area you'd like feedback on at this stage.

gnprice · 2023-08-25T03:35:22Z

lib/api/model/model.dart

@@ -305,6 +307,121 @@ sealed class Message {
  Map<String, dynamic> toJson();
 }

+/// A message's reactions, in a convenient data structure.
+class Reactions {


Let's put this in its own file, now that it's growing this data-structure complexity. Say lib/api/model/reaction.dart, and then the other reaction-related types like Reaction and ReactionType can go there too.

gnprice · 2023-08-25T03:38:48Z

lib/api/model/model.dart

+  Set<int> get userIds => _userIds;
+  final Set<int> _userIds = {};


This can just be final Set<int> userIds = {}; — because it's final, there's no setting it anyway, so the getter gives just as much access as the field.

gnprice · 2023-08-25T03:40:31Z

lib/api/model/model.dart

@@ -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;


Yeah, I think cleaner to leave this out.

For efficiently counting the total in order to implement UserSettings.displayEmojiReactionUsers, we can keep a field like int total, and just increment and decrement that appropriately.

gnprice · 2023-08-25T03:44:21Z

lib/api/model/model.dart

+  /// [ReactionWithVotes.reactionType] and [ReactionWithVotes.emojiCode].
+  /// (We don't also key on [ReactionWithVotes.emojiName];
+  /// see [ReactionWithVotes].)
+  late final List<ReactionWithVotes> aggregated;


Instead of late final, the idiom that I think I've seen more in the Flutter tree is

have a plain final field;

have a constructor that takes that directly as an argument, but is private;

then have a factory constructor that does the computation akin to your constructor below, and ends by calling the private constructor.

That's basically the pattern we used in RecentDmConversationsView, too.

gnprice · 2023-08-25T03:50:16Z

lib/api/model/model.dart

+      final key = Object.hash(reaction.reactionType, reaction.emojiCode);
+      final current = byReaction[key] ??= ReactionWithVotes(


This can give incorrect results, because Object.hash can have collisions. (That is, it's not a cryptographic hash function.)

Instead, you can use the Reaction objects themselves as the keys, but with a custom equality and hash function. These can be supplied as part of the map object, using the LinkedHashMap constructor.

gnprice · 2023-08-25T03:55:04Z

lib/api/model/model.dart

+    } else {
+      current.addVote(reaction.userId);
+    }
+    // TODO could reposition `current` in list to keep it sorted by userIds size


Yeah, that's probably best. Keeps things more consistent between what we get from processing events, and what we'd get if we re-fetched the message from scratch.

Can do that by walking backward in the list until we find an entry that has at least this many votes, or the beginning of the list; this entry then goes just after that one.

gnprice · 2023-08-25T04:11:08Z

lib/api/model/model.dart

+  Reactions(this.unaggregated) {
+    final byReaction = <int, ReactionWithVotes>{};
+    for (final reaction in unaggregated) {


As far as efficiency is concerned, I think probably the most important single thing to do on this model is to minimize the work that's done for a message that has no reactions. That's probably the vast majority of messages.

A basic step to take in that direction would be to just give this constructor a short-cut path, where if the given list of reactions is empty then we skip making a map and just say the aggregated list is empty.

A more complete step would be to make Message.reactions nullable, with null representing empty, in the same way as User.profileData. That will make consuming code slightly more complicated; but I think the number of places in the code that will consume a message's reactions is fairly small, so that probably isn't too bad.

gnprice · 2023-08-25T04:18:19Z

lib/api/model/model.dart

+
+  void add(Reaction reaction) {
+    unaggregated.add(reaction);
+    final current = aggregated.where((r) {


This linear-scan approach seems fine. In principle it could be slow, if the message has a long list of distinct reactions; but I think that's very uncommon.

In the constructor, though, good to use a hash table, as you do. That's potentially processing quite a lot of reactions at once (even though usually not that many distinct ones), plus it can be part of processing a whole bunch of messages at once. So the same speed difference there would have a much greater risk of causing a dropped frame.

One important optimization for efficiency is, when a message has no reactions, to initialize its `.reactions` to `null` instead of an empty [Reactions] instance: zulip#286 (comment) In the message-list-model tests, remove the part that inspects how the reactions data structure ends up looking -- that's now covered in dedicated unit tests for the data structure. Instead, just make sure the data structure's `add` and `remove` are called as appropriate.

chrisbobbe · 2023-08-31T02:25:15Z

Thanks for the review! Revision pushed, and I've addressed those self-TODOs in the tests this round. 🙂

gnprice

Thanks for the revision! Comments below.

I'm also seeing a number of failures in the tests, particularly in the form of MalformedServerResponseException in MessageListView.fetchInitial. So those will call for some debugging.

gnprice · 2023-08-31T21:46:49Z

lib/model/message_list.dart

@@ -3,6 +3,7 @@ import 'package:flutter/foundation.dart';

 import '../api/model/events.dart';
 import '../api/model/model.dart';
+import '../api/model/reaction.dart';


This is probably a good use case for an export directive. In lib/api/model/model.dart, add:

export 'reaction.dart';

as a new stanza after the imports. Then all these files that just want to talk about the model, with reactions included, don't need a separate import for reaction.dart.

Effectively that means that the internal organization of the model into files — the fact that model.dart refers to another file to get the Reactions type that Message.reactions uses, rather than define it within the same file — isn't something other code needs to care about.

gnprice · 2023-08-31T21:53:33Z

lib/api/model/reaction.dart

+/// As in [Message.reactions].
+@JsonSerializable(fieldRename: FieldRename.snake)
+class Reaction {


This dartdoc needs updating. :-)

Probably it should become less terse, now that this class's relationships are more complicated. I think the key fact about this class is that it describes the reaction objects found inside message objects in the Zulip API. That's no longer expressed by a reference to [Message.reactions], so probably this should gain a link directly to the API docs as part of that explanation.

gnprice · 2023-08-31T21:59:43Z

lib/api/model/reaction.dart

+    required int userId
+  })? debugOnRemove;
+
+  factory Reactions.fromJson(List<dynamic> json) {


Let's put all the constructors next to each other; I think that's helpful for finding them. The toJson method can go right after that, to keep it near fromJson.

gnprice · 2023-08-31T22:01:12Z

lib/api/model/reaction.dart

+  int get total => _total;
+  int _total;


It looks like this doesn't get updated on add/remove.

Relatedly, it should get checked in the tests. 😉

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?

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.

gnprice · 2023-08-31T22:02:43Z

lib/api/model/reaction.dart

+  void addVote(int userId) {
+    userIds.add(userId);
+  }
+
+  void removeVote(int userId) {
+    userIds.remove(userId);
+  }


These are probably cleanest to just inline.

gnprice · 2023-08-31T22:37:20Z

test/api/model/model_checks.dart

+    return which(it()
+      ..reactionType.equals(first.reactionType)
+      ..emojiCode.equals(first.emojiCode)
+      ..userIds.deepEquals(reactions.map((r) => r.userId).toSet())


nit: can pull this set out as a local variable, and then reuse in the assert above

gnprice · 2023-08-31T22:41:30Z

test/api/model/reaction_test.dart

+      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};


Would be good to also include reactions with the same type and code but different names. That's a direction in which it'd be easy to regress the logic someday (partly because the desired/specified logic is a bit peculiar).

I see that case is exercised for add, which is good, and for remove is made impossible by its API, which is great. But the constructors have their own logic which could independently have a regression, so should also test this case.

gnprice · 2023-08-31T22:51:27Z

test/api/model/reaction_test.dart

+        matchesReactions([reaction1]),
+      ]);
+
+      // …Same reactionType, different emojiCode


Another fun case which should be tested: different reactionType but the same emojiCode.

For realistic data for that, try grep -P "'\d+'," src/emoji/codePointMap.js in zulip-mobile. That finds Unicode emoji known to Zulip whose emoji codes look the same as a decimal integer, which means they look the same as some realm emoji.

Ha, interesting.

gnprice · 2023-08-31T23:09:25Z

test/model/message_list_test.dart

-        check(model).messages.single
-          ..identicalTo(message)
-          ..reactions.jsonEquals([eg.unicodeEmojiReaction]);
+        check(model).messages.single.identicalTo(message);
+        check(reactionsDotAddCalls).single
+          ..reactionType.equals(eventReaction.reactionType)
+          ..emojiCode.equals(eventReaction.emojiCode)
+          ..emojiName.equals(eventReaction.emojiName)
+          ..userId.equals(eventReaction.userId);


Hmm, this feels like quite a bit more boilerplate in the test code. I'm also a bit wary of putting in these spy hooks and testing on those — I'd rather test the end state, where possible. (Plugins are an area where that's not possible, hence the "binding" indirection for those.)

I think we can actually test the end state pretty reasonably. Here's a simple version:

check(model).messages.single ..identicalTo(message) ..reactions.isNotNull().aggregated.jsonEquals([eg.unicodeEmojiReaction]);

As long as there's only a very few reactions involved, so no complexity from wanting to not care about the order of them etc., that should work pretty well.

If that were to start to get complicated, then the principle I think I'd want to express is "this Reactions object is in an equivalent state to where it'd be if we initialized a Reactions object from the following list of reactions". That keeps it pretty well isolated from the internal implementation details of Reactions, and I think in any given test case it should be straightforward to come up with what that list of reactions should be. Then it'd be a matter of implementing that, loosely along the lines of your matchesReactions condition on ReactionWithVotes.

Do you mean

..reactions.isNotNull().jsonEquals([eg.unicodeEmojiReaction]);

in place of

..reactions.isNotNull().aggregated.jsonEquals([eg.unicodeEmojiReaction]);

? If checking aggregated, we probably also want to check its userIds, right? And if we want to check aggregated with jsonEquals, we'll need to give ReactionWithVotes a toJson method (one that will probably never run in app code).

Hmm yeah, I think that probably is what I had in mind — get the disaggregated list of reactions and check that that's what we expect.

gnprice · 2023-08-31T23:14:45Z

test/model/message_list_test.dart

-        final someMessage = eg.streamMessage(id: 1, reactions: [eg.unicodeEmojiReaction]);
+        final someMessage = eg.streamMessage(id: 1, reactions: Reactions([eg.unicodeEmojiReaction]));


Seems like it's more convenient for callers of eg.streamMessage if this reactions parameter remains just a (nullable) list. The implementation can then say something like

'reactions': reactions == null ? null : Reactions(reactions),

One important optimization for efficiency is, when a message has no reactions, to initialize its `.reactions` to `null` instead of an empty [Reactions] instance: zulip#286 (comment) In the message-list-model tests, remove the part that inspects how the reactions data structure ends up looking -- that's now covered in dedicated unit tests for the data structure. Instead, just make sure the data structure's `add` and `remove` are called as appropriate.

chrisbobbe · 2023-09-06T01:05:37Z

Thanks for the review! Revision pushed, and see one small query here: #286 (comment)

gnprice

Thanks for the revision! Generally all now looks good; just a handful of comments.

gnprice · 2023-09-06T21:43:44Z

lib/api/model/reaction.dart

+        aggregated.setRange(newIndex, currentIndex + 1,
+          [current].followedBy(aggregated.sublist(newIndex, currentIndex)));


This version may currently work, but I think the List.setRange docs disclaim any promise that it works:

If iterable is this list, the operation correctly copies the elements originally in the range from skipCount to skipCount + (end - start) to the range start to end, even if the two ranges overlap.

If iterable depends on this list in some other way, no guarantees are made.

Here the iterable argument depends on the receiver list (namely aggregated), in a way other than directly being that list.

So instead the setRange call will need to pass aggregated itself as the iterable; it can use the skipCount parameter to control what part of the list is used. Then moving current can be a plain []= call.

gnprice · 2023-09-06T21:52:25Z

test/model/message_list_test.dart

@@ -381,7 +381,7 @@ void main() async {
        checkNotifiedOnce();


nit: an early commit adds an import to this file that's redundant in this revision, which a later commit removes

gnprice · 2023-09-06T21:53:50Z

test/model/message_list_test.dart

-          ..reactions.jsonEquals([eg.unicodeEmojiReaction]);
+          ..reactions.isNotNull().jsonEquals([eg.unicodeEmojiReaction]);


nit: the main final commit's message says:

In the message-list-model tests, remove the part that inspects how the reactions data structure ends up looking -- that's now covered in dedicated unit tests for the data structure. Instead, just make sure the data structure's `add` and `remove` are called as appropriate.

which is something that no longer happens in this revision

gnprice · 2023-09-06T22:01:45Z

lib/api/model/model.dart

+  static Object _reactionsToJson(Reactions? value) {
+    return value ?? [];


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

One important optimization for efficiency is, when a message has no reactions, to initialize its `.reactions` to `null` instead of an empty [Reactions] instance: zulip#286 (comment)

chrisbobbe · 2023-09-06T23:46:21Z

Thanks for the review! Revision pushed.

gnprice · 2023-09-07T00:15:48Z

Thanks! All now looks good, but there's conflicts with your #298 which I just merged. Would you rebase past those?

One important optimization for efficiency is, when a message has no reactions, to initialize its `.reactions` to `null` instead of an empty [Reactions] instance: zulip#286 (comment)

chrisbobbe · 2023-09-07T00:48:13Z

Sure! Thanks for the review; just rebased past those conflicts and pushed the result here.

We'll add more things here soon.

One important optimization for efficiency is, when a message has no reactions, to initialize its `.reactions` to `null` instead of an empty [Reactions] instance: zulip#286 (comment)

gnprice · 2023-09-07T18:08:49Z

Thanks! Merging.

chrisbobbe requested a review from gnprice August 25, 2023 00:24

gnprice reviewed Aug 25, 2023

View reviewed changes

chrisbobbe force-pushed the pr-reactions-data-structure branch from f2a1b1f to 22e7d7e Compare August 31, 2023 02:24

chrisbobbe marked this pull request as ready for review August 31, 2023 02:25

chrisbobbe changed the title ~~wip model: Add Reactions data structure, for efficient access in UI~~ model: Add Reactions data structure, for efficient access in UI Aug 31, 2023

gnprice reviewed Aug 31, 2023

View reviewed changes

chrisbobbe force-pushed the pr-reactions-data-structure branch from 22e7d7e to ca49ed4 Compare September 6, 2023 01:05

gnprice reviewed Sep 6, 2023

View reviewed changes

chrisbobbe mentioned this pull request Sep 6, 2023

model: Use enum for Message.flags[] #298

Merged

chrisbobbe force-pushed the pr-reactions-data-structure branch from ca49ed4 to 2bb3f1e Compare September 6, 2023 23:46

chrisbobbe force-pushed the pr-reactions-data-structure branch from 2bb3f1e to 7b0bdb6 Compare September 7, 2023 00:47

chrisbobbe added 2 commits September 7, 2023 11:06

model [nfc]: Pull out new api/model/reaction.dart

783c132

We'll add more things here soon.

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

744c1f8

One important optimization for efficiency is, when a message has no reactions, to initialize its `.reactions` to `null` instead of an empty [Reactions] instance: zulip#286 (comment)

gnprice force-pushed the pr-reactions-data-structure branch from 7b0bdb6 to 744c1f8 Compare September 7, 2023 18:08

gnprice merged commit 744c1f8 into zulip:main Sep 7, 2023

chrisbobbe deleted the pr-reactions-data-structure branch September 7, 2023 18:09

		Set<int> get userIds => _userIds;
		final Set<int> _userIds = {};

		final key = Object.hash(reaction.reactionType, reaction.emojiCode);
		final current = byReaction[key] ??= ReactionWithVotes(

		final someMessage = eg.streamMessage(id: 1, reactions: [eg.unicodeEmojiReaction]);
		final someMessage = eg.streamMessage(id: 1, reactions: Reactions([eg.unicodeEmojiReaction]));

		aggregated.setRange(newIndex, currentIndex + 1,
		[current].followedBy(aggregated.sublist(newIndex, currentIndex)));

		..reactions.jsonEquals([eg.unicodeEmojiReaction]);
		..reactions.isNotNull().jsonEquals([eg.unicodeEmojiReaction]);

		static Object _reactionsToJson(Reactions? value) {
		return value ?? [];

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

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

Conversation

chrisbobbe commented Aug 25, 2023 • edited Loading

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Aug 31, 2023

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Sep 6, 2023

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Sep 6, 2023

gnprice commented Sep 7, 2023

chrisbobbe commented Sep 7, 2023

gnprice commented Sep 7, 2023

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

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

chrisbobbe commented Aug 25, 2023 •

edited

Loading