msglist [nfc]: Remove one Center widget (because no-op); comment to explain another

[chrisbobbe]

No description provided.

[PIG208]

Do you have a screenshot when that happens? I wasn't able to reproduce the effect if I remove Center on my devices:

Linux

Android

Maybe I looked at the wrong place? Oh! I see, I can reproduce that when the compose box is absent in the combined feed, because in that case the message list is the only child in the Column on MessageListPage, and the message list doesn't fill horizontally like ComposeBox does, if without Center.

Details

[PIG208]

If we remove the Center here, and keep the one on MessageListPage, the combined feed looks fine. Because Center specifically deals with sizing the widget within a Scaffold, it might be clearer to remove the Center here, and comment on the Center on MessageListPage.

SizedBox.expand might be a good alternative, because Column can center its children.

[chrisbobbe]

Oh, I got some reasoning wrong 🙂:

msglist [nfc]: Remove no-op Center widget in _MessageListPageState

The Center's child is a Column with the message list and possibly a
compose box. That Column expresses that it wants to fill all the
available space […]

I think this is true in the vertical direction (mainAxisSize is at its default, MainAxisSize.max) but not the horizontal direction. I can make it not fill all the available horizontal space by just replacing its children with colored boxes:

[chrisbobbe]

If we remove the Center here, and keep the one on MessageListPage, the combined feed looks fine.

Agreed; I see this too.

Because Center specifically deals with sizing the widget within a Scaffold, it might be clearer to remove the Center here, and comment on the Center on MessageListPage.

(Did you mean positioning instead of sizing?)

There's other logic that also positions the message list horizontally on the screen, with the assumption that it's working with the full width of the screen. See in _MessageListState.build:

    // Pad the left and right insets, for small devices in landscape.
    return SafeArea(

As long as that's done in _MessageListState.build, I think the other positioning logic—the Center—belongs there too, instead of somewhere farther away. And that SafeArea can't move outward because it would prevent the compose box from stretching to the left/right edges of the screen, and we don't have a design to make that look OK:

I also think the layout is easier to reason about if the SafeArea's padding is always positioned exactly on the device insets—

—than if the SafeArea's padding is sometimes not positioned on the device insets:

especially if the insets aren't symmetric (though I don't have a device to demonstrate that). (In these screenshots I've changed the max-width from 760 to 500 so I can demonstrate using my phone.)

This basically means the Center should be a descendant of the SafeArea, and the SafeArea should cover the full width of the screen.

[chrisbobbe]

Thanks for the review! Revision pushed (atop #1290 for the sake of CI), and see my reply above: #1042 (comment)

[chrisbobbe]

(Pushed another tweak or two—I think I'm finished for this evening though 😄)

[PIG208]

Thanks! This all make sense to me except just this part from the commit message:

It turns out that the Column always takes all horizontal space, too;
the message-list and compose-box widgets both stretch to the full
screen width.

I was confused at first as I read #1042 (comment). Maybe it can be rephrased so that it is clearer that the children taking all the horizontal space is necessary.

[chrisbobbe]

Thanks for the review! Revision pushed,

I was confused at first as I read #1042 (comment). Maybe it can be rephrased so that it is clearer that the children taking all the horizontal space is necessary.

Ah, yeah: rather than what I said at #1042 (comment), I think the point I'd like to make is: in main, you can't tell just by looking locally at the Column that it takes the full horizontal width. You have to read details of its children, which are distant by many lines of code or in a different file. For example, the Column doesn't have CrossAxisAlignment.stretch; if it did, it would be clearer that the Center is a no-op in the horizontal direction.

Having gone and read those details, though, we can conclude that the Column does in fact fill the full horizontal width (thanks to the Center in the message list, which is always present), and so it's a no-op and can be removed. A new comment I add in the PR—

-        builder: (BuildContext context) => Center(
-          child: Column(children: [
+        builder: (BuildContext context) => Column(
+          // Children are expected to take the full horizontal space
+          // and handle the horizontal device insets.
+          children: [

—should help readers understand more quickly that something like the removed Center doesn't need to be added back again.

For the reason that the children need to fill the full horizontal width, I've updated a commit message:

     It turns out that the Column always takes all horizontal space, too;
     the message-list and compose-box widgets both stretch to the full
-    screen width. This is appropriate because they want to handle the
-    horizontal device insets in separate ways: the former by keeping
+    screen width. This is appropriate because they need to handle the
+    horizontal device insets in different ways: the former by keeping
     visible UI outside the insets; the latter by filling the insets with
     padding. So the Center is a no-op in the horizontal direction, too.

[PIG208]

Cool, thanks for the update! Marked for Greg's review.

[gnprice]

Thanks to you both for the clarifications!

This generally all looks good; a couple of tweaks below.

[gnprice]

nit:

Suggested change

	// Horizontally, on wide screens, this grows the SafeArea
	// to position its padding over the device insets and centers content.
	// Horizontally, on wide screens, this Center grows the SafeArea
	// to position its padding over the device insets and centers content.

That makes it a bit clearer what "this" is meant to refer to.

[gnprice]

This feels a bit too specific about how it does what it does, in the sort of way that's at risk of becoming wrong over time. How about this?

Suggested change

	/// Takes the full screen width,
	/// keeping the message list and scroll-to-bottom button
	/// out of the horizontal insets with transparent [SafeArea] padding.
	/// When there is no [ComposeBox],
	/// adds transparent [SliverSafeArea] and [SafeArea] padding
	/// to the message-list content and the scroll-to-bottom button, respectively.
	/// Takes the full screen width, keeping its contents
	/// out of the horizontal insets with transparent [SafeArea] padding.
	/// When there is no [ComposeBox], also takes responsibility
	/// for dealing with the bottom inset.

[chrisbobbe]

Thanks for the review! Revision pushed.

[gnprice]

Thanks! Looks good; merging.