Skip to content

subscription_list: Show @-mention indicator when that applies #875

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.

Sign up for GitHub

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

Closed
wants to merge 6 commits into from
Closed

subscription_list: Show @-mention indicator when that applies #875

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4362559
msglist: Add message to mentions if mentioned
Khader-1 Aug 14, 2024
b3b9f53
unreads [nfc]: Introduce _streamIdsAndTopicsByMessageId data structure
Khader-1 Aug 9, 2024
2a00ea1
inbox [nfc]: Move AtMentionMarker to unread_count_badge
Khader-1 Aug 10, 2024
ef6d4c3
inbox [nfc]: Introduce muted property to AtMentionMarker
Khader-1 Aug 14, 2024
b1e8fb7
subscription_list: Show @-mention marker when that applies
Khader-1 Aug 14, 2024
a635d4f
subscription_list: Show unread count when hasOnlyMutedMentions
Khader-1 Aug 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/model/store.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,7 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, ChannelStore, Mess
@override
void reconcileMessages(List<Message> messages) {
_messages.reconcileMessages(messages);
// TODO(#649) notify [unreads] of the just-fetched messages
unreads.reconcileMessages(messages);
// TODO(#650) notify [recentDmConversationsView] of the just-fetched messages
}

Expand Down
79 changes: 75 additions & 4 deletions lib/model/unreads.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,22 +32,24 @@ import 'channel.dart';
/// unsubscribed streams and messages sent by muted users.
// TODO When [oldUnreadsMissing], if you load a message list with very old unreads,
// sync to those unreads, because the user has shown an interest in them.
// TODO When loading a message list with stream messages, check all the stream
// messages and refresh [mentions] (see [mentions] dartdoc).
class Unreads extends ChangeNotifier {
factory Unreads({
required UnreadMessagesSnapshot initial,
required int selfUserId,
required ChannelStore channelStore,
}) {
final streams = <int, Map<String, QueueList<int>>>{};
final streamIdsAndTopicsByMessageId = <int, ({int streamId, String topic})>{};
final dms = <DmNarrow, QueueList<int>>{};
final mentions = Set.of(initial.mentions);

for (final unreadChannelSnapshot in initial.channels) {
final streamId = unreadChannelSnapshot.streamId;
final topic = unreadChannelSnapshot.topic;
(streams[streamId] ??= {})[topic] = QueueList.from(unreadChannelSnapshot.unreadMessageIds);
final messageInfo = (streamId: streamId, topic: topic);
streamIdsAndTopicsByMessageId.addEntries(
unreadChannelSnapshot.unreadMessageIds.map((messageId) => MapEntry(messageId, messageInfo)));
}

for (final unreadDmSnapshot in initial.dms) {
Expand All @@ -64,6 +66,7 @@ class Unreads extends ChangeNotifier {
return Unreads._(
channelStore: channelStore,
streams: streams,
streamIdsAndTopicsByMessageId: streamIdsAndTopicsByMessageId,
dms: dms,
mentions: mentions,
oldUnreadsMissing: initial.oldUnreadsMissing,
Expand All @@ -74,11 +77,12 @@ class Unreads extends ChangeNotifier {
Unreads._({
required this.channelStore,
required this.streams,
required Map<int, ({int streamId, String topic})> streamIdsAndTopicsByMessageId,
required this.dms,
required this.mentions,
required this.oldUnreadsMissing,
required this.selfUserId,
});
}) : _streamIdsAndTopicsByMessageId = streamIdsAndTopicsByMessageId;

final ChannelStore channelStore;

Expand All @@ -88,6 +92,11 @@ class Unreads extends ChangeNotifier {
/// Unread stream messages, as: stream ID → topic → message IDs (sorted).
final Map<int, Map<String, QueueList<int>>> streams;

/// Stream IDs and topics for unread stream messages, as: message ID → (stream ID, topic).
///
/// This is the same data as [streams], but in this message ID-keyed form.
final Map<int, ({int streamId, String topic})> _streamIdsAndTopicsByMessageId;

/// Unread DM messages, as: DM narrow → message IDs (sorted).
final Map<DmNarrow, QueueList<int>> dms;

Expand All @@ -103,7 +112,7 @@ class Unreads extends ChangeNotifier {
/// a) the message is edited at all ([UpdateMessageEvent]),
/// assuming it still has a direct or wildcard mention after the edit, or
/// b) the message gains a direct @-mention ([UpdateMessageFlagsEvent]), or
/// c) TODO unimplemented: the user loads the message in the message list
/// c) the user loads the message in the message list
/// But otherwise, assume its unread state remains unknown to [mentions].
///
/// [1] This item applies verbatim at Server 8.0+. For older servers, the
Expand Down Expand Up @@ -185,6 +194,25 @@ class Unreads extends ChangeNotifier {
return c;
}

/// The "broadest" unread count for this channel,
/// without doing any checking on visibility policy.
///
/// This includes all topics that have regardless visibility policy,
/// even if the channel is muted.
///
/// This is needed for one specific case, which is when the channel has
/// only muted unreads including a mention or more, in that case we show
/// total unread count including muted unreads.
int countAll(int streamId) {
final topics = streams[streamId];
if (topics == null) return 0;
int c = 0;
for (final entry in topics.entries) {
c = c + entry.value.length;
}
return c;
}

int countInTopicNarrow(int streamId, String topic) {
final topics = streams[streamId];
return topics?[topic]?.length ?? 0;
Expand Down Expand Up @@ -214,6 +242,38 @@ class Unreads extends ChangeNotifier {
}
}

Set<int> get channelsWithUnreadMentions {
final channels = <int>{};
for (var messageId in mentions) {
final streamId = _streamIdsAndTopicsByMessageId[messageId]?.streamId;
if (streamId != null) {
channels.add(streamId);
}
}
return channels;
}

Set<int> get channelsWithUnmutedMentions {
final channels = <int>{};
for (var messageId in mentions) {
final (:streamId, :topic) = _streamIdsAndTopicsByMessageId[messageId]!;
if (channelStore.isTopicVisible(streamId, topic)) {
channels.add(streamId);
}
}
return channels;
}

void reconcileMessages(List<Message> messages) {
for (final message in messages) {
if (message.flags.contains(MessageFlag.read)) continue;
if (message.flags.contains(MessageFlag.mentioned)
|| message.flags.contains(MessageFlag.wildcardMentioned)) {
mentions.add(message.id);
}
}
}

void handleMessageEvent(MessageEvent event) {
final message = event.message;
if (message.flags.contains(MessageFlag.read)) {
Expand Down Expand Up @@ -342,6 +402,7 @@ class Unreads extends ChangeNotifier {
case UpdateMessageFlagsAddEvent():
if (event.all) {
streams.clear();
_streamIdsAndTopicsByMessageId.clear();
dms.clear();
mentions.clear();
oldUnreadsMissing = false;
Expand Down Expand Up @@ -438,6 +499,7 @@ class Unreads extends ChangeNotifier {

void _addLastInStreamTopic(int messageId, int streamId, String topic) {
((streams[streamId] ??= {})[topic] ??= QueueList()).addLast(messageId);
_streamIdsAndTopicsByMessageId[messageId] = (streamId: streamId, topic: topic);
}

// [messageIds] must be sorted ascending and without duplicates.
Expand All @@ -450,6 +512,9 @@ class Unreads extends ChangeNotifier {
// TODO(server-6) remove 6.0 comment
(existing) => setUnion(existing, messageIds),
);
final messageInfo = (streamId: streamId, topic: topic);
_streamIdsAndTopicsByMessageId.addEntries(
messageIds.map((messageId) => MapEntry(messageId, messageInfo)));
}

// TODO use efficient model lookups
Expand All @@ -473,6 +538,9 @@ class Unreads extends ChangeNotifier {
for (final streamId in newlyEmptyStreams) {
streams.remove(streamId);
}
for (final messageId in idsToRemove) {
_streamIdsAndTopicsByMessageId.remove(messageId);
}
}

void _removeAllInStreamTopic(Set<int> incomingMessageIds, int streamId, String topic) {
Expand All @@ -489,6 +557,9 @@ class Unreads extends ChangeNotifier {
streams.remove(streamId);
}
}
for (final messageId in incomingMessageIds) {
_streamIdsAndTopicsByMessageId.remove(messageId);
}
}

// TODO use efficient model lookups
Expand Down
20 changes: 3 additions & 17 deletions lib/widgets/inbox.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,7 @@ abstract class _HeaderItem extends StatelessWidget {
overflow: TextOverflow.ellipsis,
title))),
const SizedBox(width: 12),
if (hasMention) const _AtMentionMarker(),
if (hasMention) const AtMentionMarker(muted: false),
Padding(padding: const EdgeInsetsDirectional.only(end: 16),
child: UnreadCountBadge(
backgroundColor: unreadCountBadgeBackgroundColor(context),
Expand Down Expand Up @@ -405,7 +405,7 @@ class _DmItem extends StatelessWidget {
overflow: TextOverflow.ellipsis,
title))),
const SizedBox(width: 12),
if (hasMention) const _AtMentionMarker(),
if (hasMention) const AtMentionMarker(muted: false),
Padding(padding: const EdgeInsetsDirectional.only(end: 16),
child: UnreadCountBadge(backgroundColor: null,
count: count)),
Expand Down Expand Up @@ -530,25 +530,11 @@ class _TopicItem extends StatelessWidget {
overflow: TextOverflow.ellipsis,
topic))),
const SizedBox(width: 12),
if (hasMention) const _AtMentionMarker(),
if (hasMention) const AtMentionMarker(muted: false),
Padding(padding: const EdgeInsetsDirectional.only(end: 16),
child: UnreadCountBadge(
backgroundColor: colorSwatchFor(context, subscription),
count: count)),
]))));
}
}

class _AtMentionMarker extends StatelessWidget {
const _AtMentionMarker();

@override
Widget build(BuildContext context) {
final designVariables = DesignVariables.of(context);
// Design for at-mention marker based on Figma screen:
// https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?type=design&node-id=224-16386&mode=design&t=JsNndFQ8fKFH0SjS-0
return Padding(
padding: const EdgeInsetsDirectional.only(end: 4),
child: Icon(ZulipIcons.at_sign, size: 14, color: designVariables.atMentionMarker));
}
}
34 changes: 30 additions & 4 deletions lib/widgets/subscription_list.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,16 +185,26 @@ class _SubscriptionList extends StatelessWidget {

@override
Widget build(BuildContext context) {
final channelsWithMentions = unreadsModel!.channelsWithUnreadMentions;
final channelsWithUnmutedMentions = unreadsModel!.channelsWithUnmutedMentions;
return SliverList.builder(
itemCount: subscriptions.length,
itemBuilder: (BuildContext context, int index) {
final subscription = subscriptions[index];
final unreadCount = unreadsModel!.countInChannel(subscription.streamId);
final showMutedUnreadBadge = unreadCount == 0
&& unreadsModel!.countInChannelNarrow(subscription.streamId) > 0;
final hasMentions = channelsWithMentions.contains(subscription.streamId);
final hasOnlyMutedMentions = !subscription.isMuted && hasMentions
&& !channelsWithUnmutedMentions.contains(subscription.streamId);
final mutedUnreadCount = hasOnlyMutedMentions && unreadCount == 0 ?
unreadsModel!.countAll(subscription.streamId) : 0;
Comment on lines +198 to +201
Copy link
Member

Choose a reason for hiding this comment

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

It feels like there's a lot of different conditions that are interacting here, which means there are a lot of different cases for how this can end up behaving. I can't easily tell from reading this code what all those cases are and what its behavior is in all of them, which makes it hard to tell if the behavior is what we want.

Can you try laying out (just in text, in this thread) what you believe the different cases are for a given channel, and how you believe this screen should behave in each case? Then we can discuss at that level what behavior we want, and then go back to the code and make sure the code reflects that behavior.

(Ultimately I suspect we can express the desired behavior with somewhat simpler logic than this.)

Copy link
Member

Choose a reason for hiding this comment

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

I agree with Greg that laying this out will be helpful.

Adding the @-markers to the SubscriptionItem, in addition to the unreads seem to complicate the widget more. Perhaps we should be cautious about adding more flags to that widget and simplify these conditions.

return SubscriptionItem(subscription: subscription,
unreadCount: unreadCount,
showMutedUnreadBadge: showMutedUnreadBadge);
mutedUnreadCount: mutedUnreadCount,
showMutedUnreadBadge: showMutedUnreadBadge,
hasMentions: hasMentions,
hasOnlyMutedMentions: hasOnlyMutedMentions);
});
}
}
Expand All @@ -205,20 +215,27 @@ class SubscriptionItem extends StatelessWidget {
super.key,
required this.subscription,
required this.unreadCount,
required this.mutedUnreadCount,
required this.showMutedUnreadBadge,
required this.hasMentions,
required this.hasOnlyMutedMentions,
});

final Subscription subscription;
final int unreadCount;
final int mutedUnreadCount;
final bool showMutedUnreadBadge;
final bool hasMentions;
final bool hasOnlyMutedMentions;

@override
Widget build(BuildContext context) {
final designVariables = DesignVariables.of(context);

final swatch = colorSwatchFor(context, subscription);
final hasUnreads = (unreadCount > 0);
final opacity = subscription.isMuted ? 0.55 : 1.0;
const mutedOpacity = 0.55;
Copy link
Member

Choose a reason for hiding this comment

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

It would be helpful to link the source of the opacity.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure if I got it correctly; I just used the same one that we've used before for the channel name and unread count badge.

Are you suggesting to link it to zulip mobile or web design?

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see. Yeah 0.55 is not new here; it was added in b0b8a50. The design came from Vlad here. Perhaps we can link to that GitHub comment in an NFC commit.

final opacity = subscription.isMuted ? mutedOpacity : 1.0;
return Material(
// TODO(design) check if this is the right variable
color: designVariables.background,
Expand Down Expand Up @@ -258,16 +275,25 @@ class SubscriptionItem extends StatelessWidget {
subscription.name)))),
if (hasUnreads) ...[
const SizedBox(width: 12),
// TODO(#747) show @-mention indicator when it applies
if (hasMentions) AtMentionMarker(muted: !subscription.isMuted && hasOnlyMutedMentions),
Opacity(
opacity: opacity,
child: UnreadCountBadge(
count: unreadCount,
backgroundColor: swatch,
bold: true)),
] else if (hasOnlyMutedMentions && !subscription.isMuted) ...[
const SizedBox(width: 12),
const AtMentionMarker(muted: true),
Opacity(
opacity: mutedOpacity,
child: UnreadCountBadge(
count: mutedUnreadCount,
backgroundColor: swatch,
bold: true)),
Copy link
Member

Choose a reason for hiding this comment

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

Looks like the later part is mostly identical to the previous branch except for the opacity/unread count. I think it would be beneficial to find a way that do not couple AtMentionMarker with UnreadCountBadge, so that each has individual logic, making the change easier to verify.

Copy link
Member

Choose a reason for hiding this comment

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

For example, we can have something like (not sure if the conditions are right):

          if (hasMentions) ...[
            AtMentionMarker(muted: hasOnlyMutedMentions || subscription.isMuted)
          ]

that is adjancent to but independent from the UnreadCountBadge branches

] else if (showMutedUnreadBadge) ...[
const SizedBox(width: 12),
// TODO(#747) show @-mention indicator when it applies
if (hasMentions) const AtMentionMarker(muted: true),
const MutedUnreadBadge(),
],
const SizedBox(width: 16),
Expand Down
11 changes: 9 additions & 2 deletions lib/widgets/theme.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,8 @@ class DesignVariables extends ThemeExtension<DesignVariables> {
mainBackground: const Color(0xfff0f0f0),
title: const Color(0xff1a1a1a),
channelColorSwatches: ChannelColorSwatches.light,
atMentionMarker: const HSLColor.fromAHSL(0.5, 0, 0, 0.2).toColor(),
atMentionMarker: const HSLColor.fromAHSL(0.7, 0, 0, 0.2).toColor(),
mutedAtMentionMarker: const HSLColor.fromAHSL(0.35, 0, 0, 0.2).toColor(),
dmHeaderBg: const HSLColor.fromAHSL(1, 46, 0.35, 0.93).toColor(),
errorBannerBackground: const HSLColor.fromAHSL(1, 4, 0.33, 0.90).toColor(),
errorBannerBorder: const HSLColor.fromAHSL(0.4, 3, 0.57, 0.33).toColor(),
Expand Down Expand Up @@ -147,7 +148,8 @@ class DesignVariables extends ThemeExtension<DesignVariables> {
title: const Color(0xffffffff),
channelColorSwatches: ChannelColorSwatches.dark,
// TODO(design-dark) need proper dark-theme color (this is ad hoc)
atMentionMarker: const HSLColor.fromAHSL(0.4, 0, 0, 1).toColor(),
atMentionMarker: const HSLColor.fromAHSL(0.7, 0, 0, 1).toColor(),
mutedAtMentionMarker: const HSLColor.fromAHSL(0.4, 0, 0, 1).toColor(),
dmHeaderBg: const HSLColor.fromAHSL(1, 46, 0.15, 0.2).toColor(),
errorBannerBackground: const HSLColor.fromAHSL(1, 0, 0.61, 0.19).toColor(),
errorBannerBorder: const HSLColor.fromAHSL(0.4, 3, 0.73, 0.74).toColor(),
Expand Down Expand Up @@ -182,6 +184,7 @@ class DesignVariables extends ThemeExtension<DesignVariables> {
required this.title,
required this.channelColorSwatches,
required this.atMentionMarker,
required this.mutedAtMentionMarker,
required this.dmHeaderBg,
required this.errorBannerBackground,
required this.errorBannerBorder,
Expand Down Expand Up @@ -223,6 +226,7 @@ class DesignVariables extends ThemeExtension<DesignVariables> {

// Not named variables in Figma; taken from older Figma drafts, or elsewhere.
final Color atMentionMarker;
final Color mutedAtMentionMarker;
final Color dmHeaderBg;
final Color errorBannerBackground;
final Color errorBannerBorder;
Expand Down Expand Up @@ -251,6 +255,7 @@ class DesignVariables extends ThemeExtension<DesignVariables> {
Color? title,
ChannelColorSwatches? channelColorSwatches,
Color? atMentionMarker,
Color? mutedAtMentionMarker,
Color? dmHeaderBg,
Color? errorBannerBackground,
Color? errorBannerBorder,
Expand Down Expand Up @@ -278,6 +283,7 @@ class DesignVariables extends ThemeExtension<DesignVariables> {
title: title ?? this.title,
channelColorSwatches: channelColorSwatches ?? this.channelColorSwatches,
atMentionMarker: atMentionMarker ?? this.atMentionMarker,
mutedAtMentionMarker: mutedAtMentionMarker ?? this.mutedAtMentionMarker,
dmHeaderBg: dmHeaderBg ?? this.dmHeaderBg,
errorBannerBackground: errorBannerBackground ?? this.errorBannerBackground,
errorBannerBorder: errorBannerBorder ?? this.errorBannerBorder,
Expand Down Expand Up @@ -312,6 +318,7 @@ class DesignVariables extends ThemeExtension<DesignVariables> {
title: Color.lerp(title, other.title, t)!,
channelColorSwatches: ChannelColorSwatches.lerp(channelColorSwatches, other.channelColorSwatches, t),
atMentionMarker: Color.lerp(atMentionMarker, other.atMentionMarker, t)!,
mutedAtMentionMarker: Color.lerp(mutedAtMentionMarker, other.mutedAtMentionMarker, t)!,
dmHeaderBg: Color.lerp(dmHeaderBg, other.dmHeaderBg, t)!,
errorBannerBackground: Color.lerp(errorBannerBackground, other.errorBannerBackground, t)!,
errorBannerBorder: Color.lerp(errorBannerBorder, other.errorBannerBorder, t)!,
Expand Down
Loading