From 56ab39576fc014743dd18964a137b45441602e9c Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 20 Nov 2023 11:25:59 -0500 Subject: [PATCH 1/4] msglist: Add 36px space at bottom, to reinforce that end-of-feed is reached As requested by Vlad (same link as in a code comment): https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/flutter.3A.20Mark-as-read/near/1680603 Fixes: #400 --- lib/widgets/message_list.dart | 12 ++++++++---- test/widgets/message_list_test.dart | 4 ++-- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index daa68cf86f..7fc08c0174 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -293,10 +293,10 @@ 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 - 1); + return length - 1 - (index - 2); }, controller: scrollController, - itemCount: length + 1, + 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. @@ -305,9 +305,13 @@ class _MessageListState extends State with PerAccountStoreAwareStat // TODO on new message when scrolled up, anchor scroll to what's in view reverse: true, itemBuilder: (context, i) { - if (i == 0) return MarkAsReadWidget(narrow: widget.narrow); + // 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); - final data = model!.items[length - 1 - (i - 1)]; + if (i == 1) return MarkAsReadWidget(narrow: widget.narrow); + + final data = model!.items[length - 1 - (i - 2)]; switch (data) { case MessageListHistoryStartItem(): return const Center( diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index 1de30a719c..c8555e2b74 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -81,7 +81,7 @@ void main() { testWidgets('basic', (tester) async { await setupMessageListPage(tester, foundOldest: false, messages: List.generate(200, (i) => eg.streamMessage(id: 950 + i, sender: eg.selfUser))); - check(itemCount(tester)).equals(202); + check(itemCount(tester)).equals(203); // Fling-scroll upward... await tester.fling(find.byType(MessageListPage), const Offset(0, 300), 8000); @@ -94,7 +94,7 @@ void main() { await tester.pump(Duration.zero); // Allow a frame for the response to arrive. // Now we have more messages. - check(itemCount(tester)).equals(302); + check(itemCount(tester)).equals(303); }); testWidgets('observe double-fetch glitch', (tester) async { From c8fe66e12a76f88d89fe4a617d75dbeb68d605c9 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 20 Nov 2023 11:31:12 -0500 Subject: [PATCH 2/4] msglist [nfc]: Give MessageItem the SizedBox for trailing whitespace Instead of having its caller make the SizedBox. This way, the whitespace will be naturally included when we give the whole message list's white background color to the individual MessageItems, coming next. --- lib/widgets/message_list.dart | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 7fc08c0174..4ddc99ae60 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -330,7 +330,7 @@ class _MessageListState extends State with PerAccountStoreAwareStat case MessageListMessageItem(): return MessageItem( key: ValueKey(data.message.id), - trailing: i == 1 ? const SizedBox(height: 8) : const SizedBox(height: 11), + trailingWhitespace: i == 1 ? 8 : 11, item: data); } }); @@ -450,11 +450,11 @@ class MessageItem extends StatelessWidget { const MessageItem({ super.key, required this.item, - this.trailing, + this.trailingWhitespace, }); final MessageListMessageItem item; - final Widget? trailing; + final double? trailingWhitespace; @override Widget build(BuildContext context) { @@ -466,7 +466,7 @@ class MessageItem extends StatelessWidget { isRead: message.flags.contains(MessageFlag.read), child: Column(children: [ MessageWithPossibleSender(item: item), - if (trailing != null && item.isLastInBlock) trailing!, + if (trailingWhitespace != null && item.isLastInBlock) SizedBox(height: trailingWhitespace!), ]))); } } From dff550ef275edf233714dfbd58e8fa07ec18bb35 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 20 Nov 2023 11:36:13 -0500 Subject: [PATCH 3/4] msglist: Move white background color to MessageItem, from whole list This lets `colorScheme.background` (which we matched to the Figma in 21dbae120) come through in the area with the "Mark X messages as read" button, following a Figma frame: https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=147%3A9133&mode=dev (There's a different Figma frame that shows a stream-specific color there; more on that in the next commit.) The background color (`colorScheme.background`) will now also come through in these areas: - in the area with the "No earlier messages" text, when scrolled to the earliest messages - in the new blank space we added after the most recent message, earlier in this series of commits. Fixes: #401 --- lib/widgets/message_list.dart | 66 +++++++++++++++++------------------ 1 file changed, 33 insertions(+), 33 deletions(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 4ddc99ae60..afc6867463 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -229,35 +229,33 @@ class _MessageListState extends State with PerAccountStoreAwareStat // TODO figure out text color -- web is supposedly hsl(0deg 0% 20%), // but seems much darker than that style: const TextStyle(color: Color.fromRGBO(0, 0, 0, 1)), - child: ColoredBox( - color: Colors.white, - // Pad the left and right insets, for small devices in landscape. - child: SafeArea( - // Don't let this be the place we pad the bottom inset. When there's - // no compose box, we want to let the message-list content pad it. - // TODO(#311) Remove as unnecessary if we do a bottom nav. - // The nav will pad the bottom inset, and an ancestor of this widget - // will have a `MediaQuery.removePadding` with `removeBottom: true`. - bottom: false, - - child: Center( - child: ConstrainedBox( - constraints: const BoxConstraints(maxWidth: 760), - child: NotificationListener( - onNotification: _metricsChanged, - child: Stack( - children: [ - _buildListView(context), - Positioned( - bottom: 0, - right: 0, - // TODO(#311) SafeArea shouldn't be needed if we have a - // bottom nav. That will pad the bottom inset. - child: SafeArea( - child: ScrollToBottomButton( - scrollController: scrollController, - visibleValue: _scrollToBottomVisibleValue))), - ]))))))); + // Pad the left and right insets, for small devices in landscape. + child: SafeArea( + // Don't let this be the place we pad the bottom inset. When there's + // no compose box, we want to let the message-list content pad it. + // TODO(#311) Remove as unnecessary if we do a bottom nav. + // The nav will pad the bottom inset, and an ancestor of this widget + // will have a `MediaQuery.removePadding` with `removeBottom: true`. + bottom: false, + + child: Center( + child: ConstrainedBox( + constraints: const BoxConstraints(maxWidth: 760), + child: NotificationListener( + onNotification: _metricsChanged, + child: Stack( + children: [ + _buildListView(context), + Positioned( + bottom: 0, + right: 0, + // TODO(#311) SafeArea shouldn't be needed if we have a + // bottom nav. That will pad the bottom inset. + child: SafeArea( + child: ScrollToBottomButton( + scrollController: scrollController, + visibleValue: _scrollToBottomVisibleValue))), + ])))))); } Widget _buildListView(context) { @@ -464,10 +462,12 @@ class MessageItem extends StatelessWidget { header: RecipientHeader(message: message), child: _UnreadMarker( isRead: message.flags.contains(MessageFlag.read), - child: Column(children: [ - MessageWithPossibleSender(item: item), - if (trailingWhitespace != null && item.isLastInBlock) SizedBox(height: trailingWhitespace!), - ]))); + child: ColoredBox( + color: Colors.white, + child: Column(children: [ + MessageWithPossibleSender(item: item), + if (trailingWhitespace != null && item.isLastInBlock) SizedBox(height: trailingWhitespace!), + ])))); } } From 3b95070d3613b57e1b5c784016625ed37180db03 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 20 Nov 2023 11:49:10 -0500 Subject: [PATCH 4/4] msglist [nfc]: Move a TODO; note question for Vlad --- lib/widgets/message_list.dart | 43 +++++++++++++++++++---------------- 1 file changed, 23 insertions(+), 20 deletions(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index afc6867463..3050dac91c 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -53,6 +53,12 @@ class _MessageListPageState extends State { Widget build(BuildContext context) { return Scaffold( appBar: AppBar(title: MessageListAppBarTitle(narrow: widget.narrow)), + // TODO question for Vlad: for a stream view, should we set + // [backgroundColor] based on stream color, as in this frame: + // https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=132%3A9684&mode=dev + // That's not obviously preferred over the default background that + // we matched to the Figma in 21dbae120. See another frame, which uses that: + // https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=147%3A9088&mode=dev body: Builder( builder: (BuildContext context) => Center( child: Column(children: [ @@ -405,26 +411,23 @@ class MarkAsReadWidget extends StatelessWidget { secondChild: SizedBox(width: double.infinity, // Design referenced from: // https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?type=design&node-id=132-9684&mode=design&t=jJwHzloKJ0TMOG4M-0 - child: ColoredBox( - // TODO(#368): this should pull from stream color - color: Colors.transparent, - child: Padding( - // vertical padding adjusted for tap target height (48px) of button - padding: const EdgeInsets.symmetric(horizontal: 10, vertical: 10 - ((48 - 38) / 2)), - child: FilledButton.icon( - style: FilledButton.styleFrom( - backgroundColor: _UnreadMarker.color, - minimumSize: const Size.fromHeight(38), - textStyle: const TextStyle( - fontFamily: 'Source Sans 3', - fontSize: 18, - height: (23 / 18), - ).merge(weightVariableTextStyle(context)), - shape: RoundedRectangleBorder(borderRadius: BorderRadius.circular(7)), - ), - onPressed: () => _handlePress(context), - icon: const Icon(Icons.playlist_add_check), - label: Text(zulipLocalizations.markAsReadLabel(unreadCount))))))); + child: Padding( + // vertical padding adjusted for tap target height (48px) of button + padding: const EdgeInsets.symmetric(horizontal: 10, vertical: 10 - ((48 - 38) / 2)), + child: FilledButton.icon( + style: FilledButton.styleFrom( + backgroundColor: _UnreadMarker.color, + minimumSize: const Size.fromHeight(38), + textStyle: const TextStyle( + fontFamily: 'Source Sans 3', + fontSize: 18, + height: (23 / 18), + ).merge(weightVariableTextStyle(context)), + shape: RoundedRectangleBorder(borderRadius: BorderRadius.circular(7)), + ), + onPressed: () => _handlePress(context), + icon: const Icon(Icons.playlist_add_check), + label: Text(zulipLocalizations.markAsReadLabel(unreadCount)))))); } }