msglist: Leave blank space for "mark as read" button so using it doesn't cause messages to shift #565

abelaba · 2024-03-14T08:16:29Z

This pull request addresses the issue of the message list shifting downwards when the "mark as read" button is pressed. The previous implementation would return an empty widget when the button was pressed and cause the message list to shift down. Now it takes a widget with a definite height, which is equal to the size of the button, and won't cause the message list to shift down.

Fixes: #562

Before	After
https://github.com/zulip/zulip-flutter/assets/61546411/e13f6a05-0226-4a14-9654-8312271281d3	https://github.com/zulip/zulip-flutter/assets/61546411/b8a66ad3-394f-499f-9079-51591e4d0766

chrisbobbe · 2024-03-15T22:52:03Z

Thanks for the PR!

What happens if you go to your device's settings and increase the font size? My guess is that the button's height will be more than 58, and the empty space that replaces the button will be too small. When the button appears or disappears, the layout will seem like it's "jumping".

Instead of replacing the button with a blank placeholder with an explicit height, can you try an approach that still lays out the button, but just doesn't show it? Maybe with a combination of AnimatedOpacity and IgnorePointer?

abelaba · 2024-03-17T09:18:13Z

Thank you for your comment. I hadn't thought about the font size. You are right with my first implementation when I increased my device font size the layout would shift, now I have updated the widget with AnimatedOpacity and IgnorePointer and even if I increase my device font size the layout won't shift.

I don't know why the CI is failing. It says it has something to do with the intl package, but I haven't modified that.

gnprice

Thanks @abelaba for the contribution!

Trying this out (and from your screen captures above), the behavior generally looks good but there's one aspect we'll definitely want to fix: when marking as read, the button fades out quickly, but the unread markers on the individual messages fade out more slowly. The juxtaposition is kind of jarring. The two animations represent the same action happening (the messages getting marked as read), so let's have them happen on the same schedule.

One other small comment on the implementation code, and there's also more work needed in order to have solid tests for this; details below.

gnprice · 2024-03-20T20:36:23Z

lib/widgets/message_list.dart

+      ignoring: unreadCount == 0,
+      child: AnimatedOpacity(
+        opacity: (unreadCount > 0) ? 1 : 0,


It's important that these two conditions are saying exactly the same thing (in opposite directions). So let's pull them out as a local variable.

I have fixed this and pulled it out as a local boolean variable.

I have also synchronized the fade out of the unread messages marker and "Mark all messages as read" button.

video_2024-03-22_20-50-38.mp4

gnprice · 2024-03-20T20:56:14Z

test/widgets/message_list_test.dart

+      // The [MarkAsReadWidget] is a [IgnorePointer] wrapped in a [AnimatedOpacity].
+      // We need to check the state of both to determine if the widget is visible.
+      // The [IgnorePointer] is visible when `ignoring` is false and the [AnimatedOpacity]
+      // is visible when `opacity` is 1.0.
+      final ignorePointerFinder = find.descendant(
+        of: markAsReadWidget,
+        matching: find.byType(IgnorePointer),
+      );
+
+      final animatedOpacityFinder = find.descendant(
+        of: markAsReadWidget,
+        matching: find.byType(AnimatedOpacity),
+      );
+
+      final IgnorePointer ignorePointer = tester.widget(ignorePointerFinder);
+      final AnimatedOpacity animatedOpacity = tester.widget(animatedOpacityFinder);
+
+      return !ignorePointer.ignoring && animatedOpacity.opacity == 1.0;


This logic is all about the internal implementation details of (the new version of) MarkAsReadWidget. That will cause it to be brittle as we make future changes to that UI.

Instead, it's better to write tests so that they focus on what the user actually sees and interacts with: is there a mark-as-read button on the screen?

Concretely, that means:

a button that has the label "Mark all messages as read"

and is actually visible

and can be pressed.

To look around at what kinds of features package:flutter_test offers for checking this sort of thing, use your IDE to navigate to the definition of find.byType or a similar method, and browse around that finders.dart file.

If you can find a way to check either one of the "is actually visible" or "can be pressed" conditions, that'd be an excellent start and I think good enough to make this test work. Checking both would be ideal.

For any questions, please start a thread in #mobile-team on chat.zulip.org and describe what you've found so far, and we'll be happy to discuss.

I have addressed this by checking the visibility of the button. I accomplished this by checking if the "Mark all messages as read" text is visible or not. I did this by using find.text method and checking if the found element is reachable by a hit test.

Cool — yeah, Finder.hitTestable is a good solution.

gnprice · 2024-03-20T21:00:26Z

test/widgets/message_list_test.dart

+      final IgnorePointer ignorePointer = tester.widget(ignorePointerFinder);
+      final AnimatedOpacity animatedOpacity = tester.widget(animatedOpacityFinder);
+
+      return !ignorePointer.ignoring && animatedOpacity.opacity == 1.0;
    }

    testWidgets('from read to unread', (WidgetTester tester) async {


In addition to updating the existing tests, this change should add a new test: we want to check that the messages don't shift position when marking as read.

I have added a new test that addresses this. It checks if the vertical position of the first message item is the same before and after the "Mark all messages as read" button is pressed.

gnprice

Thanks for the revision! Comments below.

gnprice · 2024-03-23T00:37:04Z

test/widgets/message_list_test.dart

+      final markAsReadWidgetButtonDescendant = find.descendant(
+        of: markAsReadWidget,
+        matching: find.text(zulipLocalizations.markAllAsReadLabel),
+      ).hitTestable();


Let's just use find.text and hitTestable — we can skip the "is a descendant of MarkAsReadWidget" part.

That makes the test more purely dependent on the aspects the user sees and cares about, and conveniently also simplifies it.

I have fixed this.

gnprice · 2024-03-23T00:44:08Z

test/widgets/message_list_test.dart

+      // Locate the [Text] widget with the markAllAsReadLabel
+      // to determine if the [MarkAsReadWidget] is visible.
+      // This is done by finding a descendant of the [MarkAsReadWidget]
+      // that is a [Text] widget with the markAllAsReadLabel.
+      // The hitTestable() method is used to make the descendant widget hit-testable,
+      // allowing it to be found and evaluated in the widget tree.


These comments are largely repeating the information that's already visible in the code below; the names like hitTestable already mostly tell the story, and if the reader has any points of uncertainty they can look at the first line of each method's documentation. That will pretty much cover all the information in this comment.

So let's leave this comment out. In general, comments are useful when they explain things that can't easily be gotten from reading the code. Every comment has a cost in that it's something for the reader to have to read through — plus that it's very easy for comments to become wrong over time as the code changes, at which point they become much worse than nothing. So we only want to have comments that pull their weight.

Thank you for the explanation. I hadn't thought about it that way. I have removed the comments.

gnprice · 2024-03-23T00:44:39Z

test/widgets/message_list_test.dart

+      // Locate the [Text] widget with the markAllAsReadLabel
+      // to determine if the [MarkAsReadWidget] is visible.
+      // This is done by finding a descendant of the [MarkAsReadWidget]
+      // that is a [Text] widget with the markAllAsReadLabel.
+      // The hitTestable() method is used to make the descendant widget hit-testable,
+      // allowing it to be found and evaluated in the widget tree.


These comments are largely repeating the information that's already visible in the code below; the names like hitTestable already mostly tell the story, and if the reader has any points of uncertainty they can look at the first line of each method's documentation. That will pretty much cover all the information in this comment.

So let's leave this comment out. In general, comments are useful when they explain things that can't easily be gotten from reading the code. Every comment has a cost in that it's something for the reader to have to read through — plus that it's very easy for comments to become wrong over time as the code changes, at which point they become much worse than nothing. So we only want to have comments that pull their weight.

gnprice · 2024-03-23T00:47:06Z

test/widgets/message_list_test.dart

+        final message = eg.streamMessage(flags: []);
+        final unreadMsgs = eg.unreadMsgs(streams:[
+          UnreadStreamSnapshot(topic: message.subject, streamId: message.streamId, unreadMessageIds: [message.id])
+        ]);


Are these different from the message and unreadMsgs that the neighboring tests are using, which are set up at the group level?

gnprice · 2024-03-23T00:48:03Z

test/widgets/message_list_test.dart

+        final unreadMsgs = eg.unreadMsgs(streams:[
+          UnreadStreamSnapshot(topic: message.subject, streamId: message.streamId, unreadMessageIds: [message.id])
+        ]);
+        await setupMessageListPage(tester, messages: [message], unreadMsgs: unreadMsgs);


In general, please follow the style of neighboring code. This line is long; see similar calls in neighboring tests.

gnprice · 2024-03-23T00:50:41Z

test/widgets/message_list_test.dart

+        store.handleEvent(UpdateMessageFlagsAddEvent(
+          id: 1,
+          flag: MessageFlag.read,
+          messages: [message.id],
+          all: false,
+        ));
+
+        await tester.pumpAndSettle();


This test doesn't seem to involve actually pressing the button. That's fine; but the comment, the test's name, and the group it's in all say it does. So they should be accurate.

Probably cleanest is to move this to just above the "onPressed behavior" group, next to the other tests that act similarly.

I have moved the test to just above the "onPressed behavior" group. I have also fixed the long lines.

gnprice · 2024-03-23T00:55:17Z

test/widgets/message_list_test.dart

+        final firstMessageItemTopPositionAfter = tester.getTopLeft(find.byType(MessageItem).first).dy;
+
+        check(firstMessageItemTopPositionBefore).equals(firstMessageItemTopPositionAfter);


This comparison isn't reliable, because these might be different messages before and after. If the message list were to shift by exactly the height of the top message, then the test would pass when it shouldn't.

… I guess in this test's setup there's only one message. That works, then, but these checks should make explicit that they're relying on that: use .single instead of .first.

I tried to use the.singlemethod, but that returns an Element rather than a Finder and the tester.getTopLeft method takes a Finder. So in order to explicitly show that there is only one MessageItem on this test, I have added a check that checks if the number of MessageItem widgets found is equal to 1. If there is a better way to handle this test I am open to suggestions.

Hmm, I see. Sure, this works.

gnprice · 2024-03-23T01:00:05Z

test/widgets/message_list_test.dart

+
+        final firstMessageItemTopPositionAfter = tester.getTopLeft(find.byType(MessageItem).first).dy;
+
+        check(firstMessageItemTopPositionBefore).equals(firstMessageItemTopPositionAfter);


These variable names are extremely long. That makes for rather a mouthful when reading through the code; and it also means that a line like this one, with just two of these variables and otherwise very simple, is already overly long.

Better names would be messagePositionBefore / messagePositionAfter.

Or just: before and after. These are local variables, which only exist within this fairly short function, so the names only have to say enough detail to clearly distinguish them from the other things going on within the function. And it's clear from the initializer expressions, like tester.getTopLeft(find.byType(MessageItem).first).dy, that this is a position of the top of the first message item.

I have renamed the variables to before and after.

gnprice · 2024-03-25T06:55:32Z

test/widgets/message_list_test.dart

-      return tester.getSize(
-        find.byType(MarkAsReadWidget, skipOffstage: false)).height > 0;
+      final zulipLocalizations = GlobalLocalizations.zulipLocalizations;
+      final markAsReadWidgetButton = find.text(


This isn't really a button, but a finder for a button. So good names would include findButton, buttonFinder, or just finder. For a local with such a short scope, the concise finder works well — it's clear which finder is meant.

gnprice · 2024-03-25T07:00:44Z

test/widgets/message_list_test.dart

+        final firstMessageItemTopPositionAfter = tester.getTopLeft(find.byType(MessageItem).first).dy;
+
+        check(firstMessageItemTopPositionBefore).equals(firstMessageItemTopPositionAfter);


Hmm, I see. Sure, this works.

gnprice

Thanks for the revision! Just small comments remain.

(I also haven't yet tested this or the previous revision live, but it's late in the evening now here.)

gnprice · 2024-03-25T07:02:33Z

test/widgets/message_list_test.dart

+      await setupMessageListPage(tester,
+        messages: [message], unreadMsgs: unreadMsgs);
+      check(isMarkAsReadButtonVisible(tester)).isTrue();
+      check(tester.widgetList(find.byType(MessageItem)).length).equals(1);


nit: do more processing on the result of check, instead of its argument:

Suggested change

check(tester.widgetList(find.byType(MessageItem)).length).equals(1);

check(tester.widgetList(find.byType(MessageItem))).length.equals(1);

This allows package:checks to give more informative output if the check fails.

Okay. Fixed.

gnprice · 2024-03-25T07:16:16Z

test/widgets/message_list_test.dart

+      check(isMarkAsReadButtonVisible(tester)).isFalse();
+      check(tester.widgetList(find.byType(MessageItem)).length).equals(1);
+      final after = tester.getTopLeft(find.byType(MessageItem)).dy;
+      check(before).equals(after);


Check actual equals expected:

Suggested change

check(before).equals(after);

check(after).equals(before);

That way check is able to give accurate output if the check fails. (To see the failure, use git checkout @~ lib/ and then flutter test test/widgets/message_list_test.dart, to run your new test on the old code.)

Thanks for the explanation. Fixed.

gnprice · 2024-03-25T07:17:01Z

test/widgets/message_list_test.dart

@@ -648,6 +645,31 @@ void main() {
      check(isMarkAsReadButtonVisible(tester)).isFalse();
    });

+    testWidgets('messages don\'t shift position', (WidgetTester tester) async {


nit: make this a double-quoted string, to look a bit cleaner with the embedded apostrophe

…ift up/down This commit addresses the issue of the message list shifting downwards when the "mark as read" button is pressed. The previous implementation would return an empty widget when the button was pressed and cause the message list to shift down. Now it takes a widget which will display the button based on the opacity level and will disable the region the button holds once the button is pressed, thus won't cause the message list to shift down. Fixes: zulip#562

gnprice · 2024-03-25T20:04:18Z

Thanks for the revision! This looks good. Merging, with one fix in the commit message:

msglist: Leave blank space for "mark as read" button so using it doesn't cause messages to shift

This commit addresses the issue of the message list shifting downwards when the "mark as read" button is pressed. The previous implementation would return an empty widget when the button was pressed and cause the message list to shift down. Now it takes a widget which will display the button based on the opacity level and will disable the region the button holds once the button is pressed, thus won't cause the message list to shift down.

Fixes: #562

Prose should be wrapped to about 68 columns, and at most 70:
https://zulip.readthedocs.io/en/latest/contributing/commit-discipline.html#formatting-guidelines

And then the summary line can go a bit longer than that, because it can be hard to squeeze it down while conveying all the information; but I did some tightening of that too. The result:

msglist: Reserve space for "mark as read" button so messages don't shift up/down

This commit addresses the issue of the message list shifting downwards
when the "mark as read" button is pressed. The previous implementation
would return an empty widget when the button was pressed and cause the
message list to shift down. Now it takes a widget which will display
the button based on the opacity level and will disable the region the
button holds once the button is pressed, thus won't cause the message
list to shift down.

Fixes: #562

abelaba force-pushed the issue-562 branch from 092dc2d to b2a3cbf Compare March 17, 2024 09:01

gnprice reviewed Mar 20, 2024

View reviewed changes

abelaba force-pushed the issue-562 branch from b2a3cbf to 2370a64 Compare March 22, 2024 17:36

gnprice reviewed Mar 23, 2024

View reviewed changes

abelaba force-pushed the issue-562 branch from 2370a64 to 234a095 Compare March 24, 2024 22:22

gnprice reviewed Mar 25, 2024

View reviewed changes

abelaba force-pushed the issue-562 branch from 234a095 to c0964d7 Compare March 25, 2024 07:52

gnprice force-pushed the issue-562 branch from c0964d7 to 79a7384 Compare March 25, 2024 20:04

gnprice merged commit 79a7384 into zulip:main Mar 25, 2024

gnprice mentioned this pull request Apr 3, 2024

Try faster fade-out on mark-as-read button #613

Closed

		final firstMessageItemTopPositionAfter = tester.getTopLeft(find.byType(MessageItem).first).dy;

		check(firstMessageItemTopPositionBefore).equals(firstMessageItemTopPositionAfter);

	check(tester.widgetList(find.byType(MessageItem)).length).equals(1);
	check(tester.widgetList(find.byType(MessageItem))).length.equals(1);

msglist: Leave blank space for "mark as read" button so using it doesn't cause messages to shift #565

msglist: Leave blank space for "mark as read" button so using it doesn't cause messages to shift #565

Conversation

abelaba commented Mar 14, 2024

chrisbobbe commented Mar 15, 2024

abelaba commented Mar 17, 2024

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice commented Mar 25, 2024