Handle internal message moves causing message muting/unmuting

[sm-sayedi]

Fixes: #1476

In addition to addressing the main problem stated in #1476's description, which is about removing messages in some narrows after becoming muted by a message move, this PR also addresses a closely related problem: showing messages in the same narrows after becoming unmuted by a message move.

[chrisbobbe]

Thanks! Comments below, from reading the implementation but not yet the tests.

Also a small commit-message nit, for the first two commits: "keyword" has a special meaning, so I'd avoid saying '"channel" keyword'.

[chrisbobbe]

I appreciate not wanting to use a method against its interface—I see the added dartdoc—but this change causes confusion in the code. The method's name is also part of its interface, and in the new case (messages moved internally but got unmuted) it's very confusing that the name contradicts what's actually happening. For what it means for a message to be in a narrow, see Narrow.containsMessage; in particular:

  /// This does not necessarily mean the message list would show this message
  /// when navigated to this narrow; in particular it does not address the
  /// question of whether the stream or topic […] is muted.

Some options to fix:

• Make the name, dartdoc if needed, and callers agree on what the method is for.
• Make a new method just for the new case.
• Just write the code inline, as we do for the corresponding code handling MutedUsersEvents.

Similarly for _messagesMovedFromNarrow.

[sm-sayedi]

Agree. TBH, I was hesitant to reuse these methods, and in fact, the first revisions had the code inlined. 🙂 Anyway, renamed these methods to _messagesMoved(From/Into)MessageList. I think the callers can now agree on the new names.

[chrisbobbe]

I'm looking at the two methods _handleMessagesMovedInternally and _messagesMovedInternally. Those names are very similar, and the difference ("handle" in the first, not in the second) doesn't effectively carry meaning for me as a reader. This invites confusion and wasted time, when readers either

• don't notice that two different methods are being called, or
• feel pressure to load both implementations into their heads, because they're not satisfied with some aspect of the system—even an aspect unrelated to the feature you're implementing—and there aren't any clues that it's correct to call one method instead of the other, except by understanding the implementations.

[chrisbobbe]

here aren't any clues that it's correct to call one method instead of the other, except by understanding the implementations

In fact here's a concrete example: _messagesMovedInternally is called in the MentionsNarrow, StarredMessagesNarrow, and KeywordSearchNarrow cases, but _handleMessagesMovedInternally isn't. This difference isn't explained by the fact that your method's name starts with _handle and the other one doesn't, so I have to go read the implementations.

[chrisbobbe]

_messagesMovedInternally is called in the MentionsNarrow, StarredMessagesNarrow, and KeywordSearchNarrow cases, but _handleMessagesMovedInternally isn't.

I see from _messageVisible that MentionsNarrow, StarredMessagesNarrow, and KeywordSearchNarrow don't filter out messages by channel/topic muting, so there would be nothing for _handleMessagesMovedInternally to do in those cases, and I guess that's what led to the apparent inconsistency.

[chrisbobbe]

Does the new method _handleMessagesMovedInternally, and its helper _messageInternalMoveCanAffectVisibility, need to exist? What if we just cut those out, inlining the switches on MessageMoveVisibilityEffect right there in messagesMoved?

      case CombinedFeedNarrow():
        switch (store.mightAffectIsTopicVisible(messageMove)) {
          case .unmutedToMuted:       // etc.
          case .mutedToUnmuted:       // etc.
          case .unmutedToUnmuted:     _messagesMovedInternally(messageIds);
          case .mutedToMuted: return;
        }

(and the appropriate thing for the ChannelNarrow case below)

[chrisbobbe]

The dartdoc is incoherent: "moved messages" don't have a "channel and topic"; they have an original channel/topic and a new channel/topic. I worry about bugs caused by calling this with a messageMove where the channel changes.

Let's focus this method on what we need it to do, making it impossible to call except in the case we care about: messages having just their topic changed, in the context of a specific channel focused in the UI. Here's a suggestion:

  /// following a topic rename in the given channel.
  MessageMoveVisibilityEffect mightAffectIsTopicVisibleInChannel({
    required int channelId,
    required TopicName origTopic,
    required TopicName newTopic,
  }) {
    return MessageMoveVisibilityEffect._fromBeforeAfter(
      isTopicVisibleInChannel(channelId, origTopic),
      isTopicVisibleInChannel(channelId, newTopic));
  }

Alternatively, instead of origTopic and newTopic, it could take the whole UpdateMessageMoveData, but with asserts and dartdoc saying it can't be a move between channels.

[sm-sayedi]

Looking carefully at this, I think we can allow this method to be used with a message move where the channel changes. I agree that this method is related to a specific channel being focused in the UI, but a message can be moved from/into the current focused channel. In fact, this method is used for such cases in a commit that I recently added:

1be11a3 msglist: Optimize cross-channel message moves in channel narrow

[chrisbobbe]

We have these names:

• ChannelStore.willChangeIfTopicVisibleInChannel
• ChannelStore.willChangeIfTopicVisible
• UserStore.mightChangeShouldMuteDmConversation

and now we have these new names:

• ChannelStore.mightAffectIsTopicVisibleInChannel
• ChannelStore.mightAffectIsTopicVisible

I'd like to settle on a consistent pattern, since all of these have similar jobs, rather than varying between "will" and "might", and between "change" and "affect". Could you add a prep commit that makes the existing names consistent, then change your commit to follow that pattern? You'll need to disambiguate your new methods from the existing ones, and we might as well do that semantically. For example:

• ChannelStore.muteEventWillAffectIfTopicVisibleInChannel
• ChannelStore.muteEventWillAffectIfTopicVisible
• ChannelStore.messageMoveWillAffectIfTopicVisibleInChannel
• ChannelStore.messageMoveWillAffectIfTopicVisible

[sm-sayedi]

Thanks for the review @chrisbobbe. Pushed new changes, PTAL.