Unread markers #317

sirpengi · 2023-10-11T13:24:16Z

This PR introduces unread markers with animations whenever the read state changes for a message. Bundled into this PR is also:

Associated integration tests and docs on obtaining performance metrics from a physical device that I used to confirm the performance of the animation being used
A fix for animation state being lost when the number of items in the message list changes. This involved adding a unique key to every message as well as providing a findChildIndexCallback to StickyHeaderListView.builder.

Discussion related to design of this on CZO: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/flutter.3A.20unread.20marker

Fixes: #79

sirpengi · 2023-10-11T13:27:48Z

@gnprice Would appreciate a review of this while design discussions are still ongoing

gnprice

Thanks @sirpengi! The integration tests (or performance profiling) are an interesting new capability for us to have. Comments below.

gnprice · 2023-10-11T22:57:00Z

lib/widgets/message_list.dart

+// Historically, web handled the left-side unread 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


This wasn't the unread marker — it was a recipient marker, as the existing version of the comment says. That is, it's part of indicating what stream is the recipient, or whether the recipient is a set of individuals via DM.

If we're removing the left-side recipient marker that this is talking about, then we can remove the comment at the same time.

gnprice · 2023-10-11T23:02:03Z

lib/widgets/message_list.dart

+    return Positioned(
+      top: 0,
+      left: 0,
+      bottom: 0,


This makes the interface of this _UnreadMarker widget pretty intimately tied in with the Stack at the call site. (That is, this widget has very specific expectations from its caller, requiring the latter to put it in a Stack.) The cleanest thing is probably to have the Stack and the Positioned appear next to each other in the source.

At the call site that can look like this:

_UnreadMarker( isRead: message.flags.contains(MessageFlag.read), child: MessageWithPossibleSender(item: item))

using the idiom of a child property to let the caller still control the underlying message widget, and the unread-marker widget remain focused on the unread marker itself.

gnprice · 2023-10-11T23:10:38Z

lib/widgets/message_list.dart

+        child: Container(
+          width: 4,
+          decoration: BoxDecoration(
+            color: const HSLColor.fromAHSL(1, 227, 0.78, 0.59).toColor()))));


Where do these values come from? They don't appear to match any of the colors in the comment above.

It came from the figma mockup, but that didn't seem like something that I could link to and no other place in the code refers to it. I've added a note pointing to the CZO discussion on this topic for this.

Cool, sounds good. It seems like Figma does have some deep-linking capability, but a chat message is probably better to link to anyway because it gives the context of the discussion.

gnprice · 2023-10-11T23:10:59Z

test/widgets/message_list_test.dart

@@ -300,4 +300,47 @@ void main() {
      debugNetworkImageHttpClientProvider = null;
    });
  });
+
+  group('unread marker', () {


Tests should be added in the same commit as the code they test

gnprice · 2023-10-11T23:38:19Z

lib/widgets/message_list.dart

-    final recipientBorder = BorderSide(color: highlightBorderColor, width: 3);
-    final restBorder = BorderSide(color: restBorderColor, width: 1);


msglist: Remove borders around messages and adjust padding The original implementation here tried to match the design of the webapp as it existed at the time. Since then, the design of web has moved on and

This part is true, but…

the design of mobile is expected to continue on the same direction of having messages in the chat expand from screen edge to screen edge.

it's not really connected to this part, nor to what this commit (or PR) is doing.

In particular the design on web still has borders around the messages. If we were following the updated web design, we'd have borders, and we'd have a gray background for the overall message list. (That background would show through between one block of messages and the next, and to the left and right of the blocks of messages.)

Relatedly, having the messages extend from edge to edge of the screen isn't something where mobile will be following in a direction set by web. It's something that zulip-mobile has been doing since forever (because it makes sense for a mobile environment with a narrow screen), and that web doesn't do and has no plans to do.

What's happening when we remove the borders and extend from edge to edge is that we're moving from a design based on web — and yes it was based on the pre-2023 design of web, but this aspect would be exactly the same based on web's design today — to a mobile-specific design. And we're doing so by taking a design element found in the zulip-mobile RN app.

I think I meant what you outlined above, my statement of "the design of web has moved on" meant "the design the original implementation tried to match isn't even the current design of the web" and on top of that mobile had it's own separate design and is expected to stay separate (and so here again we're not sticking with web but following the mobile design evolution). I do think the language was overly verbose, so I've edited it down and hopefully added clarity to the point being made.

msglist: Messages changed to render edge-to-edge and borderless This is a small change in line with #157 and moves the implementation closer to the new 2023+ design for mobile. Messages now span from screen edge to screen edge and borders around groups of messages (in particular the left-side recipient border) was removed.

This is much closer, but the first sentence is still inaccurate. The change this is making is in a direction completely orthogonal to #157 or to any new design for mobile — it's about moving from a design based on web to a mobile-specific design. The key design features here are exactly what zulip-mobile has had for many years.

In the summary line, a nit: verb should be in the imperative (or infinitive): https://zulip.readthedocs.io/en/latest/contributing/commit-discipline.html#commit-summary-part-2

Here's a revised version:

msglist: Go edge-to-edge, for a mobile-oriented design The existing design here was designed for web; the prototype copied precisely from web's design as of 2022-12. But in the zulip-mobile RN app, we've long had a design that's optimized for a narrow mobile screen instead: notably by using the full available width (up to a maximum, anyway), with no left or right borders, and consequently less padding too. Do that here.

gnprice · 2023-10-12T00:25:32Z

pubspec.lock

      name: platform
-      sha256: "4a451831508d7d6ca779f7ac6e212b4023dd5a7d08a27a63da33756410e32b76"
+      sha256: ae68c7bfcd7383af3629daafb32fb4e8681c7154428da4febcff06200585f102
      url: "https://pub.dev"
    source: hosted
-    version: "3.1.0"
+    version: "3.1.2"


Is this also an unrelated change?

Having unrelated changes mixed in with the intended ones makes the changes harder to review. When sending a PR (or revision), please reread the changes with git log --stat -p or with gitk or equivalent, and look for changes that aren't part of what is intended.

In this case, if some of the changes are introduced automatically by commands like flutter pub get, it's fine to include the change at the start of the branch to simplify managing the rest of the changes — but, like any unrelated changes, it still needs to be a separate commit.

(Ultimately we make such a change in a commit like 790a910, just merged as part of #316. So if you make it in that form, we can merge it as part of the PR; it's also fine to make the changes as just a draft commit, and then it'll disappear when rebased after we merge such an upgrade PR.)

gnprice · 2023-10-12T00:54:24Z

test_driver/perf_driver.dart

+        // Convert the Timeline into a TimelineSummary that's easier to
+        // read and understand.
+        final summary = driver.TimelineSummary.summarize(timeline);


This reads like the voice of Flutter upstream, not your voice. Is this code copied from an example somewhere?

I see, yes. A web search on the text finds this:
https://docs.flutter.dev/cookbook/testing/integration/profiling#3-save-the-results-to-disk

Two important general points, then:

When copying example code (or anything else) from somewhere, please flag the source clearly, either in the commit message or in a comment next to the copied material.

This is important for the reviewer to be able to properly review the changes, by understanding how you're making the choices you made.

When copying material from somewhere that isn't within the Zulip project, you absolutely must clearly flag that.

After we merge code into our tree, we distribute it to the world under a free license. We need that to be legal. Generally that means that the code either was contributed to Zulip by its author, or comes under a free license that allows us to include it.

And that means we have to be able to have confidence that 100% of what's in our tree is material we legally can have there. So when something is copied from elsewhere, we need to be explicit about that. That way I can review the source we're drawing from and confirm that it comes under a compatible free license.

And one that's specific to this file:

This comment and the one below it are useful in the cookbook recipe they come from, because they're addressed to the reader of the recipe. But they're largely beside the point here in our tree. I think the one relevant bit is the one about chrome://tracing; and that's really more relevant when running flutter drive with this file, rather than when editing it.

The main place we discuss running flutter drive in this PR is over in the handy docs file. So that advice can go there, and then I think the relevant information for comments in this file is:

a cross-reference to that docs file, for background and for instructions on using the file;

a link to that cookbook recipe that the file is based on, for context that may be helpful when editing the file.

gnprice · 2023-10-13T01:03:13Z

integration_test/unreadmarker_test.dart

+        await tester.pumpAndSettle();
+        await tester.pumpAndSettle();
+      },
+      reportKey: 'test',


OK, and now that I've read the cookbook recipe: why pass this argument? There's already a default, of timeline, which seems no less informative than calling it test.

As the cookbook says:

Specify the reportKey when running more than one traceAction.

and we're running just one of them here.

gnprice · 2023-10-13T01:16:49Z

docs/integration_tests.md

+in `integration_tests/`.
+
+The integration tests also interact with test driver
+code, which a sample exists as `test_driver/perf_driver.dart`.


A "sample" is something that's not necessarily usable by itself, and that one is expected to probably want to adapt when using it. Does that apply here — what adaptations are expected to be needed?

Or on the other hand is this simply the driver we expect to use for performance profiling? (Possibly accumulating some changes over time, naturally — just like any of the rest of our code.)

gnprice · 2023-10-13T01:19:18Z

docs/integration_tests.md

+writes both the entire timeline as well as a summary
+to disk inside the `build` directory.


Writes them where — how does one locate the file (or files?) it writes out?

Probably the best place for that information is just after the sample command line below.

sirpengi · 2023-10-25T16:07:24Z

@gnprice Ready for review! In general we've kept small prep commits at the beginning of the PR chain but I've put two commits more related to the animation bugfix before the fix itself as I think they're more semantically connected. They are quite easily reordered in any case.

gnprice

Thanks for the revision! For this round I've read through everything but the tests and the integration tests.

gnprice · 2023-10-25T20:23:29Z

test/widgets/message_list_test.dart

-        eg.dmMessage(from: eg.selfUser, to: []),
-        eg.dmMessage(from: eg.otherUser, to: [eg.selfUser]),
-        eg.dmMessage(from: eg.thirdUser, to: [eg.selfUser, eg.otherUser]),
+        eg.dmMessage(id: 1, from: eg.selfUser, to: []),
+        eg.dmMessage(id: 2, from: eg.otherUser, to: [eg.selfUser]),
+        eg.dmMessage(id: 3, from: eg.thirdUser, to: [eg.selfUser, eg.otherUser]),


Huh interesting — why are these needed? The commit message says

msglist test [nfc]: Add distinct ids to messages in 'show names on DMs' setup

but it's not clear why these need distinct IDs now and didn't before.

If I leave out this change, I see that this test fails (and also the one after it, awkwardly).

I'm guessing the change that makes this necessary is where you add keys to the message items based on the message IDs.

The commit message should say why the change is needed, because that helps us think through whether it's a pattern that applies more broadly. Here, it seems like we probably need all our tests to start providing distinct message IDs, at least when building a message list. Otherwise, they're all skating on thin ice — even if they happen not to trigger the problems caused by reusing keys for things that are meant to be distinct, they're vulnerable to potentially failing, just like this one, after some minor unrelated change causes them to start doing so.

msglist test [nfc]: Add distinct ids to messages in 'show names on DMs' setup This will prevent a future test failure when other parts of the app begin assuming that messages have unique ids.

Cool, that's helpful.

As I suggested above, though, it seems like we probably need to do this more broadly. I've just gone and implemented that, as #343. So you can stack this PR atop that one (even before it's merged), and then this isn't needed.

Similarly #343 will make unnecessary several other uses of id on eg.streamMessage and eg.dmMessage calls in these tests, just like the ones in main now that it removes.

gnprice · 2023-10-25T21:30:25Z

lib/widgets/message_list.dart

-    final recipientBorder = BorderSide(color: highlightBorderColor, width: 3);
-    final restBorder = BorderSide(color: restBorderColor, width: 1);


msglist: Messages changed to render edge-to-edge and borderless This is a small change in line with #157 and moves the implementation closer to the new 2023+ design for mobile. Messages now span from screen edge to screen edge and borders around groups of messages (in particular the left-side recipient border) was removed.

This is much closer, but the first sentence is still inaccurate. The change this is making is in a direction completely orthogonal to #157 or to any new design for mobile — it's about moving from a design based on web to a mobile-specific design. The key design features here are exactly what zulip-mobile has had for many years.

In the summary line, a nit: verb should be in the imperative (or infinitive): https://zulip.readthedocs.io/en/latest/contributing/commit-discipline.html#commit-summary-part-2

Here's a revised version:

msglist: Go edge-to-edge, for a mobile-oriented design The existing design here was designed for web; the prototype copied precisely from web's design as of 2022-12. But in the zulip-mobile RN app, we've long had a design that's optimized for a narrow mobile screen instead: notably by using the full available width (up to a maximum, anyway), with no left or right borders, and consequently less padding too. Do that here.

gnprice · 2023-10-25T21:31:33Z

lib/widgets/message_list.dart

+// transition using the ease-out curve.
+// See zulip-mobile:src/webview/static/base.css .
+//
+// Modern web uses a block element will a linear-gradient


nit:

Suggested change

// Modern web uses a block element will a linear-gradient

// Modern web uses a block element with a linear-gradient

Also can just say "web" rather than "modern web", as there's no mention of the pre-2023 design to contrast "modern" with. And the description "modern" is doomed to become out of date anyway as time passes and things continue to evolve :-)

gnprice · 2023-10-25T21:44:38Z

lib/widgets/message_list.dart

+            child: Container(
+              width: 4,
+              decoration: BoxDecoration(
+                color: const HSLColor.fromAHSL(1, 227, 0.78, 0.59).toColor())))),


Can reduce this to simpler parts:

Suggested change

child: Container(

width: 4,

decoration: BoxDecoration(

color: const HSLColor.fromAHSL(1, 227, 0.78, 0.59).toColor())))),

child: SizedBox(width: 4,

child: ColoredBox(

color: const HSLColor.fromAHSL(1, 227, 0.78, 0.59).toColor())))),

Also the SizedBox (or Container) can go outside the animated widget. I think that doesn't actually end up mattering for AnimatedOpacity in particular — the thing it does on each frame of an animation is to re-composite layers, not to re-build — but it's easy here, and it's a good habit to default to to push complexity outside of what's animated.

Even better I've figured out the Positioned widget can also handle the width aspect. Also used ColoredBox instead of a Container as suggested.

gnprice · 2023-10-25T21:58:21Z

lib/widgets/message_list.dart

+}
+
+/// Widget responsible for showing the read status of a message.
+// Mobile currently uses a single inset box-shadow:


In the future "mobile currently" will mean this codebase :-) So this should be made more specific: "zulip-mobile uses …", or "The zulip-mobile RN app uses …".

gnprice · 2023-10-25T22:09:14Z

lib/widgets/message_list.dart

+          child: AnimatedOpacity(
+            opacity: isRead ? 0 : 1,
+            duration: Duration(milliseconds: isRead ? 2000 : 300),
+            curve: Curves.easeOut,
+            child: Container(
+              width: 4,
+              decoration: BoxDecoration(
+                color: const HSLColor.fromAHSL(1, 227, 0.78, 0.59).toColor())))),


Here's a version integrating my other comment, and then putting the key pieces of information about the design into concise comments right at the relevant bits of the code:

Suggested change

child: AnimatedOpacity(

opacity: isRead ? 0 : 1,

duration: Duration(milliseconds: isRead ? 2000 : 300),

curve: Curves.easeOut,

child: Container(

width: 4,

decoration: BoxDecoration(

color: const HSLColor.fromAHSL(1, 227, 0.78, 0.59).toColor())))),

child: SizedBox(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: ColoredBox(

// The color hsl(227deg 78% 59%) comes from the Figma mockup at:

// https://github.com/zulip/zulip-mobile/issues/5511

// https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=132-9684

color: const HSLColor.fromAHSL(1, 227, 0.78, 0.59).toColor())))),

Then I think that replaces most of the comment that's at the top of the class.

Ah right or in light of the discussion above at #317 (comment) , the link to that GitHub issue can be replaced with a chat link:
https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/flutter.3A.20unread.20marker/near/1658008

(Best to link to a specific message ID, especially when the thread is already long. That way it's clear which part of the discussion you're referring to.)

The issue is linked from the chat thread, and it doesn't add anything specific about the colors besides the link to Figma, so the issue link can be dropped when the chat link is present.

gnprice · 2023-10-25T22:16:29Z

lib/widgets/message_list.dart

+// both with an ease-out curve.
+// See zulip:web/styles/message_row.css .
+//
+// Current design work is ongoing and values used here come


A remark like "current design work is ongoing" is something that typically won't still be true when we merge a change. So that means it generally doesn't belong in the code of the PR, which is a proposal for code to merge. Instead it's a good remark to make in the PR thread, e.g. in the form of an inline comment (a GitHub "review" comment).

gnprice · 2023-10-25T22:19:20Z

lib/widgets/message_list.dart

      // TODO: Offer `ScrollViewKeyboardDismissBehavior.interactive` (or
      //   similar) if that is ever offered:
      //     https://github.com/flutter/flutter/issues/57609#issuecomment-1355340849
+      findChildIndexCallback: (Key key) {


This comment is about the code it was immediately next to, namely keyboardDismissBehavior. So it should stay attached to that — otherwise the comment starts becoming puzzling to understand, and also someone looking at the relevant code is no longer likely to notice the comment.

gnprice · 2023-10-25T22:24:01Z

lib/widgets/message_list.dart

+        // be improved but for now it only triggers for materialized
+        // widgets. As a simple test, scrolling through All Messages in
+        // CZO on a Pixel 5, this only runs about 10 times per rebuild
+        // and the timing for each call is measured in microseconds.


microseconds

Great!

Well, hm — does that mean "a handful of microseconds" or does it mean "less than a millisecond"?

The latter is something people might often mean by "measured in microseconds", and in this context it wouldn't actually be very encouraging — 10 times nearly-a-ms would be nearly 10ms, which would be a real problem. In general "measured in $unit" is not a very clear way of saying how big something is. For example, physicists routinely measure both subatomic and intergalactic distances in meters.

Better to be more concrete: like "<100 microseconds" or "<10 microseconds", or whatever you can most easily measure that's still specific enough to be reassuring.

Specified that it was "<100 microseconds". The total impact would be around 1ms so it's quite reassuring the current behavior will be okay. Also changed "scrolling" to "flinging".

gnprice · 2023-10-25T22:32:00Z

lib/widgets/message_list.dart

+          return null;
+        }
+        // Note `itemBuilder` pulls from `model.items` in reverse order.
+        return length - index - 1;


Clearest to group these terms in the same way as in itemBuilder:

Suggested change

return length - index - 1;

return length - 1 - index;

That way when comparing the two, "length - 1" can be visually taken together as a single common quantity with a minimum of mental algebra.

sirpengi · 2023-10-26T21:20:02Z

@gnprice ready for the next round!

gnprice

Thanks for the revision! Got to this a bit late in the day, so for this round just went back over what was in the previous round #317 (review) . Small comments below.

gnprice · 2023-10-27T04:33:31Z

lib/widgets/message_list.dart

+          width: 4,
+          child: AnimatedOpacity(
+            opacity: isRead ? 0 : 1,
+            // Web used 2s and 0.3s durations, and a CSS ease-out curve.


Suggested change

// Web used 2s and 0.3s durations, and a CSS ease-out curve.

// Web uses 2s and 0.3s durations, and a CSS ease-out curve.

gnprice · 2023-10-27T05:11:33Z

test/widgets/message_list_test.dart

-        eg.dmMessage(from: eg.selfUser, to: []),
-        eg.dmMessage(from: eg.otherUser, to: [eg.selfUser]),
-        eg.dmMessage(from: eg.thirdUser, to: [eg.selfUser, eg.otherUser]),
+        eg.dmMessage(id: 1, from: eg.selfUser, to: []),
+        eg.dmMessage(id: 2, from: eg.otherUser, to: [eg.selfUser]),
+        eg.dmMessage(id: 3, from: eg.thirdUser, to: [eg.selfUser, eg.otherUser]),


msglist test [nfc]: Add distinct ids to messages in 'show names on DMs' setup This will prevent a future test failure when other parts of the app begin assuming that messages have unique ids.

Cool, that's helpful.

As I suggested above, though, it seems like we probably need to do this more broadly. I've just gone and implemented that, as #343. So you can stack this PR atop that one (even before it's merged), and then this isn't needed.

Similarly #343 will make unnecessary several other uses of id on eg.streamMessage and eg.dmMessage calls in these tests, just like the ones in main now that it removes.

gnprice · 2023-10-27T05:13:42Z

lib/widgets/message_list.dart

+      },
      // TODO: Offer `ScrollViewKeyboardDismissBehavior.interactive` (or


nit:

Suggested change

},

// TODO: Offer `ScrollViewKeyboardDismissBehavior.interactive` (or

},

// TODO: Offer `ScrollViewKeyboardDismissBehavior.interactive` (or

Each of these is a sizable chunk of code, and they're fairly separate from each other in meaning, so the blank line helps visually set them off.

Especially since the latter chunk starts with a comment — the blank line separates it from the unrelated chunk above, helping it stay visually attached to the chunk below.

gnprice · 2023-10-27T05:17:06Z

lib/widgets/message_list.dart

+        // Note `itemBuilder` pulls from `model.items` in reverse order.
+        return length - 1 - index;


Suggested change

// Note `itemBuilder` pulls from `model.items` in reverse order.

return length - 1 - index;

return length - 1 - index;

The corresponding code in itemBuilder isn't far away, and that's the most dependable thing to refer to to understand why this expression is what it is.

Also we should have tests that check this is correct. That's the dependable way to make sure that this and itemBuilder stay in sync. … And at a quick experiment, things look good on that front — if I edit this to return index;, several tests fail.

The corresponding code in itemBuilder isn't far away

Speaking of which, though, a nit: this findChildIndexCallback should go after keyboardDismissBehavior, not before. That way it's adjacent to the stanza that has itemBuilder in it, which is good because as we see their logic is interdependent with each other. And there isn't such an interdependence involving keyboardDismissBehavior.

sirpengi · 2023-10-27T12:27:26Z

@gnprice comments handled, ready again!

gnprice · 2023-10-27T17:13:23Z

Cool, that looks good. Later today I'll plan to review the tests and the integration tests.

gnprice

OK, and here's from a read through the tests!

Left for another day is the commit adding an integration test:
eadba4e test: Add integration test of _UnreadMarker animation

gnprice · 2023-10-27T23:40:40Z

test/widgets/message_list_test.dart

+      check(getAnimation(tester, message.id).value).equals(0);
+
+      store.handleEvent(eg.updateMessageFlagsRemoveEvent(
+        MessageFlag.read, [message]));
+      await tester.pump(); // process handleEvent
+      check(getAnimation(tester, message.id).status).equals(AnimationStatus.forward);
+
+      await tester.pumpAndSettle();
+
+      check(getAnimation(tester, message.id).value).equals(1.0);


It's generally best if the "before" and "after" checks in a test inspect the before and after of the same data — that way it's easy for the reader to see that the test's checks definitely mean something changed.

Specifically here, if we're checking the status is forward after we start things, it's good to check its value before we start things too. Otherwise, maybe forward is just what it is from the beginning.

The value is good to check too — at the middle stage, we could check it's more than 0 and less than 1.

gnprice · 2023-10-27T23:42:18Z

test/widgets/message_list_test.dart

+      final message = eg.streamMessage(flags: [MessageFlag.read]);
+      await setupMessageListPage(tester, messages: [message]);
+
+      check(getAnimation(tester, message.id).value).equals(0);


To put this in idiomatic package:checks style, we should add handling for Animation in test/flutter_checks.dart.

That will be especially helpful for checking several things on a single value, like its value and its status.

gnprice · 2023-10-27T23:46:24Z

test/widgets/message_list_test.dart

+      await tester.pumpAndSettle();
+
+      check(getAnimation(tester, message.id).value).equals(1.0);


nit:

Suggested change

await tester.pumpAndSettle();

check(getAnimation(tester, message.id).value).equals(1.0);

await tester.pumpAndSettle();

check(getAnimation(tester, message.id).value).equals(1.0);

Generally I often find it helpful to separate one "scene" of a test from the next: make some changes, check the new state of things, then a blank line before the next round of changes. But the checks go together logically with the changes that lead to them.

gnprice · 2023-10-27T23:50:38Z

test/widgets/message_list_test.dart

+      final message = eg.streamMessage(flags: []);
+      await setupMessageListPage(tester, messages: [message]);
+
+      check(getAnimation(tester, message.id).value).equals(1.0);


nit: this and the zero in the other check are parallel semantically, so match them stylistically — both written with ".0", or neither.

Otherwise it looks like there's some distinction being intentionally made.

gnprice · 2023-10-27T23:50:51Z

test/widgets/message_list_test.dart

+      // Check that _UnreadMarker maintains its in-progress animation
+      // as the number of items change in MessageList.


nit:

Suggested change

// Check that _UnreadMarker maintains its in-progress animation

// as the number of items change in MessageList.

// Check that _UnreadMarker maintains its in-progress animation

// as the number of items changes in MessageList.

gnprice · 2023-10-27T23:55:02Z

test/widgets/message_list_test.dart

+      // If we introduce an animation bug (remove findChildIndexCallback
+      // on StickyHeaderListView.builder) this returns 1.


This is already true of the check above, right? I.e. that if we introduce the same bug, that check fails.

In general it's expected that if we remove some logic, then some test we added in the same commit will fail.

It is helpful for the reader of this test to have a reference to the name findChildIndexCallback, to see what code it's meant to test. The best place for that is in a comment at the top of the test, or in the name of the test.

gnprice · 2023-10-28T00:04:08Z

test/widgets/message_list_test.dart

+    Animation getAnimation(WidgetTester tester, int messageId) {
+      final widget = tester.widget<FadeTransition>(find.descendant(
+        of: find.byKey(ValueKey(messageId)),
+        matching: find.byType(FadeTransition)));
+      return widget.opacity;


This feels kind of brittle. But I don't have a better suggestion off the top of my head, and I'll be OK with merging it. If we play with it we might be able to find something better.

I agree, it's unfortunate that the AnimatedOpacity widget that is being used in the tree doesn't give us access to the live values and if flutter internally changes how the widget is implemented this would break. Also, if there are further animations added into the MessageItem widget (perhaps we have animations on the reactions, or another UI element in each message) this will likely find the wrong one. I've tried playing with it but haven't found anything that isn't equally brittle.

gnprice · 2023-10-28T00:16:01Z

test/widgets/message_list_test.dart

@@ -300,4 +300,87 @@ void main() {
      debugNetworkImageHttpClientProvider = null;
    });
  });
+
+  group('_UnreadMarker animations', () {
+    Animation getAnimation(WidgetTester tester, int messageId) {


This last commit:

30cb3cd msglist test [nfc]: Pull out getAnimation helper

has the feeling that within the branch an early commit went one direction, then a later commit decided a different direction was better and went that way instead.

Better to revise the branch as a whole to give it a coherent narrative. That means squashing that commit into the one that originally introduces its own version of getAnimation:
8605d56 msglist: Add animated marker for unread messages

Then this later commit:
765de16 msglist: Maintain _UnreadMarker animation state when MessageListView.items changes size

has no need to introduce an alternate version, and there's no need for the third commit moving the alternate version to replace the original version.

gnprice · 2023-10-28T00:24:03Z

pubspec.yaml

 dev_dependencies:
+  flutter_driver:
+    sdk: flutter


deps: Add integration_test and flutter_driver, in preparation for integration tests

Following the style of past commits in this section, it's helpful to flag that these are only dev dependencies (and not dependencies that go into the published app):

deps: Add dev:integration_test, dev:flutter_driver

Cf the other previous commits that added entries here:
381c802 deps: Add dev:fake_async
483a3ef deps: Add drift, sqlite3_flutter_libs, path_provider, path, dev:drift_dev
7a9bf81 deps: Add dev:checks
6c9b94c deps: Add dev:test
9ea09ce deps: Add json_annotation, dev:json_serializable, dev:build_runner

I found those with git log -L 65,85:pubspec.yaml. (Where the line range corresponds to the dev_dependencies: section. There are fancier ways to write git log -L ranges in terms of regexps, which could make the command robust to different versions of the file; but in ad hoc interactive use, I'd just read off the line numbers from looking at the file in my editor.)

The explanation about why we're adding these packages can then go in the body of the commit message. Though in this instance I'd be happy to leave it out — one of the package names already tells the reader it's about integration tests.

BTW the dev: notation there is from the syntax accepted by flutter pub add (or dart pub add). See flutter pub add --help.

terpimost · 2023-10-30T00:47:24Z

After some closer look I think we should do slight modification:

Clarifications
In the dark mode the blue bar is lighter than background and lighter than device screen border:

In the light mode the bar is darker than background, closer to the darkness of device screen border (black)
So the contrast is lower and it is hard to see such bar:

I suggest to put 1 px 60% white on top of such bar in the light mode:

This gives a better distinction for the light mode unread markers:

gnprice · 2023-10-30T20:18:42Z

In the light mode the bar is darker than background, closer to the darkness of device screen border (black) So the contrast is lower and it is hard to see such bar: […]
I suggest to put 1 px 60% white on top of such bar in the light mode:

Great, thanks! That makes sense and I'll be interested to see it.

(We don't yet have dark mode in the Flutter app — that's #95 and is slated for the beta. @sirpengi for this PR, let's leave a TODO(#95) comment saying that extra 1px bar won't be there in dark mode, pointing to @terpimost's comment above. That way it'll be easy for us to see how to handle it when we sweep through for #95.)

The existing design here was designed for web; the prototype copied precisely from web's design as of 2022-12. But in the zulip-mobile RN app, we've long had a design that's optimized for a narrow mobile screen instead: notably by using the full available width (up to a maximum, anyway), with no left or right borders, and consequently less padding too. Do that here.

sirpengi · 2023-10-30T22:11:01Z

Added the 1px 60% white border as specified and updated the screenshow in the top comment as well

sirpengi · 2023-10-30T22:56:10Z

@gnprice comments handled, and the design change from @terpimost included too!

gnprice

Thanks for the revision!

Leaving out for the moment the commit adding the integration test, this all looks good modulo just a couple of nits below.

I'll take a look at that integration-test commit next. But probably the easiest workflow will be if we split that out, along with its prep commit:
8ff65f4 deps: Add dev:flutter_driver, dev:integration_test
00a6449 test: Add integration test of _UnreadMarker animation
to a separate followup PR.

gnprice · 2023-10-31T00:38:52Z

lib/widgets/message_list.dart

@@ -286,6 +304,7 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
              header: header, child: header);
          case MessageListMessageItem():
            return MessageItem(
+              key: ValueKey(data.message.id),


This now appears in a different commit from findChildIndexCallback. The two of them work in tandem and make most sense to understand together, so they should appear in the same commit.

Hmm I see, this commit uses it in tests.

As I mentioned in chat, I'd be hesitant to add keys just for the sake of tests. Here it's OK because we'll be using it in the app code just a few commits later; so we should just make that clear in the commit message. Can add something like "Also add a key to MessageItem widgets in the message list; we'll want those in an upcoming commit that passes findChildIndexCallback, so we just bring them forward to here for convenience in tests."

gnprice · 2023-10-31T00:41:13Z

lib/widgets/message_list.dart

+            child: Container(
+              decoration: BoxDecoration(


Suggested change

child: Container(

decoration: BoxDecoration(

child: DecoratedBox(

decoration: BoxDecoration(

That's what this boils down to; see the implementation of Container.build.

gnprice · 2023-10-31T00:50:23Z

test/widgets/message_list_test.dart

+      check(getAnimation(tester, message.id))
+        ..value.equals(1.0)
+        ..status.equals(AnimationStatus.dismissed);


Cool — this extra detail in fact serves as helpful color on how Animation works, how the status interacts with the value. Here the value is at 1 while the status is dismissed, whereas in the read->unread case it was at 0 when dismissed and 1 corresponded to completed.

gnprice · 2023-10-31T00:54:21Z

test/widgets/message_list_test.dart

+
+      // 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);


Cool, this test is now all nice and clear, with a crisp narrative.

gnprice

OK, and here's various small comments about the integration tests. This has also gotten much closer!

gnprice · 2023-10-31T01:06:00Z

docs/integration_tests.md

+Integration tests in Flutter allow self-driven end-to-end
+testing of app code running with the full GUI.


What does "self-driven" mean?

In this case it's as a vehicle driving itself without human interaction. It's the same language the integration_test package uses to describe itself: https://github.com/flutter/flutter/tree/master/packages/integration_test (This package enables self-driving testing of Flutter code on devices and emulators.). I think "autonomous" might be an alternative but I think the "drive" verb is used often in similar contexts (Selenium describes this automation as driving).

gnprice · 2023-10-31T01:07:12Z

docs/integration_tests.md

+The focus in this document are notes for using integration
+tests to capture performance metrics on physical devices.


Suggested change

The focus in this document are notes for using integration

tests to capture performance metrics on physical devices.

This document is about using integration

tests to capture performance metrics on physical devices.

gnprice · 2023-10-31T01:09:12Z

docs/integration_tests.md

+[flutter-docs]: https://docs.flutter.dev/testing/integration-tests
+
+## Capturing performance metrics


nit: two blank lines before Markdown headings, unless preceded by a parent heading or the start of the file:

Suggested change

[flutter-docs]: https://docs.flutter.dev/testing/integration-tests

## Capturing performance metrics

[flutter-docs]: https://docs.flutter.dev/testing/integration-tests

## Capturing performance metrics

I find that helpful for giving a visual hierarchy when reading the Markdown source file.

gnprice · 2023-10-31T01:49:41Z

docs/integration_tests.md

+
+## Obtaining performance metrics
+
+First, obtain a `device_id` using `flutter devices`.


Suggested change

First, obtain a `device_id` using `flutter devices`.

First, obtain a device ID using `flutter devices`.

The string device_id isn't something that appears verbatim in any API or other form of code this is talking about, so doesn't belong in code font.

(It does appear in your example command below, so putting it in code font would make sense if it were clearly referring to that. But when it's here before that command, simpler to just use the English phrase.)

gnprice · 2023-10-31T01:52:03Z

docs/integration_tests.md

+$ flutter drive \
+  --driver=integration_test/perf_driver.dart \
+  --target=integration_test/unreadmarker_test.dart \


nit: indent arguments deeper than the start of the command:

Suggested change

$ flutter drive \

--driver=integration_test/perf_driver.dart \

--target=integration_test/unreadmarker_test.dart \

$ flutter drive \

--driver=integration_test/perf_driver.dart \

--target=integration_test/unreadmarker_test.dart \

(as in #317 (comment) above)

gnprice · 2023-10-31T01:57:02Z

docs/integration_tests.md

+A data file with raw event timings will be produced in
+`build/trace_output.timeline.json`.
+
+A more readily consumable file will also be produced in
+`build/trace_output.timeline_summary.json`. This file
+contains widget build and render timing data.


Cool. What does one do next with these files?

We don't need to write a lot of text here — a link to whatever resource you found helpful would be great.

And if there was no such resource and you just inspected them manually, then we can say just that — that's still useful to the reader, by letting them know there isn't information that we had before and they're missing.

gnprice · 2023-10-31T02:10:28Z

integration_test/unreadmarker_test.dart

+    // short pause for buffer during video-capture
+    await tester.pump(const Duration(seconds: 2));


Interesting — what's the video-capture about? Is that involved in the performance test?

Removing this for now. I also used this integration test to automate the app for video recording purposes, and it was useful to have a bit of buffer before the action started, but that isn't relevant to a performance test.

gnprice · 2023-10-31T02:13:19Z

integration_test/unreadmarker_test.dart

+  testWidgets('_UnreadMarker animation performance test', (tester) async {
+    final messages = await setupMessageListPage(tester, 500);


I think a short comment setting the context here would be helpful. Otherwise it's a bit puzzling how this "test" doesn't appear to actually check anything as it goes.

Suggested change

testWidgets('_UnreadMarker animation performance test', (tester) async {

final messages = await setupMessageListPage(tester, 500);

testWidgets('_UnreadMarker animation performance test', (tester) async {

// This integration test is meant for measuring performance.

// See docs/integration_test.md for how to use it.

final messages = await setupMessageListPage(tester, 500);

gnprice · 2023-10-31T02:18:06Z

integration_test/perf_driver.dart

+Future<void> main() {
+  return integrationDriver(
+    responseDataCallback: (data) async {
+      if (data != null) {


nit: let's de-indent by using an early return:

Suggested change

if (data != null) {

if (data == null) return;

gnprice · 2023-10-31T02:23:29Z

integration_test/perf_driver.dart

@@ -0,0 +1,24 @@
+// This file is derived from BSD licensed example code in


Let's put this part (about being derived from some other code, and the license of that code) in the commit message rather than an in-tree comment. We've now modified this file a fair bit, and may modify it more in the future; so that's the form that will best avoid getting outdated with such changes.

The link to the docs is still good as context for someone editing this file in the future.

Adds a new _UnreadMarker that renders as a 4px solid border on the left edge of messages. Also add a key to MessageItem widgets in the message-list; we'll want those in an upcoming commit that passes findChildIndexCallback, so we just bring them forward to here for convenience in tests. Fixes: zulip#79

…items changes size

sirpengi · 2023-10-31T14:47:29Z

@gnprice split off the integration tests bit to #349 as well as dealt with the other comments.

gnprice · 2023-10-31T17:50:44Z

Thanks @sirpengi for the revision, and for all your work on this! Looks good; merging. I'll take a look at #349 next.

sirpengi force-pushed the pr-unread-markers branch from 71b5c7a to e660635 Compare October 11, 2023 13:27

sirpengi force-pushed the pr-unread-markers branch from e660635 to 9e660b5 Compare October 11, 2023 17:15

sirpengi requested a review from gnprice October 12, 2023 17:05

gnprice reviewed Oct 13, 2023

View reviewed changes

gnprice mentioned this pull request Oct 16, 2023

model: Add Unreads model, for tracking unread-message counts #304

Merged

sirpengi force-pushed the pr-unread-markers branch 7 times, most recently from 85243fd to 508f86f Compare October 25, 2023 15:25

sirpengi changed the title ~~[WIP] Unread markers~~ Unread markers Oct 25, 2023

gnprice reviewed Oct 25, 2023

View reviewed changes

gnprice mentioned this pull request Oct 25, 2023

android: Bump minSdkVersion to 24, aka Android 7 Nougat #336

Merged

sirpengi force-pushed the pr-unread-markers branch from 508f86f to b1491dc Compare October 26, 2023 21:08

gnprice reviewed Oct 27, 2023

View reviewed changes

gnprice mentioned this pull request Oct 27, 2023

test: Generate random, increasing message IDs for example data by default #343

Merged

sirpengi force-pushed the pr-unread-markers branch 2 times, most recently from 803be54 to 30cb3cd Compare October 27, 2023 12:09

gnprice reviewed Oct 28, 2023

View reviewed changes

sirpengi force-pushed the pr-unread-markers branch from 30cb3cd to b1ba656 Compare October 30, 2023 20:29

sirpengi force-pushed the pr-unread-markers branch 2 times, most recently from bdba1f6 to 743cd2c Compare October 30, 2023 20:34

sirpengi force-pushed the pr-unread-markers branch from 743cd2c to f60ef5b Compare October 30, 2023 22:39

gnprice reviewed Oct 31, 2023

View reviewed changes

sirpengi added 3 commits October 31, 2023 13:22

msglist [nfc]: Make _MessageSequence.findItemWithMessageId non-private

e80db81

msglist: Maintain _UnreadMarker animation state when MessageListView.…

33dcbf9

…items changes size

sirpengi mentioned this pull request Oct 31, 2023

Add integration test of _UnreadMarker animation #349

Merged

sirpengi force-pushed the pr-unread-markers branch from f60ef5b to 33dcbf9 Compare October 31, 2023 14:14

gnprice merged commit 33dcbf9 into zulip:main Oct 31, 2023
1 check passed

sirpengi deleted the pr-unread-markers branch January 23, 2024 14:17

		final recipientBorder = BorderSide(color: highlightBorderColor, width: 3);
		final restBorder = BorderSide(color: restBorderColor, width: 1);

		writes both the entire timeline as well as a summary
		to disk inside the `build` directory.

	// Modern web uses a block element will a linear-gradient
	// Modern web uses a block element with a linear-gradient

	// Web used 2s and 0.3s durations, and a CSS ease-out curve.
	// Web uses 2s and 0.3s durations, and a CSS ease-out curve.

		},
		// TODO: Offer `ScrollViewKeyboardDismissBehavior.interactive` (or

		// Note `itemBuilder` pulls from `model.items` in reverse order.
		return length - 1 - index;

		await tester.pumpAndSettle();

		check(getAnimation(tester, message.id).value).equals(1.0);

		// Check that _UnreadMarker maintains its in-progress animation
		// as the number of items change in MessageList.

		// If we introduce an animation bug (remove findChildIndexCallback
		// on StickyHeaderListView.builder) this returns 1.

		Integration tests in Flutter allow self-driven end-to-end
		testing of app code running with the full GUI.

		The focus in this document are notes for using integration
		tests to capture performance metrics on physical devices.

		[flutter-docs]: https://docs.flutter.dev/testing/integration-tests

		## Capturing performance metrics


		## Obtaining performance metrics

		First, obtain a `device_id` using `flutter devices`.

	First, obtain a `device_id` using `flutter devices`.
	First, obtain a device ID using `flutter devices`.

		// short pause for buffer during video-capture
		await tester.pump(const Duration(seconds: 2));

		testWidgets('_UnreadMarker animation performance test', (tester) async {
		final messages = await setupMessageListPage(tester, 500);

		@@ -0,0 +1,24 @@
		// This file is derived from BSD licensed example code in

Unread markers #317

Unread markers #317

Conversation

sirpengi commented Oct 11, 2023 • edited Loading

sirpengi commented Oct 11, 2023

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

sirpengi commented Oct 25, 2023

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

sirpengi Oct 26, 2023 • edited Loading

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

sirpengi commented Oct 26, 2023

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

sirpengi commented Oct 27, 2023

gnprice commented Oct 27, 2023

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

terpimost commented Oct 30, 2023

gnprice commented Oct 30, 2023

sirpengi commented Oct 30, 2023

sirpengi commented Oct 30, 2023

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

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

sirpengi commented Oct 11, 2023 •

edited

Loading

sirpengi Oct 26, 2023 •

edited

Loading