api: Add update_message_flags events #299

chrisbobbe · 2023-09-07T19:26:51Z

We'll need to handle these events in the upcoming unread-messages model, for #253.

This PR also includes a draft commit:

wip msglist: Handle update_message_flags events in model

That would be helpful toward #79, but I'm really unsure about the efficiency of using _findMessageWithId (binary search) in this hot codepath. (The most common call will be for messages getting marked as read on scroll.) If that turns out to be OK, I can give that commit the tests it needs, and unmark as draft.

gnprice

Thanks! Comments below just on the algorithmic question about the second commit; haven't yet read the first commit.

gnprice · 2023-09-07T20:01:40Z

lib/model/message_list.dart

+      for (final messageId in event.messages) {
+        // TODO(perf) could be more efficient?
+        final index = _findMessageWithId(messageId);


I think this algorithm is fine. For marking $k$ messages, with $n$ in the message list, it will take $k \log_2 n$ steps to find them all (or respectively determine they're not present). Even if there are thousands of messages in this message list, that's no more than 10-15 steps each (and 15 would be really a lot of messages, over 32k of them).

I think the most efficient possible with these data structures is probably $k \log_2 (n/k)$, and the complication doesn't seem worth it.

We could make it linear in $k$ with more data structures. But that can be out of scope here.

gnprice · 2023-09-07T20:04:09Z

lib/model/message_list.dart

+      messagesToProcess = [];
+      for (final messageId in event.messages) {
+        // TODO(perf) could be more efficient?
+        final index = _findMessageWithId(messageId);
+        if (index != -1) {
+          messagesToProcess.add(messages[index]);


The algorithmic comment I'd make on a different axis, though, is that I'd rather not build up this list. Instead, this loop can go ahead and do the add/remove of flags itself. Can have a boolean local variable that says if it's add or remove (from looking once at the type of the event), and condition on that inside the loop.

gnprice

OK, and read the first commit too. Generally looks great; one comment below.

gnprice · 2023-09-07T23:44:27Z

lib/api/model/events.dart

+  final List<int>? userIds;
+  final int? streamId;
+  final String? topic;


I think we'll probably want a crunchy shell on ensuring that when type is private we always have userIds, and when it's stream we always have streamId and topic.

That is, in zulip-flutter I think we've been pretty successful in making it so that malformed server data can't make us outright crash or misbehave, and instead just interrupts talking to the server (thanks mostly to the autogenerated fromJson implementations), so I'd like to try to hold the line on that as an invariant. And I suspect that doing so in the code that consumes this data, if it's just three independent nullables like this, will be annoying and make that code hard to read.

One way to get the crunchy shell would be to have a base class and two subclasses, akin to Message itself. But another solution might be more expedient and would also suffice: just have our fromJson check those conditions, and throw if they're not met, in much the same way as the autogenerated code does when an expected field is missing. For example our fromJson can take a few lines to do that on the Map, before then calling the autogenerated implementation as usual.

The downside of that approach is that the consuming code is slightly uglier, with ! operators scattered around, than it would be with subclasses. But I think that's a good trade because there's not that much consuming code for this type, and the subclasses mean significantly more code here at the definitions. (For Message the tradeoff goes heavily the other way, because that's such a fundamental type and so much of our code consumes it.)

For example our fromJson can take a few lines to do that on the Map, before then calling the autogenerated implementation as usual.

How about calling the autogenerated implementation first? Then we can do an exhaustive switch on type, which will be a MessageType enum value.

chrisbobbe · 2023-09-12T05:01:31Z

Thanks for the review! Revision pushed.

chrisbobbe · 2023-09-12T19:55:55Z

Just updated to do crunchy-shell validation that [UpdateMessageFlagsRemoveEvent.messageDetails] is present when it's supposed to be.

gnprice · 2023-09-12T20:47:01Z

lib/model/message_list.dart

+    bool didUpdateAny = false;
+    if (event.all) {
+      for (final message in messages) {
+        (isAdd ? message.flags.add : message.flags.remove).call(event.flag);


It looks like call is dynamically typed. If for example I replace remove with join here — a random other method that should take a different type of argument — then there's no error.

Can clean up by expanding things to be just slightly more explicit:

isAdd ? message.flags.add(event.flag) : message.flags.remove(event.flag);

which also has the virtue of looking more like conventional function calls we have in the rest of our code.

gnprice · 2023-09-12T20:48:41Z

lib/model/message_list.dart

+    if (event.all) {
+      for (final message in messages) {
+        (isAdd ? message.flags.add : message.flags.remove).call(event.flag);
+        didUpdateAny |= true;


nit: this can be simplified to a plain assignment, which is equivalent:

Suggested change

didUpdateAny |= true;

didUpdateAny = true;

gnprice · 2023-09-12T20:53:23Z

lib/api/model/events.dart

+  final List<int>? userIds;
+  final int? streamId;
+  final String? topic;


gnprice · 2023-09-12T20:54:39Z

Thanks for the revision!

Two comments above. Let's also have tests for the API code, since there's now nontrivial non-autogenerated logic in it… oh and also tests on the model code in the second commit 🙂

chrisbobbe · 2023-09-13T20:56:36Z

Oh right! I see I mentioned those in the PR description but forgot to follow through, thanks for the bump. 🙂

chrisbobbe · 2023-09-14T00:22:40Z

Pushed a revision with both those sets of tests.

gnprice

Thanks for the revision! Sorry for the delay in review.

All looks good except a few small comments below. Please merge at will modulo those.

gnprice · 2023-09-26T02:33:16Z

test/api/model/events_test.dart

    check(mkEvent([MessageFlag.read])).message.flags.deepEquals([MessageFlag.read]);
  });

+  test('update_message_flags/remove: require messageDetails in mark-as-unread', () {


nit: put the tests in the same order as the code they test

gnprice · 2023-09-26T02:37:07Z

test/api/model/events_test.dart

+    check(() => UpdateMessageFlagsRemoveEvent.fromJson(json)).returnsNormally();
+    json['flag'] = 'read';
+    check(() => UpdateMessageFlagsRemoveEvent.fromJson(json)).throws();
+    json['message_details'] = {
+      '123': {'type': 'private', 'mentioned': false, 'user_ids': [2]}
+    };
+    check(() => UpdateMessageFlagsRemoveEvent.fromJson(json)).returnsNormally();


This test looks good except that seeing the re-use of the json object makes me just a bit worried that a fromJson implementation might mutate the data it's passed, and confuse the test. (I'm not sure we don't already have such a fromJson somewhere.)

Something like { ...baseJson, 'flag': 'read' } would leave me reassured. (In principle I guess the messages array could be mutated too, but that seems less likely and I wouldn't worry.)

gnprice · 2023-09-26T02:38:20Z

test/model/message_list_test.dart

+    UpdateMessageFlagsAddEvent mkAddEvent(
+      MessageFlag flag,
+      List<int> messageIds, {
+      bool all = false


nit:

Suggested change

bool all = false

bool all = false,

After this, we'll be able to show a message's flags in the UI, such as unread markers (zulip#79).

chrisbobbe · 2023-09-27T08:40:16Z

Thanks! Merged, with those tweaks.

gnprice · 2023-09-28T00:14:59Z

test/api/model/events_test.dart

+    check(() => UpdateMessageFlagsRemoveEvent.fromJson({
+      ...baseJson, 'message_details': {'123': {'type': 'private', 'mentioned': false, 'user_ids': [2]}},
+    })).returnsNormally();


This test should set 'flag': 'read' too, right?

Eep, thanks! 89fd1f7

Thanks to Greg for catching this: #299 (comment)

chrisbobbe added a-api Implementing specific parts of the Zulip server API a-model Implementing our data model (PerAccountStore, etc.) labels Sep 7, 2023

chrisbobbe requested a review from gnprice September 7, 2023 19:26

gnprice reviewed Sep 7, 2023

View reviewed changes

chrisbobbe force-pushed the pr-update-message-flags-events branch from 80fc7e9 to 59f24e9 Compare September 12, 2023 05:01

chrisbobbe marked this pull request as ready for review September 12, 2023 05:01

chrisbobbe force-pushed the pr-update-message-flags-events branch from 59f24e9 to 82f7876 Compare September 12, 2023 19:55

gnprice reviewed Sep 12, 2023

View reviewed changes

chrisbobbe force-pushed the pr-update-message-flags-events branch from 82f7876 to d8a3ea0 Compare September 14, 2023 00:22

chrisbobbe mentioned this pull request Sep 14, 2023

model: Add Unreads model, for tracking unread-message counts #304

Merged

gnprice reviewed Sep 26, 2023

View reviewed changes

chrisbobbe force-pushed the pr-update-message-flags-events branch from d8a3ea0 to ea173e4 Compare September 27, 2023 08:38

chrisbobbe added 5 commits September 27, 2023 01:39

events test [nfc]: Put tests in the same order as the code they test

5d2e6fd

api: Add update_message_flags events

fbe72e8

test: Add ListChecks to stdlib_checks.dart, with [] operator

4568db8

test: Have streamMessage and dmMessage take MessageFlag objects

68fe7ba

msglist: Handle update_message_flags events in model

8da16f1

After this, we'll be able to show a message's flags in the UI, such as unread markers (zulip#79).

chrisbobbe force-pushed the pr-update-message-flags-events branch from ea173e4 to 8da16f1 Compare September 27, 2023 08:39

chrisbobbe merged commit 8da16f1 into zulip:main Sep 27, 2023

chrisbobbe deleted the pr-update-message-flags-events branch September 27, 2023 08:40

gnprice reviewed Sep 28, 2023

View reviewed changes

chrisbobbe added a commit that referenced this pull request Sep 28, 2023

events tests: Fix wrong test input

89fd1f7

Thanks to Greg for catching this: #299 (comment)

api: Add update_message_flags events #299

api: Add update_message_flags events #299

Uh oh!

Conversation

chrisbobbe commented Sep 7, 2023

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Sep 12, 2023

Uh oh!

chrisbobbe commented Sep 12, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice commented Sep 12, 2023

Uh oh!

chrisbobbe commented Sep 13, 2023

Uh oh!

chrisbobbe commented Sep 14, 2023

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Sep 27, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants