Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle UpdateMessageEvent in MessageListView (#118) #238

Merged

Conversation

oxling
Copy link
Contributor

@oxling oxling commented Jul 24, 2023

This propagates the UpdateMessageEvent to the MessageListView, allowing you to see changes to your messages in real time.

Putting this up as a draft, since I have a few questions - leaving my comments inline.

Fixes: #118

lib/model/message_list.dart Outdated Show resolved Hide resolved
lib/model/message_list.dart Outdated Show resolved Hide resolved
lib/model/message_list.dart Outdated Show resolved Hide resolved
lib/model/message_list.dart Outdated Show resolved Hide resolved
@oxling oxling force-pushed the feature/adyer/118/handle-UpdateMessageEvent branch from 4e3e99b to 456c7e4 Compare July 24, 2023 18:34
lib/model/message_list.dart Outdated Show resolved Hide resolved
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @oxling! See replies below.

lib/model/message_list.dart Outdated Show resolved Hide resolved
lib/model/message_list.dart Outdated Show resolved Hide resolved
lib/model/message_list.dart Outdated Show resolved Hide resolved
lib/model/message_list.dart Outdated Show resolved Hide resolved
lib/api/model/model.dart Outdated Show resolved Hide resolved
lib/model/message_list.dart Outdated Show resolved Hide resolved
lib/model/message_list.dart Outdated Show resolved Hide resolved
lib/model/message_list.dart Outdated Show resolved Hide resolved
@gnprice
Copy link
Member

gnprice commented Jul 24, 2023

(Also, just to be explicit: we'll want some unit tests for this change. Perfectly reasonable to have left them out when it's a draft, though.)

@oxling
Copy link
Contributor Author

oxling commented Jul 24, 2023

(Also, just to be explicit: we'll want some unit tests for this change. Perfectly reasonable to have left them out when it's a draft, though.)

Will do before I mark this as ready!

@oxling oxling force-pushed the feature/adyer/118/handle-UpdateMessageEvent branch 4 times, most recently from 8e034c4 to c952a6a Compare July 25, 2023 15:21
lib/model/message_list.dart Outdated Show resolved Hide resolved
lib/model/message_list.dart Outdated Show resolved Hide resolved
lib/model/message_list.dart Outdated Show resolved Hide resolved
@oxling oxling force-pushed the feature/adyer/118/handle-UpdateMessageEvent branch from c952a6a to 0b1209e Compare July 25, 2023 15:24
@oxling
Copy link
Contributor Author

oxling commented Jul 25, 2023

OK, this is all of the requested changes, except for the unit test. I just threw a message in #mobile-team about doing that - I'm a little stuck on how to proceed getting that test stubbed out correctly.

@oxling oxling force-pushed the feature/adyer/118/handle-UpdateMessageEvent branch from 0b1209e to 6c3d733 Compare July 25, 2023 19:43
@oxling oxling marked this pull request as ready for review July 25, 2023 19:44
@oxling oxling force-pushed the feature/adyer/118/handle-UpdateMessageEvent branch from 6c3d733 to 832fb98 Compare July 26, 2023 01:18
@oxling oxling requested a review from gnprice July 26, 2023 01:19
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @oxling! Comments below.

lib/model/message_list.dart Outdated Show resolved Hide resolved
lib/model/message_list.dart Outdated Show resolved Hide resolved
lib/model/message_list.dart Outdated Show resolved Hide resolved
lib/model/message_list.dart Show resolved Hide resolved
lib/model/message_list.dart Outdated Show resolved Hide resolved
test/model/message_list_test.dart Outdated Show resolved Hide resolved
test/model/message_list_test.dart Outdated Show resolved Hide resolved
test/model/message_list_test.dart Outdated Show resolved Hide resolved
test/model/message_list_test.dart Outdated Show resolved Hide resolved
test/model/message_list_test.dart Outdated Show resolved Hide resolved
@oxling oxling force-pushed the feature/adyer/118/handle-UpdateMessageEvent branch 5 times, most recently from 90257cf to 5629234 Compare July 26, 2023 20:12
@oxling oxling force-pushed the feature/adyer/118/handle-UpdateMessageEvent branch 3 times, most recently from a045cc7 to c339c0a Compare July 26, 2023 20:52
@gnprice
Copy link
Member

gnprice commented Jul 26, 2023

Pardon all of my force pushes here - sorry, this would have made more sense to make individual edits and then squash them all before merging.

Yeah, that'd be a very reasonable workflow — but this force-push workflow is actually our usual one, so it's no problem at all.

That's because we often have a PR that should end up as several commits, e.g. because it make preparatory refactors, or adds a feature in one place and then another feature that builds on that. So in order to support those, our usual workflow is that each revision is a new draft of the whole final sequence of commits to merge, which means the author squashes edits in.

That does cause GitHub to be less helpful about comparing different revisions. The reviewer instead relies on having fetched a local copy, and we have a couple of scripts to help make that easy.

@oxling
Copy link
Contributor Author

oxling commented Jul 26, 2023

Pardon all of my force pushes here - sorry, this would have made more sense to make individual edits and then squash them all before merging.

Yeah, that'd be a very reasonable workflow — but this force-push workflow is actually our usual one, so it's no problem at all.

That's because we often have a PR that should end up as several commits, e.g. because it make preparatory refactors, or adds a feature in one place and then another feature that builds on that. So in order to support those, our usual workflow is that each revision is a new draft of the whole final sequence of commits to merge, which means the author squashes edits in.

That does cause GitHub to be less helpful about comparing different revisions. The reviewer instead relies on having fetched a local copy, and we have a couple of scripts to help make that easy.

Ah, interesting. Normally I use a lot of Github's "since you last checked" feature to do reviews.

@oxling oxling force-pushed the feature/adyer/118/handle-UpdateMessageEvent branch 2 times, most recently from 0b91ab9 to 5df994d Compare July 26, 2023 22:20
@oxling oxling changed the title #118: Handle UpdateMessageEvent in MessageListView msglist: Handle UpdateMessageEvent in MessageListView (#118) Jul 26, 2023
@oxling oxling changed the title msglist: Handle UpdateMessageEvent in MessageListView (#118) Handle UpdateMessageEvent in MessageListView (#118) Jul 26, 2023
@oxling oxling force-pushed the feature/adyer/118/handle-UpdateMessageEvent branch from 5df994d to 2925113 Compare July 26, 2023 22:30
@oxling oxling requested a review from gnprice July 26, 2023 22:32
@oxling
Copy link
Contributor Author

oxling commented Jul 26, 2023

OK, assuming I got the test state issue sorted out correctly, this is ready for round three!

@oxling oxling force-pushed the feature/adyer/118/handle-UpdateMessageEvent branch from 2925113 to d6144bb Compare July 26, 2023 22:34
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @oxling for the revision!

I think at this point the behavior all looks good. Some substantive comments about the tests, as well as various comments for clarity and style; see details below.

test/model/message_list_test.dart Show resolved Hide resolved
final mockMessage = eg.streamMessage(id: 972, stream: stream, content: oldContent);
final messageList = await messageListViewWithMessages([mockMessage], store, narrow);

final updateEvent = UpdateMessageEvent(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's also include an explicit renderingOnly: null in this test event, since that's a key part of the data this test case wants.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah my assumption was that for older server versions, it will omit the renderingOnly key entirely.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah. Yeah, the server will leave it out of its response — old servers don't know about anything named rendering_only. But then in the UpdateMessageEvent object that we deserialize from the server's JSON, that class has a field renderingOnly with type bool?:

  final bool? renderingOnly; // TODO(server-5)

which means the object will always have some value in a spot called renderingOnly, and that value is of type bool? — which means it's either null or a value of type bool.

So when we deserialize the JSON and there's no rendering_only key in that JSON object, the value we use for the renderingOnly field in the resulting UpdateMessageEvent is naturally null.

This does mean that if a key is present in some JSON object but with a value of null, we usually don't distinguish that from the key being absent — either one will deserialize to a field with value null. This has been fine, because it's rare for an API to have both those possibilities for a given key and have them mean different things. (Certainly it's rare in the Zulip API, and I'd expect it to be rare in sane APIs generally.) There's only one such instance I can think of in the Zulip API; here's discussion on how we expect to handle it:
https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/flutter.3A.20omitted.20vs.2E.20null.20in.20JSON/near/1551522

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this test, if there weren't a way to express explicitly in the code that this event came without a (non-null) rendering_only, I'd want to still make it explicit by using a comment — like

        // renderingOnly omitted

at the spot where the renderingOnly: true, (or whatever) would have appeared.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bump

Comment on lines 70 to 76
//Before the first element
var idx = messageList.findMessageWithId(1);
check(idx).equals(-1);

//Is the first element
idx = messageList.findMessageWithId(2);
check(idx).equals(0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this test's code can become easier to read by making it a lot more compact — one line per check, rather than four. Like so:

      check(messageList.findMessageWithId(1)).equals(-1);  //Before the first element
      check(messageList.findMessageWithId(2)).equals(0);  //Is the first element

but really at that point I think the comments become unnecessary, so:

Suggested change
//Before the first element
var idx = messageList.findMessageWithId(1);
check(idx).equals(-1);
//Is the first element
idx = messageList.findMessageWithId(2);
check(idx).equals(0);
check(messageList.findMessageWithId(1)).equals(-1);
check(messageList.findMessageWithId(2)).equals(0);

(Unnecessary because now the query arguments are nicely aligned next to each other, and also all not too many lines after the setup of the list of messages so the eye can easily look back and forth between those.)

Comment on lines 141 to 123
final updatedMessage = messageList.messages[0];
check(updatedMessage).identicalTo(message);
check(listenersNotified).equals(true);

check(message)
..content.equals(newContent)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The listeners-notified check feels like it's interrupting here between a group of checks that are more closely related to each other. I think it becomes easier to see what's being checked if the listenersNotified check comes first (or at the end might work too).

Then we can also group together the updated-message checks more closely by making them a single check call:

Suggested change
final updatedMessage = messageList.messages[0];
check(updatedMessage).identicalTo(message);
check(listenersNotified).equals(true);
check(message)
..content.equals(newContent)
check(listenersNotified).equals(true);
check(messageList.messages[0])
..identicalTo(message)
..content.equals(newContent)

Comment on lines 181 to 162
check(message).content.equals(oldContent);

});
test('rendering-only update does not change timestamp', () async {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nits: blank line separating these two multi-line calls; and generally no blank line at end of a block of statements (like the function-expression body that's ending here):

Suggested change
check(message).content.equals(oldContent);
});
test('rendering-only update does not change timestamp', () async {
check(message).content.equals(oldContent);
});
test('rendering-only update does not change timestamp', () async {

Comment on lines 127 to 106
userId: userId
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: use trailing comma

Suggested change
userId: userId
);
userId: userId,
);

Comment on lines 54 to 55
const narrow = StreamNarrow(streamId);
final stream = eg.stream(streamId: streamId);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can avoid needing a magic-number constant for the stream ID by putting these in the opposite order, to make the stream object available:

Suggested change
const narrow = StreamNarrow(streamId);
final stream = eg.stream(streamId: streamId);
final stream = eg.stream();
final narrow = StreamNarrow(stream.streamId);

Comment on lines 196 to 197
messageId: 972,
messageIds: [972],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the approach you took in one of the test cases above: instead of writing out a magic number as message ID that crucially happens to be the same number as used in the original message, refer directly to the original message object and its id property. That helps make it clear that the point is it's the same message.

Comment on lines 164 to 165
messageId: 972,
messageIds: [972],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conversely, here the point is that it's not the same message ID, and it'd be good to make that explicit too.

One way to do that would be something like:

      messageId: message.id + 1,

test/model/message_list_test.dart Show resolved Hide resolved
@oxling oxling force-pushed the feature/adyer/118/handle-UpdateMessageEvent branch from d6144bb to 46e7b7b Compare July 28, 2023 21:05
@oxling
Copy link
Contributor Author

oxling commented Jul 28, 2023

OK, addressed the last round of changes!

@oxling oxling force-pushed the feature/adyer/118/handle-UpdateMessageEvent branch from 46e7b7b to c463d5d Compare July 28, 2023 21:06
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @oxling for the revision! This is quite close now; just a handful of small comments below.

@@ -1,5 +1,7 @@
import 'package:collection/collection.dart';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The analyzer now complains this import is unused. (In VS Code, you should be getting a corresponding warning — a yellow squiggle here on the code, and yellow highlighting on the filename.)

Comment on lines 114 to 111
}

}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: stray blank line

Suggested change
}
}
}
}

Comment on lines 94 to 93
// not the user.

// TODO(server-5): Cut this fallback; rely on renderingOnly from FL 114
final isRenderingOnly = event.renderingOnly ?? (event.userId == null);

if (event.editTimestamp != null && !isRenderingOnly) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's join these as one stanza:

Suggested change
// not the user.
// TODO(server-5): Cut this fallback; rely on renderingOnly from FL 114
final isRenderingOnly = event.renderingOnly ?? (event.userId == null);
if (event.editTimestamp != null && !isRenderingOnly) {
// not the user.
//
// TODO(server-5): Cut this fallback; rely on renderingOnly from FL 114
final isRenderingOnly = event.renderingOnly ?? (event.userId == null);
if (event.editTimestamp != null && !isRenderingOnly) {

so that the remaining blank lines in the function delimit the updates to the various fields from each other.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(And I guess part of this corresponds to my previous comment at https://github.com/zulip/zulip-flutter/pull/238/files#r1274275108 .)

Comment on lines 147 to 143
final idx = findMessageWithId(event.messageId);

if (idx == -1) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: let's group these as one stanza:

Suggested change
final idx = findMessageWithId(event.messageId);
if (idx == -1) {
final idx = findMessageWithId(event.messageId);
if (idx == -1) {

Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, and there are a few items from the previous round of review which are still open; see below.

final mockMessage = eg.streamMessage(id: 972, stream: stream, content: oldContent);
final messageList = await messageListViewWithMessages([mockMessage], store, narrow);

final updateEvent = UpdateMessageEvent(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bump

Comment on lines 94 to 93
// not the user.

// TODO(server-5): Cut this fallback; rely on renderingOnly from FL 114
final isRenderingOnly = event.renderingOnly ?? (event.userId == null);

if (event.editTimestamp != null && !isRenderingOnly) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(And I guess part of this corresponds to my previous comment at https://github.com/zulip/zulip-flutter/pull/238/files#r1274275108 .)

message.flags = event.flags;

if (event.renderedContent != null) {
assert(message.contentType == 'text/html', "Expected message to have contentType 'text/html'. Instead, got ${message.contentType}");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will have quotes around the expected value, but not around the actual value. That seems confusing; better to have quotes around either neither or both.

Also let's break this onto two lines, because the line is quite long. In my previous comment I included a suggested version which did that:
#238 (comment)

oxling and others added 3 commits August 3, 2023 12:56
Processes an UpdateMessageEvent and hands it off to the
MessageListView to update, if the message is visible in the
MessageListView.

Fixes: zulip#118
Also put them consistently in the terminology found in the API doc.
@gnprice gnprice force-pushed the feature/adyer/118/handle-UpdateMessageEvent branch from c463d5d to 1fc230f Compare August 3, 2023 20:14
@gnprice gnprice merged commit 1fc230f into zulip:main Aug 3, 2023
@gnprice
Copy link
Member

gnprice commented Aug 3, 2023

OK, merged with those tweaks. I also included a comment-only change on top, as well as a small comment-only change elsewhere that that reminded me of the need for. (Generally our practice is that comment-only changes don't require code review; if that other change affected more than comments, I'd have sent it as a separate PR instead.) Thanks again @oxling for your work on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle edited messages (update_message events, 1/2)
2 participants