Skip to content

ui: Use faster animation for mark as read #710

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jul 28, 2024
Merged

ui: Use faster animation for mark as read #710

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0d577a0
msglist: Handle loading state in MarkAsReadWidget
Khader-1 Jul 27, 2024
030128a
msglist [nfc]: Introduce `MarkAsReadAnimation` widget
Khader-1 Jul 27, 2024
9b105a3
msglist: Use faster fade to hidden animation for mark as read button
Khader-1 Jul 27, 2024
791b703
msglist: Use scale animation for mark-as-read button is pressed
Khader-1 Jul 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 71 additions & 14 deletions lib/widgets/message_list.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,30 +458,40 @@ class ScrollToBottomButton extends StatelessWidget {
}
}

class MarkAsReadWidget extends StatelessWidget {
class MarkAsReadWidget extends StatefulWidget {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

msglist: Use scale animation for mark-as-read button i pressed state

When Vlad said "i pressed state", I think he probably meant "in pressed state". 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That explains everything!😅

const MarkAsReadWidget({super.key, required this.narrow});

final Narrow narrow;

@override
State<MarkAsReadWidget> createState() => _MarkAsReadWidgetState();
}

class _MarkAsReadWidgetState extends State<MarkAsReadWidget> {
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();
}
}
Expand All @@ -490,15 +500,14 @@ 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,
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
Expand All @@ -507,8 +516,7 @@ 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,
splashFactory: NoSplash.splashFactory,
minimumSize: const Size.fromHeight(38),
textStyle:
// Restate [FilledButton]'s default, which inherits from
Expand All @@ -522,11 +530,60 @@ 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))))));
}
}

class MarkAsReadAnimation extends StatefulWidget {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The commit that introduces this widget class:

msglist: Use faster fade to hidden animation for mark as read button

has a commit message that's about a behavior change, and I think there's effectively just one line that's changing to carry out that behavior change. But that change happens in the middle of a larger amount of code that's getting moved and rearranged.

To make clear and coherent commits:
https://zulip.readthedocs.io/en/latest/contributing/commit-discipline.html
so that it's easy to see what each of the changes are, those two things should happen as two separate commits. One NFC commit introducing this widget, and a separate commit just changing the duration (plus any other changes this is doing that I'm missing because the simultaneous move obscures them).

final bool loading;
final bool hidden;
final Widget child;

const MarkAsReadAnimation({
super.key,
required this.loading,
required this.hidden,
required this.child
});

@override
State<MarkAsReadAnimation> createState() => _MarkAsReadAnimationState();
}

class _MarkAsReadAnimationState extends State<MarkAsReadAnimation> {
bool _isPressed = false;

void _setIsPressed(bool isPressed) {
setState(() {
_isPressed = isPressed;
});
}

@override
Widget build(BuildContext context) {
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)));
}
}

Expand Down
59 changes: 59 additions & 0 deletions test/widgets/message_list_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -883,6 +883,65 @@ void main() {
unreadMessageIds: [message.id]),
]);

group('MarkAsReadAnimation', () {
void checkAppearsLoading(WidgetTester tester, bool expected) {
final semantics = tester.firstWidget<Semantics>(find.descendant(
of: find.byType(MarkAsReadWidget),
matching: find.byType(Semantics)));
check(semantics.properties.enabled).equals(!expected);

final opacity = tester.widget<AnimatedOpacity>(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,
Expand Down