From f0689d3eac1bdad367bda34343b80628d669c302 Mon Sep 17 00:00:00 2001 From: E-m-i-n-e-n-c-e Date: Fri, 17 Jan 2025 13:34:46 +0530 Subject: [PATCH 1/5] Finished the job but idk why the tests are failing.Have to troubleshoot --- lib/model/message_list.dart | 183 +++++++++++++++++++++++++++++++--- lib/widgets/message_list.dart | 158 +++++++++++++++++++++++------ 2 files changed, 296 insertions(+), 45 deletions(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index 670785ac4e..5da716c56a 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -56,9 +56,13 @@ class MessageListLoadingItem extends MessageListItem { final MessageListDirection direction; const MessageListLoadingItem(this.direction); + } -enum MessageListDirection { older } +enum MessageListDirection { + older, + newer +} /// Indicates we've reached the oldest message in the narrow. class MessageListHistoryStartItem extends MessageListItem { @@ -85,9 +89,7 @@ mixin _MessageSequence { bool _fetched = false; /// Whether we know we have the oldest messages for this narrow. - /// - /// (Currently we always have the newest messages for the narrow, - /// once [fetched] is true, because we start from the newest.) + bool get haveOldest => _haveOldest; bool _haveOldest = false; @@ -118,6 +120,38 @@ mixin _MessageSequence { BackoffMachine? _fetchOlderCooldownBackoffMachine; + + /// Whether we are currently fetching the next batch of newer messages. + /// + /// When this is true, [fetchNewer] is a no-op. + /// That method is called frequently by Flutter's scrolling logic, + /// and this field helps us avoid spamming the same request just to get + /// the same response each time. + /// + /// See also [fetchNewerCoolingDown]. + bool get fetchingNewer => _fetchingNewer; + bool _fetchingNewer = false; + + /// Whether [fetchNewer] had a request error recently. + /// + /// When this is true, [fetchNewer] is a no-op. + /// That method is called frequently by Flutter's scrolling logic, + /// and this field helps us avoid spamming the same request and getting + /// the same error each time. + /// + /// "Recently" is decided by a [BackoffMachine] that resets + /// when a [fetchNewer] request succeeds. + /// + /// See also [fetchingNewer]. + bool get fetchNewerCoolingDown => _fetchNewerCoolingDown; + bool _fetchNewerCoolingDown = false; + + BackoffMachine? _fetchNewerCooldownBackoffMachine; + + /// Whether we know we have the newest messages for this narrow. + bool get haveNewest => _haveNewest; + bool _haveNewest = false; + /// The parsed message contents, as a list parallel to [messages]. /// /// The i'th element is the result of parsing the i'th element of [messages]. @@ -133,7 +167,8 @@ mixin _MessageSequence { /// before, between, or after the messages. /// /// This information is completely derived from [messages] and - /// the flags [haveOldest], [fetchingOlder] and [fetchOlderCoolingDown]. + /// the flags [haveOldest], [fetchingOlder] and [fetchOlderCoolingDown] + /// and [haveNewest], [fetchingNewer] and [fetchNewerCoolingDown]. /// It exists as an optimization, to memoize that computation. final QueueList items = QueueList(); @@ -152,6 +187,7 @@ mixin _MessageSequence { case MessageListLoadingItem(): switch (item.direction) { case MessageListDirection.older: return -1; + case MessageListDirection.newer: return 1; } case MessageListRecipientHeaderItem(:var message): case MessageListDateSeparatorItem(:var message): @@ -271,6 +307,10 @@ mixin _MessageSequence { _fetchOlderCooldownBackoffMachine = null; contents.clear(); items.clear(); + _fetchingNewer = false; + _fetchNewerCoolingDown = false; + _fetchNewerCooldownBackoffMachine = null; + _haveNewest = false; } /// Redo all computations from scratch, based on [messages]. @@ -318,24 +358,53 @@ mixin _MessageSequence { void _updateEndMarkers() { assert(fetched); assert(!(fetchingOlder && fetchOlderCoolingDown)); + assert(!(fetchingNewer && fetchNewerCoolingDown)); + final effectiveFetchingOlder = fetchingOlder || fetchOlderCoolingDown; + final effectiveFetchingNewer = fetchingNewer || fetchNewerCoolingDown; + assert(!(effectiveFetchingOlder && haveOldest)); + assert(!(effectiveFetchingNewer && haveNewest)); + + // Handle start marker (older messages) final startMarker = switch ((effectiveFetchingOlder, haveOldest)) { (true, _) => const MessageListLoadingItem(MessageListDirection.older), (_, true) => const MessageListHistoryStartItem(), (_, _) => null, }; + + // Handle end marker (newer messages) + final endMarker = switch ((effectiveFetchingNewer, haveNewest)) { + (true, _) => const MessageListLoadingItem(MessageListDirection.newer), + (_, _) => null, // No "history end" marker needed since we start from newest + }; + final hasStartMarker = switch (items.firstOrNull) { MessageListLoadingItem() => true, MessageListHistoryStartItem() => true, _ => false, }; + + final hasEndMarker = switch (items.lastOrNull) { + MessageListLoadingItem() => true, + _ => false, + }; + + // Update start marker switch ((startMarker != null, hasStartMarker)) { case (true, true): items[0] = startMarker!; case (true, _ ): items.addFirst(startMarker!); case (_, true): items.removeFirst(); case (_, _ ): break; } + + // Update end marker + switch ((endMarker != null, hasEndMarker)) { + case (true, true): items[items.length - 1] = endMarker!; + case (true, _ ): items.add(endMarker!); + case (_, true): items.removeLast(); + case (_, _ ): break; + } } /// Recompute [items] from scratch, based on [messages], [contents], and flags. @@ -408,16 +477,20 @@ bool _sameDay(DateTime date1, DateTime date2) { /// * Add listeners with [addListener]. /// * Fetch messages with [fetchInitial]. When the fetch completes, this object /// will notify its listeners (as it will any other time the data changes.) -/// * Fetch more messages as needed with [fetchOlder]. +/// * Fetch more messages as needed with [fetchOlder] or [fetchNewer]. /// * On reassemble, call [reassemble]. /// * When the object will no longer be used, call [dispose] to free /// resources on the [PerAccountStore]. class MessageListView with ChangeNotifier, _MessageSequence { - MessageListView._({required this.store, required this.narrow}); + MessageListView._({required this.store, required this.narrow, this.anchorMessageId}); + // Anchor message ID is used to fetch messages from a specific point in the list. + // It is set when the user navigates to a message list page with a specific anchor message. + int? anchorMessageId; + int? get anchorIndex => anchorMessageId != null ? findItemWithMessageId(anchorMessageId!) : null; factory MessageListView.init( - {required PerAccountStore store, required Narrow narrow}) { - final view = MessageListView._(store: store, narrow: narrow); + {required PerAccountStore store, required Narrow narrow, int? anchorMessageId}) { + final view = MessageListView._(store: store, narrow: narrow, anchorMessageId: anchorMessageId); store.registerMessageList(view); return view; } @@ -496,20 +569,30 @@ class MessageListView with ChangeNotifier, _MessageSequence { } } + + /// Fetch messages, starting from scratch. Future fetchInitial() async { // TODO(#80): fetch from anchor firstUnread, instead of newest - // TODO(#82): fetch from a given message ID as anchor - assert(!fetched && !haveOldest && !fetchingOlder && !fetchOlderCoolingDown); + + assert(!fetched && !haveOldest && !fetchingOlder && !fetchOlderCoolingDown && !fetchingNewer && !fetchNewerCoolingDown && !haveNewest); assert(messages.isEmpty && contents.isEmpty); // TODO schedule all this in another isolate final generation = this.generation; final result = await getMessages(store.connection, narrow: narrow.apiEncode(), - anchor: AnchorCode.newest, - numBefore: kMessageListFetchBatchSize, - numAfter: 0, + anchor: anchorMessageId != null + ? NumericAnchor(anchorMessageId!) + : AnchorCode.newest, + numBefore: anchorMessageId != null + ? kMessageListFetchBatchSize ~/ 2 // Fetch messages before and after anchor + : kMessageListFetchBatchSize, // Fetch only older messages when no anchor + numAfter: anchorMessageId != null + ? kMessageListFetchBatchSize ~/2 // Fetch messages before and after anchor + : 0, // Don't fetch newer messages when no anchor ); + anchorMessageId ??= result.messages.last.id; + if (this.generation > generation) return; store.reconcileMessages(result.messages); store.recentSenders.handleMessages(result.messages); // TODO(#824) @@ -520,10 +603,12 @@ class MessageListView with ChangeNotifier, _MessageSequence { } _fetched = true; _haveOldest = result.foundOldest; + _haveNewest = result.foundNewest; _updateEndMarkers(); notifyListeners(); } + /// Fetch the next batch of older messages, if applicable. Future fetchOlder() async { if (haveOldest) return; @@ -589,6 +674,76 @@ class MessageListView with ChangeNotifier, _MessageSequence { } } + /// Fetch the next batch of newer messages, if applicable. + Future fetchNewer() async { + if (haveNewest) return; + if (fetchingNewer) return; + if (fetchNewerCoolingDown) return; + assert(fetched); + assert(messages.isNotEmpty); + + _fetchingNewer = true; + _updateEndMarkers(); + notifyListeners(); + + final generation = this.generation; + bool hasFetchError = false; + + try { + final GetMessagesResult result; + try { + result = await getMessages(store.connection, + narrow: narrow.apiEncode(), + anchor: NumericAnchor(messages.last.id), + includeAnchor: false, + numBefore: 0, + numAfter: kMessageListFetchBatchSize, + ); + } catch (e) { + hasFetchError = true; + rethrow; + } + if (this.generation > generation) return; + + if (result.messages.isNotEmpty + && result.messages.first.id == messages.last.id) { + // TODO(server-6): includeAnchor should make this impossible + result.messages.removeAt(0); + } + + store.reconcileMessages(result.messages); + store.recentSenders.handleMessages(result.messages); + + final fetchedMessages = _allMessagesVisible + ? result.messages + : result.messages.where(_messageVisible); + + _insertAllMessages(messages.length, fetchedMessages); + + _haveNewest = result.foundNewest; + + } finally { + if (this.generation == generation) { + _fetchingNewer = false; + if (hasFetchError) { + assert(!fetchNewerCoolingDown); + _fetchNewerCoolingDown = true; + unawaited((_fetchNewerCooldownBackoffMachine ??= BackoffMachine()) + .wait().then((_) { + if (this.generation != generation) return; + _fetchNewerCoolingDown = false; + _updateEndMarkers(); + notifyListeners(); + })); + } else { + _fetchNewerCooldownBackoffMachine = null; + } + _updateEndMarkers(); + notifyListeners(); + } + } + } + void handleUserTopicEvent(UserTopicEvent event) { switch (_canAffectVisibility(event)) { case VisibilityEffect.none: diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index f5416e3ccf..abed7727db 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -182,12 +182,12 @@ abstract class MessageListPageState { } class MessageListPage extends StatefulWidget { - const MessageListPage({super.key, required this.initNarrow}); - + const MessageListPage({super.key, required this.initNarrow, this.anchorMessageId}); + final int? anchorMessageId; static Route buildRoute({int? accountId, BuildContext? context, - required Narrow narrow}) { + required Narrow narrow, int? anchorMessageId}) { return MaterialAccountWidgetRoute(accountId: accountId, context: context, - page: MessageListPage(initNarrow: narrow)); + page: MessageListPage(initNarrow: narrow, anchorMessageId: anchorMessageId)); } /// The [MessageListPageState] above this context in the tree. @@ -302,7 +302,7 @@ class _MessageListPageState extends State implements MessageLis removeBottom: ComposeBox.hasComposeBox(narrow), child: Expanded( - child: MessageList(narrow: narrow, onNarrowChanged: _narrowChanged))), + child: MessageList(narrow: narrow, onNarrowChanged: _narrowChanged, anchorMessageId: widget.anchorMessageId))), if (ComposeBox.hasComposeBox(narrow)) ComposeBox(key: _composeBoxKey, narrow: narrow) ])))); @@ -443,11 +443,11 @@ const _kShortMessageHeight = 80; const kFetchMessagesBufferPixels = (kMessageListFetchBatchSize / 2) * _kShortMessageHeight; class MessageList extends StatefulWidget { - const MessageList({super.key, required this.narrow, required this.onNarrowChanged}); + const MessageList({super.key, required this.narrow, required this.onNarrowChanged, this.anchorMessageId}); final Narrow narrow; final void Function(Narrow newNarrow) onNarrowChanged; - + final int? anchorMessageId; @override State createState() => _MessageListState(); } @@ -456,6 +456,8 @@ class _MessageListState extends State with PerAccountStoreAwareStat MessageListView? model; final ScrollController scrollController = ScrollController(); final ValueNotifier _scrollToBottomVisibleValue = ValueNotifier(false); + List newItems = []; + List oldItems = []; @override void initState() { @@ -476,10 +478,14 @@ class _MessageListState extends State with PerAccountStoreAwareStat super.dispose(); } - void _initModel(PerAccountStore store) { - model = MessageListView.init(store: store, narrow: widget.narrow); + void _initModel(PerAccountStore store) async{ + model = MessageListView.init(store: store, narrow: widget.narrow, anchorMessageId: widget.anchorMessageId); model!.addListener(_modelChanged); - model!.fetchInitial(); + await model!.fetchInitial(); + setState(() { + oldItems = model!.items.sublist(0, model!.anchorIndex!+1); + newItems = model!.items.sublist(model!.anchorIndex!+1, model!.items.length); + }); } void _modelChanged() { @@ -488,10 +494,54 @@ class _MessageListState extends State with PerAccountStoreAwareStat // [PropagateMode.changeAll] or [PropagateMode.changeLater]. widget.onNarrowChanged(model!.narrow); } + + final previousLength = oldItems.length + newItems.length; + setState(() { + oldItems = model!.items.sublist(0, model!.anchorIndex!+1); + newItems = model!.items.sublist(model!.anchorIndex!+1, model!.items.length); // The actual state lives in the [MessageListView] model. // This method was called because that just changed. }); + + + // Auto-scroll when new messages arrive if we're already near the bottom + if (model!.items.length > previousLength && // New messages were added + scrollController.hasClients) { + // Use post-frame callback to ensure scroll metrics are up to date + WidgetsBinding.instance.addPostFrameCallback((_) async { + // This is to prevent auto-scrolling when fetching newer messages + if(model!.fetchingNewer || model!.fetchingOlder || model!.fetchNewerCoolingDown || model!.fetchOlderCoolingDown || !model!.haveNewest ){ + return; + } + + final viewportDimension = scrollController.position.viewportDimension; + final maxScrollExtent = scrollController.position.maxScrollExtent; + final currentScroll = scrollController.position.pixels; + + // If we're within 300px of the bottommost viewport, auto-scroll + if (maxScrollExtent - currentScroll - viewportDimension < 300) { + + final distance = scrollController.position.pixels; + final durationMsAtSpeedLimit = (1000 * distance / 8000).ceil(); + final durationMs = max(300, durationMsAtSpeedLimit); + + await scrollController.animateTo( + scrollController.position.maxScrollExtent, + duration: Duration(milliseconds: durationMs), + curve: Curves.ease); + + + + if (scrollController.position.pixels + 40 < scrollController.position.maxScrollExtent ) { + await scrollController.animateTo( + scrollController.position.maxScrollExtent, + duration: Duration(milliseconds: durationMs), + curve: Curves.ease); + } + } + }); + } } void _handleScrollMetrics(ScrollMetrics scrollMetrics) { @@ -510,6 +560,11 @@ class _MessageListState extends State with PerAccountStoreAwareStat // still not yet updated to account for the newly-added messages. model?.fetchOlder(); } + + // Check for fetching newer messages when near the bottom + if (scrollMetrics.extentAfter < kFetchMessagesBufferPixels) { + model?.fetchNewer(); + } } void _scrollChanged() { @@ -562,7 +617,8 @@ class _MessageListState extends State with PerAccountStoreAwareStat } Widget _buildListView(BuildContext context) { - final length = model!.items.length; + final length = oldItems.length; + final newLength = newItems.length; const centerSliverKey = ValueKey('center sliver'); Widget sliver = SliverStickyHeaderList( @@ -587,22 +643,32 @@ class _MessageListState extends State with PerAccountStoreAwareStat final valueKey = key as ValueKey; final index = model!.findItemWithMessageId(valueKey.value); if (index == -1) return null; - return length - 1 - (index - 3); + return length - 1 - index; }, - childCount: length + 3, + childCount: length, (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); - - if (i == 2) return TypingStatusWidget(narrow: widget.narrow); - - final data = model!.items[length - 1 - (i - 3)]; + final data = oldItems[length - 1 - i]; return _buildItem(data, i); })); + Widget newMessagesSliver = SliverStickyHeaderList( + headerPlacement: HeaderPlacement.scrollingStart, + delegate: SliverChildBuilderDelegate( + findChildIndexCallback: (Key key) { + final valueKey = key as ValueKey; + final index = model!.findItemWithMessageId(valueKey.value); + if (index == -1) return null; + return index-3; + }, + childCount: newLength+3, + (context, i) { + if (i == newLength) return TypingStatusWidget(narrow: widget.narrow); + if (i == newLength+1) return MarkAsReadWidget(narrow: widget.narrow); + if (i == newLength+2) return const SizedBox(height: 36); + final data = newItems[i]; + return _buildItem(data, i-newLength); + })); + if (!ComposeBox.hasComposeBox(widget.narrow)) { // TODO(#311) If we have a bottom nav, it will pad the bottom // inset, and this shouldn't be necessary @@ -623,16 +689,16 @@ class _MessageListState extends State with PerAccountStoreAwareStat controller: scrollController, semanticChildCount: length + 2, - anchor: 1.0, + anchor: 0.85, center: centerSliverKey, slivers: [ - sliver, - - // 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. + sliver, // Main message list (grows upward) + // Center point - everything before this grows up, everything after grows down const SliverToBoxAdapter(key: centerSliverKey), + // Static widgets and new messages (will grow downward) + newMessagesSliver, // New messages list (will grow downward) + ]); } @@ -674,14 +740,28 @@ class ScrollToBottomButton extends StatelessWidget { final ValueNotifier visibleValue; final ScrollController scrollController; - Future _navigateToBottom() { + Future _navigateToBottom() async { + // Calculate initial scroll parameters final distance = scrollController.position.pixels; final durationMsAtSpeedLimit = (1000 * distance / 8000).ceil(); final durationMs = max(300, durationMsAtSpeedLimit); - return scrollController.animateTo( - 0, + + // Do a single scroll attempt with a completion check + await scrollController.animateTo( + scrollController.position.maxScrollExtent, duration: Duration(milliseconds: durationMs), curve: Curves.ease); + var count =1; + // Check if we actually reached bottom, if not try again + // This handles cases where content was loaded during scroll + while (scrollController.position.pixels + 40 < scrollController.position.maxScrollExtent) { + await scrollController.animateTo( + scrollController.position.maxScrollExtent, + duration: const Duration(milliseconds: 300), + curve: Curves.ease); + count++; + } + print("count: $count"); } @override @@ -728,6 +808,7 @@ class _TypingStatusWidgetState extends State with PerAccount } void _modelChanged() { + setState(() { // The actual state lives in [model]. // This method was called because that just changed. @@ -1348,6 +1429,21 @@ class MessageWithPossibleSender extends StatelessWidget { return GestureDetector( behavior: HitTestBehavior.translucent, onLongPress: () => showMessageActionSheet(context: context, message: message), + onDoubleTap: () { + if (context.findAncestorWidgetOfExactType()?.initNarrow is MentionsNarrow) { + final store = PerAccountStoreWidget.of(context); + final narrow = switch (message) { + StreamMessage(:var streamId, :var topic) => TopicNarrow(streamId, topic), + DmMessage(:var allRecipientIds) => DmNarrow( + allRecipientIds: allRecipientIds.toList(), + selfUserId: store.selfUserId), + }; + Navigator.push(context, + MessageListPage.buildRoute(context: context, narrow: narrow, anchorMessageId: message.id)); + + + } + }, child: Padding( padding: const EdgeInsets.symmetric(vertical: 4), child: Column(children: [ From 9eafb82f3e6c2d054ec2f675245705386a332d4d Mon Sep 17 00:00:00 2001 From: E-m-i-n-e-n-c-e Date: Mon, 20 Jan 2025 23:00:40 +0530 Subject: [PATCH 2/5] check --- lib/widgets/message_list.dart | 111 ++++++++++++++++++++++++---------- 1 file changed, 79 insertions(+), 32 deletions(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index abed7727db..c8d7e736bd 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -1,4 +1,4 @@ -import 'dart:math'; +import 'dart:math' as math; import 'package:collection/collection.dart'; import 'package:flutter/material.dart'; @@ -516,36 +516,47 @@ class _MessageListState extends State with PerAccountStoreAwareStat } final viewportDimension = scrollController.position.viewportDimension; - final maxScrollExtent = scrollController.position.maxScrollExtent; final currentScroll = scrollController.position.pixels; - // If we're within 300px of the bottommost viewport, auto-scroll - if (maxScrollExtent - currentScroll - viewportDimension < 300) { + // If we're one viewportDimension from the bottomList, scroll to it + if (currentScroll + viewportDimension > 0) { - final distance = scrollController.position.pixels; - final durationMsAtSpeedLimit = (1000 * distance / 8000).ceil(); - final durationMs = max(300, durationMsAtSpeedLimit); + // Calculate initial scroll parameters + final distanceToCenter = scrollController.position.pixels; + final durationMsAtSpeedLimit = (1000 * distanceToCenter / 8000).ceil(); + final durationMs = math.max(300, durationMsAtSpeedLimit); - await scrollController.animateTo( - scrollController.position.maxScrollExtent, - duration: Duration(milliseconds: durationMs), - curve: Curves.ease); + // If we're not at the bottomSliver,scroll to it + if(distanceToCenter<36){ + await scrollController.animateTo( + 36, //Scroll 36 px inside bottomSliver.The sizedBox is 36px high. so theres no chance of overscrolling + duration: Duration(milliseconds: durationMs), + curve: Curves.easeIn); + await Future.delayed(const Duration(milliseconds: 50)); + } + // Wait for the layout to settle so scrollController.position.pixels is updated properly - if (scrollController.position.pixels + 40 < scrollController.position.maxScrollExtent ) { + final distanceToBottom = scrollController.position.maxScrollExtent - scrollController.position.pixels; + final durationMsToBottom = math.min(1500, (1000 * distanceToBottom / 8000).ceil()); + // If we go too fast, we'll overscroll.as + + // After scroling to the bottom sliver, scroll to the bottom of the bottomSliver if we're not already there + if (distanceToBottom > 36) { await scrollController.animateTo( scrollController.position.maxScrollExtent, - duration: Duration(milliseconds: durationMs), - curve: Curves.ease); - } + duration: Duration(milliseconds: durationMsToBottom), + curve: Curves.ease); + } + } }); } } void _handleScrollMetrics(ScrollMetrics scrollMetrics) { - if (scrollMetrics.extentAfter == 0) { + if (scrollMetrics.extentAfter < 40) { _scrollToBottomVisibleValue.value = false; } else { _scrollToBottomVisibleValue.value = true; @@ -675,10 +686,13 @@ class _MessageListState extends State with PerAccountStoreAwareStat sliver = SliverSafeArea(sliver: sliver); } + + return CustomScrollView( // TODO: Offer `ScrollViewKeyboardDismissBehavior.interactive` (or // similar) if that is ever offered: // https://github.com/flutter/flutter/issues/57609#issuecomment-1355340849 + keyboardDismissBehavior: switch (Theme.of(context).platform) { // This seems to offer the only built-in way to close the keyboard // on iOS. It's not ideal; see TODO above. @@ -734,6 +748,30 @@ class _MessageListState extends State with PerAccountStoreAwareStat } } + +class NoOverScrollPhysics extends ScrollPhysics { + const NoOverScrollPhysics({super.parent}); + + @override + NoOverScrollPhysics applyTo(ScrollPhysics? ancestor) { + return NoOverScrollPhysics(parent: buildParent(ancestor)); + } + + @override + double applyBoundaryConditions(ScrollMetrics position, double value) { + // Prevent overscroll at the top + if (value < position.minScrollExtent) { + return position.minScrollExtent; + } + // Prevent overscroll at the bottom + if (value > position.maxScrollExtent) { + return position.maxScrollExtent; + } + return 0.0; // Allow normal scrolling within bounds + } +} + + class ScrollToBottomButton extends StatelessWidget { const ScrollToBottomButton({super.key, required this.scrollController, required this.visibleValue}); @@ -742,26 +780,34 @@ class ScrollToBottomButton extends StatelessWidget { Future _navigateToBottom() async { // Calculate initial scroll parameters - final distance = scrollController.position.pixels; - final durationMsAtSpeedLimit = (1000 * distance / 8000).ceil(); - final durationMs = max(300, durationMsAtSpeedLimit); + final distanceToCenter = scrollController.position.pixels; + final durationMsAtSpeedLimit = (1000 * distanceToCenter / 8000).ceil(); + final durationMs = math.max(300, durationMsAtSpeedLimit); - // Do a single scroll attempt with a completion check - await scrollController.animateTo( - scrollController.position.maxScrollExtent, + // If we're not at the bottomSliver,scroll to it + if(distanceToCenter<36){ + await scrollController.animateTo( + 36, //Scroll 36 px inside bottomSliver.The sizedBox is 36px high. so theres no chance of overscrolling duration: Duration(milliseconds: durationMs), - curve: Curves.ease); - var count =1; - // Check if we actually reached bottom, if not try again - // This handles cases where content was loaded during scroll - while (scrollController.position.pixels + 40 < scrollController.position.maxScrollExtent) { + curve: Curves.easeIn); + } + + + // Wait for the layout to settle so scrollController.position.pixels is updated properly + await Future.delayed(const Duration(milliseconds: 50)); + + + final distanceToBottom = scrollController.position.maxScrollExtent - scrollController.position.pixels; + final durationMsToBottom = math.min(1000, (1000 * distanceToBottom / 8000).ceil()); + // If we go too fast, we'll overscroll. + + // After scroling to the bottom sliver, scroll to the bottom of the bottomSliver if we're not already there + if (distanceToBottom > 36) { await scrollController.animateTo( scrollController.position.maxScrollExtent, - duration: const Duration(milliseconds: 300), - curve: Curves.ease); - count++; + duration: Duration(milliseconds: durationMsToBottom), + curve: Curves.easeOut); } - print("count: $count"); } @override @@ -1221,7 +1267,8 @@ class DmRecipientHeader extends StatelessWidget { .where((id) => id != store.selfUserId) .map((id) => store.users[id]?.fullName ?? zulipLocalizations.unknownUserName) .sorted() - .join(", ")); + .join(", ") + ); } else { // TODO pick string; web has glitchy "You and $yourname" title = zulipLocalizations.messageListGroupYouWithYourself; From 6cf427fb12615ffce0de2fd1053937f3131b07fe Mon Sep 17 00:00:00 2001 From: E-m-i-n-e-n-c-e Date: Tue, 21 Jan 2025 00:21:28 +0530 Subject: [PATCH 3/5] If it's working Don't touch it --- lib/widgets/message_list.dart | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index c8d7e736bd..afb9f9f638 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -515,11 +515,13 @@ class _MessageListState extends State with PerAccountStoreAwareStat return; } + // Might be used in the future + // ignore: unused_local_variable final viewportDimension = scrollController.position.viewportDimension; final currentScroll = scrollController.position.pixels; // If we're one viewportDimension from the bottomList, scroll to it - if (currentScroll + viewportDimension > 0) { + if (currentScroll + 300 > 0) { // Calculate initial scroll parameters final distanceToCenter = scrollController.position.pixels; @@ -537,17 +539,18 @@ class _MessageListState extends State with PerAccountStoreAwareStat // Wait for the layout to settle so scrollController.position.pixels is updated properly - - final distanceToBottom = scrollController.position.maxScrollExtent - scrollController.position.pixels; - final durationMsToBottom = math.min(1500, (1000 * distanceToBottom / 8000).ceil()); + var distanceToBottom = scrollController.position.maxScrollExtent - scrollController.position.pixels; + final durationMsToBottom = math.min(200, (1000 * distanceToBottom / 8000).ceil()); // If we go too fast, we'll overscroll.as // After scroling to the bottom sliver, scroll to the bottom of the bottomSliver if we're not already there - if (distanceToBottom > 36) { + while (distanceToBottom > 36) { await scrollController.animateTo( scrollController.position.maxScrollExtent, duration: Duration(milliseconds: durationMsToBottom), curve: Curves.ease); + distanceToBottom = scrollController.position.maxScrollExtent - scrollController.position.pixels; + await Future.delayed(const Duration(milliseconds: 50)); } } @@ -783,7 +786,6 @@ class ScrollToBottomButton extends StatelessWidget { final distanceToCenter = scrollController.position.pixels; final durationMsAtSpeedLimit = (1000 * distanceToCenter / 8000).ceil(); final durationMs = math.max(300, durationMsAtSpeedLimit); - // If we're not at the bottomSliver,scroll to it if(distanceToCenter<36){ await scrollController.animateTo( @@ -796,18 +798,21 @@ class ScrollToBottomButton extends StatelessWidget { // Wait for the layout to settle so scrollController.position.pixels is updated properly await Future.delayed(const Duration(milliseconds: 50)); - - final distanceToBottom = scrollController.position.maxScrollExtent - scrollController.position.pixels; - final durationMsToBottom = math.min(1000, (1000 * distanceToBottom / 8000).ceil()); + var distanceToBottom = scrollController.position.maxScrollExtent - scrollController.position.pixels; + final durationMsToBottom = math.min(1000, (1200 * distanceToBottom / 8000).ceil()); // If we go too fast, we'll overscroll. - // After scroling to the bottom sliver, scroll to the bottom of the bottomSliver if we're not already there - if (distanceToBottom > 36) { + var count = 0; + while (distanceToBottom > 36) { await scrollController.animateTo( scrollController.position.maxScrollExtent, duration: Duration(milliseconds: durationMsToBottom), curve: Curves.easeOut); + await Future.delayed(const Duration(milliseconds: 50)); + distanceToBottom = scrollController.position.maxScrollExtent - scrollController.position.pixels; + count++; } + print("count: $count"); } @override From 0725cfe1594e4e2978881b3025f3001cdd115546 Mon Sep 17 00:00:00 2001 From: E-m-i-n-e-n-c-e Date: Tue, 21 Jan 2025 01:57:24 +0530 Subject: [PATCH 4/5] 3 More test cases to go bitch --- lib/model/message_list.dart | 6 ++++-- lib/widgets/message_list.dart | 8 ++++---- test/widgets/message_list_test.dart | 10 +++++++++- 3 files changed, 17 insertions(+), 7 deletions(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index 5da716c56a..8c1409a70f 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -487,7 +487,7 @@ class MessageListView with ChangeNotifier, _MessageSequence { // Anchor message ID is used to fetch messages from a specific point in the list. // It is set when the user navigates to a message list page with a specific anchor message. int? anchorMessageId; - int? get anchorIndex => anchorMessageId != null ? findItemWithMessageId(anchorMessageId!) : null; + int get anchorIndex => anchorMessageId != null ? findItemWithMessageId(anchorMessageId!) : 0; factory MessageListView.init( {required PerAccountStore store, required Narrow narrow, int? anchorMessageId}) { final view = MessageListView._(store: store, narrow: narrow, anchorMessageId: anchorMessageId); @@ -591,7 +591,9 @@ class MessageListView with ChangeNotifier, _MessageSequence { ? kMessageListFetchBatchSize ~/2 // Fetch messages before and after anchor : 0, // Don't fetch newer messages when no anchor ); - anchorMessageId ??= result.messages.last.id; + if(result.messages.isNotEmpty){ + anchorMessageId ??= result.messages.last.id; + } if (this.generation > generation) return; store.reconcileMessages(result.messages); diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index afb9f9f638..608f6b4901 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -483,8 +483,8 @@ class _MessageListState extends State with PerAccountStoreAwareStat model!.addListener(_modelChanged); await model!.fetchInitial(); setState(() { - oldItems = model!.items.sublist(0, model!.anchorIndex!+1); - newItems = model!.items.sublist(model!.anchorIndex!+1, model!.items.length); + oldItems = model!.items.sublist(0, model!.anchorIndex+1); + newItems = model!.items.sublist(model!.anchorIndex+1, model!.items.length); }); } @@ -498,8 +498,8 @@ class _MessageListState extends State with PerAccountStoreAwareStat final previousLength = oldItems.length + newItems.length; setState(() { - oldItems = model!.items.sublist(0, model!.anchorIndex!+1); - newItems = model!.items.sublist(model!.anchorIndex!+1, model!.items.length); + oldItems = model!.items.sublist(0, model!.anchorIndex+1); + newItems = model!.items.sublist(model!.anchorIndex+1, model!.items.length); // The actual state lives in the [MessageListView] model. // This method was called because that just changed. }); diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index c220dc0e0c..02ed19d667 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -366,7 +366,7 @@ void main() { await tester.tap(find.byType(ScrollToBottomButton)); await tester.pumpAndSettle(); check(isButtonVisible(tester)).equals(false); - check(scrollController.position.pixels).equals(0); + check(scrollController.position.maxScrollExtent - scrollController.position.pixels).isLessThan(36); }); }); @@ -720,6 +720,10 @@ void main() { final existingMessage = eg.streamMessage( stream: eg.stream(), topic: 'new topic', content: 'Existing message'); prepareGetMessageResponse([existingMessage, message]); + // Prepare response for fetchInitial after move + connection.prepare(json: eg.newestGetMessagesResult( + foundOldest: true, + messages: [existingMessage, message]).toJson()); handleMessageMoveEvent([message], 'new topic'); await tester.pump(const Duration(seconds: 1)); @@ -732,6 +736,10 @@ void main() { await setupMessageListPage(tester, narrow: narrow, messages: [message], streams: [channel]); prepareGetMessageResponse([message]); + // Prepare response for fetchInitial after move + connection.prepare(json: eg.newestGetMessagesResult( + foundOldest: true, + messages: [message]).toJson()); handleMessageMoveEvent([message], 'new topic'); await tester.pump(const Duration(seconds: 1)); From f013da3685a2cc1d59a115c0356f430b48f3ac63 Mon Sep 17 00:00:00 2001 From: E-m-i-n-e-n-c-e Date: Thu, 23 Jan 2025 19:35:07 +0530 Subject: [PATCH 5/5] All test cases pass now --- lib/widgets/message_list.dart | 31 +++++++++++++------------------ pubspec.lock | 4 ++-- 2 files changed, 15 insertions(+), 20 deletions(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 608f6b4901..cd416fc000 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -489,21 +489,23 @@ class _MessageListState extends State with PerAccountStoreAwareStat } void _modelChanged() { - if (model!.narrow != widget.narrow) { - // A message move event occurred, where propagate mode is - // [PropagateMode.changeAll] or [PropagateMode.changeLater]. - widget.onNarrowChanged(model!.narrow); - } - final previousLength = oldItems.length + newItems.length; - setState(() { + // Update both slivers with the new message positions oldItems = model!.items.sublist(0, model!.anchorIndex+1); newItems = model!.items.sublist(model!.anchorIndex+1, model!.items.length); // The actual state lives in the [MessageListView] model. // This method was called because that just changed. }); + if (model!.narrow != widget.narrow) { + // A message move event occurred, where propagate mode is + // [PropagateMode.changeAll] or [PropagateMode.changeLater]. + widget.onNarrowChanged(model!.narrow); + return; // Let the parent widget handle the rebuild with new narrow + } + + // Auto-scroll when new messages arrive if we're already near the bottom if (model!.items.length > previousLength && // New messages were added @@ -534,7 +536,6 @@ class _MessageListState extends State with PerAccountStoreAwareStat 36, //Scroll 36 px inside bottomSliver.The sizedBox is 36px high. so theres no chance of overscrolling duration: Duration(milliseconds: durationMs), curve: Curves.easeIn); - await Future.delayed(const Duration(milliseconds: 50)); } // Wait for the layout to settle so scrollController.position.pixels is updated properly @@ -544,13 +545,12 @@ class _MessageListState extends State with PerAccountStoreAwareStat // If we go too fast, we'll overscroll.as // After scroling to the bottom sliver, scroll to the bottom of the bottomSliver if we're not already there - while (distanceToBottom > 36) { + while (distanceToBottom > 36 && context.mounted) { await scrollController.animateTo( scrollController.position.maxScrollExtent, duration: Duration(milliseconds: durationMsToBottom), curve: Curves.ease); distanceToBottom = scrollController.position.maxScrollExtent - scrollController.position.pixels; - await Future.delayed(const Duration(milliseconds: 50)); } } @@ -781,7 +781,7 @@ class ScrollToBottomButton extends StatelessWidget { final ValueNotifier visibleValue; final ScrollController scrollController; - Future _navigateToBottom() async { + Future _navigateToBottom(BuildContext context) async { // Calculate initial scroll parameters final distanceToCenter = scrollController.position.pixels; final durationMsAtSpeedLimit = (1000 * distanceToCenter / 8000).ceil(); @@ -796,23 +796,18 @@ class ScrollToBottomButton extends StatelessWidget { // Wait for the layout to settle so scrollController.position.pixels is updated properly - await Future.delayed(const Duration(milliseconds: 50)); var distanceToBottom = scrollController.position.maxScrollExtent - scrollController.position.pixels; final durationMsToBottom = math.min(1000, (1200 * distanceToBottom / 8000).ceil()); // If we go too fast, we'll overscroll. // After scroling to the bottom sliver, scroll to the bottom of the bottomSliver if we're not already there - var count = 0; - while (distanceToBottom > 36) { + while (distanceToBottom > 36 && context.mounted) { await scrollController.animateTo( scrollController.position.maxScrollExtent, duration: Duration(milliseconds: durationMsToBottom), curve: Curves.easeOut); - await Future.delayed(const Duration(milliseconds: 50)); distanceToBottom = scrollController.position.maxScrollExtent - scrollController.position.pixels; - count++; } - print("count: $count"); } @override @@ -829,7 +824,7 @@ class ScrollToBottomButton extends StatelessWidget { iconSize: 40, // Web has the same color in light and dark mode. color: const HSLColor.fromAHSL(0.5, 240, 0.96, 0.68).toColor(), - onPressed: _navigateToBottom)); + onPressed: () => _navigateToBottom(context))); } } diff --git a/pubspec.lock b/pubspec.lock index 4d5f5d63df..5c2ddec9a4 100644 --- a/pubspec.lock +++ b/pubspec.lock @@ -1044,10 +1044,10 @@ packages: dependency: transitive description: name: stream_channel - sha256: "4ac0537115a24d772c408a2520ecd0abb99bca2ea9c4e634ccbdbfae64fe17ec" + sha256: "969e04c80b8bcdf826f8f16579c7b14d780458bd97f56d107d3950fdbeef059d" url: "https://pub.dev" source: hosted - version: "2.1.3" + version: "2.1.4" stream_transform: dependency: transitive description: