model: Add data models for typing events. #783

PIG208 · 2024-07-01T22:11:55Z

Still mostly a work in progress.

This should partially address #665

gnprice

Neat! Here's a couple of high-level comments on modeling the API.

lib/api/model/initial_snapshot.dart

lib/api/model/events.dart

PIG208 · 2024-07-02T22:51:18Z

Updated the PR to support the view-model for typing status. This should be ready for review now. I will follow up with the UI change in a separate PR.

PIG208 · 2024-07-03T00:01:04Z

Because MessageType expects either "stream" or "private", modern servers that send "direct" in the typing events are not supported unless we fix #785.

chrisbobbe

Exciting, thanks! Small comments below.

chrisbobbe · 2024-07-05T23:11:54Z

lib/api/model/events.dart

+  @JsonKey(name: 'sender', readValue: _readSenderId)
+  final int senderId;


Let's have _readSenderId do all of the work of reading the value, instead of sharing it with name: 'sender'. That way the reader doesn't have to go back and forth between the two places to see how the work gets done.

Like how in SubscriptionRemoveEvent, even though streamIds is computed from information at a subscriptions key, it doesn't use name: 'subscriptions', and instead the string 'subscriptions' is used in the _readStreamIds implementation:

/// A [SubscriptionEvent] with op `remove`: https://zulip.com/api/get-events#subscription-remove @JsonSerializable(fieldRename: FieldRename.snake) class SubscriptionRemoveEvent extends SubscriptionEvent { @override @JsonKey(includeToJson: true) String get op => 'remove'; @JsonKey(readValue: _readStreamIds) final List<int> streamIds; static List<int> _readStreamIds(Map<dynamic, dynamic> json, String key) { return (json['subscriptions'] as List<dynamic>) .map((e) => (e as Map<String, dynamic>)['stream_id'] as int) .toList(); } SubscriptionRemoveEvent({required super.id, required this.streamIds}); factory SubscriptionRemoveEvent.fromJson(Map<String, dynamic> json) => _$SubscriptionRemoveEventFromJson(json); @override Map<String, dynamic> toJson() => _$SubscriptionRemoveEventToJson(this); }

I wonder if the as int part is necessary for _readSenderId because the code generator handles that. Otherwise this refactor should be good.

I agree it looks like it's not necessary.

test/api/model/events_test.dart

chrisbobbe · 2024-07-05T23:18:39Z

lib/model/store.dart

@@ -361,6 +369,8 @@ class PerAccountStore extends ChangeNotifier with StreamStore, MessageStore {

  final RecentDmConversationsView recentDmConversationsView;

+  final TypingStatus typingStatus;


Hmm. I'm not sure this fits in the category "Messages, and summaries of messages". Maybe it's a better fit for "Users and data about them" and could go there?

Yeah, neither is a perfect fit.

I like the "users" area a bit better, because that's definitely where presence data will naturally go, and this is a lot like presence.

lib/model/typing_status.dart

test/model/typing_status_test.dart

chrisbobbe · 2024-07-05T23:51:56Z

test/model/typing_status_test.dart

+      checkNotNotified();
+    }));
+
+    test('remove another typist in a narrow', () => awaitFakeAsync((async) async {


I'm not sure I understand; would the meaning change if this test were called 'remove a typist in a narrow' instead?

This is specifically for testing the case when there are multiple typists but we only want to remove one. Renaming this to "remove one of two typists in the same narrow" might be better.

test/model/typing_status_test.dart

PIG208 · 2024-07-10T20:22:51Z

Thanks for the review @gnprice ! This is ready for another one now.

gnprice

Thanks for the revision! Comments below.

I've now read all the test cases. Some of the comments in this round should make the test cases more compact and so easier to scan through as a whole, so in a future round I'll reread them with an eye to thinking of any other cases that should be added.

gnprice · 2024-07-10T20:31:13Z

lib/api/model/events.dart

+  @override
+  MessageType fromJson(String json) {
+    if (json == 'private') json = 'direct';
+    return $enumDecode(_$MessageTypeEnumMap,json);


nit: spacing

Suggested change

return $enumDecode(_$MessageTypeEnumMap,json);

return $enumDecode(_$MessageTypeEnumMap, json);

gnprice · 2024-07-10T20:35:04Z

test/api/model/events_test.dart

+      check(UpdateMessageFlagsRemoveEvent.fromJson({
+        ...baseJson,
+        'flag': 'read',
+        'message_details': messageDetails..['123']!['type'] = 'private',


This mutates messageDetails, potentially affecting other tests. That's an annoying type of issue to debug, so best to systematically avoid — use the { ...foo, bar: baz } spread syntax instead to make a copy.

gnprice · 2024-07-10T20:39:09Z

test/api/model/events_test.dart

+        ...baseJson,
+        'flag': 'read',
+        'message_details': messageDetails..['123']!['type'] = 'private',
+      })).has((e) => e.messageDetails!.values.single.type, 'messageDetail.type')


Generally keep has calls in FooChecks extensions, and following the boring predictable forms we use there. So add to event_checks.dart as needed, and then this looks like:

Suggested change

})).has((e) => e.messageDetails!.values.single.type, 'messageDetail.type')

})).messageDetails.isNotNull().values.single.type

gnprice · 2024-07-10T20:40:06Z

test/api/model/events_test.dart

+      'type': 'delete_message',
+      'message_ids': [1, 2, 3],
+      'message_type': 'private',
+    })).has((e) => e.messageType, 'messageType').equals(MessageType.direct);


similarly, add to event_checks.dart

gnprice · 2024-07-10T20:44:17Z

lib/api/model/events.dart

+  @MessageTypeConverter()
  final MessageType messageType;


Suggested change

@MessageTypeConverter()

final MessageType messageType;

// The server never actually sends "direct" here yet (it's "private" instead),

// but we accept both forms for forward-compatibility.

@MessageTypeConverter()

final MessageType messageType;

It's a bit of a mismatch with the existing API, so it deserves a short comment acknowledging that.

Similarly on UpdateMessageFlagsMessageDetail.

gnprice · 2024-07-10T21:28:07Z

test/model/typing_status_test.dart

+      checkTypists({
+          dmNarrow: [eg.otherUser],
+          groupNarrow: [eg.thirdUser],
+          topicNarrow: [eg.fourthUser]});


nit: two-space indent

Suggested change

checkTypists({

dmNarrow: [eg.otherUser],

groupNarrow: [eg.thirdUser],

topicNarrow: [eg.fourthUser]});

checkTypists({

dmNarrow: [eg.otherUser],

groupNarrow: [eg.thirdUser],

topicNarrow: [eg.fourthUser]});

gnprice · 2024-07-10T21:33:26Z

test/model/typing_status_test.dart

+      prepareModel();
+
+      model.handleTypingEvent(eg.typingEvent(groupNarrow, TypingOp.start, eg.otherUser.userId));
+      checkNotifiedOnce();
+      model.handleTypingEvent(eg.typingEvent(groupNarrow, TypingOp.start, eg.thirdUser.userId));
+      checkNotifiedOnce();
+      checkTypists({groupNarrow: [eg.otherUser, eg.thirdUser]});


Most of these test cases have to start by adding some typists, when that part is effectively part of the setup of the test and isn't the thing the test is about.

So one helpful step for making the tests more focused on the main story they're trying to tell would be to have a shared helper take care of that, so that the test cases just specify what state they want. Here, that can probably just be an optional parameter on prepareModel. So:

Suggested change

prepareModel();

model.handleTypingEvent(eg.typingEvent(groupNarrow, TypingOp.start, eg.otherUser.userId));

checkNotifiedOnce();

model.handleTypingEvent(eg.typingEvent(groupNarrow, TypingOp.start, eg.thirdUser.userId));

checkNotifiedOnce();

checkTypists({groupNarrow: [eg.otherUser, eg.thirdUser]});

prepareModel(typists: {groupNarrow: [eg.otherUser, eg.thirdUser]});

and then the implementation of prepareModel can make the handleTypingEvent calls, check notified, and even use checkTypists to double-check that the state came out as intended.

gnprice · 2024-07-10T21:39:45Z

test/model/typing_status_test.dart

+      checkNotNotified();
+    });
+
+    test('remove one of two typists in the same narrow', () async {


Let's also have a test for removing the last typist in a narrow, checking that we don't leak an empty list for that narrow into our map.

… On rereading, I see you do cover that case at the end of "remove typists from different narrows". But it'd be good to have a separate test case just about that case, with a description to match. Otherwise it's easy to miss that that's there, as I did. Also if some future change were to accidentally break this behavior, if all that did was cause that last step of a larger test case to fail, it'd be easy to think it's just cleanup code and so miss that it's checking something meaningful, defeating the purpose of the test.

This is also one of the benefits of tightening the setup further, with something like prepareModel(typists: …) like in my immediate previous comment on the code just below here. When setup is concise, it's cheap to make more separate test cases and there isn't much reason to be tempted to combine conceptually different questions into a single test case.

gnprice · 2024-07-10T21:46:22Z

test/model/typing_status_test.dart

+    }));
+
+
+    test('typist is removed when the expiry period ends', () => awaitFakeAsync((async) async {


nit: put this before the other expiry test case — the narrative makes most sense when this one is read first, because the other is basically this plus an extra wrinkle

gnprice · 2024-07-10T21:47:25Z

test/model/typing_status_test.dart

+    });
+
+  });


nit: stray blank line at end of block

There's also a double blank line later in the file.

PIG208 · 2024-07-11T00:17:40Z

The PR has been updated to address the comments. Thanks for the review!

gnprice

Thanks for the revision! The changes look good. Comments below, including thinking through other test cases to make the tests complete.

gnprice · 2024-07-11T17:30:21Z

test/api/model/events_test.dart

+      'type': 'delete_message',
+      'message_ids': [1, 2, 3],
+      'message_type': 'private',
+    })).has((e) => e.messageType, 'messageType').equals(MessageType.direct);


gnprice · 2024-07-11T18:11:07Z

lib/model/typing_status.dart

+  Iterable<int> typistIdsInNarrow(SendableNarrow narrow) =>
+    _timerMapsByNarrow[narrow]?.keys ?? [];
+
+  final Map<SendableNarrow, Map<int, Timer>> _timerMapsByNarrow = {};


Agreed, yeah that's helpful.

gnprice · 2024-07-11T18:17:05Z

test/model/typing_status_test.dart

+      prepareModel();
+
+      handleTypingEvent(groupNarrow, TypingOp.start, eg.otherUser);
+      checkTypists({groupNarrow: [eg.otherUser]});
+      checkNotifiedOnce();


This can also benefit from prepareModel(typistsByNarrow: …).

(previous discussion: #783 (comment) )

Updated all tests other than "add typists in separate narrows" and "sort dm recipients" to use this.

gnprice · 2024-07-11T18:17:15Z

test/model/typing_status_test.dart

+      prepareModel();
+
+      handleTypingEvent(groupNarrow, TypingOp.start, eg.otherUser);
+      checkTypists({groupNarrow: [eg.otherUser]});
+      checkNotifiedOnce();


gnprice · 2024-07-11T18:17:30Z

test/model/typing_status_test.dart

+      prepareModel();
+
+      handleTypingEvent(dmNarrow, TypingOp.start, eg.otherUser);
+      checkTypists({dmNarrow: [eg.otherUser]});
+      checkNotifiedOnce();


gnprice · 2024-07-11T18:24:32Z

test/model/typing_status_test.dart

+  final groupNarrow = DmNarrow.withOtherUsers(
+    [eg.otherUser.userId, eg.thirdUser.userId], selfUserId: eg.selfUser.userId);
+
+  group('handle typing start events', () {


Another case to have an explicit test case for: a start event in a topic narrow.

(This gets covered by the "remove typists from different narrows" case; but because it's not the point of the test, it'd be easy for that to go away in a future revision of the tests and then that coverage would get dropped.)

gnprice · 2024-07-11T18:32:00Z

test/model/typing_status_test.dart

+  group('cancelling old timer', () {
+    test('when typing stopped early', () => awaitFakeAsync((async) async {


This group doesn't feel to me like the logical way to organize these two tests. Both test cases are good ones to have; but this one, let's put under the "stop events" group, with name "cancel timer".

The other case below, let's put under the "start events" group, with name "cancel existing timer".

In particular, these don't feel to me like two cases of one idea — it's more like two separate pieces of logic, that are part of what's expected for a stop event and a start event respectively.

Or perhaps better: these two could be organized together with the cases currently under "expiry period" below, as a group that's about the timers.

Then the narrative, test case by test case, could be:

start sets a timer — look, just wait 15 seconds and the typist gets removed (the current "typist is removed when the expiry period ends")

stop cancels the timer (the current "when typing stopped early")

repeated start resets the timer — look, there's just one timer pending, and you have to wait 15 seconds before the typist gets removed, even if it'd already been 10 seconds since the original start (the current "repeated typing start event resets the timer", plus the pendingTimers checks from the current "when typing repeatedly started")

I think that makes a pretty clean story.

gnprice · 2024-07-11T18:32:54Z

test/model/typing_status_test.dart

+      async.elapse(const Duration(seconds: 10));
+      checkTypists({});
+    }));


should also checkNotifiedOnce here

and for good measure, checkNotNotified at the 5-second mark

Also at the end check pendingTimers is empty. (No need for checking in the middle that it's nonempty — the fact that async.elapse causes a spontaneous change is enough to make that point.)

A related update: for the revised "repeated typing start event resets the timer" test, I kept some pendingTimers checks. It probably makes more sense there because we don't want duplicated timers.

gnprice · 2024-07-11T18:42:45Z

test/model/typing_status_test.dart

+      checkNotifiedOnce();
+    }));
+  });
+}


One more test case to add: test dispose.

gnprice · 2024-07-11T18:45:35Z

lib/model/typing_status.dart

+  void dispose() {
+    for (final timersByTypistId in _timerMapsByNarrow.values) {
+      for (final timer in timersByTypistId.values) {
+        timer.cancel();
+      }
+    }
+    super.dispose();


Suggested change

void dispose() {

for (final timersByTypistId in _timerMapsByNarrow.values) {

for (final timer in timersByTypistId.values) {

timer.cancel();

}

}

super.dispose();

void dispose() {

for (final timersByTypistId in _timerMapsByNarrow.values) {

for (final timer in timersByTypistId.values) {

timer.cancel();

}

}

_timerMapsByNarrow.clear();

super.dispose();

In general there isn't a need for dispose methods to clear maps and such just for the sake of clearing them; it's the garbage collector's job to discard them when the object is truly discarded.

But in this case, since we're disposing the timers, let's also clear the map, because that way this maintains the basic invariant of this data structure: all the timers in the map are live, and all the live timers this class created are in the map.

PIG208 · 2024-07-12T18:48:36Z

Thanks for the review! I have updated the PR to address the comments.

gnprice · 2024-07-14T23:23:27Z

Thanks for the revision! This all looks great — merging.

We still offer support for 'private', which gets converted to 'direct' as well when deserialized. Signed-off-by: Zixuan James Li <zixuan@zulip.com>

Signed-off-by: Zixuan James Li <zixuan@zulip.com>

The map of typist-id-to-timer maps design was inspired by the web app's implementation of typing status. We use a nested map instead to avoid introducing a custom hashing algorithm, and to make it easier to query typists in the same narrow. See also: https://github.com/zulip/zulip/blob/09bad82131abdedf253d53b4cb44c8b95f6a49f1/static/js/typing_data.js Signed-off-by: Zixuan James Li <zixuan@zulip.com>

gnprice reviewed Jul 1, 2024

View reviewed changes

lib/api/model/initial_snapshot.dart Outdated Show resolved Hide resolved

lib/api/model/events.dart Outdated Show resolved Hide resolved

lib/api/model/events.dart Outdated Show resolved Hide resolved

lib/api/model/events.dart Outdated Show resolved Hide resolved

PIG208 force-pushed the typing-events branch 2 times, most recently from d907d48 to e5d1e5d Compare July 2, 2024 22:06

PIG208 changed the title ~~(WIP) Handle typing events~~ model: Add data model for typing events. Jul 2, 2024

PIG208 changed the title ~~model: Add data model for typing events.~~ model: Add data models for typing events. Jul 2, 2024

PIG208 force-pushed the typing-events branch 9 times, most recently from afb464b to 5014510 Compare July 2, 2024 22:49

PIG208 marked this pull request as ready for review July 2, 2024 22:50

gnprice requested a review from chrisbobbe July 3, 2024 04:16

gnprice added the maintainer review PR ready for review by Zulip maintainers label Jul 3, 2024

gnprice mentioned this pull request Jul 3, 2024

model: Introduce data structures for "recent senders criterion" of user-mention autocomplete #692

Merged

PIG208 force-pushed the typing-events branch from 5014510 to 5e32bbf Compare July 3, 2024 22:43

PIG208 mentioned this pull request Jul 5, 2024

msglist: Display typing indicators on typing. #790

Merged

PIG208 force-pushed the typing-events branch from 5e32bbf to f1c68fd Compare July 5, 2024 21:22

chrisbobbe reviewed Jul 5, 2024

View reviewed changes

PIG208 force-pushed the typing-events branch 6 times, most recently from 86b27a6 to 25ae300 Compare July 8, 2024 20:09

PIG208 force-pushed the typing-events branch 7 times, most recently from 0d9d5e8 to e813313 Compare July 10, 2024 20:19

gnprice reviewed Jul 10, 2024

View reviewed changes

PIG208 force-pushed the typing-events branch 5 times, most recently from 2f5f104 to 13b526e Compare July 11, 2024 00:13

gnprice reviewed Jul 11, 2024

View reviewed changes

PIG208 force-pushed the typing-events branch 4 times, most recently from 6cf6a7a to a8df398 Compare July 12, 2024 18:34

chrisbobbe removed the maintainer review PR ready for review by Zulip maintainers label Jul 12, 2024

PIG208 added 4 commits July 14, 2024 16:24

api: Start using 'direct' for MessageType.

4d94975

We still offer support for 'private', which gets converted to 'direct' as well when deserialized. Signed-off-by: Zixuan James Li <zixuan@zulip.com>

api: Read typing constants from the server.

ca4c3d0

Signed-off-by: Zixuan James Li <zixuan@zulip.com>

api: Add typing events.

fb36a8a

Signed-off-by: Zixuan James Li <zixuan@zulip.com>

gnprice force-pushed the typing-events branch from a8df398 to 5148c45 Compare July 14, 2024 23:24

gnprice merged commit 5148c45 into zulip:main Jul 14, 2024
1 check passed

PIG208 deleted the typing-events branch July 15, 2024 04:30

		@JsonKey(name: 'sender', readValue: _readSenderId)
		final int senderId;

		@@ -361,6 +369,8 @@ class PerAccountStore extends ChangeNotifier with StreamStore, MessageStore {

		final RecentDmConversationsView recentDmConversationsView;

		final TypingStatus typingStatus;

	return $enumDecode(_$MessageTypeEnumMap,json);
	return $enumDecode(_$MessageTypeEnumMap, json);

	})).has((e) => e.messageDetails!.values.single.type, 'messageDetail.type')
	})).messageDetails.isNotNull().values.single.type

		}));


		test('typist is removed when the expiry period ends', () => awaitFakeAsync((async) async {

		group('cancelling old timer', () {
		test('when typing stopped early', () => awaitFakeAsync((async) async {

model: Add data models for typing events. #783

model: Add data models for typing events. #783

Conversation

PIG208 commented Jul 1, 2024 • edited Loading

gnprice left a comment

Choose a reason for hiding this comment

PIG208 commented Jul 2, 2024

PIG208 commented Jul 3, 2024

chrisbobbe 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

PIG208 commented Jul 10, 2024

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

PIG208 commented Jul 11, 2024

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

PIG208 commented Jul 12, 2024

gnprice commented Jul 14, 2024

PIG208 commented Jul 1, 2024 •

edited

Loading