inbox: Style adjustments, after channel folders

[chrisbobbe]

Fixes #350.
Fixes #1528.
Fixes #2108.
Fixes-partly #417.

See Figma: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=16056-6580&m=dev
And discussion (I sent a bunch of questions to Vlad but don't know if he has bandwidth to respond): #mobile-design > channel folders in inbox: design @ 💬

This PR is stacked atop #2186, introducing channel folders in the Inbox. These are the 21 new commits, roughly in chunks of related work, and with some commits marked ❓ for specific things I'd like to resolve before merge:

28a4a4b inbox [nfc]: Remove a resolved TODO about topic-visibility icon color
5983b23 inbox [nfc]: Express rows' outer horizontal padding using Padding widgets

11224ac recent dms [nfc]: Factor out helper widget for DM-conversation avatar/icon
7328a07 recent dms: Use group-DM icons that distinguish number of users
b96d850 inbox: Use DM-conversation avatars in Inbox

2c02924 inbox: Put collapse/uncollapse icon after channel name, shrinking start margin

283ee1f inbox: Put unread-count badges 12px from edge of screen, down from 16px
87f26d0 inbox: Increase vertical padding in folder headers from 8px to 10px
903b18d inbox: Adjust spacing after row text to match Figma
0645cf1 inbox: Adjust text styles to match Figma
d50e91a inbox: Add gradient effect for channel rows
❓ 4432108 inbox: Add top border to channel-folder header
4ec1d0b inbox: Remove "splash" touch feedback

46b2660 text [nfc]: Pull out InlineIcon from iconWidgetSpan
b90765c text [nfc]: Have InlineIcon accept a custom (e.g. clamped) TextScaler
f271f7d inbox/topic_list: Baseline-align markers at row end with each other
59db6dc topic_list: Use vertical padding instead of minHeight in topic rows
0fd98d6 topic_list: Baseline-align topic-row content
0e05867 inbox: Use checkmark icon for resolved topics, like in topic list
8303331 inbox: Use vertical padding instead of minHeight in topic items
29f8cac inbox: Baseline-align topic-row content

---

❓: To resolve before merging this iteration?

• The gradient effect [using solid color when row is collapsed]: (this was resolved in chat: #mobile-design > channel folders in inbox: design @ 💬)
• I don't love the thin gray line above channel-folder headers; I would just drop it.
• In dark mode, the Figma consistently doesn't paint the rows on a surface that's different (darker) than the background. I remember discussing this in the office but I don't remember where we landed on it. Here's a screenshot showing this difference from the Figma (which is in main and unchanged in this revision):

Screenshots coming soon.

cc @alya

[chrisbobbe]

Screenshots

Some of the inbox improvements also apply naturally on the recent-DMs and topic-list pages, so I've included screenshots of those. Specifically fixes for these issues:

• RecentDmConversationsPage: Use group-DM icons that distinguish number of users #350
• Align text and icons to baseline, in topic-list topic items #1528
• "@" marker is sized and colored differently between Inbox page and topic-list page #2108

(the screenshots)

Before (i.e. at #2186)	After

End of screenshots (but I can make more if needed).

[chrisbobbe]

Revision pushed, with a new commit on top: the chevron now only appears for collapsed channel rows, as specced by Vlad: #mobile-design > channel folders in inbox: design @ 💬

I'd prefer not to spend time redoing all the screenshots for this change; for review, can you just imagine this change applied to the existing screenshots?

[chrisbobbe]

Moved out of maintainer review to keep up momentum, because Rajesh mentioned (in a private channel) being less available for the next several days.

[gnprice]

Thanks for taking care of all this! Generally all looks good; various comments below.

[gnprice]

Maybe rename the first of these to group_dm_3? Looks like it fits the pattern — it's 3 dots in the same style as the rest are 4, 5, etc.

[gnprice]

Fun. Is this in the Figma somewhere? (I see an example with 9, using the icon, but not more.) Would you make a screenshot for it?

[chrisbobbe]

Yeah here's the Figma frame: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=4920-2671&m=dev

And I've made some screenshots (which prompted me to add special-casing for 99+ 😝, shown here, by saying "99+" with slightly smaller text; also to disable text-size scaling for the number). I've obviously used faked numbers here rather than create gigantic group DMs:

Light	Dark

[gnprice]

Great, glad that question was productive 🙂

[gnprice]

inbox: Remove "splash" touch feedback

The default effect here has been pretty ugly, to me (on iOS) -- a
gray circle, with a hard edge, that expands rather slowly from the
point of contact. The gray doesn't look nice with the
channel-colored rows.

The color would be easy to fix on its own terms: just pass splashColor. For turning off the splash entirely, I think the others are better reasons.

[chrisbobbe]

The color would be easy to fix on its own terms: just pass splashColor.

True, I guess we could just as easily remove the default gray in channel headers by passing our chosen highlightColor for splashColor instead, and setting highlightColor: Colors.transparent.

The channel-header gradient effect makes any touch-feedback fix easier, because we haven't approved a light/dark color pair that's similar to swatch.barBackground—the solid, opaque color of channel rows before—but different such that it's visible when painted on it. With the gradient effect in place, we can just use (a lower-alpha version of) swatch.barBackground, as in this revision. I think that helps explain why I let the ugly gray sit for so long without just fixing it, despite my irritation. I lost some steam when it seemed we might not want the gradient effect anymore (#2186 (comment) ) and I was glad when that was resolved (#mobile-design > channel folders in inbox: design @ 💬).

[gnprice]

nit: use block body since this is multi-line

(as is, the name InlineIcon gets a bit swallowed here at the end of this line)

[gnprice]

inbox/topic_list: Baseline-align markers at row end with each other

Hmm, I see. Is there a pair of screenshots that shows this difference? (I looked and all the rows I found had at most one of a mention and a visibility icon.)

[gnprice]

and nit: perhaps "inbox, topic_list:"? I think of a slash there as meaning nesting, as in "docs/foo" for edits to docs/foo.md or docs/something/foo.md .

[chrisbobbe]

Sure, here are screenshots before and after just that commit. Quoting the whole commit message for convenience:

inbox, topic_list: Baseline-align markers at row end with each other

Fixes #2108.

And sync the size and color of the icons between the inbox and the
topic list, fixing #2108.

Next up, #1528: in the topic list, we'll baseline-align all of the
row's content.

Before	After

[gnprice]

Thanks! Hmm, the "after" for the topic list looks to have the visibility icons too low, in the rows that don't also have an unread-count badge. The "before" looks reasonable and if anything a bit too low already.

[gnprice]

Then it looks like the two different markers don't change their alignment relative to each other, though they do move relative to the unread-count badge and the topic name.

[chrisbobbe]

Ah thanks for catching this! It's an implementation bug. Here are screenshots from my upcoming revision.

First, the topic list—and this time it's three screenshots: before, between, and after these two consecutive commits:

5f5754531 inbox, topic_list: Baseline-align markers at row end with each other
138e9f715 topic_list: Baseline-align topic-row content

Before	Between	After

[chrisbobbe]

Then, the inbox:

Before and after this commit (labeled 1 and 2):

5f5754531 inbox, topic_list: Baseline-align markers at row end with each other

And before and after this commit (labeled 3 and 4):

ec8a12382 inbox: Baseline-align topic-row content

1	2	3	4

[gnprice]

I think this is clearest when the topic.isResolved condition is right next to the mention of ZulipIcons.check. Otherwise the reader may wonder why we're showing a check icon.

[gnprice]

Yeah. Maybe pull out these details to a widget next to InboxRowTrailingMarkers? Then that widget's _buildIcon can use it too.

[chrisbobbe]

Makes sense; I'll add a commit at the end for this.

[gnprice]

This font size of 17 is also implicitly assumed (via InboxRowTrailingMarkers) to be the same as the inbox uses.

[chrisbobbe]

Makes sense; I'll add a commit at the end for this.

[gnprice]

(same remark as on the topic list; otherwise haven't closely read this commit:
ee4407ae2 inbox: Use vertical padding instead of minHeight in topic items

)

[gnprice]

nit:

and as we just did in the topic-list page in the previous commit.

that was several commits earlier

[chrisbobbe]

Thanks for the review! Revision pushed.

[gnprice]

Thanks! These code changes all look good; a couple of questions below, and it'd be good to tweak one commit message to reflect #2262 (comment) .

[gnprice]

Thanks! Hmm, the "after" for the topic list looks to have the visibility icons too low, in the rows that don't also have an unread-count badge. The "before" looks reasonable and if anything a bit too low already.

[gnprice]

Then it looks like the two different markers don't change their alignment relative to each other, though they do move relative to the unread-count badge and the topic name.

[gnprice]

Great, glad that question was productive 🙂

[chrisbobbe]

Thanks for the review! Revision pushed. This revision adds AdjustBaseline, the custom-render-object widget you proposed in #1528 (comment), with an explanation of how that fixes a layout bug. I think that particular layout bug was masked at the tip commit of my previous revision. Still, best to fix the underlying issue. Masked because:

• When InboxRowTrailingMarkers is a child of a Row with centered cross-axis alignment (channel and DM rows in the inbox), and one of these icons can appear (the "@" in channel rows), the InboxRowTrailingMarkers's height isn't determined by shrink-wrapping to the icon but rather to the unread-count badge, which is always present, and which extends farther up and down than the icon.
• When InboxRowTrailingMarkers is a child of a Row with baseline cross-axis alignment (topic rows in the Inbox and the topic list), the Transform.translate hack was effective.

I made some other changes not discussed yet, so I'll include a fresh set of screenshots:

• The checkmark icon's size is 1px bigger. It carries important information, and I kind of still think we could make it more prominent, especially in dark mode where it looks kind of dim? Not sure about this.
• Removed an unintended margin after the last icon when not followed by an unread-count badge.

Before (i.e. main)	After

[chrisbobbe]

Also see my replies above, with more targeted screenshots from this same revision: #2262 (comment)

[chrisbobbe]

Next on my queue after this PR: tweaking the channel-row gradient effect; see #mobile-design > channel folders in inbox: design @ 💬.

[gnprice]

Cool! This looks great, with small comments below.

After this is in, I think a good follow-up would be to send it upstream. That'll involve writing somewhat more dartdoc, and also tests. The other ingredient needed for that will be to write up an issue that really crisply makes the case (which I think is a good one) for why this belongs in the framework.

[gnprice]

Neat!

I'd forgotten that we already had a draft of this:

kind of a hack, and that it's better to make a widget with a custom
render object, as Greg originally proposed in #1528 and as Zixuan
made a draft of:
  https://chat.zulip.org/#narrow/channel/243-mobile-team/topic/topic.20list.20item.20alignment/near/2173433

Glad you found that and revived it.

[gnprice]

Huh interesting, I don't see covariant used on the couple of existing examples I looked at. (One in our code, one in the framework upstream.) What's the effect of it?

[chrisbobbe]

Thanks for flagging! I think there's not really an effect, good or bad, but it's not needed, so better to leave it out. Here's my reasoning, from reading code and docs (and I challenged myself to do this without Claude 🙂).

The method we're overriding is RenderObjectWidget.updateRenderObject, which is this:

  /// Copies the configuration described by this [RenderObjectWidget] to the
  /// given [RenderObject], which will be of the same type as returned by this
  /// object's [createRenderObject].
  ///
  /// This method should not do anything to update the children of the render
  /// object. That should instead be handled by the method that overrides
  /// [RenderObjectElement.update] in the object rendered by this object's
  /// [createElement] method. See, for example,
  /// [SingleChildRenderObjectElement.update].
  @protected
  void updateRenderObject(BuildContext context, covariant RenderObject renderObject) {}

The covariant there allows subclasses to type the renderObject param as the relevant RenderObject subtype—in our case, RenderAdjustBaseline—rather than RenderObject itself.

• This saves subclass implementers having to spell out control flow against render-object values that are not their specific kind of render object…which should never happen, as long as Flutter upholds its own contract in the dartdoc quoted above: "the given [RenderObject], which will be of the same type as returned by this object's [createRenderObject]".
• On the other hand, this is unsound: because you can pass an AdjustBaseline anywhere that accepts a RenderObjectWidget, and RenderObjectWidget's method declares that it can be passed any RenderObject, it follows that in theory an AdjustBaseline could arrive somewhere that ends up calling the method with a RenderObject that is not a RenderAdjustBaseline. That's true regardless of our annotation of the param as RenderAdjustBaseline.
• So the covariant is a bit of a hack (it silences a class of legitimate complaints from the analyzer), although a helpful one; as I mentioned it saves consumers having to write control flow. When we type the param as RenderAdjustBaseline, we're essentially delegating that control flow to the Dart compiler, I think? because it adds a runtime type check. From the Dart doc:

  Some (rarely used) coding patterns rely on tightening a type by overriding a parameter's type with a subtype, which is invalid. In this case, you can use the covariant keyword to tell the analyzer that you're doing this intentionally. This removes the static error and instead checks for an invalid argument type at runtime.

(I remember talking about how the Flow type system handles function subtyping correctly, unlike TypeScript; this is coming back to me 🙂—or maybe TypeScript has config for this?—something like, the function subtype relation is covariant in the return type and contravariant in the parameter type.)

Anyway, RenderObjectWidget uses this covariant "fudge factor" to reduce cognitive load on subclass implementers. In a quick experiment (because the docs aren't explicit on this), I made toy subclasses of AdjustBaseline and RenderAdjustBaseline, then removed the covariant in our AdjustBaseline that you flagged here, and found that it propagates down the class hierarchy without intermediate subclasses having to reapply it:

Details

diff --git lib/widgets/text.dart lib/widgets/text.dart
index cfddf5d26..a849e3efc 100644
--- lib/widgets/text.dart
+++ lib/widgets/text.dart
@@ -561,7 +561,7 @@ class AdjustBaseline extends SingleChildRenderObjectWidget {
   @override
   void updateRenderObject(
     BuildContext context,
-    covariant RenderAdjustBaseline renderObject,
+    RenderAdjustBaseline renderObject,
   ) {
     renderObject.dy = dy;
   }
@@ -573,6 +573,24 @@ class AdjustBaseline extends SingleChildRenderObjectWidget {
   }
 }
 
+class Foo extends AdjustBaseline {
+  const Foo({super.key, required super.dy});
+
+  // No analyzer error here unless I remove `covariant`
+  // in Flutter's RenderObjectWidget.updateRenderObject
+  @override
+  void updateRenderObject(
+    BuildContext context,
+    RenderFoo renderObject,
+  ) {
+    renderObject.dy = dy;
+  }
+}
+
+class RenderFoo extends RenderAdjustBaseline {
+  RenderFoo({required super.dy});
+}
+
 class RenderAdjustBaseline extends RenderProxyBox {
   RenderAdjustBaseline({required this._dy, RenderBox? child})
     : super(child);

So the covariant in our code isn't even helpful in the unlikely event that we subclass AdjustBaseline.

[chrisbobbe]

Similar story for the covariant that Claude added in RenderAdjustBaseline.computeDryBaseline:

  @override
  @protected
  double? computeDryBaseline(
    covariant BoxConstraints constraints,
    TextBaseline baseline,
  ) {

which overrides RenderBox.computeDryBaseline:

Details

  /// Computes the value returned by [getDryBaseline].
  ///
  /// This method is for overriding only and shouldn't be called directly. To
  /// get this [RenderBox]'s speculative baseline location for the given
  /// `constraints`, call [getDryBaseline] instead.
  ///
  /// The "dry" in the method name means the implementation must not produce
  /// observable side effects when called. For example, it must not change the
  /// [size] of the [RenderBox], or its children's paint offsets, otherwise that
  /// would results in UI changes when [paint] is called, or hit-testing behavior
  /// changes when [hitTest] is called. Moreover, accessing the current layout
  /// of this [RenderBox] or child [RenderBox]es (including accessing [size], or
  /// `child.size`) usually indicates a bug in the implementation, as the current
  /// layout is typically calculated using a set of [BoxConstraints] that's
  /// different from the `constraints` given as the first parameter. To get the
  /// size of this [RenderBox] or a child [RenderBox] in this method's
  /// implementation, use the [getDryLayout] method instead.
  ///
  /// The implementation must return a value that represents the distance from
  /// the top of the box to the first baseline of the box's contents, for the
  /// given `constraints`, or `null` if the [RenderBox] has no baselines. It's
  /// the same exact value [RenderBox.computeDistanceToActualBaseline] would
  /// return, when this [RenderBox] was laid out at `constraints` in the same
  /// exact state.
  ///
  /// Not all [RenderBox]es support dry baseline computation. For example, to
  /// compute the dry baseline of a [LayoutBuilder], its `builder` may have to
  /// be called with different constraints, which may have side effects such as
  /// updating the widget tree, violating the "dry" contract. In such cases the
  /// [RenderBox] must call [debugCannotComputeDryLayout] in an assert, and
  /// return a dummy baseline offset value (such as `null`).
  @visibleForOverriding
  @protected
  double? computeDryBaseline(covariant BoxConstraints constraints, TextBaseline baseline) {

except that in that case I don't currently understand the benefit of the upstream covariant—is subclassing BoxConstraints a useful thing to do?—only (a) that Flutter must be taking responsibility for it being helpful and (b) we don't need to repeat it. 😛

[gnprice]

This last bit reads to me as if "baseline offset" is its own noun phrase, until I reread it.

Perhaps "as offset by [dy] from its child's baseline"?

[gnprice]

Hmm, this will call child.getDryBaseline twice if the first time returns null.

How about calling super at the outset? (Which will be the RenderProxyBox implementation.) I think that makes this code a bit simpler, too.

[chrisbobbe]

Indeed. Hmm, what should happen when the child doesn't have a baseline to report? This is the case with Image for example—RenderImage doesn't implement the relevant compute-baseline methods—so it may be relevant if we want to use AdjustBaseline for image emoji (which we might soon, with #966 being worked on currently).

How about we take the bottom of the child and apply dy to that?

If we want that, then I think we don't want to call super.computeDryBaseline here. The RenderProxyBox implementation does call .getDryBaseline on its child, which makes sense for its role:

  @override
  @protected
  double? computeDryBaseline(covariant BoxConstraints constraints, TextBaseline baseline) {
    final double? result = child?.getDryBaseline(constraints, baseline);
    return result ?? super.computeDryBaseline(constraints, baseline);
  }

But when that returns null (when the child has no baseline) it calls super.computeDryBaseline, which is the RenderBox implementation, which throws:

  @visibleForOverriding
  @protected
  double? computeDryBaseline(covariant BoxConstraints constraints, TextBaseline baseline) {
    assert(
      debugCannotComputeDryLayout(
        error: FlutterError.fromParts(<DiagnosticsNode>[
          ErrorSummary(
            'The ${objectRuntimeType(this, 'RenderBox')} class does not implement "computeDryBaseline".',
          ),
          ErrorHint(
            'If you are not writing your own RenderBox subclass, then this is not\n'
            'your fault. Contact support: https://github.com/flutter/flutter/issues/new?template=02_bug.yml',
          ),
        ]),
      ),
    );
    return null;
  }

Instead of panicking, we can instead ask for the child's height (using the "dry" way, getDryLayout, because we're in computeDryBaseline) and apply dy to that. I'm not sure if there's a perf concern with calling child.getDryLayout? and didn't find examples of doing this; see dartdoc:

  /// Returns the [Size] that this [RenderBox] would like to be given the
  /// provided [BoxConstraints].
  ///
  /// The size returned by this method is guaranteed to be the same size that
  /// this [RenderBox] computes for itself during layout given the same
  /// constraints.
  ///
  /// This function should only be called on one's children. Calling this
  /// function couples the child with the parent so that when the child's layout
  /// changes, the parent is notified (via [markNeedsLayout]).
  ///
  /// This layout is called "dry" layout as opposed to the regular "wet" layout
  /// run performed by [performLayout] because it computes the desired size for
  /// the given constraints without changing any internal state.
  ///
  /// Calling this function is expensive as it can result in O(N^2) behavior.
  ///
  /// Do not override this method. Instead, implement [computeDryLayout].

(And in the non-"dry" method computeDistanceToActualBaseline, we'd return child.size.height + dy when the child has no baseline.)

[chrisbobbe]

In fact the "child-has-no-baseline-what-do-we-do" case kind of seems like…the normal use case for this widget, actually? But Icon doesn't technically qualify because it does have a baseline, which IIUC is always the bottom edge, basically as a side effect of it being implemented with RichText, probably. (Possibly there are nuances involving font metrics, not sure.)

[chrisbobbe]

Thanks for the review! Revision pushed.

[gnprice]

Thanks! Looks good; merging.