store: Add RecentDmConversationsView view-model #210

chrisbobbe · 2023-07-03T21:48:30Z

Toward #119, the list of recent DM conversations.

Inspired by how we do this in zulip-mobile, including in the tests.

gnprice

Thanks @chrisbobbe! This looks good — mainly comment-comments and small style comments below, plus one point about testing the call to notifyListeners.

In the commit message of the main commit:

store: Add RecentDmConversationsView view-model

Toward #119, the list of recent DM conversations.

Inspired by how we do this in zulip-mobile, including in the tests.

I think it'd be good to say more explicitly that much of the code was transcribed from zulip-mobile and mention the specific filename src/pm-conversations/pmConversationsModel.js. (And the tests' filename too.) If someone's reading the Git history and wants to understand how the code came to be as it is, that'll be something they want to go consult.

That also makes for a nice proof of concept: we've talked a few times about being able to effectively transcribe some kinds of code from JS to Dart in order to save work in the rewrite, and I think this is the most complex example yet where we've done so.

gnprice · 2023-07-03T22:43:33Z

lib/api/model/model.dart

+/// For docs, search for "recent_private_conversations:"
+/// in <https://zulip.com/api/register-queue>.
+@JsonSerializable(fieldRename: FieldRename.snake)
+class RecentDmConversation {


Let's put this in initial_snapshot.dart at the end of that file, I think.

Unlike users, streams, subscriptions, and messages, this shape of object really only appears in one place in the API; and it's also not an object we'll be keeping around in our data structures. So it's really quite specific to the initial snapshot.

gnprice · 2023-07-03T22:43:41Z

lib/api/model/model.dart

+  factory RecentDmConversation.fromJson(Map<String, dynamic> json) =>
+      _$RecentDmConversationFromJson(json);


nit:

Suggested change

factory RecentDmConversation.fromJson(Map<String, dynamic> json) =>

_$RecentDmConversationFromJson(json);

factory RecentDmConversation.fromJson(Map<String, dynamic> json) =>

_$RecentDmConversationFromJson(json);

gnprice · 2023-07-03T22:45:31Z

test/example_data.dart

+    zulipMergeBase: zulipMergeBase ?? recentZulipVersion,
+    alertWords: alertWords ?? ['klaxon'],
+    customProfileFields: customProfileFields ?? [],
+    recentPrivateConversations: recentPrivateConversations ?? [], // TODO add recentPrivateConversations to default


I think I'm happy to leave this empty in the default, just like we do the custom profile fields.

Like the latter, and unlike subscriptions and streams (and users), this isn't something that will be needed in the background of a wide range of tests that aren't primarily about it. Rather, the only tests that I expect will need this to be nonempty are the tests that are about this subsystem specifically, and that therefore should be explicitly specifying this data anyway.

Makes sense.

Also, I realized after making this commit—

test [nfc]: Let callers customize initial snapshot from example data This will be helpful when we add tests for the recent-DM- conversations view model, coming up.

—that I didn't actually end up using eg.initialSnapshot for the new tests. RecentDmConversationsView doesn't take an InitialSnapshot as input; it takes a List<RecentDmConversation> that's supposed to be taken from the initial snapshot, and I just built a List<RecentDmConversation> directly.

gnprice · 2023-07-03T22:48:27Z

lib/model/narrow.dart

@@ -70,6 +70,9 @@ class StreamNarrow extends Narrow {
  @override
  ApiNarrow apiEncode() => [ApiNarrowStream(streamId)];

+  @override
+  String toString() => 'StreamNarrow(streamId: $streamId)';


Suggested change

String toString() => 'StreamNarrow(streamId: $streamId)';

String toString() => 'StreamNarrow($streamId)';

This way it matches the constructor.

Also, our making the constructor's parameter positional like that reflects a judgement that it's clear this way without the name and the name would only add a bit of noise. So the same applies here.

(Similarly TopicNarrow below.)

gnprice · 2023-07-03T22:50:05Z

lib/model/narrow.dart

+  /// A [DmNarrow] from an item in [InitialSnapshot.recentPrivateConversations].
+  factory DmNarrow.ofRecentDmConversation(RecentDmConversation c, {required int selfUserId}) {
+    return DmNarrow(
+      allRecipientIds: [...c.userIds, selfUserId]..sort(),


Suggested change

allRecipientIds: [...c.userIds, selfUserId]..sort(),

allRecipientIds: [...conversation.userIds, selfUserId]..sort(),

gnprice · 2023-07-03T23:25:57Z

lib/model/recent_dm_conversations.dart

+  /// particularly in the helper _insertSorted.
+  void handleMessageEvent(MessageEvent event) {


Per my other comment above, and adapted from the corresponding zulip-mobile comment:

Suggested change

/// particularly in the helper _insertSorted.

void handleMessageEvent(MessageEvent event) {

/// particularly in the helper _insertSorted.

///

/// (*) In fact at present that's the only possible case.

/// The alternative will become possible when we have the analogue of

/// zulip-mobile's FETCH_MESSAGES_COMPLETE, with fetches done for a

/// [MessageListView] reporting their messages back via the [PerAccountStore]

/// to all our central data structures.

/// Then that can race with a new-message event: for example,

/// say we get a fetch-messages result that includes the just-sent

/// message 1002, and only after that get the event about message 1001,

/// sent moments earlier. The event queue always delivers events in order, so

/// even the race is possible only because we fetch messages outside of the

/// event queue.

void handleMessageEvent(MessageEvent event) {

Great; thanks!

gnprice · 2023-07-03T23:39:37Z

test/model/recent_dm_conversations_test.dart

+        check(setupView()
+          ..handleMessageEvent(MessageEvent(id: 1, message: eg.streamMessage())))
+          ..map.deepEquals(expected.map)


Hmm, it's a little disconcerting that these two lines appear to be two cascade calls on the same receiver, but in fact are quite separate due to that one last paren at the end of the handleMessageEvent line.

The simplest solution is probably to indent the handleMessageEvent line another two spaces, similar to how we handle if-conditions that would otherwise abut the body:

Suggested change

check(setupView()

..handleMessageEvent(MessageEvent(id: 1, message: eg.streamMessage())))

..map.deepEquals(expected.map)

check(setupView()

..handleMessageEvent(MessageEvent(id: 1, message: eg.streamMessage())))

..map.deepEquals(expected.map)

Here's a fun alternative solution I'd also be happy with:

Suggested change

check(setupView()

..handleMessageEvent(MessageEvent(id: 1, message: eg.streamMessage())))

..map.deepEquals(expected.map)

check(setupView()

..handleMessageEvent(MessageEvent(id: 1, message: eg.streamMessage()))

) ..map.deepEquals(expected.map)

Interesting; yeah. In an earlier draft I had this:

check(setupView() ..handleMessageEvent(MessageEvent(id: 1, message: eg.streamMessage())) ) ..map.deepEquals(expected.map)

but then there's that closing paren all by itself on a line. I'll go with your second proposal for my next revision.

gnprice · 2023-07-03T23:41:40Z

test/model/recent_dm_conversations_test.dart

+        // This is here mostly for checked documentation of what's in
+        // baseState, to help in reading the other test cases.


Suggested change

// This is here mostly for checked documentation of what's in

// baseState, to help in reading the other test cases.

// This is here mostly for checked documentation of what

// setupView returns, to help in reading the other test cases.

gnprice · 2023-07-03T23:43:59Z

test/model/recent_dm_conversations_test.dart

+          ..sorted.deepEquals([key([1]), key([1, 2])]);
+      });
+
+      test('stream message -> do nothing', () {


In addition to checking map and sorted, the other thing that would be nice to check here is that notifyListeners didn't get called. (I think this should be straightforward by registering a listener.)

That's the analogue of the expect(state).toBe(baseState); on the zulip-mobile side, in the persistent-data-structures world where object identity is the signal that nothing has changed.

Hmm and in fact, the tests should definitely check the inverse fact: that notifyListeners is indeed called in the cases where things do change. A bug in that direction would be a nasty one.

gnprice · 2023-07-03T23:52:58Z

test/model/recent_dm_conversations_test.dart

+          ..sorted.deepEquals([key([1]), key([1, 2])]);
+      });
+
+      test('existing conversation, not newest in conversation', () {


(This case can leave out a check as to whether notifyListeners is called; it doesn't need to be because nothing changed, and in zulip-mobile we return the old state which is the equivalent of not calling it, but in this implementation it does get called, and that's fine because this should be a rare case and there's really no need for optimizations on it.)

chrisbobbe · 2023-07-04T01:10:04Z

Thanks for the review! Revision pushed.

gnprice

Thanks! All looks good modulo two nits below.

gnprice · 2023-07-04T01:26:33Z

test/model/recent_dm_conversations_test.dart

+        // This is here mostly for checked documentation of what's in
+        // baseState, to help in reading the other test cases.


gnprice · 2023-07-04T01:27:33Z

lib/model/recent_dm_conversations.dart

+import '../api/model/events.dart';
+import 'narrow.dart';
+
+/// A view-model for the recent DM conversations UI.


nit: I think this parses better with some hyphens in that attributive compound noun:

Suggested change

/// A view-model for the recent DM conversations UI.

/// A view-model for the recent-DM-conversations UI.

chrisbobbe · 2023-07-04T17:35:25Z

Thanks! Revision pushed.

Technically maxFileUploadSizeMib is for the whole server, I think, but it seems fine to treat it as realm info for the purpose of ordering these. See Greg's bulleted list here: zulip#183 (review)

This will be helpful for test-failure output, particularly with DmNarrows as we start using those as Map keys, soon. Of our current Narrow subclasses, this just leaves AllMessagesNarrow without a custom `toString` implementation. That's OK, because the default `toString` already says "AllMessagesNarrow", which is all we need to know about the narrow.

Toward zulip#119, the list of recent DM conversations. Much of this code was transcribed from zulip-mobile; in particular, from: src/pm-conversations/pmConversationsModel.js src/pm-conversations/__tests__/pmConversationsModel-test.js

gnprice · 2023-07-05T22:17:52Z

Thanks! LGTM; merging.

chrisbobbe added a-model Implementing our data model (PerAccountStore, etc.) a-home The home screens of the app; finding and starting conversations labels Jul 3, 2023

chrisbobbe requested a review from gnprice July 3, 2023 21:48

gnprice reviewed Jul 4, 2023

View reviewed changes

chrisbobbe force-pushed the pr-recent-dm-conversations-view branch from 2c29cdd to 88ac528 Compare July 4, 2023 01:09

gnprice approved these changes Jul 4, 2023

View reviewed changes

chrisbobbe force-pushed the pr-recent-dm-conversations-view branch from 88ac528 to cc4e929 Compare July 4, 2023 17:34

chrisbobbe added 8 commits July 5, 2023 15:17

store [nfc]: Reorder fields to put realm info together

b75dc7d

Technically maxFileUploadSizeMib is for the whole server, I think, but it seems fine to treat it as realm info for the purpose of ordering these. See Greg's bulleted list here: zulip#183 (review)

store [nfc]: Add some one-line headings for PerAccountStore data fields

d7af63e

test [nfc]: Let callers customize initial snapshot from example data

68d30ee

api: Add recentPrivateConversations list in initial snapshot

8a7d664

deps: Add collection as direct dependency

ac11ef8

test [nfc]: Allow passing id in eg.{stream,dm}Message

a7e3529

store: Add RecentDmConversationsView view-model

3550d87

Toward zulip#119, the list of recent DM conversations. Much of this code was transcribed from zulip-mobile; in particular, from: src/pm-conversations/pmConversationsModel.js src/pm-conversations/__tests__/pmConversationsModel-test.js

gnprice force-pushed the pr-recent-dm-conversations-view branch from cc4e929 to 3550d87 Compare July 5, 2023 22:17

gnprice merged commit 3550d87 into zulip:main Jul 5, 2023

chrisbobbe deleted the pr-recent-dm-conversations-view branch July 5, 2023 22:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

store: Add RecentDmConversationsView view-model #210

store: Add RecentDmConversationsView view-model #210

chrisbobbe commented Jul 3, 2023

gnprice left a comment

gnprice Jul 3, 2023

gnprice Jul 3, 2023

gnprice Jul 3, 2023

chrisbobbe Jul 4, 2023

gnprice Jul 3, 2023

gnprice Jul 3, 2023

gnprice Jul 3, 2023

chrisbobbe Jul 4, 2023

gnprice Jul 3, 2023

chrisbobbe Jul 4, 2023

gnprice Jul 3, 2023

gnprice Jul 4, 2023

gnprice Jul 3, 2023

gnprice Jul 3, 2023

gnprice Jul 3, 2023

chrisbobbe commented Jul 4, 2023

gnprice left a comment

gnprice Jul 4, 2023

gnprice Jul 4, 2023

chrisbobbe commented Jul 4, 2023

gnprice commented Jul 5, 2023

		factory RecentDmConversation.fromJson(Map<String, dynamic> json) =>
		_$RecentDmConversationFromJson(json);

	String toString() => 'StreamNarrow(streamId: $streamId)';
	String toString() => 'StreamNarrow($streamId)';

	allRecipientIds: [...c.userIds, selfUserId]..sort(),
	allRecipientIds: [...conversation.userIds, selfUserId]..sort(),

		/// particularly in the helper _insertSorted.
		void handleMessageEvent(MessageEvent event) {

-  /// particularly in the helper _insertSorted.
-  void handleMessageEvent(MessageEvent event) {
+  /// particularly in the helper _insertSorted.
+  ///
+  /// (*) In fact at present that's the only possible case.
+  /// The alternative will become possible when we have the analogue of
+  /// zulip-mobile's FETCH_MESSAGES_COMPLETE, with fetches done for a
+  /// [MessageListView] reporting their messages back via the [PerAccountStore]
+  /// to all our central data structures.
+  /// Then that can race with a new-message event: for example,
+  /// say we get a fetch-messages result that includes the just-sent
+  /// message 1002, and only after that get the event about message 1001,
+  /// sent moments earlier.  The event queue always delivers events in order, so
+  /// even the race is possible only because we fetch messages outside of the
+  /// event queue.
+  void handleMessageEvent(MessageEvent event) {

		// This is here mostly for checked documentation of what's in
		// baseState, to help in reading the other test cases.

	/// A view-model for the recent DM conversations UI.
	/// A view-model for the recent-DM-conversations UI.

store: Add RecentDmConversationsView view-model #210

store: Add RecentDmConversationsView view-model #210

Conversation

chrisbobbe commented Jul 3, 2023

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

Choose a reason for hiding this comment

chrisbobbe commented Jul 4, 2023

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

chrisbobbe commented Jul 4, 2023

gnprice commented Jul 5, 2023