inbox: Add "Inbox" page!

[chrisbobbe]

Stacked atop #371.

Still a draft for now, but wanted to share what I have so far. 🙂

[chrisbobbe]

#380 in its current form would fix a small deviation from the Figma. No need to have it block progress on this though.

[gnprice]

(Cross-linking: this fixes #117, right?)

[gnprice]

Thanks! Generally looks good; comments below.

A couple of comments are about efficiency or getting work out of the build methods. But I want to highlight this high-order bit about that aspect:

As long as this feels reasonably responsive in manual testing, though, then the right thing for now is to prioritize speed of writing the code over speed of running it, and just leave a TODO.

Thoughts on the TODO items in the main commit's message:

TODO (before merge?):

- tests

Yeah. For now let's keep the tests simple, just in the interest of prioritizing our other Alpha and Beta 1 work. But it'd be good to have (a) a simple smoke test where there's some unread DMs and some unread stream conversations; and (b) a quick test of the collapse/uncollapse functionality.

(Fancier tests would include that if you collapse one section, then scroll it far offscreen and scroll it back on, it's still collapsed. That's something that's significant to the UX but would fail in a plausible alternate implementation, where the collapsed state was local to the list item for the section.)

- sorting (of streams; and within a stream, of topics)

These are OK to leave as TODOs if adding them to the implementation seems complicated.

- icons:
    https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/design.3A.20.23F117.20.22Inbox.22.20screen/near/1680637
  (that message, and the next)

Will be good to fix but doesn't need to block merging; if we merge before fixing those, we can file follow-up issues for Beta 2.

- currently the whole stream row is tappable to collapse/uncollapse;
  should we limit that to just the area near the
  arrow_down/arrow_right, and have the rest of the row open a
  message list to the stream narrow?

Yeah, I'd be inclined to do the latter. If the Figma design expresses the former (or if this needs further discussion for another reason), then this would be a good question for that chat thread.

- @-mention indicator next to stream/topic unread badge count

Let's file a Beta 2 followup issue for this. Can sort it toward the top of Beta 2 on the priorities board.

- helper widgets; code structure etc.?

Comments on this below.

- commit msg. with "fixes" line

[gnprice]

Some of these others should come from stream too.

[gnprice]

(This may end up coming first from #382.)

[gnprice]

nit: I think I like color.dart a bit better. Partly it's that the word "color" in the singular is the name of a concept — so this is the file that "deals with color". And then partly it's that after experience in zulip-mobile where it was often hard to predict or remember whether a name was in the singular or plural, I've come to prefer resolving any ambiguity there in favor of singular.

[gnprice]

Can this be private? (Or @visibleForTesting?)

[chrisbobbe]

Sure, it can be @visibleForTesting. If I make it private, by prefixing with _, I get analysis feedback saying "Invalid use of a private type in a public API" in several places. Such as:

class AllDmsHeaderItem extends StatelessWidget {
  const AllDmsHeaderItem({
    super.key,
    required this.count,
    required this.collapsed,
    required this.pageState, // <-- here
  });

  final int count;
  final bool collapsed;
  final _InboxPageState pageState; // <-- here

[gnprice]

Ah, I guess that points toward this other comment then 🙂 : #381 (comment)

[gnprice]

Suggested change

	unreadsModel?.removeListener(_modelChanged);
	unreadsModel = newStore.unreads..addListener(_modelChanged);
	recentDmConversationsModel = newStore.recentDmConversationsView
	..addListener(_modelChanged);
	unreadsModel?.removeListener(_modelChanged);
	unreadsModel = newStore.unreads..addListener(_modelChanged);
	recentDmConversationsModel?.removeListener(_modelChanged);
	recentDmConversationsModel = newStore.recentDmConversationsView
	..addListener(_modelChanged);

right?

[chrisbobbe]

Yep, thanks for the catch!

[gnprice]

Suggested change

	// We also update some state that lives elsewhere: we reset a collapsible
	// row's collapsed state when it's cleared of unreads.
	// We also update some state that lives locally: we reset a collapsible
	// row's collapsed state when it's cleared of unreads.

"elsewhere" is a bit confusing because it can sound like it means somewhere other than here (whereas you mean other than those two models mentioned above).

[gnprice]

Hmm I see, I guess the bit in _modelChanged above is just the tip of an iceberg here. The ideal version of this would definitely involve a separate view-model class, updated incrementally on events rather than recomputed in full each time something changes.

As long as this feels reasonably responsive in manual testing, though, then the right thing for now is to prioritize speed of writing the code over speed of running it, and just leave a TODO. So:

Suggested change

	final sections = [];
	// TODO(perf) make an incrementally-updated view-model for InboxPage
	final sections = [];

[gnprice]

The StickyHeaderItem can be pushed down inside AllDmsSection. That'd be a bit cleaner because the latter is already constructing the same AllDmsHeaderItem widget, to include in its Column.

[gnprice]

Suggested change

	final title = switch (narrow.otherRecipientIds) {
	final title = switch (narrow.otherRecipientIds) { // TODO dedupe with [RecentDmConversationsItem]

and add a similar comment on that end — that way at least if we look at editing one side of this, we'll be aware of the other one.

[gnprice]

The bulk of this code would be good to share with StreamHeaderItem. Basically there's a bunch of code for the layout and visual design, and it takes several inputs: bar color, icon color, IconData icon, title, baseStreamColor. Computing those inputs will be separate logic for a stream vs. all-DMs.

Three reasonable ways to organize that sharing:

• The widgets StreamHeaderItem and AllDmsHeaderItem remain, but they invoke a shared widget _HeaderItemLayout that takes those inputs. Similar to our _ComposeBoxLayout.
• There's a single widget class _HeaderItem, taking… hmm I guess this aspect gets annoying for this option, as we want to take either a stream or "all DMs". Could be expressed like int? streamId, but that's unpleasantly implicit.
  • OTOH this option works well for TopicItem vs. DmItem — can take a SendableNarrow narrow.
  • Then the build method computes these inputs as locals, initialized by the two cases of a switch.
• I guess here's a third way: an abstract base class _HeaderItem, and these two become subclasses. The inputs are abstract methods or getters on the base class, implemented by the subclasses.
  • This gets awkward in that the icon color and bar color want to share computation of the clamped color.

So probably the easiest way here is the shared helper _HeaderItemLayout.

[chrisbobbe]

Would the shared helper _HeaderItemLayout take an int param for the unread count?

[gnprice]

Ah yeah, that too. And I guess the onTap callback as well.

[chrisbobbe]

I wasn't seeing a smooth path to the first way, so in my next revision I do something I found easy enough. It's basically option 3 (like the abstract MessageActionSheetMenuItemButton and its subclasses in lib/widgets/action_sheet.dart), except the awkward bit is solved. The icon color and bar color do share computation of the clamped color. As a bonus, all of a stream's unread-count badge instances share computation of their color, too. (There are a lot of those, when a stream has unreads across many topics.) A ColorSwatch of colors related to the stream color is memoized on the Subscription object.

[gnprice]

Can these widget classes be private?

If they remain public, they should get more specific names that make it clear they're for the inbox. E.g., InboxAllDmsHeaderItem. That will mitigate having them show up in IDE autocomplete when working in unrelated parts of the code.

[chrisbobbe]

Thanks for the review! Revision pushed, and please see my reply at #381 (comment).

[gnprice]

Thanks! A few comments below, but otherwise this all looks good — it just needs tests. (Which can be kept pretty simple, as described above at #381 (review) .)

We've talked about several TODO items here that you're going to file as issues for Beta 2.

---

As I mentioned in our call today, I played with this on my Pixel 5 to see the performance, using a test user on chat.zulip.org who has a ton of unreads. In debug mode the performance was pretty choppy — occasional pauses while scrolling that I believe reflected a new section scrolling into view, so that with the current version the whole section has to be built and laid out, which can be hundreds of items for different topics. Those pauses in debug mode were sometimes around a second long.

It's fine for debug mode to be choppy, though, so I tried it in profile mode. There there's still enough jank to be noticeable, but it's a lot milder — mostly a couple of dropped frames at a time, sometimes like 150ms. So this level of performance is fine for Beta 1, but it's something we'll want to revisit for Beta 2.

Probably that means I'll rework something in StickyHeaderListView to make it possible to have a separate list item for each conversation, just like you did in a previous draft, and without the glitchy interactions that that led to between one section's header and the next. (Effectively the current StickyHeaderListView assumes that each header is no taller than the item it belongs to; that's true in the message list, and it's true here by making each entire section into an item, but it's not true if each conversation is its own item.)

[gnprice]

Yeah, agreed on both counts — this doesn't feel like the right home, but it's fine for now (for beta 1) and I can try refactoring it later.

[gnprice]

The cast is needed because backgroundColor is a field, which means as far as the language semantics are concerned it's a getter — and in principle this could be on some subclass which overrides the backgroundColor getter with something that doesn't return the same thing every time.

One way to handle this is to destructure in the pattern-matching:

Suggested change

	final effectiveBackgroundColor = switch (backgroundColor) {
	// TODO why is the "as" casting needed?
	StreamColorSwatch() => (backgroundColor as StreamColorSwatch).unreadCountBadgeBackground,
	final effectiveBackgroundColor = switch (backgroundColor) {
	StreamColorSwatch(:var unreadCountBadgeBackground) => unreadCountBadgeBackground,

Or can avoid repeating the long name:

      StreamColorSwatch(unreadCountBadgeBackground: var color) => color,

The other solution is to make a local:

    final backgroundColor = this.backgroundColor;
    final effectiveBackgroundColor = switch (backgroundColor) {
      StreamColorSwatch() => backgroundColor.unreadCountBadgeBackground,

[chrisbobbe]

Makes sense! Thanks.

[gnprice]

Suggested change

	_ => const Color.fromRGBO(102, 102, 153, 0.15),
	null => const Color.fromRGBO(102, 102, 153, 0.15),

The explicitness seems helpful. And the type-checker verifies that it's still exhaustive.

[chrisbobbe]

Thanks for the review! Revision pushed (edit: er, now it's pushed, a moment after posting this). Oh and here's a screenshot; @alya and @terpimost FYI:

---

And for Greg:

• I rebased past Subscription api events #382 (which helpfully made PerAccountStoreTestExtension.addSubscriptions easy to implement; thanks @sirpengi!)
• I did a first pass at ordering streams and topics, and I don't think it caused significant extra lag in my manual testing, but definitely will be good to get that work out of the build method.
• I added and used one more color on StreamColorSwatch: the one named iconOnPlainBackground. (I think I renamed some others since the last revision.)
• I added some simple tests. I tried to write them so that they wouldn't break when we reimplement with separate StickyHeaderListView items per topic and DM conversation (Inbox: Reduce some scrolling jank by making one StickyHeaderListView item per topic / DM conversation #389). In particular, I didn't give them access to our custom widgets like _StreamHeaderItem and _TopicItem. It might help to give the tests that access eventually; e.g., the recent-DMs widget tests are helped in their work by having access to RecentDmConversationsItem. But I felt like it might be good to hold off for now, especially with Inbox: Reduce some scrolling jank by making one StickyHeaderListView item per topic / DM conversation #389 on the horizon.

[gnprice]

Thanks! This generally all looks good; a couple of comments below.

Since this is so close, in the interest of efficiency (and since you're focusing on the next feature, the reactions UI), I'll just make the tweaks to fix these and merge.

[gnprice]

Suggested change

	// On how to extract expected results from the replit, see:
	// TODO

Or write a line or two sketching how you did it. But if not, I think the basic idea can be figured out from the example of the other group above.

[chrisbobbe]

Oh! This time I did save my work. 🙂 I meant to come back to this before submitting my revision; oops.

Here's the code I added in the replit, just under ZULIP_ASSIGNMENT_COLORS. It duplicates some of the logic, where it wasn't exposed with a convenient helper function. But it can be checked against the implementation.

const dartColorIntFromHexString = (hexString) => {
  const lowercaseWithoutHash = hexString.substring(1).toLowerCase();
  switch (lowercaseWithoutHash.length) {
    case 6: // opaque (alpha omitted)
      return `0xff${lowercaseWithoutHash}`;
    case 8: // not opaque
      // convert rrggbbaa to aarrggbb
      return '0x'
        + lowercaseWithoutHash.substring(6, 8)
        + lowercaseWithoutHash.substring(0, 2);
    default:
      throw new Error();
  }
};

const dartCheckLine = (baseHex, expectedHex) => {
  return `runCheck(${dartColorIntFromHexString(baseHex)}, const Color(${dartColorIntFromHexString(expectedHex)}));\n`;
};

let iconOnPlainBackgroundResult = '';
let iconOnBarBackgroundResult = '';
let barBackgroundResult = '';
ZULIP_ASSIGNMENT_COLORS.forEach(color => {
  const correctedColor = correctStreamColor({ color });
  const barBackground = getRecipientBarColor({ color: correctStreamColor({ color })});
  const iconOnPlainBackground = correctedColor;
  const iconOnBarBackground = colord(correctedColor).darken(0.12).toHex();

  iconOnPlainBackgroundResult += dartCheckLine(color, iconOnPlainBackground);
  iconOnBarBackgroundResult += dartCheckLine(color, iconOnBarBackground);
  barBackgroundResult += dartCheckLine(color, barBackground);
});

console.log(iconOnPlainBackgroundResult);
console.log(iconOnBarBackgroundResult);
console.log(barBackgroundResult);

[chrisbobbe]

Pushed 640caef linking to my message here with that code.

[gnprice]

Great, thanks!

[gnprice]

From the [List.sort] doc:

The sort function is not guaranteed to be stable, so distinct objects that compare as equal may occur in any order in the result

That means that in sorting twice, the first sort may not have a useful effect. Instead, we'll want a single sort that checks one condition and then tie-breaks with the other.

(It seems to work in manual testing ­— so possibly the current implementation in the Dart VM is indeed stable. But that might change.)

[chrisbobbe]

Ah OK, makes sense.

[gnprice]

Suggested change

	return bLastUnreadId - aLastUnreadId;
	return bLastUnreadId.compareTo(aLastUnreadId);

Using subtraction for comparison makes me a bit nervous with the possibility of overflow. Not super likely for Zulip message IDs, but doesn't seem impossible in principle, and we might as well make an explicit comparison anyway.

[gnprice]

… I guess here's one good reason to think overflow will never happen: these message IDs are all over the web client, represented there as plain JS numbers. So they need to fit into 54 bits in order to not break web.

Still, compareTo feels like it says a bit more directly what we mean here.

[chrisbobbe]

Thanks for the review and those fixes!