@-mention markers #509

sirpengi · 2024-02-08T16:51:16Z

This PR is built on top of #510

Fixes: #384

gnprice

Thanks @sirpengi! Generally this looks good. Small comments below.

gnprice · 2024-02-08T19:29:35Z

lib/widgets/inbox.dart

+class _AtMentionMarker extends StatelessWidget {
+  const _AtMentionMarker();
+
+  static Color markerColor = const HSLColor.fromAHSL(0.5, 0, 0, 0.2).toColor();


Suggested change

static Color markerColor = const HSLColor.fromAHSL(0.5, 0, 0, 0.2).toColor();

static final markerColor = const HSLColor.fromAHSL(0.5, 0, 0, 0.2).toColor();

(If we wanted to be explicit about the type, it'd be static final Color markerColor = ….)

gnprice · 2024-02-08T19:43:14Z

lib/widgets/inbox.dart

+            isMentioned: isMentioned,
            streamCount: data.count,
+            streamIsMentioned: data.isMentioned,


These "is mentioned" names work if the subject is implicitly taken to be the self-user — the user is mentioned — but because they're attached to objects describing a topic or a stream etc., they sound a lot more like "this topic is mentioned", "this stream is mentioned", etc.

Let's therefore say "has mention" rather than "is mentioned": so hasMention, streamHasMention, and so on.

gnprice · 2024-02-08T19:51:27Z

lib/widgets/inbox.dart

+        final isMentioned = messageIds.any((messageId) => unreadsModel!.mentions.contains(messageId));
+        topicItems.add((topic, countInTopic, isMentioned, messageIds.last));
        countInStream += countInTopic;
+        if (isMentioned) mentionedInStream = true;


nit:

Suggested change

final isMentioned = messageIds.any((messageId) => unreadsModel!.mentions.contains(messageId));

topicItems.add((topic, countInTopic, isMentioned, messageIds.last));

countInStream += countInTopic;

if (isMentioned) mentionedInStream = true;

final isMentioned = messageIds.any((messageId) => unreadsModel!.mentions.contains(messageId));

if (isMentioned) mentionedInStream = true;

topicItems.add((topic, countInTopic, isMentioned, messageIds.last));

countInStream += countInTopic;

This ordering is a bit more consistent internally — mentions, then counts, rather than mention, count, mention — and also keeps this code parallel with the similar code for DMs above.

gnprice · 2024-02-08T20:10:20Z

lib/widgets/inbox.dart

      header: _AllDmsHeaderItem(
        count: allDmsCount,
+        isMentioned: isMentioned,


This seems wrong — it's using the is-mentioned flag for the individual DM conversation, but passing it to use for the all-DMs header.

Hmm but I can't actually trigger any symptom from that bug.

… Aha. This header has no effect! That's a pre-existing latent bug in this file. What's happening is that the items of the list view are the entire sections — all DMs, then each stream — and not the individual conversations, so the StickyHeaderItem widgets on the individual conversations never get consulted.

I'll send a quick PR removing those StickyHeaderItems, to avoid causing us similar confusion in the future. Then you can rebase atop that PR, and I think it will slightly simplify some of this code.

gnprice · 2024-02-08T20:15:43Z

lib/widgets/inbox.dart

              Padding(padding: const EdgeInsetsDirectional.only(end: 16),
                child: UnreadCountBadge(backgroundColor: subscription.colorSwatch(),
                  count: count)),
            ])))));
  }
 }
+
+class _AtMentionMarker extends StatelessWidget {


These two commits are best squashed together:
ce34ec0 inbox: Show at-mention marker on topics
b6ac522 inbox [nfc]: Pull out _AtMentionMarker

because the latter basically amounts to rethinking how to arrange some of the code added by the former, and so it's simplest to go straight to the revised way of arranging it.

In fact let's squash those together with the next commit, handling streams:
ce34ec0 inbox: Show at-mention marker on topics
b6ac522 inbox [nfc]: Pull out _AtMentionMarker
6e74d2a inbox: Show at-mention marker on streams

That way there isn't an intermediate state where we have @-mention markers on the individual topics but missing on the streams, which feels a little glitchy given expectations set by web. And conversely the squashed commit is still a quite reasonable size.

sirpengi · 2024-02-09T17:10:10Z

@gnprice ready for review again!

gnprice

Thanks for the revision! Generally this all looks good now, pending #510. One nit below.

gnprice · 2024-02-09T21:31:13Z

lib/widgets/inbox.dart

-          // TODO(#384) for streams, show @-mention indicator when it applies
+          if (hasMention) const _AtMentionMarker(),


nit in commit message:

inbox: Show at-mention marker on topics

Should say "topics and streams", since the same commit does it for streams too.

sirpengi · 2024-02-12T09:24:26Z

@gnprice Commit message updated per comment, now just waiting on #510

Taken today from Figma at: https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?type=design&node-id=171-12359&mode=design&t=LQ4ZzSZnD39eDpDf-0 Exported svg was processed in inkscape to expand strokes to paths and resulting file cleaned of unnecessary markup.

This will be used later to match other types of rows.

The `hasMention` flag was added to the abstract `_HeaderItem` class so `_AllDmsHeaderItem` is affected by this change. It isn't used in the DMs header for now so will be hard-coded as False and changed in the next commit.

Fixes: zulip#384

gnprice · 2024-02-12T20:19:32Z

Thanks! Looks good, and #510 is now in, so merging.

sirpengi requested a review from gnprice February 8, 2024 16:51

gnprice reviewed Feb 8, 2024

View reviewed changes

sirpengi force-pushed the pr-at-mention-markers branch 2 times, most recently from c57bd8a to 1b4adc0 Compare February 9, 2024 17:04

gnprice reviewed Feb 9, 2024

View reviewed changes

sirpengi force-pushed the pr-at-mention-markers branch from 1b4adc0 to aad1988 Compare February 12, 2024 09:20

sirpengi added 4 commits February 12, 2024 12:18

inbox test [nfc]: Pull out findRowByLabel

cf8d453

This will be used later to match other types of rows.

inbox: Show at-mention marker on topics and streams

97e87c0

The `hasMention` flag was added to the abstract `_HeaderItem` class so `_AllDmsHeaderItem` is affected by this change. It isn't used in the DMs header for now so will be hard-coded as False and changed in the next commit.

inbox: Show at-mention markers on DMs

ee99129

Fixes: zulip#384

gnprice force-pushed the pr-at-mention-markers branch from aad1988 to ee99129 Compare February 12, 2024 20:19

gnprice merged commit ee99129 into zulip:main Feb 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

@-mention markers #509

@-mention markers #509

Uh oh!

sirpengi commented Feb 8, 2024 •

edited

Loading

Uh oh!

gnprice left a comment

Uh oh!

gnprice Feb 8, 2024

Uh oh!

gnprice Feb 8, 2024

Uh oh!

gnprice Feb 8, 2024

Uh oh!

gnprice Feb 8, 2024

Uh oh!

gnprice Feb 8, 2024

Uh oh!

gnprice Feb 8, 2024

Uh oh!

gnprice Feb 8, 2024

Uh oh!

sirpengi commented Feb 9, 2024

Uh oh!

gnprice left a comment

Uh oh!

gnprice Feb 9, 2024

Uh oh!

sirpengi commented Feb 12, 2024

Uh oh!

gnprice commented Feb 12, 2024

Uh oh!

Uh oh!

	static Color markerColor = const HSLColor.fromAHSL(0.5, 0, 0, 0.2).toColor();
	static final markerColor = const HSLColor.fromAHSL(0.5, 0, 0, 0.2).toColor();

		// TODO(#384) for streams, show @-mention indicator when it applies
		if (hasMention) const _AtMentionMarker(),

@-mention markers #509

@-mention markers #509

Uh oh!

Conversation

sirpengi commented Feb 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sirpengi commented Feb 9, 2024

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sirpengi commented Feb 12, 2024

Uh oh!

gnprice commented Feb 12, 2024

Uh oh!

Uh oh!

sirpengi commented Feb 8, 2024 •

edited

Loading