Decrease whitespace at end of message feed #1628

chrisbobbe · 2025-06-24T19:57:18Z

This does two things:

Shrinks a 36px gap to 12px (the gap below the mark-as-read button)
Removes 11px bottom padding on the last message in the feed (but keeps it when followed by a recipient header; see local echo (7/7): Support simplified version of local echo #1453 (comment))

Proposed by Tim in chat:

#mobile > space at end of thread @ 💬

I bet those two tweaks might fully address this issue.

Screenshots coming soon.

chrisbobbe · 2025-06-24T20:01:06Z

Dark mode:

Current	11px to 0px	11px to 5px

chrisbobbe · 2025-06-24T20:03:42Z

Light mode:

Before	11px to 0px	11px to 5px

chrisbobbe · 2025-07-07T19:26:20Z

Updated, this time decreasing the 11px to 5px instead of making it zero. I've kept the screenshots from the previous revision, though, and added ones from this revision. PTAL!

alya · 2025-07-07T20:13:36Z

5px seems like a fine adjustment to me.

gnprice

Thanks for taking care of this! Comments below.

gnprice · 2025-07-07T22:51:12Z

lib/model/message_list.dart

-      || oneMessagePerBlock
-      || !haveSameRecipient(prevMessage, message)
-    ) {
+    if (prevMessage == null) {
      items.add(MessageListRecipientHeaderItem(message));
      canShareSender = false;
    } else {
      assert(items.last is MessageListMessageBaseItem);


msglist [nfc]: Pull out some asserts that also apply when haveSameRecipient These invariants apply regardless of `haveSameRecipient`.

That also apply when not haveSameRecipient, right?

(The case where they were previously being asserted is one where haveSameRecipient is always true.)

gnprice · 2025-07-07T22:53:31Z

lib/widgets/message_list.dart

+        if (item.isLastInBlock && !item.isLastInFeed)
+          const SizedBox(height: 11)
+        else if (item.isLastInFeed)
+          const SizedBox(height: 5),


nit: can swap these cases to simplify one of the conditions

gnprice · 2025-07-07T22:59:00Z

lib/model/message_list.dart


-      if (!messagesSameDay(prevMessageItem.message, message)) {
-        items.add(MessageListDateSeparatorItem(message));
+      prevMessageItem.isLastInFeed = false;


I'd rather avoid making this _addItemsForMessage method more complicated if we can help it — it has a complicated job already as it is.

The question "is this the last item in the feed" seems like it can be answered easily within the MessageList widget, by looking at the index in the list. After all, the invariant says:

..isLastInFeed.equals(i == model.items.length);

(where i is the index after the item in question).

So instead how about:

MessageItem gains a boolean field isLastInFeed.

_buildItem gains a matching parameter.

Its two call sites compare itemIndex to totalItems to determine that parameter.

Sure, that's reasonable; I've done this in my new revision.

It puts a bit more of the work in widget code, which doesn't have tests for this new padding logic; would that be a blocker to merging?

Yeah, ideally there'd be a widget test or two for this. (I guess in particular one for the usual case, and one for the case where the bottom sliver is empty but the top is nonempty, checking effectively that that first _buildItem call site is doing the right thing.)

If that isn't quick to write, though, it's not a blocker for merging.

See discussion linked in the comment.

chrisbobbe · 2025-07-07T23:56:30Z

Thanks! Revision pushed.

I've left in the two NFC commits because they seemed potentially helpful, but are now orthogonal to the interesting changes here:

359b99a msglist [nfc]: Make a common codepath for prevMessage != null
9c17fca msglist [nfc]: Pull out asserts that also apply at start of new block

Happy to remove or revise though if helpful.

gnprice

Thanks for the revision! Just small comments.

gnprice · 2025-07-08T00:16:51Z

lib/widgets/message_list.dart

          narrow: widget.narrow,
          header: header,
-          item: data);
+          item: data,
+          isLastInFeed: isLastInFeed);


nit: metadata before payload / child-like argument (so here, isLastInFeed before header and item)

gnprice · 2025-07-08T00:20:45Z

lib/model/message_list.dart


-      if (!messagesSameDay(prevMessageItem.message, message)) {
-        items.add(MessageListDateSeparatorItem(message));
+      prevMessageItem.isLastInFeed = false;


Yeah, ideally there'd be a widget test or two for this. (I guess in particular one for the usual case, and one for the case where the bottom sliver is empty but the top is nonempty, checking effectively that that first _buildItem call site is doing the right thing.)

If that isn't quick to write, though, it's not a blocker for merging.

gnprice · 2025-07-08T00:23:52Z

lib/model/message_list.dart

      final prevMessageItem = items.last as MessageListMessageBaseItem;
      assert(identical(prevMessageItem.message, prevMessage));
-      assert(prevMessageItem.isLastInBlock);
-      prevMessageItem.isLastInBlock = false;
+      assert(prevMessageItem.isLastInBlock); // though this may mutate; see below

-      if (!messagesSameDay(prevMessageItem.message, message)) {
-        items.add(MessageListDateSeparatorItem(message));
+      if (oneMessagePerBlock || !haveSameRecipient(prevMessage, message)) {
+        items.add(MessageListRecipientHeaderItem(message));
        canShareSender = false;
      } else {
-        canShareSender = prevMessageItem.message.senderId == message.senderId;
+        prevMessageItem.isLastInBlock = false;


Yeah, I think the refactors in the middle two commits are no longer helpful. The assertions are really there to support the prevMessageItem local, and then the last one is for checking the precondition of the assignment right after it:

assert(prevMessageItem.isLastInBlock); prevMessageItem.isLastInBlock = false;

So separating them from that line, and from the uses of prevMessageItem in general, isn't an improvement.

See discussions: zulip#1453 (comment) https://chat.zulip.org/#narrow/channel/48-mobile/topic/space.20at.20end.20of.20thread/near/2203693

chrisbobbe · 2025-07-08T00:41:02Z

Thanks! Revision pushed.

gnprice · 2025-07-08T01:19:37Z

Thanks! Merged.

chrisbobbe added the launch feedback Things users specifically asked for upon launch label Jun 24, 2025

chrisbobbe force-pushed the pr-space-at-end-of-feed branch from 15b7af9 to ae80e4f Compare July 7, 2025 19:14

chrisbobbe requested a review from gnprice July 7, 2025 22:38

chrisbobbe assigned gnprice Jul 7, 2025

chrisbobbe added the integration review Added by maintainers when PR may be ready for integration label Jul 7, 2025

gnprice reviewed Jul 7, 2025

View reviewed changes

msglist: Decrease end-of-feed gap from 36px to 12px

5539e14

See discussion linked in the comment.

chrisbobbe force-pushed the pr-space-at-end-of-feed branch from ae80e4f to 9ee0db2 Compare July 7, 2025 23:55

gnprice reviewed Jul 8, 2025

View reviewed changes

msglist: Change end-of-feed 11px padding to 5px

47ae5e3

See discussions: zulip#1453 (comment) https://chat.zulip.org/#narrow/channel/48-mobile/topic/space.20at.20end.20of.20thread/near/2203693

chrisbobbe force-pushed the pr-space-at-end-of-feed branch from 9ee0db2 to 47ae5e3 Compare July 8, 2025 00:41

gnprice merged commit 47ae5e3 into zulip:main Jul 8, 2025
1 check passed

chrisbobbe deleted the pr-space-at-end-of-feed branch July 8, 2025 01:22

Decrease whitespace at end of message feed #1628

Decrease whitespace at end of message feed #1628

Uh oh!

Conversation

chrisbobbe commented Jun 24, 2025

Uh oh!

chrisbobbe commented Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisbobbe commented Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisbobbe commented Jul 7, 2025

Uh oh!

alya commented Jul 7, 2025

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!

chrisbobbe commented Jul 7, 2025

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!

chrisbobbe commented Jul 8, 2025

Uh oh!

Uh oh!

gnprice commented Jul 8, 2025

Uh oh!

Uh oh!

chrisbobbe commented Jun 24, 2025 •

edited

Loading

chrisbobbe commented Jun 24, 2025 •

edited

Loading