msglist: Update layout in messages

[sirpengi]

Rebased on top of #447 as that impacted tests passing.

As a foundation for my work on #170, I needed to update the layout of messages so that the star icon could be positioned in the appropriate place. This also enables the placement of future decorations to messages (like markers for moved/edited) and moves us closer to #157

Screenshot of the new message layout:

Because the new layout is more compact, this caused the basic test of the fetch older messages on scroll group to fail as a second request for new messages is triggered. This is due to the first batch of new messages not being tall enough to be beyond the threshold (see kFetchMessagesBufferPixels in lib/widgets/message_list.dart) for loading another batch. A proper fix to this would be to tweak the threshold behavior, but instead opted to fix the test by adjusting the response size and opened #445.

[chrisbobbe]

Thanks @sirpengi! Small comments below. It's nice to have more horizontal room for message contents. It could feel kind of cramped sometimes, in the previous design.

I did notice some potential downsides, though. Not to block merging this, but just maybe something we might be interested in soliciting feedback about in the alpha/beta.

• It seems like it might be harder to tell who sent a given piece of message content, if there's a lot of message content above it and below the corresponding sender information. When scanning upward for the sender information, message content will be more heterogenous and distracting than pure empty space, if that makes sense.

• It will be more difficult (sometimes a bit, sometimes a lot) to tell where message boundaries are. Before, this was made easy by the date markers; now, I think they have to be identified by the insertion of an 8px gap, between blocks that usually don't have clearly marked boundaries, and may contain their own vertical whitespace. To exaggerate the point:

  Before	After

• We've lost the information of when a message was sent, except if it's the first message in a run of messages from the same sender, or if it's the first message after a date boundary. So for example:

  Before	After

  This might be something we can fix as a followup, but it seems like it might be complicated. The Figma has a design to support checking the date/time for any message; it looks like it wants us to let the user slide a message to the left, to reveal the timestamp: https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?type=design&node-id=147-8975&mode=design&t=P97AuMzjt47Mudyg-0

  

  (I wonder if we can make the slide gesture reveal dates for all visible messages at once, like Messages on my iPhone, so you don't have to slide once for each message you want to check.)

[chrisbobbe]

msglist: Adjust message avatar size according to new design

Is the 35 in 3 + 35 + 11 supposed to change to 32 in this commit, to go along with the change in the Avatar's size?

Ah I see the 3 + 35 + 11 goes away in a later commit. Still, cleanest to not have a state in the branch where we have an orphaned/unexplained 35.

[sirpengi]

I've ended up consolidating the commits so the orphaned 35 no longer exists

[chrisbobbe]

msglist: Move message sender out of row containing message contents

This commit adds 4px of vertical space between the sender/date row and the message contents. I think that doesn't sound intentional, from reading the commit message.

[gnprice]

I agree, and I think those extra 4px don't correspond to anything in the Figma design.

AFAICT this last commit is meant to have no effect on the visible layout. Is that right? I'm not sure the effect on the source code is an improvement; so perhaps that commit can just be left out. If something like it is needed for a followup change like indicating starred messages, then it can appear as a prep commit (but with those extra 4px fixed) in that PR, where it'll be easier to assess what it's doing for us.

[sirpengi]

From: https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=134%3A8312&mode=dev

The mockup here shows each distinct message item has 4px vertical margins. The sender widget is complicated in the design as it has 2px of vertical padding (making a total of 6px top margin if the sender is shown) and there is a 2px margin above the message content and the sender:

So taking the figma as is, message contents would have dynamic padding depending if the sender is visible or not. But I think to simplify things, given there is effectively 4px spacing between the contents and the sender (2px coming from the sender padding and 2px between a message content and the sender), there ends up being 4px vertical padding for message contents in all instances. If we then give the sender 6px top margin we achieve the same layout.

(This discussion ignores that bottom padding on messages are expanded when it is before a new recipient header, but that case is already handled by introducing extra space in those situations)

[gnprice]

So taking the figma as is, message contents would have dynamic padding depending if the sender is visible or not. But I think to simplify things, given there is effectively 4px spacing between the contents and the sender (2px coming from the sender padding and 2px between a message content and the sender), there ends up being 4px vertical padding for message contents in all instances. If we then give the sender 6px top margin we achieve the same layout.

Cool. Yeah, in general it's fine for the layout to be expressed differently from how it is in Figma as long as it produces an equivalent result. We've done that in a fair number of places already. For us the Figma design expresses a visual result, but not the details of how to implement it.

[chrisbobbe]

msglist: Move message timestamp together with message sender

After this commit, the Row here has one interesting, contentful child sandwiched between two SizedBox(width: 16)s. So I think the commit could be followed up by a simplifying refactor to replace the row and SizedBoxes with a Padding.

[sirpengi]

Leaving this as is as the SizedBox will be used as gutters for subsequent decorations (e.g., the stars coming in #449 will be inserted in this space).

[gnprice]

Thanks @sirpengi, and thanks @chrisbobbe for the initial review!

Generally these changes look good. Some comments below, and I also agree with Chris's comments above.

One branch-level observation: I think a lot of these substantive changes don't make a lot of sense done separately. So I think a good structure probably looks like: some prep commits, then one commit that does all the main layout changes, then perhaps some followup NFC commits. For example:

• tweak that one test to have only one message instead of 10 (… or perhaps skip that change entirely, see below)
• maybe pull out the senderWidget (or senderRow) local, an NFC change
• maybe change the fonts
• then all the other changes squashed together, except for the last one "4f04046f9 msglist: Move message sender out of row containing message contents" (probably leave it out entirely, as discussed below; but also it's one that rearranges more of the code than the other changes)
• then the Row → Padding change Chris points out above, an NFC change

[gnprice]

I don't understand what "changed to 200" means here. Where does 200 appear?

If the comment is referring to this line where 150 messages are returned, then this is a good illustration of why it's often good for a comment to be less specific when possible. :-) If it just says there are more messages here than the app asks for, without repeating the exact number that there are, then it can't so easily get out of sync with the code.

[gnprice]

msglist test: Adjust message list test

The prefix already says it's about message-list tests, so let's find a way to add more information than that in the rest of the summary line. 🙂

For example: "msglist test: Adjust for upcoming layout changes".

[gnprice]

The only place we use the time local is now inside this item.showSender conditional. So that local's definition can be moved inside the conditional too.

[gnprice]

I agree, and I think those extra 4px don't correspond to anything in the Figma design.

AFAICT this last commit is meant to have no effect on the visible layout. Is that right? I'm not sure the effect on the source code is an improvement; so perhaps that commit can just be left out. If something like it is needed for a followup change like indicating starred messages, then it can appear as a prep commit (but with those extra 4px fixed) in that PR, where it'll be easier to assess what it's doing for us.

[gnprice]

In the end this widget is only partly about the sender, because it also has the timestamp; so senderWidget is a bit misleading. A good name might be senderRow — it's the row with the sender (which also happens to have a bit more than the sender).

[gnprice]

nit:

Suggested change

	senderWidget,
	if (senderWidget != null) senderWidget,

and then instead of SizedBox.shrink, can set the variable to null. I think that'd make it a bit more evident when looking at this spot that it's conditional whether there's really any sender anything being shown.

[gnprice]

One message instead of ten seems fine — but also I don't actually get any failure if I revert this change. So perhaps simplify the explanation by just leaving it out.

[gnprice]

Here's a simpler solution:

Suggested change

	messages: List.generate(150, (i) => eg.streamMessage(id: 950 + i, sender: eg.selfUser)));
	check(itemCount(tester)).equals(153);
	messages: List.generate(300, (i) => eg.streamMessage(id: 950 + i, sender: eg.selfUser)));
	check(itemCount(tester)).equals(303);

and then the line at the end of the test expects 403 items instead of 303.

This way the new batch when it fetches more can remain 100 messages like it is now. That also makes the apologetic comment unnecessary — the message list starts with 300 messages, which is more than 100, but it's perfectly normal for a message list to have 300 messages in it if the user has already scrolled up some.

I think the key to what's going on here isn't actually anything untoward in how we're deciding when to fetch more messages. Rather, it's this line of the test:

      await tester.pump(const Duration(seconds: 3)); // Fast-forward to end of fling.

where we don't let any app code run between when the user starts a fling scroll (one at full speed, 8000 px/sec), and three seconds later when the fling scroll should be complete. With 200 very compact example messages (short contents, and all from the same sender to the same recipient on the same day), it's likely we've scrolled past all of them by that point. In normal operation we'd have started the next fetch long before then, but with this fast-forward we don't get the chance.

[gnprice]

• It will be more difficult (sometimes a bit, sometimes a lot) to tell where message boundaries are. Before, this was made easy by the date markers; […]
• We've lost the information of when a message was sent, except if it's the first message in a run of messages from the same sender, or if it's the first message after a date boundary. […]

Yeah, I don't love those aspects either. But both are basically already true in zulip-mobile — the information is there if you know where to look, but I think it's awfully subtle and wouldn't call our design there a success. So I'm OK with losing that information for now, as the price of making good use of the available width, and trying to get it back later as a followup:

The Figma has a design to support checking the date/time for any message; it looks like it wants us to let the user slide a message to the left, to reveal the timestamp […] (I wonder if we can make the slide gesture reveal dates for all visible messages at once, like Messages on my iPhone, so you don't have to slide once for each message you want to check.)

And I agree that last part sounds like a nice touch.

[gnprice]

Filed a followup issue for revealing the message timestamps:

• Show message timestamps on swipe #457

[sirpengi]

@gnprice ready for review again! The commits were re-ordered, and some adjustments were made for comments and to polish up the layout. I was also able to minimize the test changes thanks to your comments.

[gnprice]

I'm confused by the outcome of #446 (comment) . It looks like the change we were talking there about removing is still in the PR, and in fact has gotten squashed into the main commit. That's a change I specifically asked at #446 (review) to leave unsquashed:

• then all the other changes squashed together, except for the last one "4f04046f9 msglist: Move message sender out of row containing message contents" (probably leave it out entirely, as discussed below; but also it's one that rearranges more of the code than the other changes)

The main commit is pretty hard to read now, and I suspect it's because that change has gotten squashed into it. Can you please take it back out?

[sirpengi]

@gnprice apologies! In my revision I found the change important to include (it separates the senderRow from the element containing the gutters so that future items in the gutter can be laid out relative to the message), but it was hard to detangle it from the other changes I wanted to make. I think it was problematic in the first iteration because I also tweaked the layout when I moved elements around. Here I've got another attempt, with the same result, but tried to be cleaner with combining conceptual changes. I'm not entirely happy with the result and maybe we can walk through it on our call today.

I also adjusted where the 4px margin for the message content is defined as I found in the star/unstar feature that the gutters should include that top margin as well (to align it with the top of the message content).

[gnprice]

We looked at this in our call today, as suggested above, and the plan is that you're going to make another revision of those last two commits, in which the change that had been the last commit of the December revision becomes its own commit again, this time an NFC commit. (That change is the one that rearranges the 16px horizontal padding, so that it applies separately to the sender vs. the content-plus-reactions.)

[sirpengi]

@gnprice ready for review again! The order of operations was shifted around a bit in this latest version, but I'm quite satisfied with the separation of changes here.

[gnprice]

Thanks for the revision! My high-level comments are again about the branch structure.

The main one (details below) is about early commits introducing a broken state that later commits fix up, which is something we avoid in Zulip's style of a clean commit history.

This goes back to the overall comment I made in my original review above at #446 (review) :

One branch-level observation: I think a lot of these substantive changes don't make a lot of sense done separately. So I think a good structure probably looks like: some prep commits, then one commit that does all the main layout changes, then perhaps some followup NFC commits.

That structure would mean that all the changes in these three commits:
1d3d41d msglist: Consolidate sender avatar, name and message time into same row
[…]
53edaad msglist: Update message sender style and layout
30dbcc5 msglist: Adjust layout and spacing in messages

would go into a single commit. Those correspond to this item in my original review:

• then all the other changes squashed together, except for the last one "4f04046f9 msglist: Move message sender out of row containing message contents" (probably leave it out entirely, as discussed below; but also it's one that rearranges more of the code than the other changes)

Your previous revision (the one at #446 (comment) ) successfully squashed those together, but it also squashed in that other change that I asked to be kept separate. So what I'd still like to see is a branch structure of the kind I requested in my original review.

Once we have that, I think there'll still be a few comments to make about the code and the layout, but it'll be a lot easier to review and discuss in that form.

[gnprice]

Great, glad to see that the commit at the end that makes this rearrangement:

msglist: Move senderRow into a separate row

Moved widgets around such that the gutters to the
left and right of message contents are ready for
future decorative elements.

appears to be NFC — it appears to have no effect on the layout as seen by the user (or on any other behavior).

To make the intent clear, let's say that in the commit message, with " [nfc]" as usual.

[gnprice]

This spaceBetween is missing in the commit
1d3d41d msglist: Consolidate sender avatar, name and message time into same row

and as a result the timestamp goes immediately after the sender name:

[gnprice]

More generally, it seems like a lot of what this later commit:
53edaad msglist: Update message sender style and layout

is doing is just fixing bugs in the earlier commit — things that became necessities as a consequence of creating this Row for the sender avatar, name, and timestamp.

The lack of spaceBetween is the most visible, but for another example: similarly to spaceBetween, if we're going to have the name and the timestamp in one Row then it seems like we naturally want CrossAxisAlignment.baseline for that. And we need Flexible and TextOverflow.ellipsis so that things don't break if a sender has a long name.

As discussed in Zulip's general commit style guide:
https://zulip.readthedocs.io/en/latest/contributing/commit-discipline.html#each-commit-must-be-coherent
each commit should leave things in a state it would be reasonable for us to deploy as is. (I.e. to release as is, to use the verb that's applicable for a client.) So if part of a PR introduces a bug and then a later part fixes the bug, the fix should instead get squashed into the commit that introduced it.

[gnprice]

nit: these are giant statements, so separate by a blank line:

Suggested change

	: null;
	return GestureDetector(
	: null;
	return GestureDetector(

[sirpengi]

@gnprice updated again!

[gnprice]

Great, thanks — with this branch structure, the PR is a lot easier to read and review now.

Comments below, but all of them small; I'm glad this is now getting close to merge.

I looked into the question of the vertical spacing between sender name/avatar and message body that we discussed above at #446 (comment), and I believe this version faithfully matches what's in the Figma design. I'm not sure if the behavior here differs from what was in the original revision, or if it's purely a matter of the cleaner branch structure in this revision making it clear what's intended, but either way I'm happy with the result.

[gnprice]

msglist [nfc]: Move senderRow into a separate row

Moved widgets around such that the gutters to the
left and right of message contents are ready for
future decorative elements.

Fixes-partly: #157

The "Fixes-partly" doesn't make sense here — this commit has no visible effect, so it can't be resolving any part of #157 which is all about how things look.

That line would make sense on the parent commit, though:

msglist: Update message layout to new design

[gnprice]

Also on this commit message, a nit: "move into a separate row" doesn't seem to me to really capture the point. After all, it's already a Row, right? (As the name senderRow suggests.) And if by "row" one means "element of a Column"… it was already that, too.

I think a better summary line would be
msglist [nfc]: Move senderRow to an outer Column

That is, the thing that's changing is that now senderRow appears as an element of a new outer Column, one that's outside of the Row that provides the gutters.

Or perhaps better yet:
msglist [nfc]: Group message and sender as Column of Rows, not vice versa

[gnprice]

nit in commit message:

msglist: Update message layout to new design

Test 'basic' in group 'fetch older messages on scroll'
adjusted as layout change has caused the test to fail.

That body paragraph can be simplified to read:

Adjusted a test that was broken by the layout change.

i.e. to leave out the name of the test. For a high-level understanding of what the commit is about, the test's name doesn't matter at all — only that it's some test that got broken by the change. And if you're reading in detail, it's redundant with the code.

[gnprice]

I think the layout here is somewhat easier to read and understand if the padding around senderRow stays here:

Suggested change

	if (senderRow != null) senderRow,
	if (senderRow != null)
	Padding(padding: const EdgeInsets.fromLTRB(16, 2, 16, 4),
	child: senderRow),

That way the "16" here is just a couple of lines away from the "16" that gives the widths of the gutters around the message body, below. And similarly the vertical 2px and 4px are close by the 4px vertical padding around the whole thing, which they interact with closely.

(The version I've given also contains a smaller change: the use of EdgeInsets.fromLTRB is helpful for keeping things compact.)

[gnprice]

nit: This is a real long conditional expression. I think things get a bit cleaner if we instead use a normal if conditional:

Suggested change

	final Widget? senderRow = item.showSender
	? Padding(
	padding: const EdgeInsets.only(top: 2, bottom: 4, left: 16, right: 16),
	child: Row(
	Widget? senderRow;
	if (item.showSender) {
	senderRow = Row(

(That version is combined with my other comment about keeping the Padding below.)

Then we can also leave out the null case; it's covered by the implicit initialization in Widget? senderRow;.

A follow-up bonus to using if rather than a conditional expression is that we can then have local variables within the if block to organize things. In particular, it's then possible to keep a local variable time, in the way I suggested in my original review:
#446 (comment)
rather than inlining it and making the expression lengthier.

[gnprice]

This should get a spacer too, in case the sender name is long. Otherwise the name can run right into the timestamp:

I think 4px does it.

[sirpengi]

@gnprice ready for review again!

[gnprice]

Thanks! All now looks good — merging.