From 0d577a0376ab534df4ff2ce72cf39665132d128e Mon Sep 17 00:00:00 2001 From: Khader M Khudair Date: Sat, 27 Jul 2024 11:19:21 +0300 Subject: [PATCH 1/4] msglist: Handle loading state in MarkAsReadWidget MarkAsReadWidget needs to be disabled during loading state to prevent multiple requests to the server. --- lib/widgets/message_list.dart | 36 ++++++++++++------ test/widgets/message_list_test.dart | 59 +++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+), 11 deletions(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 5cebbbe019..79a61beef4 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -458,30 +458,40 @@ class ScrollToBottomButton extends StatelessWidget { } } -class MarkAsReadWidget extends StatelessWidget { +class MarkAsReadWidget extends StatefulWidget { const MarkAsReadWidget({super.key, required this.narrow}); final Narrow narrow; + @override + State createState() => _MarkAsReadWidgetState(); +} + +class _MarkAsReadWidgetState extends State { + bool _loading = false; + void _handlePress(BuildContext context) async { if (!context.mounted) return; final store = PerAccountStoreWidget.of(context); final connection = store.connection; final useLegacy = connection.zulipFeatureLevel! < 155; + setState(() => _loading = true); try { - await markNarrowAsRead(context, narrow, useLegacy); + await markNarrowAsRead(context, widget.narrow, useLegacy); } catch (e) { if (!context.mounted) return; final zulipLocalizations = ZulipLocalizations.of(context); - await showErrorDialog(context: context, + showErrorDialog(context: context, title: zulipLocalizations.errorMarkAsReadFailedTitle, message: e.toString()); // TODO(#741): extract user-facing message better return; + } finally { + setState(() => _loading = false); } if (!context.mounted) return; - if (narrow is CombinedFeedNarrow && !useLegacy) { + if (widget.narrow is CombinedFeedNarrow && !useLegacy) { PerAccountStoreWidget.of(context).unreads.handleAllMessagesReadSuccess(); } } @@ -490,13 +500,13 @@ class MarkAsReadWidget extends StatelessWidget { Widget build(BuildContext context) { final zulipLocalizations = ZulipLocalizations.of(context); final store = PerAccountStoreWidget.of(context); - final unreadCount = store.unreads.countInNarrow(narrow); + final unreadCount = store.unreads.countInNarrow(widget.narrow); final areMessagesRead = unreadCount == 0; return IgnorePointer( ignoring: areMessagesRead, child: AnimatedOpacity( - opacity: areMessagesRead ? 0 : 1, + opacity: areMessagesRead ? 0 : _loading ? 0.5 : 1, duration: Duration(milliseconds: areMessagesRead ? 2000 : 300), curve: Curves.easeOut, child: SizedBox(width: double.infinity, @@ -507,8 +517,6 @@ class MarkAsReadWidget extends StatelessWidget { padding: const EdgeInsets.symmetric(horizontal: 10, vertical: 10 - ((48 - 38) / 2)), child: FilledButton.icon( style: FilledButton.styleFrom( - // TODO(#95) need dark-theme colors (foreground and background) - backgroundColor: _UnreadMarker.color, minimumSize: const Size.fromHeight(38), textStyle: // Restate [FilledButton]'s default, which inherits from @@ -522,11 +530,17 @@ class MarkAsReadWidget extends StatelessWidget { height: (23 / 18)) .merge(weightVariableTextStyle(context, wght: 400))), shape: RoundedRectangleBorder(borderRadius: BorderRadius.circular(7)), + ).copyWith( + // Give the buttons a constant color regardless of whether their + // state is disabled, pressed, etc. We handle those states + // separately, via MarkAsReadAnimation. + // TODO(#95) need dark-theme colors (foreground and background) + foregroundColor: WidgetStateColor.resolveWith((_) => Colors.white), + backgroundColor: WidgetStateColor.resolveWith((_) => _UnreadMarker.color), ), - onPressed: () => _handlePress(context), + onPressed: _loading ? null : () => _handlePress(context), icon: const Icon(Icons.playlist_add_check), - label: Text(zulipLocalizations.markAllAsReadLabel))))), - ); + label: Text(zulipLocalizations.markAllAsReadLabel)))))); } } diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index d5744f2051..749e417566 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -883,6 +883,65 @@ void main() { unreadMessageIds: [message.id]), ]); + group('MarkAsReadAnimation', () { + void checkAppearsLoading(WidgetTester tester, bool expected) { + final semantics = tester.firstWidget(find.descendant( + of: find.byType(MarkAsReadWidget), + matching: find.byType(Semantics))); + check(semantics.properties.enabled).equals(!expected); + + final opacity = tester.widget(find.descendant( + of: find.byType(MarkAsReadWidget), + matching: find.byType(AnimatedOpacity))); + check(opacity.opacity).equals(expected ? 0.5 : 1.0); + } + + testWidgets('loading is changed correctly', (WidgetTester tester) async { + final narrow = TopicNarrow.ofMessage(message); + await setupMessageListPage(tester, + narrow: narrow, messages: [message], unreadMsgs: unreadMsgs); + check(isMarkAsReadButtonVisible(tester)).isTrue(); + + connection.prepare( + delay: const Duration(milliseconds: 2000), + json: UpdateMessageFlagsForNarrowResult( + processedCount: 11, updatedCount: 3, + firstProcessedId: null, lastProcessedId: null, + foundOldest: true, foundNewest: true).toJson()); + + checkAppearsLoading(tester, false); + + await tester.tap(find.byType(MarkAsReadWidget)); + await tester.pump(); + checkAppearsLoading(tester, true); + + await tester.pump(const Duration(milliseconds: 2000)); + checkAppearsLoading(tester, false); + }); + + testWidgets('loading is changed correctly if request fails', (WidgetTester tester) async { + final narrow = TopicNarrow.ofMessage(message); + await setupMessageListPage(tester, + narrow: narrow, messages: [message], unreadMsgs: unreadMsgs); + check(isMarkAsReadButtonVisible(tester)).isTrue(); + + connection.prepare(httpStatus: 400, json: { + 'code': 'BAD_REQUEST', + 'msg': 'Invalid message(s)', + 'result': 'error', + }); + + checkAppearsLoading(tester, false); + + await tester.tap(find.byType(MarkAsReadWidget)); + await tester.pump(); + checkAppearsLoading(tester, true); + + await tester.pump(const Duration(milliseconds: 2000)); + checkAppearsLoading(tester, false); + }); + }); + testWidgets('smoke test on modern server', (WidgetTester tester) async { final narrow = TopicNarrow.ofMessage(message); await setupMessageListPage(tester, From 030128a2fe572cf1af4ad2e3137bae8ebe37cb0a Mon Sep 17 00:00:00 2001 From: Khader M Khudair Date: Sat, 27 Jul 2024 11:21:35 +0300 Subject: [PATCH 2/4] msglist [nfc]: Introduce `MarkAsReadAnimation` widget MarkAsReadAnimation is expected to get more complex in the upcoming commits, so it's better to have it in a separate widget. --- lib/widgets/message_list.dart | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 79a61beef4..531f358f54 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -505,10 +505,9 @@ class _MarkAsReadWidgetState extends State { return IgnorePointer( ignoring: areMessagesRead, - child: AnimatedOpacity( - opacity: areMessagesRead ? 0 : _loading ? 0.5 : 1, - duration: Duration(milliseconds: areMessagesRead ? 2000 : 300), - curve: Curves.easeOut, + child: MarkAsReadAnimation( + loading: _loading, + hidden: areMessagesRead, child: 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 @@ -544,6 +543,28 @@ class _MarkAsReadWidgetState extends State { } } +class MarkAsReadAnimation extends StatelessWidget { + final bool loading; + final bool hidden; + final Widget child; + + const MarkAsReadAnimation({ + super.key, + required this.loading, + required this.hidden, + required this.child + }); + + @override + Widget build(BuildContext context) { + return AnimatedOpacity( + opacity: hidden ? 0 : loading ? 0.5 : 1, + duration: Duration(milliseconds: hidden ? 2000 : 300), + curve: Curves.easeOut, + child: child); + } +} + class RecipientHeader extends StatelessWidget { const RecipientHeader({super.key, required this.message, required this.narrow}); From 9b105a39053e43cf087557c86d01838ee4fc378d Mon Sep 17 00:00:00 2001 From: Khader M Khudair Date: Sat, 27 Jul 2024 11:24:28 +0300 Subject: [PATCH 3/4] msglist: Use faster fade to hidden animation for mark as read button Fixes: #613 --- lib/widgets/message_list.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 531f358f54..34647477b6 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -559,7 +559,7 @@ class MarkAsReadAnimation extends StatelessWidget { Widget build(BuildContext context) { return AnimatedOpacity( opacity: hidden ? 0 : loading ? 0.5 : 1, - duration: Duration(milliseconds: hidden ? 2000 : 300), + duration: const Duration(milliseconds: 500), curve: Curves.easeOut, child: child); } From 791b703c5c2d577197077a9d9f398d0a133f27a2 Mon Sep 17 00:00:00 2001 From: Khader M Khudair Date: Sat, 27 Jul 2024 11:25:02 +0300 Subject: [PATCH 4/4] msglist: Use scale animation for mark-as-read button is pressed --- lib/widgets/message_list.dart | 34 ++++++++++++++++++++++++++++------ 1 file changed, 28 insertions(+), 6 deletions(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 34647477b6..15b78ffbfd 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -516,6 +516,7 @@ class _MarkAsReadWidgetState extends State { padding: const EdgeInsets.symmetric(horizontal: 10, vertical: 10 - ((48 - 38) / 2)), child: FilledButton.icon( style: FilledButton.styleFrom( + splashFactory: NoSplash.splashFactory, minimumSize: const Size.fromHeight(38), textStyle: // Restate [FilledButton]'s default, which inherits from @@ -543,7 +544,7 @@ class _MarkAsReadWidgetState extends State { } } -class MarkAsReadAnimation extends StatelessWidget { +class MarkAsReadAnimation extends StatefulWidget { final bool loading; final bool hidden; final Widget child; @@ -555,13 +556,34 @@ class MarkAsReadAnimation extends StatelessWidget { required this.child }); + @override + State createState() => _MarkAsReadAnimationState(); +} + +class _MarkAsReadAnimationState extends State { + bool _isPressed = false; + + void _setIsPressed(bool isPressed) { + setState(() { + _isPressed = isPressed; + }); + } + @override Widget build(BuildContext context) { - return AnimatedOpacity( - opacity: hidden ? 0 : loading ? 0.5 : 1, - duration: const Duration(milliseconds: 500), - curve: Curves.easeOut, - child: child); + return GestureDetector( + onTapDown: (_) => _setIsPressed(true), + onTapUp: (_) => _setIsPressed(false), + onTapCancel: () => _setIsPressed(false), + child: AnimatedScale( + scale: _isPressed ? 0.95 : 1, + duration: const Duration(milliseconds: 100), + curve: Curves.easeOut, + child: AnimatedOpacity( + opacity: widget.hidden ? 0 : widget.loading ? 0.5 : 1, + duration: const Duration(milliseconds: 500), + curve: Curves.easeOut, + child: widget.child))); } }