Prep for supporting set typing status #888

PIG208 · 2024-08-14T05:07:22Z

No description provided.

gnprice · 2024-08-14T05:28:17Z

lib/model/typing_status.dart

+  }
+
+  Future<void> _notifyStart(PerAccountStore store) async {
+    _nextStartTime = DateTime.now().add(typingStartedWaitPeriod);


Quick high-level comment: let's write this so that we definitely don't have to think about time zones to reason about it. 🙂

I think an OK version could be made with DateTime.timestamp(), which uses UTC. Probably a cleaner approach is to use Stopwatch — then we're really not thinking about calendar dates or anything like that, just elapsed durations, which is exactly right for this subsystem.

PIG208 · 2024-08-20T06:50:02Z

Moved the main commits to #897, so that we can get some earlier independent changes reviewed separately.

chrisbobbe

Thanks! Comments below.

chrisbobbe · 2024-08-22T21:25:20Z

test/api/fake_api.dart

@@ -29,11 +29,11 @@ class _PreparedSuccess extends _PreparedResponse {
 /// An [http.Client] that accepts and replays canned responses, for testing.
 class FakeHttpClient extends http.BaseClient {

-  http.BaseRequest? lastRequest;
+  List<http.BaseRequest> requestHistory = [];


It might be helpful to make this a getter that returns an Iterable, which is backed by a private field that's a List? Then consumers of the request history can't mess with things by modifying the list.

chrisbobbe · 2024-08-22T21:26:29Z

test/api/fake_api.dart

+    // TODO: Prevent a source of bugs by ensuring that there is no outstanding
+    // prepared responses when the test ends.


nit: "that there are no outstanding prepared responses"

chrisbobbe · 2024-08-22T21:35:14Z

lib/api/route/typing.dart

+      return connection.post('setTypingStatus', (_) {}, 'typing', {
+        'op': RawParameter(op.toJson()),
+        'to': destination.userIds,
+      });


What's the effect of not passing 'type' in the DM case? From the doc, it looks like the preferred client behavior (assuming Zulip Server 4+) is to pass 'private' when FL is <174, else 'direct'. I'm not sure I've read closely enough to know what happens across the range of supported servers if we just don't pass 'type' at all.

This was intended to match the mobile apps' implementation, where we omit the type field for dm typing notifications. I think it is not clear that the default was "private" back then. Skipping type this way allows us to not worry about the "private" -> "direct" transition of this API.

See: https://chat.zulip.org/#narrow/stream/378-api-design/topic/stream.20typing.20indicator

I see. I guess since zulip-mobile does that not-preferred thing and presumably still works, probably makes it harmless if we also do the not-preferred thing. 🙂 But also, the burden of handling the "private" -> "direct" transition isn't heavy—

pass 'private' when FL is <174, else 'direct'

—so I'd be in favor of just doing that. If not, though, let's include a comment to make it clear that we're intentionally doing something that's different from the thing that seems natural/expected from a reading of the API doc.

Yeah, cleanest to just do the thing that's preferred in the current API.

(And then adapt for old servers as necessary — but "type" has been accepted since server-4, so effectively forever now.)

chrisbobbe · 2024-08-22T21:38:14Z

lib/api/route/typing.dart

+      return connection.post('setTypingStatus', (_) {}, 'typing', {
+        'type': RawParameter((supportsTypeChannel) ? 'channel' : 'stream'),
+        'op': RawParameter(op.toJson()),
+        'stream_id': destination.streamId,


The API doc says 'stream_id' is new in FL 215, and there's a different way we need to express the destination for <215.

Interesting, thanks for catching that!

chrisbobbe · 2024-08-22T21:39:09Z

test/api/route/typing_test.dart

+      destination: destination,
+    );


Suggested change

destination: destination,

);

destination: destination);

chrisbobbe · 2024-08-22T21:39:25Z

test/api/route/typing_test.dart

+        },
+      );


Suggested change

},

);

});

PIG208 · 2024-08-22T22:35:56Z

Thanks for the review @chrisbobbe! I have updated the PR.

Dropped fromSendableNarrow to keep the api code from depending on lib/model.
Reorganized the tests and made them shorter.
Handle the legacy case for FL 215.

PIG208 · 2024-08-23T21:37:00Z

Thanks! I should push an update later addressing these comments.

PIG208 · 2024-08-23T22:04:10Z

OK, this has been updated :)

PIG208 · 2024-09-05T18:34:22Z

Fixed indentation in a test.

PIG208 · 2024-09-07T04:27:59Z

Pushed to rebase.

PIG208 · 2024-09-12T21:32:36Z

Dropped the checkDmExpectedOp helper because the TypingOp.toJson is already tested with the typing status for topic tests. Also revised the wording of some tests.

gnprice · 2024-09-13T08:13:35Z

lib/api/route/typing.dart

+      final supportsTypeChannel = connection.zulipFeatureLevel! >= 248; // TODO(server-9)
+      final supportsStreamId = connection.zulipFeatureLevel! >= 215; // TODO(server-8)
+      return connection.post('setTypingStatus', (_) {}, 'typing', {
+        'type':  RawParameter((supportsTypeChannel) ? 'channel' : 'stream'),


nit: extra parens

gnprice · 2024-09-13T08:18:48Z

lib/api/route/typing.dart

+        'topic': RawParameter(destination.topic),
+        ...(supportsStreamId) ? {'stream_id': destination.streamId}
+                              : {'to': [destination.streamId]}


nit: put in logical order: channel/stream ID before topic, because the topic lives within the channel

Similarly, put type just above the channel ID (or list of DM recipients), because it's the next layer of namespacing above those.

gnprice · 2024-09-13T08:21:16Z

lib/api/route/typing.dart

+        ...(supportsStreamId) ? {'stream_id': destination.streamId}
+                              : {'to': [destination.streamId]}


A fun Dart 3 way to write this:

Suggested change

...(supportsStreamId) ? {'stream_id': destination.streamId}

: {'to': [destination.streamId]}

if (supportsStreamId) 'stream_id': destination.streamId

else 'to': [destination.streamId],

gnprice · 2024-09-13T08:22:58Z

test/api/route/typing_test.dart

+  Future<void> checkSetTypingStatus(FakeApiConnection connection, TypingOp op, {
+    required MessageDestination destination,


nit:

Suggested change

Future<void> checkSetTypingStatus(FakeApiConnection connection, TypingOp op, {

required MessageDestination destination,

Future<void> checkSetTypingStatus(FakeApiConnection connection,

TypingOp op, {

required MessageDestination destination,

else "op" gets a bit lost visually

gnprice · 2024-09-13T08:23:51Z

test/api/route/typing_test.dart

+  Future<void> checkSetTypingStatusForTopic(TypingOp op, String expectedOp) =>
+    FakeApiConnection.with_((connection) =>
+      checkSetTypingStatus(connection, op,


nit: block bodies

see that Flutter style guide section I've linked a few times before 🙂

gnprice · 2024-09-13T08:24:01Z

test/api/route/typing_test.dart

+  test('send typing status start for topic', () =>
+    checkSetTypingStatusForTopic(TypingOp.start, 'start'));


And definitely block bodies for test cases.

gnprice · 2024-09-13T08:26:47Z

lib/api/route/typing.dart

+      return connection.post('setTypingStatus', (_) {}, 'typing', {
+        'op': RawParameter(op.toJson()),
+        'to': destination.userIds,
+      });


Yeah, cleanest to just do the thing that's preferred in the current API.

(And then adapt for old servers as necessary — but "type" has been accepted since server-4, so effectively forever now.)

gnprice · 2024-09-13T08:30:29Z

Thanks @PIG208, and thanks @chrisbobbe for the previous reviews!

Generally this all looks good. Several comments above, all small.

PIG208 · 2024-09-13T13:58:29Z

Thanks! Pushed an update to the last commit.

UserSettingName has been moved to another file. Signed-off-by: Zixuan James Li <[email protected]>

Signed-off-by: Zixuan James Li <[email protected]>

This is not prohibited by the server and we should not log expected behaviors like this. See: https://zulip.com/api/get-events#typing-start Signed-off-by: Zixuan James Li <[email protected]>

Signed-off-by: Zixuan James Li <[email protected]>

An issue that exists prior to this change is that there can be unused prepared responses when the test ends, but we don't have a way to detect that. We can potentially do something like what testWidgets does with pending timers. Signed-off-by: Zixuan James Li <[email protected]>

The three legacy cases will need to be dropped separately at different server versions. The tests will fail accordingly as we do that. Signed-off-by: Zixuan James Li <[email protected]>

gnprice · 2024-09-16T23:12:06Z

Thanks! Looks good; merging.

PIG208 force-pushed the set-typing branch from 85b0070 to eb71911 Compare August 14, 2024 05:07

gnprice reviewed Aug 14, 2024

View reviewed changes

PIG208 force-pushed the set-typing branch 10 times, most recently from 382289d to fe71eea Compare August 20, 2024 06:46

PIG208 changed the title ~~WIP Set typing status~~ Add set typing status route Aug 20, 2024

PIG208 changed the title ~~Add set typing status route~~ Prep for supporting set typing status Aug 20, 2024

PIG208 mentioned this pull request Aug 20, 2024

Support setting typing status #897

Merged

PIG208 force-pushed the set-typing branch from fe71eea to eff9a33 Compare August 20, 2024 06:49

PIG208 marked this pull request as ready for review August 20, 2024 06:50

PIG208 requested a review from chrisbobbe August 20, 2024 06:50

PIG208 added the maintainer review PR ready for review by Zulip maintainers label Aug 20, 2024

PIG208 force-pushed the set-typing branch 3 times, most recently from 58a36d9 to e19c4de Compare August 20, 2024 23:13

chrisbobbe reviewed Aug 22, 2024

View reviewed changes

PIG208 force-pushed the set-typing branch 4 times, most recently from 6ff98a9 to 3bac13a Compare August 22, 2024 22:33

PIG208 force-pushed the set-typing branch from 3bac13a to 9ce551c Compare August 22, 2024 22:48

PIG208 force-pushed the set-typing branch 2 times, most recently from eec2173 to c823d5c Compare August 23, 2024 22:01

PIG208 requested a review from gnprice August 23, 2024 22:03

PIG208 force-pushed the set-typing branch from c823d5c to 9cc995a Compare August 23, 2024 22:04

PIG208 linked an issue Aug 29, 2024 that may be closed by this pull request

Set "typing" status #666

Closed

PIG208 removed a link to an issue Aug 29, 2024

Set "typing" status #666

Closed

PIG208 force-pushed the set-typing branch from 9cc995a to 97eceae Compare September 5, 2024 18:34

PIG208 force-pushed the set-typing branch from 97eceae to 21ff417 Compare September 7, 2024 04:19

PIG208 force-pushed the set-typing branch 2 times, most recently from 2a349d8 to 9a77275 Compare September 12, 2024 21:31

gnprice reviewed Sep 13, 2024

View reviewed changes

PIG208 force-pushed the set-typing branch from 9a77275 to 1d18851 Compare September 13, 2024 13:41

PIG208 force-pushed the set-typing branch from 1d18851 to 35e6f18 Compare September 13, 2024 17:01

PIG208 added 6 commits September 16, 2024 16:03

api [nfc]: Fix outdated update instructions.

b45a7c0

UserSettingName has been moved to another file. Signed-off-by: Zixuan James Li <[email protected]>

api [nfc]: Support const constructor for MessageDestination.

d764cea

Signed-off-by: Zixuan James Li <[email protected]>

typing_status [nfc]: Remove debugLog for adding self as typist.

850e59f

This is not prohibited by the server and we should not log expected behaviors like this. See: https://zulip.com/api/get-events#typing-start Signed-off-by: Zixuan James Li <[email protected]>

fake_api: Allow recording and taking multiple requests.

d458496

Signed-off-by: Zixuan James Li <[email protected]>

api: Add route setTypingStatus.

e974ed3

The three legacy cases will need to be dropped separately at different server versions. The tests will fail accordingly as we do that. Signed-off-by: Zixuan James Li <[email protected]>

gnprice force-pushed the set-typing branch from 35e6f18 to e974ed3 Compare September 16, 2024 23:12

gnprice merged commit e974ed3 into zulip:main Sep 16, 2024

PIG208 deleted the set-typing branch September 17, 2024 00:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prep for supporting set typing status #888

Prep for supporting set typing status #888

PIG208 commented Aug 14, 2024

gnprice Aug 14, 2024

PIG208 commented Aug 20, 2024

chrisbobbe left a comment

chrisbobbe Aug 22, 2024

chrisbobbe Aug 22, 2024

chrisbobbe Aug 22, 2024

PIG208 Aug 22, 2024 •

edited

Loading

chrisbobbe Aug 22, 2024 •

edited

Loading

gnprice Sep 13, 2024

chrisbobbe Aug 22, 2024

PIG208 Aug 22, 2024

chrisbobbe Aug 22, 2024

chrisbobbe Aug 22, 2024

PIG208 commented Aug 22, 2024

PIG208 commented Aug 23, 2024

PIG208 commented Aug 23, 2024

PIG208 commented Sep 5, 2024

PIG208 commented Sep 7, 2024

PIG208 commented Sep 12, 2024

gnprice Sep 13, 2024

gnprice Sep 13, 2024

gnprice Sep 13, 2024

gnprice Sep 13, 2024

gnprice Sep 13, 2024

gnprice Sep 13, 2024

gnprice Sep 13, 2024

gnprice commented Sep 13, 2024

PIG208 commented Sep 13, 2024

gnprice commented Sep 16, 2024

		// TODO: Prevent a source of bugs by ensuring that there is no outstanding
		// prepared responses when the test ends.

		...(supportsStreamId) ? {'stream_id': destination.streamId}
		: {'to': [destination.streamId]}

		Future<void> checkSetTypingStatus(FakeApiConnection connection, TypingOp op, {
		required MessageDestination destination,

		test('send typing status start for topic', () =>
		checkSetTypingStatusForTopic(TypingOp.start, 'start'));

Prep for supporting set typing status #888

Prep for supporting set typing status #888

Conversation

PIG208 commented Aug 14, 2024

Choose a reason for hiding this comment

PIG208 commented Aug 20, 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

PIG208 Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

chrisbobbe Aug 22, 2024 • edited Loading

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 Aug 22, 2024

PIG208 commented Aug 23, 2024

PIG208 commented Aug 23, 2024

PIG208 commented Sep 5, 2024

PIG208 commented Sep 7, 2024

PIG208 commented Sep 12, 2024

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

gnprice commented Sep 13, 2024

PIG208 commented Sep 13, 2024

gnprice commented Sep 16, 2024

PIG208 Aug 22, 2024 •

edited

Loading

chrisbobbe Aug 22, 2024 •

edited

Loading