anchors 7/n: Start splitting slivers!

[gnprice]

This is the next round after (and is stacked atop) #1486, toward #82.

(The initial draft of this PR said this PR would include the needed changes to the scroll-to-end button. Those ended up being a bigger job than I initially thought, so I've split them into their own PR as #1486.)

On my own device I've been running drafts of this PR merged with drafts of #1486 for a couple of weeks, off and on. Everything seems to behave smoothly from a user perspective.

In this PR:

• We start actually splitting the message list into back-to-back slivers, with the latest message in the bottom message list. Thanks to all the prep work up to this point, this change has only subtle effects in the UX.

• The last commit takes advantage of the back-to-back slivers to make one nice, though small, UX improvement: when opening a message list where the latest message doesn't fit on the screen or nearly doesn't, we make sure to show the top of that latest message instead of the bottom.

  In the future when the bottom sliver represents the unread messages, or the messages starting at another target anchor (anchor in the Zulip sense, not the ScrollView sense), this change will become much more important: we'll need to show the top of that potentially very tall bottom sliver, not its bottom.

Selected commit messages

9e6624b msglist: Split into back-to-back slivers

Thanks to all the preparatory changes we've made in this commit
series and those that came before it, this change should have only
subtle effects on user-visible behavior.

This therefore serves as a testing ground for all the ways that the
message list's scrolling behavior can become more complicated when
two (nontrivial) slivers are involved. If we find a situation where
the behavior does change noticeably (beyond what's described below),
that will be something to investigate and fix.

In particular:

• The sticky headers should behave exactly as they did before,
  even when the sliver boundary lies under the sticky header,
  thanks to several previous commit series.

• On first loading a given message list, it should start out
  scrolled to the end, just as before.

• When already scrolled to the end, the message list should stay
  there when a new message arrives, or a message is edited, etc.,
  even if the (new) latest message is taller than it was.
  This commit adds a test to confirm that.

Subtle differences include:

• When scrolled up from the bottom and a new message comes in,
  the behavior is slightly different from before. The current
  exact behavior is something we probably want to change anyway,
  and I think the new behavior isn't particularly better or worse.

• The behavior upon overscroll might be slightly different;
  I'm not sure.

• When a new message arrives, the previously-latest message gets a
  fresh element in the tree, and any UI state on it is lost. This
  happens because it moves between one sliver-list and the other,
  and the findChildIndexCallback mechanism only works within a
  single sliver-list.

  This causes a couple of visible glitches, but they seem tolerable.
  This will also naturally get fixed in the next PR or two toward
  Open a message list in the middle of history, when requested #82: we'll start adding new messages to the bottom sliver, rather
  than having them push anything from the bottom to the top sliver.

  Known symptoms of this are:

  • When the user has just marked messages as read and a new message
    arrives while the unread markers are fading, the marker on the
    previously-latest message will (if it was present) abruptly
    vanish while the others smoothly continue fading.

    We have a test for the smooth fading, and it happened to test
    the latest message, so this commit adjusts the test to avoid
    triggering this issue.

  • If the user had a spoiler expanded on the previously-latest
    message, it will reset to collapsed.

9ded2aa scroll: Show start of latest message if long, instead of end

This makes our first payoff in actual UX from having the message list
split into two back-to-back slivers!

With this change, if you open a message list and the latest message
is very tall, the list starts out scrolled so that you can see the
top of that latest message -- plus a bit of context above it (25% of
the viewport's height). Previously the list would always start out
scrolled to the end, so you'd have to scroll up in order to read
even the one latest message from the beginning.

In addition to a small UX improvement now, this makes a preview of
behavior we'll want to have when the bottom sliver starts at the
first unread message, and may have many messages after that.
This new behavior is nice already with one message, if the message
happens to be very tall; but it'll become critical when the bottom
sliver is routinely many screenfuls tall.

[gnprice]

Switched the assignee in order to spread out a bit the review load from this series of PRs :-)

[PIG208]

Thanks, this is exciting! I read the commits that follow #1486. Left some comments.

[PIG208]

nit: it seems helpful to have an extension on Subject<ScrollPosition>, for checks here and in scrolling_test.dart

[PIG208]

I feel that the presence of index and length makes i a bit of an confusing name. It will probably help to have a comment earlier in this method explaining how we map item indexes and sliver child indexes.

[PIG208]

For the conversion from index to i to make sense, one also need to be aware of the assumption that the top sliver and the bottom one grows in opposite directions.

[gnprice]

I feel that the presence of index and length makes i a bit of an confusing name.

Yeah, good thought. Expanding the names.

For the conversion from index to i to make sense, one also need to be aware of the assumption that the top sliver and the bottom one grows in opposite directions.

Yeah, that was obvious to me because I've been thinking about this so much (it's the whole reason to have two slivers)… but it's not obvious a priori. I'll add a few comments.

[PIG208]

Maybe we can be a bit more verbose when naming this. The parallel between size and length here is not immediately obvious to me.

[gnprice]

Sure, good thought.

[PIG208]

I feel that the choice of wrapping bottomSliver with SliverSafeArea and always making bottomSliver the center (while carefully not using the key twice) might need some documentation here. I suppose that we make it this way so that topSliver grows from bottom to top.

[gnprice]

I suppose that we make it this way so that topSliver grows from bottom to top.

Right, that's what ScrollView.center (which is inherited as MessageListScrollView.center) does. Take a look at that property's doc — does that explain it?

(while carefully not using the key twice)

This part isn't actually necessary — it's not a GlobalKey — so perhaps it's simpler not to do it. Would you find this clearer?

    Widget bottomSliver = SliverStickyHeaderList(
      key: centerSliverKey,
      // …

    if (!ComposeBox.hasComposeBox(widget.narrow)) {
      // TODO(#311) If we have a bottom nav, it will pad the bottom inset,
      //   and this can be removed; also remove mention in MessageList dartdoc
      bottomSliver = SliverSafeArea(key: bottomSliver.key, sliver: bottomSliver);
    }

[PIG208]

Yeah, that seems clearer.

[PIG208]

Looks like we don't have test cases exercising this (and the similar logic above). Removing the i < 0 doesn't fail any tests.

[gnprice]

This case only comes up if an item moves between slivers — this callback is called with an existing element's key, and this condition means it was found in the part of MessageListView.items that belongs to the other sliver. So it's actually only newly possible with this change (and only in one direction).

It also will become impossible again soon, so I'm OK with not having a test for our handling of it.

I was curious, though, what happens if we exercise this case now that it is possible. So I reverted the changes to the "animation state persistence" test, in order to exercise an item moving between the slivers, and commented out the two lines like this one.

Turns out nothing seems to break! This callback returns -1, which doesn't sound like a good index; but then this line in SliverMultiBoxAdaptorElement.performRebuild, where the callback's result gets used (as newIndex):

          newChildren[newIndex] = _childElements[index];

stores it into a SplayTreeMap, not a List, so it isn't bothered by a negative index.

At first glance, it kind of seems like that ought to then cause a duplicate copy of the item — as a "child -1" in the sliver it used to be in, in addition to "child 0" of the sliver where it now belongs. But one way or another, that doesn't end up happening, because the test gets past this check:

      check(find.byType(MessageItem).evaluate()).length.equals(2);

so apparently finds the new message and just one copy of the old message.

I haven't tried to trace through the rest of what that performRebuild method is doing in order to understand why it doesn't cause a problem. Regardless, returning -1 (or any negative value) here is wrong because the sliver won't actually have a child with index -1 — so the callback is right to return null instead.

[PIG208]

Thanks for digging into this! I took a brief look and it's puzzling. Returning -1 sounds good to me.

[PIG208]

Not sure if we want to have an assert here to ensure that centerSliverKey wasn't previously taken.

[gnprice]

(See #1468 (comment) — it doesn't actually matter for this non-GlobalKey key to appear multiple times in the tree, as long as it doesn't appear on siblings.)

[PIG208]

      semanticChildCount: length + 2,

This seems like something that risks getting out-of-date. While we do have tests ("fetch older messages on scroll") for this, it might be helpful to note where the + 2 comes from.

Since nothing breaks in this PR, I assume that the sliver changes do not affect it, but that's otherwise hard to confirm without running the tests.

[gnprice]

Hmmm, good question.

I think in fact this already has gotten out of date. Using git log -L to see history of this method, it looks like 5c70c76 probably should have bumped this to match the change to childCount, making it length + 3.

These got decoupled in 6a8cf5c — previously, with just one sliver, we were using StickyHeaderListView.builder (which is a lot like ListView.builder) and it took just one argument itemCount instead of having semanticChildCount here and separately childCount elsewhere. Before that commit, I think the connection was fairly clear because the itemCount: length + 2 was just a few lines above the two if (i == 0) and if (i == 1) lines.

The original two bumps were in e7fe06c and then 56ab395, corresponding to the mark-as-read button and then the spacer.

OTOH the spacer doesn't seem very semantic, so it probably shouldn't have counted in the first place. And the other two (the mark-as-read button and the typing-status line) are often hidden, so probably shouldn't count either when that's the case.

I'll spend a few minutes trying to work out what the right value should actually be here. Then I'll try to do that, and also leave behind some restructuring and/or comments to help them stay in sync.

[gnprice]

While we do have tests ("fetch older messages on scroll") for this

Yeah, those tests exercise this (because they query semanticChildCount) but they don't really effectively test it.

That's no fault of those tests, because they weren't meant to test it: as the name says, they're about checking that we fetch older messages on scrolling up, and checking we don't do so when we shouldn't.

One piece of evidence that these tests don't test this line is that 5c70c76 didn't update it.

Another is a thought experiment: if you make a change that causes those checks to fail because of something it does that's specific to semanticChildCount, how would you tell whether that's an intended change and the tests just need updating? It'll probably just look like noise that needs to be updated, because the tests really don't tell a story about this handful of extra children. When they check that e.g. first there are 303 children, then 403 children, the point is that there are 300-plus-a-few and later 400-plus-a-few, indicating that the original 300 messages and later those plus the additional 100 messages are shown.

So for example when 56ab395 did update this line (and probably shouldn't have), it dutifully updated those tests.

I think this makes a good example of how in order to effectively test some logic it's not enough to exercise it: the test needs to tell a clear story about the checks it's making and how they relate to the intended spec. The test's main job is to prevent regressions. When someone drafts a change that would be a regression, the test's first step to prevent it is to get the author's attention by failing; but then the second step, equally essential, is to communicate what the problem is so the author can identify the right fix.

[PIG208]

Yeah. These count checks reminds me of the self.assert_database_query_count's on the server, but over there the number is used to catch potential performance regressions. Here, the counts are used differently.

[gnprice]

I'll spend a few minutes trying to work out what the right value should actually be here.

OK, just activated TalkBack and played with it for a bit — first refreshing myself on how to use it in general, and then trying it in Zulip.

My main conclusion is that for #535 we'll have some work to do in the message list: particularly when you try scrolling around, there are some glitches that seem to be related to the sticky header. (That'll be an M5b issue, like #535 itself.)

I don't see any direct effect of this semanticChildCount value. In particular it's not getting announced to the user. It probably has an influence on the scale of tones that get played when scrolling around, to indicate how close you are to the beginning vs. the end of the list — but that's pretty subtle.

So I think I'll just simplify this to length, leave a TODO(#537), and call it done for now.

[PIG208]

Viewport height being 600px is something these tests rely on. Maybe we should make it explicit here so it provides context to the reader?

[gnprice]

Yeah, that's something I've waffled on. The tests in sticky_header_test.dart, which I wrote much earlier, are explicit about that.

The other side of it is that there are a couple of basic background facts about the environment Flutter widget tests run in by default:

• The target platform is Android.
• The viewport size is 800x600.

When writing any kind of code, there's some level of knowledge one assumes the reader has about the language you're using, any frameworks and other libraries, and the subject matter the software is operating on. Typically one assumes they've been working in the language and with the framework etc., and so already know a wide range of things that come up regularly when working with those.

So as I've written more tests using Flutter I've come to think of those two background facts as part of that expected background knowledge.

[gnprice]

Thanks @PIG208 for the review! Updated — PTAL.

[PIG208]

Thanks for the update! Manually tried this again on my device. Merging.