-
Notifications
You must be signed in to change notification settings - Fork 306
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.
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
ui: Use faster animation for mark as read #710
Conversation
b330ca3
to
74529f5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Khader-1, some comments :)
Oh, sorry this one had to be marked as a draft as I am still waiting for confirmation for some decisions. |
@Khader-1 Are there still open questions in the chat discussion? If so it'd be good to bump the discussion to clarify the question you currently have; it looks to me like the questions there have been answered and we have a direction to go with. (I think the link above is a different thread than you meant; the one for this PR looks to be here.) |
74529f5
to
3395c57
Compare
You're right, thank for the catch @gnprice! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Khader-1! Apart from some comments this LGTM.
lib/widgets/message_list.dart
Outdated
@@ -505,8 +514,7 @@ class MarkAsReadWidget extends StatelessWidget { | |||
), | |||
onPressed: () => _handlePress(context), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is possible to click on it again while previous markNarrowAsRead
is still in progress, click should be disabled while it's in loading
state, not sure if this should be different issue+PR (since it's reproducible even on main
) or just a commit in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @gnprice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Good catch, I agree.
I've created a new revision having the loading state with the button disabled in it's own commit. PTAL @rajveermalviya.
a5fe9f8
to
7665f52
Compare
lib/widgets/message_list.dart
Outdated
@@ -503,10 +513,9 @@ class MarkAsReadWidget extends StatelessWidget { | |||
.merge(weightVariableTextStyle(context, wght: 400))), | |||
shape: RoundedRectangleBorder(borderRadius: BorderRadius.circular(7)), | |||
), | |||
onPressed: () => _handlePress(context), | |||
onPressed: () => isLoading ? null : _handlePress(context), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this could be handled so that the button is disabled as in:
onPressed: isLoading ? null : () => _handlePress(context),
Although this can be more convenient, it'd force us to set the disabled style of the button to be a typical copy of the active state! So I am not sure if it is better to go with that or to keep it as is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would it force that?
lib/widgets/message_list.dart
Outdated
Widget build(BuildContext context) { | ||
final (opacity, scale) = loading | ||
? (0.5, 0.95) | ||
: visible ? (1.0 , 1.0) : (0.0, 0.0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using (0, 0) here will make the button scale down while fading after loading finishes.
lib/widgets/message_list.dart
Outdated
@@ -505,8 +514,7 @@ class MarkAsReadWidget extends StatelessWidget { | |||
), | |||
onPressed: () => _handlePress(context), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Good catch, I agree.
I've created a new revision having the loading state with the button disabled in it's own commit. PTAL @rajveermalviya.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @Khader-1!
Is this discussion the latest for the design? I think there are some differences between that design and what's implemented here:
- The 95% scaling is meant for the button's pressed state, not its loading state. So when the finger goes down, it should animate to 95% size. When the finger is lifted, it should animate back to 100%. There isn't a scaling animation assigned to the loading state.
- When loading is finished, it should just fade to transparency for 500ms. I don't see Vlad describing a scaling animation for the button's exit.
Does that seem right to you, and is there a discussion I'm missing?
7665f52
to
f841ec0
Compare
Thanks @chrisbobbe! I misunderstood what Vlad meant with i pressed state at first. I think the new implementation matches the one you described. PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @Khader-1! Small comments below.
Ah also: let's use msglist
for the prefix, like in previous commits, instead of msg_list
.
f841ec0
to
450887d
Compare
Thanks @chrisbobbe! I've made a new revision, PTAL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Small comments below.
test/widgets/message_list_test.dart
Outdated
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(json: UpdateMessageFlagsForNarrowResult( | ||
processedCount: 11, updatedCount: 3, | ||
firstProcessedId: null, lastProcessedId: null, | ||
foundOldest: true, foundNewest: true).toJson()); | ||
|
||
final state = tester.state<MarkAsReadWidgetState>(find.byType(MarkAsReadWidget)); | ||
check(state.loading).isFalse(); | ||
|
||
await tester.tap(find.byType(MarkAsReadWidget)); | ||
await tester.pump(); | ||
check(state.loading).isTrue(); | ||
|
||
await tester.idle(); | ||
await tester.pump(); | ||
check(state.loading).isFalse(); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels like we should be able to test the user's experience of the loading state more directly. The loading
field does power the user-visible signs that the button is loading, but it's also kind of an implementation detail. How about something like the following:
--- test/widgets/message_list_test.dart
+++ test/widgets/message_list_test.dart
@@ -1,4 +1,5 @@
import 'dart:convert';
+import 'dart:ui';
import 'package:checks/checks.dart';
import 'package:collection/collection.dart';
@@ -888,6 +889,20 @@ void main() {
unreadMessageIds: [message.id]),
]);
+ 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);
+ }
+
group('MarkAsReadAnimation', () {
testWidgets('loading is changed correctly', (WidgetTester tester) async {
final narrow = TopicNarrow.ofMessage(message);
@@ -895,21 +910,21 @@ void main() {
narrow: narrow, messages: [message], unreadMsgs: unreadMsgs);
check(isMarkAsReadButtonVisible(tester)).isTrue();
- connection.prepare(json: UpdateMessageFlagsForNarrowResult(
- processedCount: 11, updatedCount: 3,
- firstProcessedId: null, lastProcessedId: null,
- foundOldest: true, foundNewest: true).toJson());
+ connection.prepare(
+ delay: const Duration(milliseconds: 2000),
+ json: UpdateMessageFlagsForNarrowResult(
+ processedCount: 11, updatedCount: 3,
+ firstProcessedId: null, lastProcessedId: null,
+ foundOldest: true, foundNewest: true).toJson());
- final state = tester.state<MarkAsReadWidgetState>(find.byType(MarkAsReadWidget));
- check(state.loading).isFalse();
+ checkAppearsLoading(tester, false);
await tester.tap(find.byType(MarkAsReadWidget));
await tester.pump();
- check(state.loading).isTrue();
+ checkAppearsLoading(tester, true);
- await tester.idle();
- await tester.pump();
- check(state.loading).isFalse();
+ await tester.pump(const Duration(milliseconds: 2000));
+ checkAppearsLoading(tester, false);
});
testWidgets('loading is changed correctly if request fails', (WidgetTester tester) async {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I've created a new NFC for simulating delayed connections PTAL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh.. I see there is an implementation for that merged already! rebasing..
class MarkAsReadWidget extends StatelessWidget { | ||
class MarkAsReadWidget extends StatefulWidget { |
There was a problem hiding this comment.
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". 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That explains everything!😅
76a82f6
to
cb71841
Compare
Thanks @chrisbobbe! A new revision is here PTAL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! One commit-message nit, and I'll mark this for Greg's review.
lib/widgets/message_list.dart
Outdated
State<MarkAsReadWidget> createState() => MarkAsReadWidgetState(); | ||
} | ||
|
||
class MarkAsReadWidgetState extends State<MarkAsReadWidget> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
msglist: Handle loading state in MarkAsReadWidget
MarkAsReadWidget needs to be disabled during loading state to prevent
multiple requests to the server.
Additionally it is needed for styling/animation convenience.
That last sentence ("Additionally…") doesn't really make sense; let's drop it. (What is more "convenient" about disabling the button?) I think it was from a previous revision when this commit was focused more on an implementation detail rather than the changes that users could see.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's right. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Khader-1, and thanks @rajveermalviya and @chrisbobbe for the previous reviews! Comments below.
(And just so it doesn't get buried / so everything's mentioned in one place: there's one comment from Chris open above.)
lib/widgets/message_list.dart
Outdated
).copyWith( | ||
// clobber `FilledButton`'s default disabled state | ||
// TODO(#95) need dark-theme colors (foreground and background) | ||
foregroundColor: WidgetStateColor.resolveWith((_) => Colors.white), | ||
backgroundColor: WidgetStateColor.resolveWith((_) => _UnreadMarker.color), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite understand this comment — how does the default disabled state relate to what these lines are doing? They don't look specific to a disabled state.
I seem to recall there was a comment by Chris above that may have led to this code. It's hard to find that comment now, though, because of the unfortunate way GitHub's "resolve conversation" feature works — there's no way to show all the resolved comments without clicking on them one by one (and waiting for them each to load), which means I can't easily Ctrl+F the page to find that comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah:
So the function you pass to WidgetStateColor.resolveWith
receives a set of WidgetState
s, which are hovered, focused, pressed, etc., and disabled. Normally, you'd pass a function that checks that set of states and returns different colors based on it.
Here, we don't want different colors for the different states. We don't even want a different color for the disabled state; that's because we have other code for that. That other code uses an animation, which is especially helpful at the end of the loading state, when otherwise the button would jump to full opacity before the mark-as-read event arrives and the exit animation starts. And it doesn't seem possible to animate between disabled/enabled here using ButtonStyle
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Oh and the other key piece of information: without passing these arguments, where we set the colors unconditionally, the widget would choose its own colors for the disabled state.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, thanks. @Khader-1 here's a way to explain that in the comment:
).copyWith( | |
// clobber `FilledButton`'s default disabled state | |
// TODO(#95) need dark-theme colors (foreground and background) | |
foregroundColor: WidgetStateColor.resolveWith((_) => Colors.white), | |
backgroundColor: WidgetStateColor.resolveWith((_) => _UnreadMarker.color), | |
).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), |
test/widgets/message_list_test.dart
Outdated
}); | ||
|
||
final state = tester.state<MarkAsReadWidgetState>(find.byType(MarkAsReadWidget)); | ||
check(state.loading).isFalse(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this use checkAppearsLoading
instead?
That'd make this test be more in terms of the UI as users see it, rather than in terms of how its implementation is organized internally. For background, see:
https://zulip.readthedocs.io/en/latest/testing/philosophy.html#integration-testing-or-unit-testing
Then once that's done, can the widget's state class be made private again (as state classes normally are)? That'd be a good way to verify that tests, as well as other code, aren't poking into the internal implementation details in this sort of way.
} | ||
} | ||
|
||
class MarkAsReadAnimation extends StatefulWidget { |
There was a problem hiding this comment.
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).
test/widgets/message_list_test.dart
Outdated
await tester.pumpWidget(MaterialApp( | ||
home: MarkAsReadAnimation(hidden: false, loading: false, child: child))); | ||
|
||
check(find.byWidget(child).evaluate()).length.equals(1); | ||
check(tester.widget<AnimatedOpacity>(find.byType(AnimatedOpacity)).opacity).equals(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you write this instead in the style of checkAppearsLoading
? See the link in a comment above about testing philosophy — the MarkAsReadAnimation widget is part of the implementation details of this UI.
lib/widgets/message_list.dart
Outdated
return GestureDetector( | ||
onTapDown: (_) => _setScale(true), | ||
onTapUp: (_) => _setScale(false), | ||
onTapCancel: () => _setScale(false), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I don't love that we're effectively winding up here with a button that has two separate gesture detectors watching for a press — this one tracking whether it's currently being pressed, and the one the Material library supplies which we're using for responding to having been pressed.
In particular this feels like it's repeating the work that the Material button is already doing, and we could get a similar effect here by tracking the button's WidgetState, looking for WidgetState.pressed.
That's not a blocker for merging this, but it's something I'll want to clean up later if we end up sticking with this UI design.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree! I've tried various approaches before, but I haven't found a straightforward solution that works as a replacement.
I'd love to hear your suggestions and I'd love to implement them.
lib/widgets/message_list.dart
Outdated
void _setScale(bool isPressed) { | ||
setState(() { | ||
_isPressed = isPressed; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this method's name doesn't reflect what it does; it also makes it hard to guess what the meaning of an argument "true" vs. "false" is. A good name would be _setIsPressed
.
cb71841
to
0b00356
Compare
Thanks @chrisbobbe and @gnprice for the reviews! I've pushed a new revision PTAL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the revision! Just a few comments now.
lib/widgets/message_list.dart
Outdated
class _MarkAsReadWidgetState extends State<MarkAsReadWidget> { | ||
@visibleForTesting | ||
bool loading = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
class _MarkAsReadWidgetState extends State<MarkAsReadWidget> { | |
@visibleForTesting | |
bool loading = false; | |
class _MarkAsReadWidgetState extends State<MarkAsReadWidget> { | |
bool loading = false; |
Pretty sure this annotation has no effect when the class itself is private.
test/widgets/message_list_test.dart
Outdated
testWidgets('in loading state', (WidgetTester tester) async { | ||
final child = Container(); | ||
|
||
await tester.pumpWidget(MaterialApp( | ||
home: MarkAsReadAnimation(hidden: false, loading: true, child: child))); | ||
|
||
checkHasOpacityValue(tester, child, 0.5); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following up on #710 (comment) : as written, this is a test that doesn't give us a lot of value, because it's operating at the layer of an implementation detail of how the UI is organized and as a result it largely just repeats what's in the code of MarkAsReadAnimation.
In the context of this section on testing philosophy that I linked earlier:
https://zulip.readthedocs.io/en/latest/testing/philosophy.html#integration-testing-or-unit-testing
these are "unit tests" of the kind that doesn't serve us well.
Instead, the tests should be written in a way that's more in terms of the UI as the user experiences it. That means that instead of these four test cases that use MarkAsReadAnimation directly, the relevant behavior should be tested in the style of the two test cases earlier in this group: use setupMessageListPage
to create a message-list page where the button appears, then tap on the button and check how the button looks.
(Some of these test cases may be redundant with what those other two test cases already do, in which case they can just be left out.)
test/widgets/message_list_test.dart
Outdated
testWidgets('in loading state', (WidgetTester tester) async { | ||
final child = Container(); | ||
|
||
await tester.pumpWidget(MaterialApp( | ||
home: MarkAsReadAnimation(hidden: false, loading: true, child: child))); | ||
|
||
checkHasOpacityValue(tester, child, 0.5); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following up on #710 (comment) : as written, this is a test that doesn't give us a lot of value, because it's operating at the layer of an implementation detail of how the UI is organized and as a result it largely just repeats what's in the code of MarkAsReadAnimation.
In the context of this section on testing philosophy that I linked earlier:
https://zulip.readthedocs.io/en/latest/testing/philosophy.html#integration-testing-or-unit-testing
these are "unit tests" of the kind that doesn't serve us well.
Instead, the tests should be written in a way that's more in terms of the UI as the user experiences it. That means that instead of these four test cases that use MarkAsReadAnimation directly, the relevant behavior should be tested in the style of the two test cases earlier in this group: use setupMessageListPage
to create a message-list page where the button appears, then tap on the button and check how the button looks.
(Some of these test cases may be redundant with what those other two test cases already do, in which case they can just be left out.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. I believe now all of those are redundant as loading is changed correctly
and loading is changed correctly if request fails
can totally replace them.
lib/widgets/message_list.dart
Outdated
).copyWith( | ||
// clobber `FilledButton`'s default disabled state | ||
// TODO(#95) need dark-theme colors (foreground and background) | ||
foregroundColor: WidgetStateColor.resolveWith((_) => Colors.white), | ||
backgroundColor: WidgetStateColor.resolveWith((_) => _UnreadMarker.color), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, thanks. @Khader-1 here's a way to explain that in the comment:
).copyWith( | |
// clobber `FilledButton`'s default disabled state | |
// TODO(#95) need dark-theme colors (foreground and background) | |
foregroundColor: WidgetStateColor.resolveWith((_) => Colors.white), | |
backgroundColor: WidgetStateColor.resolveWith((_) => _UnreadMarker.color), | |
).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), |
0b00356
to
de473c9
Compare
Thanks @gnprice! I've pushed a new revision PTAL. |
MarkAsReadWidget needs to be disabled during loading state to prevent multiple requests to the server.
MarkAsReadAnimation is expected to get more complex in the upcoming commits, so it's better to have it in a separate widget.
Thanks for the revision! Merging. I squashed this commit: into the one after it, since that one is already touching neighboring code and this two-line change didn't seem like it made that any harder to read. |
de473c9
to
791b703
Compare
Fixes: #613
Final Result:
Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-07-15.at.13.00.55.mp4