subscription_list: Show a dot for unreads if channel is muted

[Khader-1]

Fixes: #712

Final Results

[sm-sayedi]

Thanks @Khader-1. A few comments below.

[alya]

We'll want to combine releasing this with a change that makes muted channel names look faded, as otherwise it will be confusing why some channels have unread counters and some have just dots.

[gnprice]

We'll want to combine releasing this with a change that makes muted channel names look faded, as otherwise it will be confusing why some channels have unread counters and some have just dots.

Yeah, good thought. This should therefore come after:

• Show muted streams as faded in streams list #424
• subscription_list: Show muted streams as faded in streams list #711

[sm-sayedi]

Thanks for the revision @Khader-1!

Added a comment to the previous thread, please check.

Also, as Ayla stated, these changes need to be combined with the ones that make muted channel names look faded. So I suggest placing this PR on top of #711, that way it will be easy to check all the three cases that I mentioned in #714 review comment. You can check this GitHub answer for this kind of rebasing, specifically git rebase --onto with 2 arguments.

[gnprice]

It looks like this PR was paused while waiting for work on #711. Since #711 is now very close to merge, @Khader-1 I think it'd be a good time to rebase this atop the current version of that, and address the comments above if there are any you haven't already.

Then comment here when this PR is ready for the next round of buddy review from @sm-sayedi.

[Khader-1]

Thanks Greg! I've just pushed a revision PTAL @sm-sayedi.

[sm-sayedi]

Thanks @Khader-1 for the revision. This shows almost the expected behavior.

One case still doesn't behave as expected:
For example, if we have a stream that is not muted and has two topics, with one topic being muted and having unreads and another being default but having no unreads, the mobile app will show a dot, although the web app doesn't show anything. The dot should only show for muted streams.

Unreads.dot.mp4

Also, I am not sure if we want to show the muted stream name in bold when it has unreads for unmuted topics!!

[sm-sayedi]

I forgot to include this review in the previous one, so here it is, separately. 😃

[Khader-1]

Thanks @sm-sayedi! Those two cases are handled in the new revision PTAL!

[sm-sayedi]

Great work @Khader-1! Looks nice to me. Moving on to the mentor review from @kenclary!

[kenclary]

this code looks nice

[alya]

Also, I am not sure if we want to show the muted stream name in bold when it has unreads for unmuted topics!!

Hm, yeah, that sounds right to me.

[chrisbobbe]

Thanks! Small comments below.

[Khader-1]

Thanks for the review @chrisbobbe! I've pushed a new revision PTAL.

[chrisbobbe]

Looks like CI is failing; could you fix that?

[Khader-1]

Sorry about that! I've fixed it now. PTAL @chrisbobbe.

[chrisbobbe]

Thanks! Small comments below, and I'll mark this for Greg's review.

[chrisbobbe]

Suggested change

	const MutedUnreadBadge({
	super.key,
	});
	const MutedUnreadBadge({super.key});

[Khader-1]

Thanks @chrisbobbe!

[gnprice]

This will include unreads that are in muted topics. That doesn't sound like the desired behavior — and it's not what web does, based on checking just now with a channel where I have some such unreads.

I think the desired behavior is: we want to see if there are any unreads here where isTopicVisibleInStream is true but the stricter isTopicVisible isn't.

In other words:

• We show an indicator on the channel/stream when there are some unreads that we would show if you narrow to the channel. (That's what countInStreamNarrow / isTopicVisibleInStream would measure.)
• When moreover some of those unreads are messages we'd show in the combined feed, then the indicator we show is a count of those unreads. (That's countInStream / isTopicVisible.)
• So we show a dot when the first condition is true but not the second.

Can you find the code in web that controls this aspect? That'd be good for confirming what web's behavior is to verify that my description above matches it.

Then for implementing that behavior, probably cleanest is to use the existing method countInStreamNarrow.

[Khader-1]

Thanks @gnprice!

Yes, I can verify you're description:

I think the desired behavior is: we want to see if there are any unreads here where isTopicVisibleInStream is true but the stricter isTopicVisible isn't.

I've taken a look into the web code and found that the only case that applies is when stream isMuted (or stream.isMuted is null) and topic policy is none. and that Equals to isTopicVisibleInStream && !isTopicVisible.

topic policy	stream isMuted	isTopicVisible	isTopicVisibleInStream	isTopicVisibleInStream && !isTopicVisible	web counts as unmuted unread
none	true	false	true	true	true
muted	true	false	false	false	false
unmuted	true	true	true	false	false
followed	true	true	true	false	false
none	null	true	true	false	true
muted	null	false	false	false	false
unmuted	null	true	true	false	false
followed	null	true	true	false	false
none	false	true	true	false	false
muted	false	false	false	false	false
unmuted	false	true	true	false	false
followed	false	true	true	false	false

You can find that in web/src/[email protected]_stream_count_info

get_stream_count_info(
    stream_id: number,
    update_first_unmuted_message_id = false,
): StreamCountInfo {
    const per_stream_bucketer = this.bucketer.get(stream_id);

    if (!per_stream_bucketer) {
        return {
            unmuted_count: 0,
            muted_count: 0,
            followed_count: 0,
            stream_is_muted: false,
        };
    }

    const sub = sub_store.get(stream_id);
    let unmuted_count = 0;
    let muted_count = 0;
    let followed_count = 0;
    for (const [topic, msgs] of per_stream_bucketer) {
        const topic_count = msgs.size;

        if (user_topics.is_topic_followed(stream_id, topic)) {
            followed_count += topic_count;
        }

        if (user_topics.is_topic_unmuted_or_followed(stream_id, topic)) {
            unmuted_count += topic_count;
            if (update_first_unmuted_message_id) {
                first_unread_unmuted_message_id = Math.min(
                    first_unread_unmuted_message_id,
                    ...msgs,
                );
            }
        } else if (user_topics.is_topic_muted(stream_id, topic)) {
            muted_count += topic_count;
        } else if (sub?.is_muted) {
            muted_count += topic_count;
        } else {
            unmuted_count += topic_count;
            if (update_first_unmuted_message_id) {
                first_unread_unmuted_message_id = Math.min(
                    first_unread_unmuted_message_id,
                    ...msgs,
                );
            }
        }
    }
    const stream_count = {
        unmuted_count,
        muted_count,
        followed_count,
        stream_is_muted: sub?.is_muted ?? false,
    };
    return stream_count;
}

And in web/src/stream_list.ts@update_count_in_dom

export function update_count_in_dom(
    $stream_li: JQuery,
    stream_counts: StreamCountInfo,
    stream_has_any_unread_mention_messages: boolean,
    stream_has_any_unmuted_unread_mention: boolean,
    stream_has_only_muted_unread_mention: boolean,
): void {
    // The subscription_block properly excludes the topic list,
    // and it also has sensitive margins related to whether the
    // count is there or not.
    const $subscription_block = $stream_li.find(".subscription_block");

    ui_util.update_unread_mention_info_in_dom(
        $subscription_block,
        stream_has_any_unread_mention_messages,
    );

    if (stream_has_any_unmuted_unread_mention) {
        $subscription_block.addClass("has-unmuted-mentions");
        $subscription_block.removeClass("has-only-muted-mentions");
    } else {
        $subscription_block.removeClass("has-unmuted-mentions");
        if (!stream_counts.stream_is_muted && stream_has_only_muted_unread_mention) {
            $subscription_block.addClass("has-only-muted-mentions");
        } else {
            $subscription_block.removeClass("has-only-muted-mentions");
        }
    }

    // Here we set the count and compute the values of two classes:
    // .stream-with-count is used for the layout CSS to know whether
    // to leave space for the unread count, and has-unmuted-unreads is
    // used in muted streams to set the fading correctly to indicate
    // those are unread
    if (stream_counts.unmuted_count > 0 && !stream_counts.stream_is_muted) {
        // Normal stream, has unmuted unreads; display normally.
        ui_util.update_unread_count_in_dom($subscription_block, stream_counts.unmuted_count);
        $subscription_block.addClass("stream-with-count");
        $subscription_block.removeClass("has-unmuted-unreads");
        $subscription_block.removeClass("has-only-muted-unreads");
    } else if (stream_counts.unmuted_count > 0 && stream_counts.stream_is_muted) {
        // Muted stream, has unmuted unreads.
        ui_util.update_unread_count_in_dom($subscription_block, stream_counts.unmuted_count);
        $subscription_block.addClass("stream-with-count");
        $subscription_block.addClass("has-unmuted-unreads");
        $subscription_block.removeClass("has-only-muted-unreads");
    } else if (stream_counts.muted_count > 0 && stream_counts.stream_is_muted) {
        // Muted stream, only muted unreads.
        ui_util.update_unread_count_in_dom($subscription_block, stream_counts.muted_count);
        $subscription_block.addClass("stream-with-count");
        $subscription_block.removeClass("has-unmuted-unreads");
        $subscription_block.removeClass("has-only-muted-unreads");
    } else if (
        stream_counts.muted_count > 0 &&
        !stream_counts.stream_is_muted &&
        stream_has_only_muted_unread_mention
    ) {
        // Normal stream, only muted unreads, including a mention:
        // Display the mention, faded, and a faded unread count too,
        // so that we don't weirdly show the mention indication
        // without an unread count.
        ui_util.update_unread_count_in_dom($subscription_block, stream_counts.muted_count);
        $subscription_block.removeClass("has-unmuted-unreads");
        $subscription_block.addClass("stream-with-count");
        $subscription_block.addClass("has-only-muted-unreads");
    } else if (stream_counts.muted_count > 0 && !stream_counts.stream_is_muted) {
        // Normal stream, only muted unreads: display nothing. The
        // current thinking is displaying those counts with muted
        // styling would be more distracting than helpful.
        ui_util.update_unread_count_in_dom($subscription_block, 0);
        $subscription_block.removeClass("has-unmuted-unreads");
        $subscription_block.removeClass("stream-with-count");
    } else {
        // No unreads: display nothing.
        ui_util.update_unread_count_in_dom($subscription_block, 0);
        $subscription_block.removeClass("has-unmuted-unreads");
        $subscription_block.removeClass("has-only-muted-unreads");
        $subscription_block.removeClass("stream-with-count");
    }
}

[gnprice]

Cool, thanks for investigating that.

In your table, in the row for none/null, isTopicVisible should be false, not true, right? We have:

  bool isTopicVisible(int streamId, String topic) {
    switch (topicVisibilityPolicy(streamId, topic)) {
      case UserTopicVisibilityPolicy.none:
        switch (subscriptions[streamId]?.isMuted) {
          case false: return true;
          case true:  return false;
          case null:  return false; // not subscribed; treat like muted

so none/null behaves the same as none/true.

That's good, though, because I think that's what makes this conclusion true:

the only case that applies is when stream isMuted (or stream.isMuted is null) and topic policy is none. and that Equals to isTopicVisibleInStream && !isTopicVisible.

[gnprice]

Thanks @Khader-1, and thanks @sm-sayedi @alya @chrisbobbe for the previous reviews!

The behavior of this appears to be wrong, as I wrote just above.

The first commit:
b3456df subscription_list: Ensure bold name applies for unmuted streams only
looks good. Comments below on the remainder of the second/main commit. I skipped commenting on that commit's changes in subscription_list.dart, because I think those will look different after revising the behavior.

[gnprice]

The structure of these test cases looks fine.

This case should have the opposite expectation, though — the topic (and not only the stream/channel) is muted, so there should be no badge.

Then let's also have test cases exercising each of the possible scenarios:

• all the possible combinations of isTopicVisible true/false and isTopicVisibleInStream true/false;
• for each of those, all the possible values for whether the stream is muted.

This muted-unreads badge is closely related to the unread-count badge. So some of those scenarios may be covered by the existing test cases above, and those can be handled by just adding another check to those cases.

[gnprice]

Can simplify with find.byType:

Suggested change

	final finder = find.byWidgetPredicate((widget) => widget is MutedUnreadBadge);
	final finder = find.byType(MutedUnreadBadge);

Then just inline into the check below.

For examples, see the similar test cases above about UnreadCountBadge.

[Khader-1]

Thank @gnprice! I've made a revision PTAL

[gnprice]

Thanks! Comments below.

[gnprice]

This field is redundant with unreadCount. The code in this widget's build method, and the code at the call site constructing one of these widgets, will both be clearer if there's just the one field — otherwise the reader has to wonder about the case where hasUnmutedUnreads doesn't agree with unreadCount > 0.

[gnprice]

As I suggested at #714 (comment), I think this logic will be cleaner if it uses the existing method countInChannelNarrow (previously countInStreamNarrow).

[Khader-1]

Thanks @gnprice! Please check the latest revision.

[gnprice]

Thanks for the revision! Looks good, with one nit below. Fixing that and merging.

[gnprice]

nit: line too long, and important information after column 80

Suggested change

	final showMutedUnreadBadge = unreadCount == 0 && unreadsModel!.countInChannelNarrow(subscription.streamId) > 0;
	final showMutedUnreadBadge = unreadCount == 0
	&& unreadsModel!.countInChannelNarrow(subscription.streamId) > 0;