From e4298682b2b53e32f8ffb188c569d491418ec2ce Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 18 Jan 2024 10:48:59 -0500 Subject: [PATCH 1/7] msglist [nfc]: Pull out a _buildItem method This will help keep down complexity (and depth of indentation) as we make the list view more complex. In particular it will avoid duplication when we start having two back-to-back slivers that both contain message-list items. --- lib/widgets/message_list.dart | 60 +++++++++++++++++++---------------- 1 file changed, 32 insertions(+), 28 deletions(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 2e01d1ff45..0c2bdfd30b 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -329,36 +329,40 @@ class _MessageListState extends State with PerAccountStoreAwareStat if (i == 1) return MarkAsReadWidget(narrow: widget.narrow); final data = model!.items[length - 1 - (i - 2)]; - switch (data) { - case MessageListHistoryStartItem(): - return const Center( - child: Padding( - padding: EdgeInsets.symmetric(vertical: 16.0), - child: Text("No earlier messages."))); // TODO use an icon - case MessageListLoadingItem(): - return const Center( - child: Padding( - padding: EdgeInsets.symmetric(vertical: 16.0), - child: CircularProgressIndicator())); // TODO perhaps a different indicator - case MessageListRecipientHeaderItem(): - final header = RecipientHeader(message: data.message, narrow: widget.narrow); - return StickyHeaderItem(allowOverflow: true, - header: header, child: header); - case MessageListDateSeparatorItem(): - final header = RecipientHeader(message: data.message, narrow: widget.narrow); - return StickyHeaderItem(allowOverflow: true, - header: header, - child: DateSeparator(message: data.message)); - case MessageListMessageItem(): - final header = RecipientHeader(message: data.message, narrow: widget.narrow); - return MessageItem( - key: ValueKey(data.message.id), - header: header, - trailingWhitespace: i == 1 ? 8 : 11, - item: data); - } + return _buildItem(data, i); }); } + + Widget _buildItem(MessageListItem data, int i) { + switch (data) { + case MessageListHistoryStartItem(): + return const Center( + child: Padding( + padding: EdgeInsets.symmetric(vertical: 16.0), + child: Text("No earlier messages."))); // TODO use an icon + case MessageListLoadingItem(): + return const Center( + child: Padding( + padding: EdgeInsets.symmetric(vertical: 16.0), + child: CircularProgressIndicator())); // TODO perhaps a different indicator + case MessageListRecipientHeaderItem(): + final header = RecipientHeader(message: data.message, narrow: widget.narrow); + return StickyHeaderItem(allowOverflow: true, + header: header, child: header); + case MessageListDateSeparatorItem(): + final header = RecipientHeader(message: data.message, narrow: widget.narrow); + return StickyHeaderItem(allowOverflow: true, + header: header, + child: DateSeparator(message: data.message)); + case MessageListMessageItem(): + final header = RecipientHeader(message: data.message, narrow: widget.narrow); + return MessageItem( + key: ValueKey(data.message.id), + header: header, + trailingWhitespace: i == 1 ? 8 : 11, + item: data); + } + } } class ScrollToBottomButton extends StatelessWidget { From 8b7c576841012b7dbaf1f34114f1ddbb666a29d3 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 18 Jan 2024 10:26:14 -0500 Subject: [PATCH 2/7] sticky_header [nfc]: Make SliverStickyHeaderList public This will allow using it as one sliver in a CustomScrollView. --- lib/widgets/sticky_header.dart | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/lib/widgets/sticky_header.dart b/lib/widgets/sticky_header.dart index 0a1e042e07..c21794b855 100644 --- a/lib/widgets/sticky_header.dart +++ b/lib/widgets/sticky_header.dart @@ -260,7 +260,7 @@ class StickyHeaderListView extends BoxScrollView { @override Widget buildChildLayout(BuildContext context) { - return _SliverStickyHeaderList( + return SliverStickyHeaderList( headerPlacement: (reverseHeader ^ reverse) ? HeaderPlacement.scrollingEnd : HeaderPlacement.scrollingStart, delegate: childrenDelegate); @@ -274,23 +274,24 @@ class StickyHeaderListView extends BoxScrollView { /// the ambient [Directionality] is RTL or LTR. enum HeaderPlacement { scrollingStart, scrollingEnd } -class _SliverStickyHeaderList extends RenderObjectWidget { - _SliverStickyHeaderList({ +class SliverStickyHeaderList extends RenderObjectWidget { + SliverStickyHeaderList({ + super.key, required this.headerPlacement, required SliverChildDelegate delegate, - }) : child = _SliverStickyHeaderListInner( + }) : _child = _SliverStickyHeaderListInner( headerPlacement: headerPlacement, delegate: delegate, ); final HeaderPlacement headerPlacement; - final _SliverStickyHeaderListInner child; + final _SliverStickyHeaderListInner _child; @override - _SliverStickyHeaderListElement createElement() => _SliverStickyHeaderListElement(this); + RenderObjectElement createElement() => _SliverStickyHeaderListElement(this); @override - _RenderSliverStickyHeaderList createRenderObject(BuildContext context) { + RenderSliver createRenderObject(BuildContext context) { final element = context as _SliverStickyHeaderListElement; return _RenderSliverStickyHeaderList(element: element); } @@ -299,10 +300,10 @@ class _SliverStickyHeaderList extends RenderObjectWidget { enum _SliverStickyHeaderListSlot { header, list } class _SliverStickyHeaderListElement extends RenderObjectElement { - _SliverStickyHeaderListElement(_SliverStickyHeaderList super.widget); + _SliverStickyHeaderListElement(SliverStickyHeaderList super.widget); @override - _SliverStickyHeaderList get widget => super.widget as _SliverStickyHeaderList; + SliverStickyHeaderList get widget => super.widget as SliverStickyHeaderList; @override _RenderSliverStickyHeaderList get renderObject => super.renderObject as _RenderSliverStickyHeaderList; @@ -334,14 +335,14 @@ class _SliverStickyHeaderListElement extends RenderObjectElement { @override void mount(Element? parent, Object? newSlot) { super.mount(parent, newSlot); - _child = updateChild(_child, widget.child, _SliverStickyHeaderListSlot.list); + _child = updateChild(_child, widget._child, _SliverStickyHeaderListSlot.list); } @override - void update(_SliverStickyHeaderList newWidget) { + void update(SliverStickyHeaderList newWidget) { super.update(newWidget); assert(widget == newWidget); - _child = updateChild(_child, widget.child, _SliverStickyHeaderListSlot.list); + _child = updateChild(_child, widget._child, _SliverStickyHeaderListSlot.list); renderObject.child!.markHeaderNeedsRebuild(); } From 80c3aaddd7e6957e1dbb8a100f7f7a6b2dafcfd1 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 18 Jan 2024 10:34:20 -0500 Subject: [PATCH 3/7] msglist [nfc]: Unpack to a CustomScrollView of a single sliver This is effectively what StickyHeaderListView.builder was abbreviating. This expanded form will then give us flexibility to introduce a second sliver. --- lib/widgets/message_list.dart | 76 ++++++++++++++++------------- test/widgets/message_list_test.dart | 9 ++-- 2 files changed, 46 insertions(+), 39 deletions(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 0c2bdfd30b..c8e125969b 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -279,7 +279,7 @@ class _MessageListState extends State with PerAccountStoreAwareStat Widget _buildListView(context) { final length = model!.items.length; - return StickyHeaderListView.builder( + return CustomScrollView( // TODO: Offer `ScrollViewKeyboardDismissBehavior.interactive` (or // similar) if that is ever offered: // https://github.com/flutter/flutter/issues/57609#issuecomment-1355340849 @@ -291,46 +291,54 @@ class _MessageListState extends State with PerAccountStoreAwareStat _ => ScrollViewKeyboardDismissBehavior.manual, }, - // To preserve state across rebuilds for individual [MessageItem] - // widgets as the size of [MessageListView.items] changes we need - // to match old widgets by their key to their new position in - // the list. - // - // The keys are of type [ValueKey] with a value of [Message.id] - // and here we use a O(log n) binary search method. This could - // be improved but for now it only triggers for materialized - // widgets. As a simple test, flinging through All Messages in - // CZO on a Pixel 5, this only runs about 10 times per rebuild - // and the timing for each call is <100 microseconds. - // - // Non-message items (e.g., start and end markers) that do not - // have state that needs to be preserved have not been given keys - // and will not trigger this callback. - findChildIndexCallback: (Key key) { - final valueKey = key as ValueKey; - final index = model!.findItemWithMessageId(valueKey.value); - if (index == -1) return null; - return length - 1 - (index - 2); - }, controller: scrollController, - itemCount: length + 2, + semanticChildCount: length + 2, + // Setting reverse: true means the scroll starts at the bottom. - // Flipping the indexes (in itemBuilder) means the start/bottom - // has the latest messages. + // Flipping the indexes (in the SliverChildBuilderDelegate callback) + // means the start/bottom has the latest messages. // This works great when we want to start from the latest. // TODO handle scroll starting at first unread, or link anchor // TODO on new message when scrolled up, anchor scroll to what's in view reverse: true, - itemBuilder: (context, i) { - // To reinforce that the end of the feed has been reached: - // https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/flutter.3A.20Mark-as-read/near/1680603 - if (i == 0) return const SizedBox(height: 36); - if (i == 1) return MarkAsReadWidget(narrow: widget.narrow); - - final data = model!.items[length - 1 - (i - 2)]; - return _buildItem(data, i); - }); + slivers: [ + SliverStickyHeaderList( + headerPlacement: HeaderPlacement.scrollingEnd, + delegate: SliverChildBuilderDelegate( + // To preserve state across rebuilds for individual [MessageItem] + // widgets as the size of [MessageListView.items] changes we need + // to match old widgets by their key to their new position in + // the list. + // + // The keys are of type [ValueKey] with a value of [Message.id] + // and here we use a O(log n) binary search method. This could + // be improved but for now it only triggers for materialized + // widgets. As a simple test, flinging through All Messages in + // CZO on a Pixel 5, this only runs about 10 times per rebuild + // and the timing for each call is <100 microseconds. + // + // Non-message items (e.g., start and end markers) that do not + // have state that needs to be preserved have not been given keys + // and will not trigger this callback. + findChildIndexCallback: (Key key) { + final valueKey = key as ValueKey; + final index = model!.findItemWithMessageId(valueKey.value); + if (index == -1) return null; + return length - 1 - (index - 2); + }, + childCount: length + 2, + (context, i) { + // To reinforce that the end of the feed has been reached: + // https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/flutter.3A.20Mark-as-read/near/1680603 + if (i == 0) return const SizedBox(height: 36); + + if (i == 1) return MarkAsReadWidget(narrow: widget.narrow); + + final data = model!.items[length - 1 - (i - 2)]; + return _buildItem(data, i); + })), + ]); } Widget _buildItem(MessageListItem data, int i) { diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index 6d2ecda5e6..0cbc1de4f8 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -17,7 +17,6 @@ import 'package:zulip/model/store.dart'; import 'package:zulip/widgets/content.dart'; import 'package:zulip/widgets/icons.dart'; import 'package:zulip/widgets/message_list.dart'; -import 'package:zulip/widgets/sticky_header.dart'; import 'package:zulip/widgets/store.dart'; import '../api/fake_api.dart'; @@ -76,13 +75,13 @@ void main() { } ScrollController? findMessageListScrollController(WidgetTester tester) { - final stickyHeaderListView = tester.widget(find.byType(StickyHeaderListView)); - return stickyHeaderListView.controller; + final scrollView = tester.widget(find.byType(CustomScrollView)); + return scrollView.controller; } group('fetch older messages on scroll', () { int? itemCount(WidgetTester tester) => - tester.widget(find.byType(StickyHeaderListView)).semanticChildCount; + tester.widget(find.byType(CustomScrollView)).semanticChildCount; testWidgets('basic', (tester) async { await setupMessageListPage(tester, foundOldest: false, @@ -527,7 +526,7 @@ void main() { testWidgets('animation state persistence', (WidgetTester tester) async { // Check that _UnreadMarker maintains its in-progress animation // as the number of items changes in MessageList. See - // `findChildIndexCallback` passed into [StickyHeaderListView.builder] + // `findChildIndexCallback` passed into [SliverStickyHeaderList] // at [_MessageListState._buildListView]. final message = eg.streamMessage(flags: []); await setupMessageListPage(tester, messages: [message]); From 89d491967be6c9ef7596eae511b5dd1bd8a1d7de Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 26 Jan 2024 14:47:33 -0800 Subject: [PATCH 4/7] sticky_header [nfc]: Make a small shortcut for _element.widget --- lib/widgets/sticky_header.dart | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/widgets/sticky_header.dart b/lib/widgets/sticky_header.dart index c21794b855..59b1f74f58 100644 --- a/lib/widgets/sticky_header.dart +++ b/lib/widgets/sticky_header.dart @@ -399,6 +399,8 @@ class _RenderSliverStickyHeaderList extends RenderSliver with RenderSliverHelper final _SliverStickyHeaderListElement _element; + SliverStickyHeaderList get _widget => _element.widget; + Widget? _headerWidget; /// The limiting edge (if any) of the header's item, @@ -425,7 +427,7 @@ class _RenderSliverStickyHeaderList extends RenderSliver with RenderSliverHelper double? endBound; if (item != null && !item.allowOverflow) { final childParentData = listChild!.parentData! as SliverMultiBoxAdaptorParentData; - endBound = switch (_element.widget.headerPlacement) { + endBound = switch (_widget.headerPlacement) { HeaderPlacement.scrollingStart => childParentData.layoutOffset! + listChild.size.onAxis(constraints.axis), HeaderPlacement.scrollingEnd => @@ -523,7 +525,7 @@ class _RenderSliverStickyHeaderList extends RenderSliver with RenderSliverHelper bool _headerAtCoordinateEnd() { return axisDirectionIsReversed(constraints.axisDirection) - ^ (_element.widget.headerPlacement == HeaderPlacement.scrollingEnd); + ^ (_widget.headerPlacement == HeaderPlacement.scrollingEnd); } @override From a0948c4e3adb0fe1e749a83b053f398344d02ae6 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 25 Jan 2024 18:27:07 -0800 Subject: [PATCH 5/7] sticky_header: Handle GrowthDirection.reverse This resolves the last of the "TODO dir" comments in this library. We had originally written this library to rely on the assumption that `constraints.growthDirection` was `GrowthDirection.forward`, i.e. that the direction in which `constraints.scrollOffset` increases was the same as `constraints.axisDirection`, in order to keep down the number of distinct directions one needs to keep track of in writing and reading (and debugging) the implementation. But now that this logic is relatively solid within that case -- and now that we have a solid test suite for this library -- we can go back and identify the handful of places that need to be flipped around for `GrowthDirection.reverse`, and do so. This support isn't yet especially *useful*: there are a couple of unrelated bugs which are triggered when the area allotted to this sliver isn't the entire viewport, and there isn't a lot of reason to have a sliver with GrowthDirection.reverse when no other sliver is potentially occupying part of the viewport. We'll fix those separately. --- lib/widgets/sticky_header.dart | 64 +++++++++++++++++---- test/widgets/sticky_header_test.dart | 86 +++++++++++++++++----------- 2 files changed, 103 insertions(+), 47 deletions(-) diff --git a/lib/widgets/sticky_header.dart b/lib/widgets/sticky_header.dart index 59b1f74f58..356a056094 100644 --- a/lib/widgets/sticky_header.dart +++ b/lib/widgets/sticky_header.dart @@ -272,7 +272,29 @@ class StickyHeaderListView extends BoxScrollView { /// For example if the list scrolls to the left, then /// [scrollingStart] means the right edge of the list, regardless of whether /// the ambient [Directionality] is RTL or LTR. -enum HeaderPlacement { scrollingStart, scrollingEnd } +enum HeaderPlacement { + scrollingStart, + scrollingEnd; + + _HeaderGrowthPlacement _byGrowth(GrowthDirection growthDirection) { + return switch ((growthDirection, this)) { + (GrowthDirection.forward, scrollingStart) => _HeaderGrowthPlacement.growthStart, + (GrowthDirection.forward, scrollingEnd) => _HeaderGrowthPlacement.growthEnd, + (GrowthDirection.reverse, scrollingStart) => _HeaderGrowthPlacement.growthEnd, + (GrowthDirection.reverse, scrollingEnd) => _HeaderGrowthPlacement.growthStart, + }; + } +} + +/// Where a header goes, in terms of the list sliver's growth direction. +/// +/// This will agree with the [HeaderPlacement] value if the growth direction +/// is [GrowthDirection.forward], but contrast with it if the growth direction +/// is [GrowthDirection.reverse]. See [HeaderPlacement._byGrowth]. +enum _HeaderGrowthPlacement { + growthStart, + growthEnd +} class SliverStickyHeaderList extends RenderObjectWidget { SliverStickyHeaderList({ @@ -427,10 +449,10 @@ class _RenderSliverStickyHeaderList extends RenderSliver with RenderSliverHelper double? endBound; if (item != null && !item.allowOverflow) { final childParentData = listChild!.parentData! as SliverMultiBoxAdaptorParentData; - endBound = switch (_widget.headerPlacement) { - HeaderPlacement.scrollingStart => + endBound = switch (_widget.headerPlacement._byGrowth(constraints.growthDirection)) { + _HeaderGrowthPlacement.growthStart => childParentData.layoutOffset! + listChild.size.onAxis(constraints.axis), - HeaderPlacement.scrollingEnd => + _HeaderGrowthPlacement.growthEnd => childParentData.layoutOffset!, }; } @@ -562,7 +584,7 @@ class _RenderSliverStickyHeaderList extends RenderSliver with RenderSliverHelper } else { // The limiting edge of the header's item, // in the outer, non-scrolling coordinates. - final endBoundAbsolute = axisDirectionIsReversed(constraints.axisDirection) + final endBoundAbsolute = axisDirectionIsReversed(constraints.growthAxisDirection) ? geometry.layoutExtent - (_headerEndBound! - constraints.scrollOffset) : _headerEndBound! - constraints.scrollOffset; @@ -614,7 +636,7 @@ class _RenderSliverStickyHeaderList extends RenderSliver with RenderSliverHelper // need to convert to the sliver's coordinate system. final headerParentData = (header!.parentData as SliverPhysicalParentData); final paintOffset = headerParentData.paintOffset; - return switch (constraints.axisDirection) { + return switch (constraints.growthAxisDirection) { AxisDirection.right => paintOffset.dx, AxisDirection.left => geometry!.layoutExtent - header!.size.width - paintOffset.dx, AxisDirection.down => paintOffset.dy, @@ -710,18 +732,36 @@ class _RenderSliverStickyHeaderListInner extends RenderSliverList { @override void performLayout() { - assert(constraints.growthDirection == GrowthDirection.forward); // TODO dir - super.performLayout(); - final child = switch (widget.headerPlacement) { - HeaderPlacement.scrollingStart => _findChildAtStart(), - HeaderPlacement.scrollingEnd => _findChildAtEnd(), - }; + final RenderBox? child; + switch (widget.headerPlacement._byGrowth(constraints.growthDirection)) { + case _HeaderGrowthPlacement.growthEnd: + child = _findChildAtEnd(); + case _HeaderGrowthPlacement.growthStart: + child = _findChildAtStart(); + } + (parent! as _RenderSliverStickyHeaderList)._rebuildHeader(child); } } +extension SliverConstraintsGrowthAxisDirection on SliverConstraints { + AxisDirection get growthAxisDirection => switch (growthDirection) { + GrowthDirection.forward => axisDirection, + GrowthDirection.reverse => axisDirection.reversed, + }; +} + +extension AxisDirectionReversed on AxisDirection { + AxisDirection get reversed => switch (this) { + AxisDirection.down => AxisDirection.up, + AxisDirection.up => AxisDirection.down, + AxisDirection.right => AxisDirection.left, + AxisDirection.left => AxisDirection.right, + }; +} + extension AxisCoordinateDirection on Axis { AxisDirection get coordinateDirection => switch (this) { Axis.horizontal => AxisDirection.right, diff --git a/test/widgets/sticky_header_test.dart b/test/widgets/sticky_header_test.dart index 011899c11a..fc363c71e8 100644 --- a/test/widgets/sticky_header_test.dart +++ b/test/widgets/sticky_header_test.dart @@ -67,38 +67,38 @@ void main() { for (final reverse in [true, false]) { for (final reverseHeader in [true, false]) { - for (final allowOverflow in [true, false]) { - final name = 'sticky headers: ' - 'scroll ${reverse ? 'up' : 'down'}, ' - 'header at ${reverseHeader ? 'bottom' : 'top'}, ' - 'headers ${allowOverflow ? 'overflow' : 'bounded'}'; - testWidgets(name, (tester) => - _checkSequence(tester, - Axis.vertical, - reverse: reverse, - reverseHeader: reverseHeader, - allowOverflow: allowOverflow, - )); - } - } - } - - for (final reverse in [true, false]) { - for (final reverseHeader in [true, false]) { - for (final allowOverflow in [true, false]) { - for (final textDirection in TextDirection.values) { + for (final growthDirection in GrowthDirection.values) { + for (final allowOverflow in [true, false]) { final name = 'sticky headers: ' - '${textDirection.name.toUpperCase()} ' - 'scroll ${reverse ? 'backward' : 'forward'}, ' - 'header at ${reverseHeader ? 'end' : 'start'}, ' + 'scroll ${reverse ? 'up' : 'down'}, ' + 'header at ${reverseHeader ? 'bottom' : 'top'}, ' + '$growthDirection, ' 'headers ${allowOverflow ? 'overflow' : 'bounded'}'; testWidgets(name, (tester) => _checkSequence(tester, - Axis.horizontal, textDirection: textDirection, + Axis.vertical, reverse: reverse, reverseHeader: reverseHeader, + growthDirection: growthDirection, allowOverflow: allowOverflow, )); + + for (final textDirection in TextDirection.values) { + final name = 'sticky headers: ' + '${textDirection.name.toUpperCase()} ' + 'scroll ${reverse ? 'backward' : 'forward'}, ' + 'header at ${reverseHeader ? 'end' : 'start'}, ' + '$growthDirection, ' + 'headers ${allowOverflow ? 'overflow' : 'bounded'}'; + testWidgets(name, (tester) => + _checkSequence(tester, + Axis.horizontal, textDirection: textDirection, + reverse: reverse, + reverseHeader: reverseHeader, + growthDirection: growthDirection, + allowOverflow: allowOverflow, + )); + } } } } @@ -111,6 +111,7 @@ Future _checkSequence( TextDirection? textDirection, bool reverse = false, bool reverseHeader = false, + GrowthDirection growthDirection = GrowthDirection.forward, required bool allowOverflow, }) async { assert(textDirection != null || axis == Axis.vertical); @@ -118,21 +119,35 @@ Future _checkSequence( Axis.horizontal => reverseHeader ^ (textDirection == TextDirection.rtl), Axis.vertical => reverseHeader, }; + final reverseGrowth = (growthDirection == GrowthDirection.reverse); final controller = ScrollController(); + const listKey = ValueKey("list"); + const emptyKey = ValueKey("empty"); await tester.pumpWidget(Directionality( textDirection: textDirection ?? TextDirection.rtl, - child: StickyHeaderListView( + child: CustomScrollView( controller: controller, scrollDirection: axis, reverse: reverse, - reverseHeader: reverseHeader, - children: List.generate(100, (i) => StickyHeaderItem( - allowOverflow: allowOverflow, - header: _Header(i, height: 20), - child: _Item(i, height: 100)))))); - - final overallSize = tester.getSize(find.byType(StickyHeaderListView)); + anchor: reverseGrowth ? 1.0 : 0.0, + center: reverseGrowth ? emptyKey : listKey, + slivers: [ + SliverStickyHeaderList( + key: listKey, + headerPlacement: (reverseHeader ^ reverse) + ? HeaderPlacement.scrollingEnd : HeaderPlacement.scrollingStart, + delegate: SliverChildListDelegate( + List.generate(100, (i) => StickyHeaderItem( + allowOverflow: allowOverflow, + header: _Header(i, height: 20), + child: _Item(i, height: 100))))), + const SliverPadding( + key: emptyKey, + padding: EdgeInsets.zero), + ]))); + + final overallSize = tester.getSize(find.byType(CustomScrollView)); final extent = overallSize.onAxis(axis); assert(extent % 100 == 0); @@ -143,7 +158,7 @@ Future _checkSequence( (extent / 2 - inset) * (headerAtCoordinateEnd ? 1 : -1)); } - final first = !(reverse ^ reverseHeader); + final first = !(reverse ^ reverseHeader ^ reverseGrowth); final itemFinder = first ? find.byType(_Item).first : find.byType(_Item).last; @@ -155,10 +170,11 @@ Future _checkSequence( Future checkState() async { // Check the header comes from the expected item. - final scrollOffset = controller.position.pixels; + final scrollOffset = controller.position.pixels * (reverseGrowth ? -1 : 1); final expectedHeaderIndex = first ? (scrollOffset / 100).floor() : (extent ~/ 100 - 1) + (scrollOffset / 100).ceil(); + // print("$scrollOffset, $extent, $expectedHeaderIndex"); check(tester.widget<_Item>(itemFinder).index).equals(expectedHeaderIndex); check(_headerIndex(tester)).equals(expectedHeaderIndex); @@ -180,7 +196,7 @@ Future _checkSequence( } Future jumpAndCheck(double position) async { - controller.jumpTo(position); + controller.jumpTo(position * (reverseGrowth ? -1 : 1)); await tester.pump(); await checkState(); } From f20592f86e14bc8d4554935560860e2b2c2d070a Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 29 Jan 2024 15:48:38 -0800 Subject: [PATCH 6/7] msglist [nfc]: Simplify fetch-older check to extentAfter This is a more readable way of expressing the exact same thing. --- lib/widgets/message_list.dart | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index c8e125969b..bac40d3954 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -218,8 +218,7 @@ class _MessageListState extends State with PerAccountStoreAwareStat _scrollToBottomVisibleValue.value = true; } - final extentRemainingAboveViewport = scrollMetrics.maxScrollExtent - scrollMetrics.pixels; - if (extentRemainingAboveViewport < kFetchMessagesBufferPixels) { + if (scrollMetrics.extentAfter < kFetchMessagesBufferPixels) { // TODO: This ends up firing a second time shortly after we fetch a batch. // The result is that each time we decide to fetch a batch, we end up // fetching two batches in quick succession. This is basically harmless From 95602456030d4c886bbc8950e56165d462d07d4b Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 29 Jan 2024 16:22:17 -0800 Subject: [PATCH 7/7] msglist [nfc]: Let ScrollView count forward, and sliver handle reverse This means that the overall scroll view has AxisDirection.down as the scroll direction, rather than AxisDirection.up, and the SliverStickyHeader is passed GrowthDirection.reverse instead of GrowthDirection.forward in order to get the same effect. This makes the scrolling a little easier to think about when focused on MessageList's code, because for example (as seen in this diff) `scrollMetrics.extentBefore` now refers to the messages that are older than the ones on screen, rather than those that are newer. This also brings us closer to the setup we'll want for #80 and #82, opening the message list at an anchor other than the end: the empty placeholder sliver at the bottom will become the list of messages after the starting anchor, while the sliver at the top will remain the list of messages above the anchor. --- lib/widgets/message_list.dart | 22 +++++++++++----------- test/widgets/message_list_test.dart | 6 +++--- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index bac40d3954..7358749fdc 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -212,13 +212,13 @@ class _MessageListState extends State with PerAccountStoreAwareStat } void _adjustButtonVisibility(ScrollMetrics scrollMetrics) { - if (scrollMetrics.extentBefore == 0) { + if (scrollMetrics.extentAfter == 0) { _scrollToBottomVisibleValue.value = false; } else { _scrollToBottomVisibleValue.value = true; } - if (scrollMetrics.extentAfter < kFetchMessagesBufferPixels) { + if (scrollMetrics.extentBefore < kFetchMessagesBufferPixels) { // TODO: This ends up firing a second time shortly after we fetch a batch. // The result is that each time we decide to fetch a batch, we end up // fetching two batches in quick succession. This is basically harmless @@ -278,6 +278,7 @@ class _MessageListState extends State with PerAccountStoreAwareStat Widget _buildListView(context) { final length = model!.items.length; + const centerSliverKey = ValueKey('center sliver'); return CustomScrollView( // TODO: Offer `ScrollViewKeyboardDismissBehavior.interactive` (or // similar) if that is ever offered: @@ -292,18 +293,12 @@ class _MessageListState extends State with PerAccountStoreAwareStat controller: scrollController, semanticChildCount: length + 2, - - // Setting reverse: true means the scroll starts at the bottom. - // Flipping the indexes (in the SliverChildBuilderDelegate callback) - // means the start/bottom has the latest messages. - // This works great when we want to start from the latest. - // TODO handle scroll starting at first unread, or link anchor - // TODO on new message when scrolled up, anchor scroll to what's in view - reverse: true, + anchor: 1.0, + center: centerSliverKey, slivers: [ SliverStickyHeaderList( - headerPlacement: HeaderPlacement.scrollingEnd, + headerPlacement: HeaderPlacement.scrollingStart, delegate: SliverChildBuilderDelegate( // To preserve state across rebuilds for individual [MessageItem] // widgets as the size of [MessageListView.items] changes we need @@ -337,6 +332,11 @@ class _MessageListState extends State with PerAccountStoreAwareStat final data = model!.items[length - 1 - (i - 2)]; return _buildItem(data, i); })), + + // This is a trivial placeholder that occupies no space. Its purpose is + // to have the key that's passed to [ScrollView.center], and so to cause + // the above [SliverStickyHeaderList] to run from bottom to top. + const SliverToBoxAdapter(key: centerSliverKey), ]); } diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index 0cbc1de4f8..407ce0817e 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -148,7 +148,7 @@ void main() { // Initial state should be not visible, as the message list renders with latest message in view check(isButtonVisible(tester)).equals(false); - scrollController.jumpTo(600); + scrollController.jumpTo(-600); await tester.pump(); check(isButtonVisible(tester)).equals(true); @@ -165,7 +165,7 @@ void main() { // Initial state should be not visible, as the message list renders with latest message in view check(isButtonVisible(tester)).equals(false); - scrollController.jumpTo(600); + scrollController.jumpTo(-600); await tester.pump(); check(isButtonVisible(tester)).equals(true); @@ -187,7 +187,7 @@ void main() { // Initial state should be not visible, as the message list renders with latest message in view check(isButtonVisible(tester)).equals(false); - scrollController.jumpTo(600); + scrollController.jumpTo(-600); await tester.pump(); check(isButtonVisible(tester)).equals(true);