diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index 1b93fc99c7..ad10acfec2 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -106,7 +106,7 @@ mixin _MessageSequence { (message, messageId) => message.id.compareTo(messageId)); } - int _findItemWithMessageId(int messageId) { + int findItemWithMessageId(int messageId) { return binarySearchByKey(items, messageId, _compareItemToMessageId); } @@ -129,7 +129,7 @@ mixin _MessageSequence { final content = parseContent(message.content); contents[index] = content; - final itemIndex = _findItemWithMessageId(message.id); + final itemIndex = findItemWithMessageId(message.id); assert(itemIndex > -1 && items[itemIndex] is MessageListMessageItem && identical((items[itemIndex] as MessageListMessageItem).message, message)); diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 8bf20d5a56..d6af914be2 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -224,9 +224,6 @@ class _MessageListState extends State with PerAccountStoreAwareStat color: Colors.white, // Pad the left and right insets, for small devices in landscape. child: SafeArea( - // Keep some padding when there are no horizontal insets, - // which is usual in portrait mode. - minimum: const EdgeInsets.symmetric(horizontal: 8), child: Center( child: ConstrainedBox( constraints: const BoxConstraints(maxWidth: 760), @@ -258,6 +255,27 @@ 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; + }, controller: scrollController, itemCount: length, // Setting reverse: true means the scroll starts at the bottom. @@ -286,6 +304,7 @@ class _MessageListState extends State with PerAccountStoreAwareStat header: header, child: header); case MessageListMessageItem(): return MessageItem( + key: ValueKey(data.message.id), trailing: i == 0 ? const SizedBox(height: 8) : const SizedBox(height: 11), item: data); } @@ -354,62 +373,55 @@ class MessageItem extends StatelessWidget { @override Widget build(BuildContext context) { - final store = PerAccountStoreWidget.of(context); final message = item.message; - - Color highlightBorderColor; - Color restBorderColor; - if (message is StreamMessage) { - final subscription = store.subscriptions[message.streamId]; - highlightBorderColor = colorForStream(subscription); - restBorderColor = _kStreamMessageBorderColor; - } else if (message is DmMessage) { - highlightBorderColor = _kDmRecipientHeaderColor; - restBorderColor = _kDmRecipientHeaderColor; - } else { - throw Exception("impossible message type: ${message.runtimeType}"); - } - - // This 3px border seems to accurately reproduce something much more - // complicated on web, involving CSS box-shadow; see comment below. - final recipientBorder = BorderSide(color: highlightBorderColor, width: 3); - final restBorder = BorderSide(color: restBorderColor, width: 1); - var borderDecoration = ShapeDecoration( - // Web actually uses, for stream messages, a slightly lighter border at - // right than at bottom and in the recipient header: black 10% alpha, - // vs. 88% lightness. Assume that's an accident. - shape: Border( - left: recipientBorder, - right: restBorder, - bottom: item.isLastInBlock ? restBorder : BorderSide.none, - )); - return StickyHeaderItem( allowOverflow: !item.isLastInBlock, header: RecipientHeader(message: message), - child: Column(children: [ - DecoratedBox( - decoration: borderDecoration, - child: MessageWithPossibleSender(item: item)), - if (trailing != null && item.isLastInBlock) trailing!, - ])); - - // Web handles the left-side recipient marker in a funky way: - // box-shadow: inset 3px 0px 0px -1px #c2726a, -1px 0px 0px 0px #c2726a; - // (where the color is the stream color.) That is, it's a pair of - // box shadows. One of them is inset. - // - // At attempt at a literal translation might look like this: - // - // DecoratedBox( - // decoration: ShapeDecoration(shadows: [ - // BoxShadow(offset: Offset(3, 0), spreadRadius: -1, color: highlightBorderColor), - // BoxShadow(offset: Offset(-1, 0), color: highlightBorderColor), - // ], shape: Border.fromBorderSide(BorderSide.none)), - // child: MessageWithSender(message: message)), - // - // But CSS `box-shadow` seems to not apply under the item itself, while - // Flutter's BoxShadow does. + child: _UnreadMarker( + isRead: message.flags.contains(MessageFlag.read), + child: Column(children: [ + MessageWithPossibleSender(item: item), + if (trailing != null && item.isLastInBlock) trailing!, + ]))); + } +} + +/// Widget responsible for showing the read status of a message. +class _UnreadMarker extends StatelessWidget { + const _UnreadMarker({required this.isRead, required this.child}); + + final bool isRead; + final Widget child; + + @override + Widget build(BuildContext context) { + return Stack( + children: [ + child, + Positioned( + top: 0, + left: 0, + bottom: 0, + width: 4, + child: AnimatedOpacity( + opacity: isRead ? 0 : 1, + // Web uses 2s and 0.3s durations, and a CSS ease-out curve. + // See zulip:web/styles/message_row.css . + duration: Duration(milliseconds: isRead ? 2000 : 300), + curve: Curves.easeOut, + child: DecoratedBox( + decoration: BoxDecoration( + // The color hsl(227deg 78% 59%) comes from the Figma mockup at: + // https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=132-9684 + // See discussion about design at: + // https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/flutter.3A.20unread.20marker/near/1658008 + color: const HSLColor.fromAHSL(1, 227, 0.78, 0.59).toColor(), + // TODO(#95): Don't show this extra border in dark mode, see: + // https://github.com/zulip/zulip-flutter/pull/317#issuecomment-1784311663 + border: Border(left: BorderSide( + width: 1, + color: Colors.white.withOpacity(0.6))))))), + ]); } } diff --git a/test/flutter_checks.dart b/test/flutter_checks.dart index 1bcb78b6ec..d6ee6d9b88 100644 --- a/test/flutter_checks.dart +++ b/test/flutter_checks.dart @@ -7,6 +7,11 @@ import 'package:checks/checks.dart'; import 'package:flutter/services.dart'; import 'package:flutter/widgets.dart'; +extension AnimationChecks on Subject> { + Subject get status => has((d) => d.status, 'status'); + Subject get value => has((d) => d.value, 'value'); +} + extension ClipboardDataChecks on Subject { Subject get text => has((d) => d.text, 'text'); } diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index 5044df096f..f95ff68793 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -17,6 +17,7 @@ import '../example_data.dart' as eg; import '../model/binding.dart'; import '../model/message_list_test.dart'; import '../model/test_store.dart'; +import '../flutter_checks.dart'; import '../stdlib_checks.dart'; import '../test_images.dart'; import 'content_checks.dart'; @@ -300,4 +301,112 @@ void main() { debugNetworkImageHttpClientProvider = null; }); }); + + group('_UnreadMarker animations', () { + // TODO: Improve animation state testing so it is less tied to + // implementation details and more focused on output, see: + // https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/robust.20widget.20finders.20in.20tests/near/1671738 + Animation getAnimation(WidgetTester tester, int messageId) { + final widget = tester.widget(find.descendant( + of: find.byKey(ValueKey(messageId)), + matching: find.byType(FadeTransition))); + return widget.opacity; + } + + testWidgets('from read to unread', (WidgetTester tester) async { + final message = eg.streamMessage(flags: [MessageFlag.read]); + await setupMessageListPage(tester, messages: [message]); + check(getAnimation(tester, message.id)) + ..value.equals(0.0) + ..status.equals(AnimationStatus.dismissed); + + store.handleEvent(eg.updateMessageFlagsRemoveEvent( + MessageFlag.read, [message])); + await tester.pump(); // process handleEvent + check(getAnimation(tester, message.id)) + ..value.equals(0.0) + ..status.equals(AnimationStatus.forward); + + await tester.pumpAndSettle(); + check(getAnimation(tester, message.id)) + ..value.equals(1.0) + ..status.equals(AnimationStatus.completed); + }); + + testWidgets('from unread to read', (WidgetTester tester) async { + final message = eg.streamMessage(flags: []); + await setupMessageListPage(tester, messages: [message]); + check(getAnimation(tester, message.id)) + ..value.equals(1.0) + ..status.equals(AnimationStatus.dismissed); + + store.handleEvent(UpdateMessageFlagsAddEvent( + id: 1, + flag: MessageFlag.read, + messages: [message.id], + all: false, + )); + await tester.pump(); // process handleEvent + check(getAnimation(tester, message.id)) + ..value.equals(1.0) + ..status.equals(AnimationStatus.forward); + + await tester.pumpAndSettle(); + check(getAnimation(tester, message.id)) + ..value.equals(0.0) + ..status.equals(AnimationStatus.completed); + }); + + 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] + // at [_MessageListState._buildListView]. + final message = eg.streamMessage(flags: []); + await setupMessageListPage(tester, messages: [message]); + check(getAnimation(tester, message.id)) + ..value.equals(1.0) + ..status.equals(AnimationStatus.dismissed); + + store.handleEvent(UpdateMessageFlagsAddEvent( + id: 0, + flag: MessageFlag.read, + messages: [message.id], + all: false, + )); + await tester.pump(); // process handleEvent + check(getAnimation(tester, message.id)) + ..value.equals(1.0) + ..status.equals(AnimationStatus.forward); + + // run animation partially + await tester.pump(const Duration(milliseconds: 30)); + check(getAnimation(tester, message.id)) + ..value.isGreaterThan(0.0) + ..value.isLessThan(1.0) + ..status.equals(AnimationStatus.forward); + + // introduce new message + final newMessage = eg.streamMessage(flags:[MessageFlag.read]); + store.handleEvent(MessageEvent(id: 0, message: newMessage)); + await tester.pump(); // process handleEvent + check(find.byType(MessageItem).evaluate()).length.equals(2); + check(getAnimation(tester, message.id)) + ..value.isGreaterThan(0.0) + ..value.isLessThan(1.0) + ..status.equals(AnimationStatus.forward); + check(getAnimation(tester, newMessage.id)) + ..value.equals(0.0) + ..status.equals(AnimationStatus.dismissed); + + final frames = await tester.pumpAndSettle(); + check(frames).isGreaterThan(1); + check(getAnimation(tester, message.id)) + ..value.equals(0.0) + ..status.equals(AnimationStatus.completed); + check(getAnimation(tester, newMessage.id)) + ..value.equals(0.0) + ..status.equals(AnimationStatus.dismissed); + }); + }); }