RecentDmConversationsPage: Show unread counts #334

chrisbobbe · 2023-10-25T02:52:47Z

Fixes: #328

chrisbobbe · 2023-10-25T15:29:49Z

test/widgets/recent_dm_conversations_test.dart

+        // When the count is zero, don't show an unread-count widget. Do keep a
+        // "0" placeholder, to approximately match the width of a nonzero
+        // one-digit unread count, given font-size settings etc. That's to
+        // reduce layout jumping of the title text (which wraps and is
+        // truncated) in the common case where a DM conversation goes between
+        // zero and a small number of unreads.


There might be a way to test this without depending so much on the implementation (in particular the use of the Visibility widget with specific params). But I don't yet know how that might look.

I think a lower-tech solution to this would be good: instead of having the whole unread-count marker widget tree be there when there are no unreads, just reserve some constant width in pixels. Can do it with ConstrainedBox and BoxConstraints.minWidth, so that it always reserves at least that much width even if the unread-count marker is slightly narrower.

One reason that would be good is that it avoids having a bunch of widgets there getting built when there's nothing we want to show.

But another is that that should be more effective at reducing jumping. In this version, the width consumed can actually get wider when all the messages get read — for example "0" is wider than "1" in this font (and basically any font that's not explicitly monospace or tabular-numerals).

(Also it's fine if things sometimes jump slightly here — it can't happen as a consequence of anything the user does on this screen, and so it won't be common that it happens at all while the user is looking at it.)

instead of having the whole unread-count marker widget tree be there when there are no unreads, just reserve some constant width in pixels.

Hmm. If it really is constant, then we can't avoid it being consistently wrong (maybe very wrong) for some users, right? The width should approximate the width of a single-digit unread counter, and that width can change dramatically with the user's choice of font size, and I assume it can change some with the user's choice of whether to force bold.

If we can access the effective font size (accounting for the user's font-size setting), we can probably use a width with a component that's proportional to that font size. Ignoring the user's force-bold setting, I assume the width of a single digit will be proportional to the effective font size, and maybe we can find the ratio experimentally? It helps that we're providing the font ("Source Sans 3"); I think the user can't clobber that with their own choice of font but I actually don't know for sure.

I had enough uncertainty there that it seemed simplest to just do the layout but make the counter invisible. Since performance is a concern, would it be easy to do the layout work just once and reuse it for all the conversations with zero unreads?

Well, I think most of all it comes down for me to the fact that it's fine if things sometimes jump slightly here (because it'll be rare for it to do so while the user is looking), and that reserving a constant width is much simpler than any solution involving a placeholder widget.

The placeholder widget doesn't completely eliminate jumping anyway, because the width varies between different values of the counter.

If the width of a counter is increased for a given user because they have a larger font size, that's actually a situation where it's probably best if we don't reserve space for the counter, and failing that we should at least not increase the space we reserve. That's because the increased font size means that space is at a premium for the text that actually is on the screen and they want to read. There's some discussion in a slightly different context here:
https://github.com/zulip/zulip-mobile/blob/main/docs/background/webview.md#why-mostly-px-and-not-rem

If the width of a counter is increased for a given user because they have a larger font size, that's actually a situation where it's probably best if we don't reserve space for the counter, and failing that we should at least not increase the space we reserve. That's because the increased font size means that space is at a premium for the text that actually is on the screen and they want to read. There's some discussion in a slightly different context here:
https://github.com/zulip/zulip-mobile/blob/main/docs/background/webview.md#why-mostly-px-and-not-rem

Oh, good point. Also: one thing we haven't considered is just not reserving empty space for the counter in any case. That would depart from Vlad's design in Figma (so tagging @terpimost), but as you said:

it's fine if things sometimes jump slightly here (because it'll be rare for it to do so while the user is looking)

and it is nice to not cut off text that we have room to show.

chrisbobbe · 2023-10-25T15:30:51Z

lib/widgets/recent_dm_conversations.dart

+  @override
+  void dispose() {
+    model?.removeListener(_modelChanged);
+    super.dispose();
+  }
+


RecentDmConversationsPage: Add missing dispose override to state

In theory this is possible to add a regression test for, but I wonder if it's more trouble than it's worth.

gnprice

Thanks! Generally this looks good; comments below.

Would you also take a screenshot of this and post it? That would help @terpimost take a look at the design. I'll give him a head start, though, by saying that I did look at the design in Figma (from the link in the issue) and I believe you've captured all the details faithfully from there 🙂

gnprice · 2023-10-26T00:44:10Z

lib/widgets/recent_dm_conversations.dart

+  void dispose() {
+    model?.removeListener(_modelChanged);
+    unreadsModel?.removeListener(_unreadsModelChanged);
+    super.dispose();


Oops yeah good catch.

I just did a quick scan of the other onNewStore overrides, and it looks like they have this covered.

Arguably it'd be helpful to add a reminder on the onNewStore doc that one may want to have dispose dispose of the state the onNewStore maintains. … But State.didChangeDependencies doesn't have any such admonition. Maybe it's best to just say that's a general pattern of writing a State subclass, and an extra reminder would be clutter and not likely to get read at the right moment to help.

One nit: let's put dispose after onNewStore, similar to what we do elsewhere, so that the lifecycle methods are in rough order of when they're called.

gnprice · 2023-10-26T00:47:33Z

lib/widgets/recent_dm_conversations.dart

+  void _unreadsModelChanged() {
+    setState(() {
+      // The actual state lives in [unreadsModel].
+      // This method was called because that just changed.
+    });
+  }


Simpler to unify these, I think:

Suggested change

void _unreadsModelChanged() {

setState(() {

// The actual state lives in [unreadsModel].

// This method was called because that just changed.

});

}

void _modelChanged() {

setState(() {

// The actual state lives in [model] and [unreadsModel].

// This method was called because one of those just changed.

});

}

gnprice · 2023-10-26T00:57:29Z

lib/widgets/recent_dm_conversations.dart

+          final narrow = sorted[index];
+          return RecentDmConversationsItem(
+            narrow: narrow,
+            unreadCount: unreadDms[narrow]?.length ?? 0,


Let's factor this out as a method on Unreads. Say int countInDmNarrow(DmNarrow narrow), and ordered after the fields and before the handleFoo methods.

Then for #130 (and the "N unreads" banner that goes with it) we'll naturally add similar methods like countInTopicNarrow etc., and possibly a convenience wrapper like countInNarrow that switches between them.

It occurs to me that it might be ideal for the pieces of the unreads model like dms and streams to be private, or @visibleForTesting. But we can see whether that feels right as we add more things consuming this model.

👀 on this, I've got sirpengi@237ea8f for my own usage in my work on #130

It occurs to me that it might be ideal for the pieces of the unreads model like dms and streams to be private, or @visibleForTesting. But we can see whether that feels right as we add more things consuming this model.

I think we'll want the unreads model to expose unread IDs so that UI code can do some filtering based on mute state, which will be tracked elsewhere.

I think we'll want the unreads model to expose unread IDs so that UI code can do some filtering based on mute state, which will be tracked elsewhere.

Hmm yeah, good point.

For the counts that currently require traversing the model and doing sums (so, total unreads and unreads per stream), I wonder about instead adding properties that the model maintains, and updating them when the model handles events etc. Then UI-code callers don't have to worry about a lot of repetitive counting work being done when they request a count.

I've sent the draft #345 for one approach to tracking those numbers.

gnprice · 2023-10-26T03:20:25Z

lib/widgets/recent_dm_conversations.dart

@@ -102,7 +139,27 @@ class RecentDmConversationsItem extends StatelessWidget {
              overflow: TextOverflow.ellipsis,
              title))),
          const SizedBox(width: 8),
-          // TODO(#253): Unread count
+          Visibility.maintain(


Hmm, fancy.

Reading the definition of what this does:

/// This is equivalent to the default [Visibility] constructor with all /// "maintain" fields set to true. This constructor should be used in place of

and among those

/// Whether to maintain the semantics for the widget when it is hidden (e.g. /// for accessibility). /// /// To set this, [maintainSize] must also be set. /// /// By default, with [maintainSemantics] set to false, the [child] is not /// visible to accessibility tools when it is hidden from the user. If this /// flag is set to true, then accessibility tools will report the widget as if /// it was present. final bool maintainSemantics;

it doesn't seem like that's something we want — we don't want to tell the user there's a "0" there for all the conversations with no unreads.

Probably also don't want maintainInteractivity, though that one may have no effect here.

gnprice · 2023-10-26T03:41:04Z

test/widgets/recent_dm_conversations_test.dart

+        // When the count is zero, don't show an unread-count widget. Do keep a
+        // "0" placeholder, to approximately match the width of a nonzero
+        // one-digit unread count, given font-size settings etc. That's to
+        // reduce layout jumping of the title text (which wraps and is
+        // truncated) in the common case where a DM conversation goes between
+        // zero and a small number of unreads.


I think a lower-tech solution to this would be good: instead of having the whole unread-count marker widget tree be there when there are no unreads, just reserve some constant width in pixels. Can do it with ConstrainedBox and BoxConstraints.minWidth, so that it always reserves at least that much width even if the unread-count marker is slightly narrower.

One reason that would be good is that it avoids having a bunch of widgets there getting built when there's nothing we want to show.

But another is that that should be more effective at reducing jumping. In this version, the width consumed can actually get wider when all the messages get read — for example "0" is wider than "1" in this font (and basically any font that's not explicitly monospace or tabular-numerals).

gnprice · 2023-10-26T03:41:58Z

lib/widgets/recent_dm_conversations.dart

+                  unreadCount.toString()),
+              )),
+          ),


nit:

Suggested change

unreadCount.toString()),

)),

),

unreadCount.toString())))),

gnprice · 2023-10-26T03:43:44Z

test/widgets/recent_dm_conversations_test.dart

+        // When the count is zero, don't show an unread-count widget. Do keep a
+        // "0" placeholder, to approximately match the width of a nonzero
+        // one-digit unread count, given font-size settings etc. That's to
+        // reduce layout jumping of the title text (which wraps and is
+        // truncated) in the common case where a DM conversation goes between
+        // zero and a small number of unreads.


(Also it's fine if things sometimes jump slightly here — it can't happen as a consequence of anything the user does on this screen, and so it won't be common that it happens at all while the user is looking at it.)

gnprice · 2023-10-26T03:56:12Z

lib/widgets/recent_dm_conversations.dart

+                    fontFamily: 'Source Sans 3',
+                    fontSize: 16,
+                    height: (18 / 16),
+                    fontFeatures: [FontFeature.enable('smcp')], // small caps


Hmm interesting — did you find it say "small caps" somewhere in the Figma design? I agree that this looks like the right match visually (and it doesn't look right without the small caps), but I'm not finding it in the metadata:

It shows up for me in Figma's "dev mode" but I didn't find it otherwise:

Hmm! I guess I should give "dev mode" a try, then. I got a modal recently advertising it as a beta feature, but hadn't tried it out.

gnprice · 2023-10-26T03:58:48Z

test/widgets/recent_dm_conversations_test.dart

+        store.handleEvent(
+          UpdateMessageFlagsAddEvent(id: message.id, flag: MessageFlag.read, all: false, messages: [message.id]),
+        );


nits: line too long; and the event ID isn't related to a message ID. Here's a tweaked version:

Suggested change

store.handleEvent(

UpdateMessageFlagsAddEvent(id: message.id, flag: MessageFlag.read, all: false, messages: [message.id]),

);

store.handleEvent(UpdateMessageFlagsAddEvent(

id: 1, flag: MessageFlag.read, all: false, messages: [message.id]));

chrisbobbe · 2023-10-26T15:18:42Z

Would you also take a screenshot of this and post it? That would help @terpimost take a look at the design. I'll give him a head start, though, by saying that I did look at the design in Figma (from the link in the issue) and I believe you've captured all the details faithfully from there 🙂

Sure! Here, @terpimost:

As Greg proposes: zulip#334 (comment)

chrisbobbe · 2023-10-27T22:46:34Z

Thanks for the review! Revision pushed.

About the subthread at #334 (comment): this revision has a simple countInDmNarrow method, but (since my #345 was closed) I'm happy to incorporate @sirpengi's proposal in sirpengi/zulip-flutter@237ea8fee if that's helpful at this point, perhaps with some variable name changes and tests.

gnprice

Thanks for the revision! All looks good with a few small comments below, one of which is a small design/layout question which I'll raise in chat.

gnprice · 2023-10-30T18:45:12Z

lib/widgets/recent_dm_conversations.dart

+            // TODO(i18n) formatting, which we might need the locale for.
+            //   For 9000, do we want 9000 or 9,000 or 9k or 9K or
+            //   (as in Figma) 999+?
+            unreadCount: unreadsModel!.countInDmNarrow(narrow),


In web we give the exact figure, so I think we want the same here. In particular that way you can see it go down as you read things, even if the total is large. (Not super likely for a DM conversation in particular, but makes sense to be consistent.)

Specifically it looks like we say "9000". So we can just do the same here without a TODO. If we later decide to change that UI choice, we can come back to it (and make it locale-sensitive so it's "9.000" in some locales, etc.).

bump — no need for a TODO comment for this

Ah oops; forgot about this, thanks.

gnprice · 2023-10-30T18:49:40Z

lib/widgets/recent_dm_conversations.dart

+            ? Container(
+                margin: const EdgeInsetsDirectional.only(start: 8, end: 16),


Suggested change

? Container(

margin: const EdgeInsetsDirectional.only(start: 8, end: 16),

? Padding(

padding: const EdgeInsetsDirectional.only(start: 8, end: 16),

That's what Container.margin boils down to — see the implementation of Container.build. And I find it a lot clearer to understand this way, particularly because it avoids sounding like CSS's "margin", which is a lot more complicated.

gnprice · 2023-10-30T18:59:29Z

lib/widgets/recent_dm_conversations.dart

+                        color: Color(0xFF222222),
+                      ).merge(weightVariableTextStyle(context)),
+                      unreadCount.toString()))))
+            : const SizedBox(width: 12), // match whitespace at start of row


Hmm yeah good question. I'll start a quick chat thread to get @terpimost's feedback on this bit — I don't see an answer for it in the Figma design.

→ thread here, as a continuation of the existing topic for this screen from before adding these unread counts.

gnprice · 2023-10-30T19:09:05Z

About the subthread at #334 (comment): this revision has a simple countInDmNarrow method, but (since my #345 was closed) I'm happy to incorporate @sirpengi's proposal in sirpengi/zulip-flutter@237ea8fee if that's helpful at this point, perhaps with some variable name changes and tests.

I'll prefer for that to be structured in a way where this more-specific method is still exposed too:

we'll naturally add similar methods like countInTopicNarrow etc., and possibly a convenience wrapper like countInNarrow that switches between them.

So this is fine, and the work on #130 can build on that.

As Greg proposes: zulip#334 (comment)

chrisbobbe · 2023-10-31T13:40:09Z

Thanks for the review! Revision pushed, and we can await Vlad's response in that CZO thread.

chrisbobbe · 2023-10-31T13:54:30Z

And Vlad responded that the symmetric 12px is good, as in this revision.

chrisbobbe · 2023-10-31T14:58:06Z

Revision pushed, adjusting the gap between the right edge of the text and the counter, from the Figma's 8px to 12px, following Vlad's advice in chat.

gnprice

Thanks! All looks good with one nit; then please go ahead and merge.

gnprice · 2023-10-31T18:09:04Z

lib/widgets/recent_dm_conversations.dart

+            // TODO(i18n) formatting, which we might need the locale for.
+            //   For 9000, do we want 9000 or 9,000 or 9k or 9K or
+            //   (as in Figma) 999+?
+            unreadCount: unreadsModel!.countInDmNarrow(narrow),


bump — no need for a TODO comment for this

There's about to be another kind of content to test for: a DM conversation's unread count.

As Greg proposes: zulip#334 (comment)

As specified by Vlad: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/design.3A.20DM-conversation.20list/near/1672137

This follows Vlad's suggestion to adjust the gap between the right edge of the text and the counter, from the Figma's 8px to 12px: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/design.3A.20DM-conversation.20list/near/1672155 Fixes: zulip#328

As Greg proposes: #334 (comment)

chrisbobbe · 2023-10-31T19:18:22Z

Thanks! Merged, fixing that nit.

chrisbobbe · 2023-10-31T19:19:16Z

Ah and for the thread: copy-pasting from CZO a screenshot with the layout we ended up with in this PR:

chrisbobbe requested a review from gnprice October 25, 2023 02:52

chrisbobbe force-pushed the pr-recent-dms-unreads branch from a76787a to fbbf4ab Compare October 25, 2023 02:56

chrisbobbe commented Oct 25, 2023

View reviewed changes

gnprice reviewed Oct 26, 2023

View reviewed changes

This was referenced Oct 26, 2023

RecentDmConversationsPage: Breaks with nonzero horizontal device insets #339

Closed

unreads: Track total and per-stream unread counts, in addition to IDs #345

Closed

chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this pull request Oct 27, 2023

unreads: Add countInDmNarrow method

629b6a7

As Greg proposes: zulip#334 (comment)

chrisbobbe force-pushed the pr-recent-dms-unreads branch from fbbf4ab to e2a31c8 Compare October 27, 2023 22:46

gnprice reviewed Oct 30, 2023

View reviewed changes

chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this pull request Oct 31, 2023

unreads: Add countInDmNarrow method

8cccea3

As Greg proposes: zulip#334 (comment)

chrisbobbe force-pushed the pr-recent-dms-unreads branch from e2a31c8 to 33acff2 Compare October 31, 2023 13:37

chrisbobbe force-pushed the pr-recent-dms-unreads branch from 33acff2 to 6c77b7c Compare October 31, 2023 13:53

chrisbobbe force-pushed the pr-recent-dms-unreads branch from 6c77b7c to 387916d Compare October 31, 2023 14:56

gnprice reviewed Oct 31, 2023

View reviewed changes

chrisbobbe added 7 commits October 31, 2023 14:11

RecentDmConversationsPage: Add missing dispose override to state

30e332e

test [nfc]: Make a helper more compact

45487c9

recent_dm_conversations test: Retitle some tests to be more specific

12c6a0c

There's about to be another kind of content to test for: a DM conversation's unread count.

unreads: Add countInDmNarrow method

386cfbd

As Greg proposes: zulip#334 (comment)

RecentDmConversationsPage: Make start/end margins symmetric, at 12px

65641a7

As specified by Vlad: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/design.3A.20DM-conversation.20list/near/1672137

RecentDmConversationsPage [nfc]: Add TODO for muting users

44e6642

chrisbobbe force-pushed the pr-recent-dms-unreads branch from 387916d to 44e6642 Compare October 31, 2023 18:14

chrisbobbe merged commit c32ab06 into zulip:main Oct 31, 2023

chrisbobbe added a commit that referenced this pull request Oct 31, 2023

unreads: Add countInDmNarrow method

a657f29

As Greg proposes: #334 (comment)

chrisbobbe deleted the pr-recent-dms-unreads branch October 31, 2023 19:18

gnprice mentioned this pull request Nov 2, 2023

Mark as read #364

Merged

		? Container(
		margin: const EdgeInsetsDirectional.only(start: 8, end: 16),

RecentDmConversationsPage: Show unread counts #334

RecentDmConversationsPage: Show unread counts #334

Uh oh!

Conversation

chrisbobbe commented Oct 25, 2023

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!

chrisbobbe Oct 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Oct 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

chrisbobbe Oct 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Oct 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Oct 26, 2023

Uh oh!

chrisbobbe commented Oct 27, 2023 • 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!

gnprice Oct 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

chrisbobbe Oct 26, 2023 •

edited

Loading

chrisbobbe Oct 26, 2023 •

edited

Loading

chrisbobbe Oct 26, 2023 •

edited

Loading

chrisbobbe Oct 27, 2023 •

edited

Loading

chrisbobbe commented Oct 27, 2023 •

edited

Loading

gnprice Oct 30, 2023 •

edited

Loading

chrisbobbe commented Oct 31, 2023 •

edited

Loading