New `read_for_sender` flag when sending a new message #456

sirpengi · 2023-12-19T15:13:59Z

PR is built on top of #447 in order for tests to pass.

Adds a new flag introduced in zulip/zulip#28204 .

Fixes: #440

sirpengi · 2023-12-19T15:15:59Z

For the time being, CZO is still showing FL 226 (that is, the changes in the linked PR have not been deployed yet). So this might be premature to merge at the moment as I haven't confirmed the functionality with a live server.

gnprice · 2023-12-20T22:34:52Z

For the time being, CZO is still showing FL 226 (that is, the changes in the linked PR have not been deployed yet).

Huh — that's surprising, because IIUC those server changes went into Zulip Server 8.0. And I'd certainly expect anything released in 8.0 to have already been deployed on CZO and on Zulip Cloud before that release happened (which was at the end of last week).

In any case, it certainly seems to be deployed now. Looking at https://chat.zulip.org/api/changelog , it shows this change as FL 236, and as the latest API change deployed:

(Same story on zulip.com, i.e. Zulip Cloud.)

So this should be ripe for testing against a live server.

gnprice

Thanks @sirpengi! Two comments below.

Let's also test this PR live against a server that has this feature — either chat.zulip.org or Zulip Cloud should work, as discussed above.

gnprice · 2023-12-20T23:07:28Z

test/api/route/messages_test.dart

+      });
+    });
+
+    test('to server without read_by_sender support', () {


Conveniently, the Zulip server actually ignores extra parameters sent in a request. So we can simplify by always sending this parameter, without checking whether the server is new enough to understand it.

gnprice · 2023-12-20T23:17:43Z

lib/api/route/messages.dart

@@ -193,6 +194,7 @@ Future<SendMessageResult> sendMessage(
    'content': RawParameter(content),
    if (queueId != null) 'queue_id': queueId,
    if (localId != null) 'local_id': localId,
+    if (supportsReadBySender) 'read_by_sender': true,


Rather than hardcoding this to true within the API bindings, let's make it a parameter of this function, and then the caller passes true for it.

That way, the binding remains a thin layer exposing basically the same API as the server does itself, just in Dart. That in turn means the call sites in the application code (the code outside lib/api/) can be read more or less directly against the Zulip Server API. I think that's helpful for reducing the number of layers one needs to look through and making it easier to understand what the code is doing.

(This function does have one significant deviation from the underlying API, in the destination argument. What's happening there is basically that this part of the Zulip Server API isn't written in a way that's readily compatible with good clear static types, and so some rearrangement is needed in order to do a solid job of translating the API into Dart. But hard-coding this parameter is a different kind of matter, which is more about the specific needs and use cases we expect our application to have rather than about translating into Dart.)

sirpengi · 2023-12-21T16:39:17Z

@gnprice updated PR as per your comments and have checked this against CZO (I looked into the issue with the outdated FL and posted about it in CZO: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/zulipFeatureLevel.20out.20of.20sync )

And in fact this turns up some behavior that seems suspicious! We have a couple of parameters that we're JSON-encoding, but they're strings, and I suspect they're not meant to be JSON-encoded. That's an aspect of the Zulip Server API which is generally not properly documented: some string parameters get JSON-encoded, others passed literally. (In either case the resulting string is ultimately HTTP-form-encoded to go into the POST request body.) So the way to learn the answer will be to actually try to use this feature and see what works. For now, just leave comments to help make that debugging easier, by disclaiming the presumption that the existing code is right.

This flag allows the client to communicate to the server that the new message should be initially marked as read for its sender. Flag was added in FL 236, see: zulip/zulip#28204 Fixes: zulip#440

I think we're always going to want this parameter to be true for all the messages we send from the app. In the API bindings themselves, i.e. in lib/api/ , I generally prefer not to hard-code this sort of thing. That lets the API bindings be a faithful reflection of the Zulip Server API, with a minimum of ad-hoc specializations for the needs of our app. So that's why the `sendMessage` binding just takes the parameter from its caller. But this method PerAccountStore.sendMessage is already something specific to our app rather than an API binding; it doesn't do much now beyond wrap the API, but the reason it exists is to be the future home of our "outbox" logic. That means (a) recording the message locally so that we can display it immediately, (b) possibly retrying the send, and (c) if the send fails or times out, recording that fact so that our UI can present the user with that information and the option to retry. So that makes it a natural home for encapsulating any other logic that this app will always want when sending a message. In particular, passing this parameter.

gnprice

Thanks for the revision!

Generally this looks good; a few comments below. Some are probably easier to explain by showing rather than telling (and others are trivial, line length); so I'll fix these up myself, including a couple of additional commits, and merge.

gnprice · 2023-12-21T21:03:50Z

lib/model/store.dart

    // TODO implement outbox; see design at
    //   https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M3881.20Sending.20outbox.20messages.20is.20fraught.20with.20issues/near/1405739
-    return _apiSendMessage(connection, destination: destination, content: content);
+    return _apiSendMessage(connection, destination: destination, content: content, readBySender: readBySender);


nit: line too long

gnprice · 2023-12-21T21:10:36Z

lib/widgets/compose_box.dart

@@ -733,7 +733,7 @@ class _SendButtonState extends State<_SendButton> {

    final store = PerAccountStoreWidget.of(context);
    final content = widget.contentController.textNormalized;
-    store.sendMessage(destination: widget.getDestination(), content: content);
+    store.sendMessage(destination: widget.getDestination(), content: content, readBySender: true);


nit: line too long

gnprice · 2023-12-21T21:38:59Z

test/api/route/messages_test.dart

@@ -304,7 +304,7 @@ void main() {
    }) async {
      connection.prepare(json: SendMessageResult(id: 42).toJson());
      final result = await sendMessage(connection,
-        destination: destination, content: content);
+        destination: destination, content: content, readBySender: true);


I'd like to write this test a little differently; but probably the simplest way to express how is by just writing it. So I'll do that before merging.

(And in fact, when I go to write the style of test I'm thinking of, I discover a pre-existing behavior that's likely wrong! So I'll end up with a prep commit adding that test and a comment, then your main commit tweaked to add this new parameter to that test.)

gnprice · 2023-12-21T21:40:35Z

lib/model/store.dart

-  Future<void> sendMessage({required MessageDestination destination, required String content}) {
+  Future<void> sendMessage({required MessageDestination destination, required String content, bool? readBySender}) {


So in contrast to the API binding layer, this method on PerAccountStore I think actually is a good place to hard-code this parameter to true. I'll just add a commit on top making that change.

Here's the commit message I wrote for that commit, explaining why:

send [nfc]: Always pass readBySender true I think we're always going to want this parameter to be true for all the messages we send from the app. In the API bindings themselves, i.e. in lib/api/ , I generally prefer not to hard-code this sort of thing. That lets the API bindings be a faithful reflection of the Zulip Server API, with a minimum of ad-hoc specializations for the needs of our app. So that's why the `sendMessage` binding just takes the parameter from its caller. But this method PerAccountStore.sendMessage is already something specific to our app rather than an API binding; it doesn't do much now beyond wrap the API, but the reason it exists is to be the future home of our "outbox" logic. That means (a) recording the message locally so that we can display it immediately, (b) possibly retrying the send, and (c) if the send fails or times out, recording that fact so that our UI can present the user with that information and the option to retry. So that makes it a natural home for encapsulating any other logic that this app will always want when sending a message. In particular, passing this parameter.

sirpengi force-pushed the pr-read-by-sender-flag branch from 34454f0 to 2a04d12 Compare December 19, 2023 15:43

sirpengi mentioned this pull request Dec 20, 2023

Send hard-coded User-Agent in requests #450

Closed

gnprice reviewed Dec 20, 2023

View reviewed changes

sirpengi force-pushed the pr-read-by-sender-flag branch 3 times, most recently from d00ea21 to 6c59f1e Compare December 21, 2023 14:32

gnprice and others added 3 commits December 21, 2023 13:33

api: Add readBySender flag to sendMessage

d30383c

This flag allows the client to communicate to the server that the new message should be initially marked as read for its sender. Flag was added in FL 236, see: zulip/zulip#28204 Fixes: zulip#440

gnprice reviewed Dec 21, 2023

View reviewed changes

gnprice force-pushed the pr-read-by-sender-flag branch from 6c59f1e to 48dc619 Compare December 21, 2023 21:44

gnprice merged commit 48dc619 into zulip:main Dec 21, 2023

gnprice mentioned this pull request Dec 22, 2023

Set the HTTP user agent #406

Closed

gnprice mentioned this pull request Jan 3, 2024

Pass new read_by_sender flag when sending messages #473

Closed

sirpengi deleted the pr-read-by-sender-flag branch January 23, 2024 14:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

New `read_for_sender` flag when sending a new message #456

New `read_for_sender` flag when sending a new message #456

Uh oh!

sirpengi commented Dec 19, 2023

Uh oh!

sirpengi commented Dec 19, 2023

Uh oh!

gnprice commented Dec 20, 2023

Uh oh!

gnprice left a comment

Uh oh!

gnprice Dec 20, 2023

Uh oh!

gnprice Dec 20, 2023

Uh oh!

sirpengi commented Dec 21, 2023

Uh oh!

gnprice left a comment •

edited

Loading

Uh oh!

gnprice Dec 21, 2023

Uh oh!

gnprice Dec 21, 2023

Uh oh!

gnprice Dec 21, 2023

Uh oh!

gnprice Dec 21, 2023

Uh oh!

Uh oh!

		Future<void> sendMessage({required MessageDestination destination, required String content}) {
		Future<void> sendMessage({required MessageDestination destination, required String content, bool? readBySender}) {

New read_for_sender flag when sending a new message #456

New read_for_sender flag when sending a new message #456

Uh oh!

Conversation

sirpengi commented Dec 19, 2023

Uh oh!

sirpengi commented Dec 19, 2023

Uh oh!

gnprice commented Dec 20, 2023

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

gnprice Dec 20, 2023

Choose a reason for hiding this comment

Uh oh!

gnprice Dec 20, 2023

Choose a reason for hiding this comment

Uh oh!

sirpengi commented Dec 21, 2023

Uh oh!

gnprice left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice Dec 21, 2023

Choose a reason for hiding this comment

Uh oh!

gnprice Dec 21, 2023

Choose a reason for hiding this comment

Uh oh!

gnprice Dec 21, 2023

Choose a reason for hiding this comment

Uh oh!

gnprice Dec 21, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

New `read_for_sender` flag when sending a new message #456

New `read_for_sender` flag when sending a new message #456

gnprice left a comment •

edited

Loading