local echo (3/n): Pull out MessageBase

[PIG208]

Related:

• Work toward Simplified local echo, with retry #1441
• Prep for local echo (n/n): Support simplified version of local echo #1453

[gnprice]

I feel like the name "displayable message" doesn't really capture what this is about — in particular the term "displayable" doesn't much help in saying how this relates to the Message class.

A good name would be BaseMessage. Here's also some revised dartdoc:

Suggested change

	/// A common class for messages that can appear in a [MessageList].
	abstract class DisplayableMessage<T extends MessageDestination> {
	/// A message or message-like object, for showing in a message list.
	///
	/// Other than [Message], we use this for "outbox messages",
	/// representing outstanding [sendMessage] requests.
	abstract class BaseMessage<T extends MessageDestination> {

Since you have a whole series of commits that use this name, doing the rename in a manual way might be a pain. Here's a command to do it automatically and I think quite painlessly:

$ FILTER_BRANCH_SQUELCH_WARNING=1 git filter-branch \
    --tree-filter 'perl -i -0pe s/DisplayableMessage/MessageBase/g $(git ls-files lib test)' \
    @~4..
Rewrite 282632f9fbafacfc53fa3629be22733ccfe7a791 (1/4) (0 seconds passed, remaining 0 predicted)
Rewrite 1e324588d6bf5791a50e2cd65d4175f4a903e3b0 (2/4) (1 seconds passed, remaining 1 predicted)
Rewrite 9b9265bfd4292728dfdabdea5756b8f1c3b4fd58 (2/4) (1 seconds passed, remaining 1 predicted)
Rewrite be02a05bdff9b4bc1f02a50ed2e4513efb92f305 (2/4) (1 seconds passed, remaining 1 predicted)

Ref 'refs/heads/main' was rewritten

Definitely use git diff --stat -p @{1} and git range-diff origin @{1} @ to review the results.

The reason for that environment variable at the start of the command is that git filter-branch is deprecated these days. When I tried the command without it, I got:

WARNING: git-filter-branch has a glut of gotchas generating mangled history
	 rewrites.  Hit Ctrl-C before proceeding to abort, then use an
	 alternative filtering tool such as 'git filter-repo'
	 (https://github.com/newren/git-filter-repo/) instead.  See the
	 filter-branch manual page for more details; to squelch this warning,
	 set FILTER_BRANCH_SQUELCH_WARNING=1.

The warning is accurate. But this rewrite is simple enough that I don't think any gotchas apply; and it's small enough you can review the results to confirm that. Given that that's the case, and that I already know how to use git filter-branch, I went with just squelching the warning.

If we needed to do something more complex, I would invest the time to do it with the newer out-of-tree tool git filter-repo. (And if it were a big job, I'd also be sure to ask Anders if he had any other recommendations.)

[gnprice]

Or perhaps better: MessageBase. This type alone isn't yet even a message; it's just the base for a message.

[PIG208]

Yeah. Thanks for the script! I guess one other thing filter-branch is capable of is rewriting the commit messages, but that's as fine to do manually.

One other thing I'm thinking of renaming is destination. The Zulip API uses to for send-message, Because this adds a level of nesting, perhaps shortening it to two letters will be more readable? So it might be used like message.to.streamId, message.to.userIds or final destination = message.to. But we don't seem to do that in general when naming things.

[gnprice]

Seeing this change and many of the other instances of this change, it makes me nervous that we now have something that's vague about exactly which user IDs are included and we're expecting it to match up with something that's specifically all the recipient IDs.

As a comment on DmNarrow says, Zulip has many ways of representing a DM conversation. In particular the Zulip server API has at least four different conventions for whether the self-user is included in the list. (Always; never; only for self-1:1; only for self-1:1 and groups.) This was a recurring source of bugs and confusion in zulip-mobile, until I invested substantial effort in cleaning it up with the abstractions found now in src/utils/recipient.js. That's the reason that DmMessage and DmNarrow have the very explicitly-named allRecipientIds, as well as DmNarrow.otherRecipientIds for when that's desired.

So I'd like to find a solution that preserves that explicitness about the semantics of each list of users.

I think the key here is that MessageDestination isn't quite the right class to be using for this purpose. Its doc says:

/// Which conversation to send a message to, in [sendMessage].
///
/// This is either a [StreamDestination] or a [DmDestination].
sealed class MessageDestination {

Similarly the two subclasses say things like:

/// A DM conversation, for specifying to [sendMessage].
///
/// The server accepts a list of Zulip API emails as an alternative to
/// a list of user IDs, but this binding currently doesn't.
class DmDestination extends MessageDestination {

So they're specifically about [sendMessage] — their job is to describe a parameter in a request to that endpoint. The information one says in [sendMessage] about where a message should go naturally has a lot in common with the information the server says in a [getMessages] or [getEvents] response about where a message did go… but it's also natural that it's not exactly the same information.

Let's therefore make a separate handful of small classes that are specifically for the information that belongs on [Message]. I think we can use the nice short/general name Recipient for this (and then StreamRecipient and DmRecipient).

Then the fields on StreamMessage and DmMessage can move verbatim onto the Recipient subclasses. And maybe leave behind getters on the message subclasses, which just proxy through to there, for convenience / to reduce how much code needs to churn.

[gnprice]

(I guess there's already a DmRecipient. It can get renamed first to something more specific, like DmDisplayRecipient. It's already quite local in how it's used.)

[PIG208]

I suppose that we don't want to move displayRecipient to DmRecipient (after renaming the original class to DmDisplayRecipient), right?

It shouldn't be necessary for sendMessage to construct a DmDisplayRecipient when it only has convenient access to a DmDestination and that other fields on DmDisplayRecipient are mostly unused.

I think this means that DmMessage.recipient should remain as a getter, much like how DmMessage.destination is in the current revision, but returns a DmRecipient computed from DmMessage.allRecipientIds.

[gnprice]

Hmm good question.

It looks like in main we never actually consult DmMessage.displayRecipient, except in its own test and in the allRecipientIds getter. So in any case we shouldn't let it get in the way of a structure we otherwise like — we can always just remove it instead, and deserialize straight to the list of IDs.

One way the further detail could in principle be useful is in DmRecipientHeader, if a user is unknown in the store, we could fall back to their name from the DmDisplayRecipient instead of to "(unknown user)". But we don't do that now, and it seems pretty marginal in value. So if the continued existence of DmDisplayRecipient feels like it's getting in the way, let's just rip it out, and we can decide how to add it back if we do ever implement that fallback functionality.

[rajveermalviya]

Thanks for working on this @PIG208! Just one small nit, otherwise looks good.

[rajveermalviya]

nit:

Suggested change

	/// the `display_recipient from the server.
	/// the `display_recipient` from the server.

[gnprice]

Thanks! Here's a review from reading the first two commits and most of the third:
66ede22 api [nfc]: Make Message.fromJson static
e35d0be api: Drop DmRecipient
bc2f584 api [nfc]: Pull out MessageBase and Recipient

[gnprice]

nit:

api [nfc]: Make Message.fromJson static

As a (factory) constructor, I'd say it's static already — it's certainly not non-static, i.e. it's not an instance method.

Instead can clarify as:

api [nfc]: Make Message.fromJson a plain static method

[gnprice]

nit:

This is unused in app code.  This is mostly NFC except that we will
ignore fields other than "id" from the list of objects from
"display_recipient" on message events.

s/message events/message objects from the server/

These objects also notably appear in getMessages responses.

[gnprice]

Suggested change

import '../route/messages.dart';

This is a wrong-direction import: specific routes should import from the model types, but not vice versa.

I think this was here only for a reference in dartdoc. It's acceptable to have those links not work when the alternative is a wrong-way import.

[gnprice]

nit: let's move the definition of TopicName up to just above these Recipient classes; it had been where it is to be just above StreamMessage, but now StreamRecipient is the more pertinent primary place it's used

[gnprice]

I'm having second thoughts about this name, even though I think I suggested it myself.

The trouble is that "recipient" is already the name of a concept in the Zulip data model — and that concept doesn't agree with this one. For a channel message, the recipient (as that term is understood in the Zulip database and server code) is the channel as a whole, not the individual conversation within it.

There's even a conceptual reason that's the right way to use that term: the unit of who's subscribed to what is the channel, not the conversation. A message sent to a given channel will have the same set of users who "receive" it, in the sense that they get UserMessage rows or that they have permission to read the message, regardless of what the topic of the message is.

I think Conversation would be a good name. And it's only 3 letters longer than Recipient.

[gnprice]

Suggested change

	///
	/// This is required to have an efficient `length`.
	final List<int> allRecipientIds;
	final List<int> allRecipientIds;

For a List, it's expected that length will be efficient.

(I guess the List.length doc doesn't say that. But lots and lots of code assumes it, because it's true for the built-in lists and more generally for any array data structure. Similarly lots of code assumes that operator [] is efficient on a List.)

[gnprice]

This looks like material for a doc comment. In general, instructions about how subclasses should implement something go in doc comments. (That's common in Flutter APIs.)

[gnprice]

These always get overridden as fields and in the same way — so they can just be fields here.

[gnprice]

I think this can also just be a field.

[gnprice]

api [nfc]: Pull out MessageBase and Recipient

This can be split into two commits:

• One introduces Recipient (or Conversation) and subclasses, and switches StreamMessage and DmMessage to use them;
• One introduces MessageBase.

I think those are fairly conceptually distinct changes, even though the first is a prerequisite for what we want from the second.

[PIG208]

Thanks! Updated the PR.

[gnprice]

Thanks for the revision! I think that split of the commit definitely helps make it easier to read, too.

Here's a review after having read the full branch now.

[gnprice]

Suggested change

	/// This lists the sender as well as all (other) participants, and it
	/// This lists the sender as well as all (other) recipients, and it

I think "recipient" is an OK term here. It does risk slight confusion with "the recipient" as the whole list of these users, but that's at least pointing in the same direction (unlike the "recipient"-as-whole-channel issue from #1463 (comment)). And "participant" definitely risks being misleading as it suggests someone who's actually sent a message.

[gnprice]

Can this be "extends"?

Sorry I wasn't clear — that's what I had in mind at #1463 (comment), as "implements" with a field on the base class doesn't make a lot of sense. (For a final field I think it ends up being no different from implementing a getter.)

[PIG208]

Oh, I see. Then #1463 (comment) might not work, since we want to override conversation in the Message<T> subclasses for (de)serialization.

[gnprice]

Hmm I see, because we need to annotate with JsonKey.

Maybe turn conversation back into a getter, then?

[gnprice]

nit: I think this condition would be clearer by splitting up onto two lines:

Suggested change

	return message.id != null && message.id! <= messageId ? -1 : 1;
	if (message.id == null) return 1;
	return message.id! <= messageId ? -1 : 1;

(then the spacing is to match the alignment of other cases in this method)

[gnprice]

I think we can handle this with more confidence:

Suggested change

	case MessageBase<Conversation>():
	assert(false, 'Unexpected concrete subclass of MessageBase<Conversation>:'
	' ${objectRuntimeType(message, 'MessageBase<Conversation>')}');
	return SizedBox.shrink();
	case MessageBase<Conversation>():
	throw StateError('Bad concrete subclass of MessageBase');

We're not going to be writing lots more of these subclasses, so there won't be many opportunities to get this wrong.

That also lets this go back to a switch-expression.

[gnprice]

Can this field move instead onto StreamConversation?

If nothing else, I think having this condition here calls for a comment — this is StreamMessageRecipientHeader, so it should already be known to be a StreamMessage, right? (The answer would be that it might be an outbox-message instead; but that's not something the reader of this widget will necessarily have top of mind, and that's a good thing — if we've designed these internal APIs well, then we should be able to read through most of our code without really thinking about the fact that there are outbox-messages.)

[PIG208]

Yeah, we can do that. I think one tricky thing might be coming up with an abstraction that fits both kinds of stream messages. We document the choice of making displayRecipient nullable that only makes sense for StreamMessage. I think we can just point that comment to the subclass, and have a more generic dartdoc for the field. Maybe something like:

  /// The name of the stream, found on stream message objects from the server.
  ///
  /// This is not updated when its name changes.  Consider using [streamId]
  /// instead to lookup stream name from the store.
  // This is not nullable API-wise for [StreamMessage], but if the message moves
  // across channels, [displayRecipient] still refers to the original channel
  // and it has to be invalidated.
  @JsonKey(required: true, disallowNullValue: true)
  String? displayRecipient;

or

  /// The name of the stream, found on stream message objects from the server.
  ///
  /// This is not updated when its name changes.  Consider using [streamId]
  /// instead to lookup stream name from the store.
  @JsonKey(
    // Make sure that this isn't nullable API-wise.  If a message moves across
    // channels, [displayRecipient] can still refer to the original channel
    // and has to be invalidated.
    required: true, disallowNullValue: true
  )
  String? displayRecipient;

since it's really explaining why we need the set of JsonKey parameters, and was about the motivation for making this nullable. The latter point is now moot because it doesn't make sense for messages to have this field unless the data is not copied from our store, in general.

[gnprice]

This is probably a bit cleaner with a conversation local, like the ChannelNarrow and TopicNarrow implementations above.