anchors 2/n: Fix three sticky-header bugs #1312
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chrisbobbe
reviewed
Jan 30, 2025
| @@ -0,0 +1,345 @@ | |||
| /// Example app for exercising the sticky_header library. | |||
There was a problem hiding this comment.
I notice the buttons don't show their full label text:
There was a problem hiding this comment.
They fit at my usual settings 🙂 :
which I guess is why I let that slide. I'll shrink the text a bit, though.
lib/widgets/sticky_header.dart
Outdated
Comment on lines
565
to
568
| // The header's item has [StickyHeaderItem.allowOverflow] true. | ||
| // Show the header in full, with one edge at the edge of the viewport, | ||
| // even if the (visible part of the) item is smaller than the header, | ||
| // and even if the whole child is smaller than the header. |
There was a problem hiding this comment.
OK, reading this in context—checking the dartdoc of _headerEndBound was helpful—I think I understand!
One question: in the part after "even if", the words "item" and "child" are both used. I believe an "item" is a wrapper for a "child" (right), and the wrapping logic isn't really relevant for this comment—would it be helpful to simplify by saying "child" or "item" in both places here?
There was a problem hiding this comment.
Hmm, thanks for reading closely — I see that wording was ambiguous. Does this clarify?
// The header's item has [StickyHeaderItem.allowOverflow] true.
// Show the header in full, with one edge at the edge of the viewport,
// even if the (visible part of the) item is smaller than the header,
- // and even if the whole child is smaller than the header.
+ // and even if the whole child sliver is smaller than the header.I'll add this too:
+ /// This sliver's child sliver, a modified [RenderSliverList].
+ ///
+ /// The child manages the items in the list (deferring to [RenderSliverList]);
+ /// and identifies which list item, if any, should be consulted
+ /// for a sticky header.
_RenderSliverStickyHeaderListInner? get child => _child;
@@ @@
void performLayout() {
+ // First, lay out the child sliver. This does all the normal work of
+ // [RenderSliverList], then calls [_rebuildHeader] on this sliver
+ // so that [header] and [_headerEndBound] are up to date.
assert(child != null);
child!.layout(constraints, parentUsesSize: true);This is adapted lightly from an example app I made back in the first week of the zulip-flutter project, 2022-12-23, as part of developing the sticky_header library. The example app was extremely helpful then for experimenting with changes and seeing the effects visually and interactively, as well as for print-debugging such experiments. So let's get it into the tree. The main reason I didn't send the example app back then is that it was a whole stand-alone app tree under example/, complete with all the stuff in android/ and ios/ and so on that `flutter create` spits out for setting up a Flutter app. That's pretty voluminous: well over 100 different files totalling about 1.1MB on disk. I did't want to permanently burden the repo with all that, nor have to maintain it all over time. Happily, I realized today that we can skip that, and still have a perfectly good example app, by reusing that infrastructure from the actual Zulip app. That way all we need is a Dart file with a `main` function, corresponding to the old example's `lib/main.dart` which was the only not-from-the-template code in the whole example app. So here it is. Other than moving the Dart file and discarding the rest, the code I wrote back then has been updated to our current formatting style; adjusted slightly for changes in Flutter's Material widgets; and updated for changes I made to the sticky_header API after that first week.
It's already a fact that the header's size in each dimension is non-negative and finite; the framework asserts that in the `layout` implementation (via debugAssertDoesMeetConstraints). So that includes `headerExtent`; and then `paintedHeaderSize` is bounded to between zero and that value.
This is the right thing, as the comment explains. Conveniently it's also simpler.
…y; note a bug The logic below -- particularly in the allowOverflow true case, where it constructs a new SliverGeometry from scratch -- has been implicitly relying on several of these facts already. These wouldn't be true of an arbitrary sliver child; but this sliver knows what type of child it actually has, and they are true of that one. So write down the specific assumptions we can take from that. Reading through [RenderSliverList.performLayout] to see what it can produce as the child geometry also made clear there's another case that this method isn't currently correctly handling at all: the case where scrollOffsetCorrection is non-null. Filed an issue for that; add a todo-comment for it.
Fixes zulip#1309. Fixes zulip#725. This is necessary when scrolling back to the origin over items that grew taller while off screen (and beyond the 250px of near-on-screen cached items). For example that can happen because a message was edited, or because new messages came in that were taller than those previously present.
…sumptions Because the child's hitTestExtent equals its paintExtent, this was producing a new hitTestExtent equal to the new paintExtent. But that's the same behavior the SliverGeometry constructor gives us by default.
This relies on (and expresses) an assumption in the new assertions: that the child's layoutExtent equals paintExtent. That assumption will be helpful in keeping this logic manageable to understand as we add an upcoming further wrinkle.
In particular this will affect the upper sliver (with older messages) in the message list, when we start using two slivers there in earnest.
|
Thanks for the review! Pushed a revision. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1309.
Fixes #725. (which is the same underlying bug)
In addition to that live bug in the app (involving scrollOffsetCorrection, which we previously weren't handling), the last two commits in this branch fix a couple of bugs in the
sticky_headerlibrary that were latent bugs in the app — they can only trigger once we start having two slivers share space in the list. That's a scenario we'll need as part of #80 / #82, as seen for example in #1169 (fyi @rajveermalviya).Specifically, when both slivers are visible, the version in main
After this PR, there's one remaining bug I'm aware of that affects sticky headers in the presence of multiple slivers:
if the top sliver should show a header, but the header doesn't fit within the sliver's share of the viewport, then the header will get pushed off screen even if it hasallowOverflowtrue. Concretely for the message list, this means that the last message in the top sliver will fail to share its recipient header with the first message in the bottom sliver, even if they're meant to share a recipient header.edit: @chrisbobbe showed me the current symptom is a bit different from this. Rather than get pushed off screen, the header will have its bottom part covered up by the bottom sliver. The fix doesn't change, though.
I have a draft fix for that bug too, and I believe it works, but it still needs tests and a bit of other polishing-up. So it'll come as a separate PR later.
This is the long-awaited followup to #496, from a year ago this week. The two fixes here involving multiple slivers, as well as the upcoming fix coming in another PR, were all in the draft branch I had then. The scrollOffsetCorrection fix (which fixes the two live bugs) is new today, after I took a fresh look last week at some of the underlying framework code as part of trying to really understand what this
performLayoutmethod needs to do clearly enough to get to an implementation I'm confident is correct.Selected commit messages
4b29045 sticky_header: Add example app
This is adapted lightly from an example app I made back in the first
week of the zulip-flutter project, 2022-12-23, as part of developing
the sticky_header library.
The example app was extremely helpful then for experimenting with
changes and seeing the effects visually and interactively, as well as
for print-debugging such experiments. So let's get it into the tree.
The main reason I didn't send the example app back then is that it
was a whole stand-alone app tree under example/, complete with all
the stuff in android/ and ios/ and so on that
flutter createspitsout for setting up a Flutter app. That's pretty voluminous: well
over 100 different files totalling about 1.1MB on disk. I did't
want to permanently burden the repo with all that, nor have to
maintain it all over time.
Happily, I realized today that we can skip that, and still have a
perfectly good example app, by reusing that infrastructure from the
actual Zulip app. That way all we need is a Dart file with a
mainfunction, corresponding to the old example's
lib/main.dartwhichwas the only not-from-the-template code in the whole example app.
So here it is. Other than moving the Dart file and discarding the
rest, the code I wrote back then has been updated to our current
formatting style; adjusted slightly for changes in Flutter's
Material widgets; and updated for changes I made to the
sticky_header API after that first week.
3de58fe sticky_header [nfc]: Add asserts from studying possible child geometry; note a bug
The logic below -- particularly in the allowOverflow true case,
where it constructs a new SliverGeometry from scratch -- has been
implicitly relying on several of these facts already. These
wouldn't be true of an arbitrary sliver child; but this sliver knows
what type of child it actually has, and they are true of that one.
So write down the specific assumptions we can take from that.
Reading through [RenderSliverList.performLayout] to see what it can
produce as the child geometry also made clear there's another case
that this method isn't currently correctly handling at all: the case
where scrollOffsetCorrection is non-null. Filed an issue for that;
add a todo-comment for it.
075a8cd sticky_header: Handle scrollOffsetCorrection
Fixes #1309.
Fixes #725.
This is necessary when scrolling back to the origin over items that
grew taller while off screen (and beyond the 250px of near-on-screen
cached items). For example that can happen because a message was
edited, or because new messages came in that were taller than those
previously present.
de20060 sticky_header: Fix _findChildAtEnd when viewport partly consumed already
In particular this will affect the upper sliver (with older messages)
in the message list, when we start using two slivers there in earnest.
7583bb4 sticky_header: Avoid header at sliver/sliver boundary