sticky_header: Rewrite sticky headers completely; write tests #288
Conversation
This widget really isn't a header (sticky or otherwise) so much as it is an item that has a sticky header.
These will come in handy soon for a reimplementation of this library, and will come in handy before that for writing tests for it.
We're about to replace this library with a fresh implementation that has a different design, in order to gain some features we'll want for the message list. These tests will help us validate the new implementation. Some of the naming in this test code reflects the interface the new implementation will have, in order to changes in the tests and so maximize their effectiveness in validating the reimplementation.
In particular this gives us two features we'll want in the Zulip message list when we go to make recipient headers appear only where needed (zulip#174): * An item's header now gets built (and laid out and painted) only if the item is actually the one at the relevant edge of the viewport such that the stickiness comes into play. This lets us attach a sticky header to every message, even when it shares a recipient header with the message that comes before it. * An item can now (optionally) allow the header's stickiness at the edge of the viewport to take precedence over the header's confinement within the item's own bounds. We'll use this to let the header stay pinned when scrolling between items that share a recipient header.
|
Thanks! I think I see the benefit to #174 of both of those features you mention, and in my manual testing, the message list continues to work well; I don't notice any behavior change. I don't yet thoroughly understand the code changes, but that's probably OK. 🙂 Will |
|
Thanks for the review! I'll go ahead and merge.
Yeah, for this PR I think that's OK. The bulk of this sticky-headers code is in layers that we don't ordinarily work in, and that I think we won't have a lot of other places where we need to work in.
I don't believe so. The children they're walking through are render objects representing items in the list, which means that they're list items that have been built in order to lay out and display. Those correspond to items that are actually visible in the viewport, and to items found within a small distance before and after the viewport, but not to all the other items in a long list. That's the laziness behavior that's documented on ListView (see "Child elements' lifecycle" at ListView); the guts of that behavior are implemented by RenderSliverList, which this PR inherits from for _RenderSliverStickyHeaderListInner. Specifically that lazy creation of children, as well as the garbage-collection of children that have scrolled out of view, is implemented in RenderSliverList.performLayout. I did read through that whole method (it's about 300 lines) for background when writing our original sticky-headers implementation last December; I am definitely glad to get to re-use it whole, and not to have to reimplement or fork it. 🙂 |
In particular this gives us two features we'll want in the Zulip
message list when we go to make recipient headers appear only
where needed (#174):
An item's header now gets built (and laid out and painted) only if
the item is actually the one at the relevant edge of the viewport
such that the stickiness comes into play. This lets us attach a
sticky header to every message, even when it shares a recipient
header with the message that comes before it.
An item can now (optionally) allow the header's stickiness at the
edge of the viewport to take precedence over the header's
confinement within the item's own bounds. We'll use this to let
the header stay pinned when scrolling between items that share a
recipient header.