diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 2e01d1ff45..7358749fdc 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -212,14 +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; } - final extentRemainingAboveViewport = scrollMetrics.maxScrollExtent - scrollMetrics.pixels; - if (extentRemainingAboveViewport < 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 @@ -279,7 +278,8 @@ class _MessageListState extends State with PerAccountStoreAwareStat Widget _buildListView(context) { final length = model!.items.length; - return StickyHeaderListView.builder( + const centerSliverKey = ValueKey('center sliver'); + return CustomScrollView( // TODO: Offer `ScrollViewKeyboardDismissBehavior.interactive` (or // similar) if that is ever offered: // https://github.com/flutter/flutter/issues/57609#issuecomment-1355340849 @@ -291,73 +291,84 @@ 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, - // Setting reverse: true means the scroll starts at the bottom. - // Flipping the indexes (in itemBuilder) 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)]; - 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); - } - }); + semanticChildCount: length + 2, + anchor: 1.0, + center: centerSliverKey, + + slivers: [ + SliverStickyHeaderList( + headerPlacement: HeaderPlacement.scrollingStart, + 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); + })), + + // 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), + ]); + } + + 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); + } } } diff --git a/lib/widgets/sticky_header.dart b/lib/widgets/sticky_header.dart index 0a1e042e07..356a056094 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); @@ -272,25 +272,48 @@ 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, + }; + } +} -class _SliverStickyHeaderList extends RenderObjectWidget { - _SliverStickyHeaderList({ +/// 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({ + 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 +322,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 +357,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(); } @@ -398,6 +421,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, @@ -424,10 +449,10 @@ class _RenderSliverStickyHeaderList extends RenderSliver with RenderSliverHelper double? endBound; if (item != null && !item.allowOverflow) { final childParentData = listChild!.parentData! as SliverMultiBoxAdaptorParentData; - endBound = switch (_element.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!, }; } @@ -522,7 +547,7 @@ class _RenderSliverStickyHeaderList extends RenderSliver with RenderSliverHelper bool _headerAtCoordinateEnd() { return axisDirectionIsReversed(constraints.axisDirection) - ^ (_element.widget.headerPlacement == HeaderPlacement.scrollingEnd); + ^ (_widget.headerPlacement == HeaderPlacement.scrollingEnd); } @override @@ -559,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; @@ -611,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, @@ -707,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/message_list_test.dart b/test/widgets/message_list_test.dart index 6d2ecda5e6..407ce0817e 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, @@ -149,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); @@ -166,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); @@ -188,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); @@ -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]); 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(); }