msglist: Use [User.avatarUrl] instead of [Message.avatarUrl] #246

chrisbobbe · 2023-08-01T21:49:55Z

Now, as demonstrated in the new test, if the author of a message
changes their avatar, we'll see the update in the message list as
soon as we get the event.

While we're at it, comment out the property on [Message] to guide
future consumers to [User].

Related: #135

chrisbobbe · 2023-08-01T21:53:32Z

test/test_images.dart

+import 'package:flutter_test/flutter_test.dart';
+
+
+class FakeHttpClient extends Fake implements HttpClient {


test [nfc]: Move fake-image-request code to its own file

N.B.: This now has the same name as FakeHttpClient in test/api/fake_api.dart, which causes awkwardness when a file wants import lines for both files. Later in this branch, this awkwardness shows up like this:

-import '../api/fake_api.dart'; +import '../api/fake_api.dart' as fake_api;

Maybe there's a better name for this class, like FakeImageHttpClient? (Then maybe also its friends, like FakeHttpClientResponse, get renamed correspondingly?)

Yeah, I think if this is going to be a public name that's imported in various tests (rather than a private name in one test file), it needs a more specific name.

The FakeImageFoo names sound good. It's likely that if we wanted to use these for faking something other than fetching a network image, we'd need to add other features to them.

chrisbobbe · 2023-08-02T00:50:31Z

This came up when thinking about avatar URLs for #119.

chrisbobbe · 2023-08-02T21:45:43Z

(Just rebased onto current main.)

gnprice

Thanks! Small comments, all in the tests.

gnprice · 2023-08-02T23:27:15Z

test/test_images.dart

+import 'package:flutter_test/flutter_test.dart';
+
+
+class FakeHttpClient extends Fake implements HttpClient {


Yeah, I think if this is going to be a public name that's imported in various tests (rather than a private name in one test file), it needs a more specific name.

The FakeImageFoo names sound good. It's likely that if we wanted to use these for faking something other than fetching a network image, we'd need to add other features to them.

gnprice · 2023-08-02T23:27:21Z

test/widgets/message_list_test.dart

 import 'package:zulip/widgets/message_list.dart';
 import 'package:zulip/widgets/sticky_header.dart';
 import 'package:zulip/widgets/store.dart';

-import '../api/fake_api.dart';
+import '../api/fake_api.dart' as fake_api;


If we were keeping the generic FakeHttpClient name so did have to deal with a collision here, I think I'd prefer in this case to use show to just expose the one name we want from this library:

Suggested change

import '../api/fake_api.dart' as fake_api;

import '../api/fake_api.dart' show FakeApiConnection;

That way we don't have to say fake_api.FakeApiConnection, which looks awfully redundant.

gnprice · 2023-08-02T23:28:38Z

test/widgets/message_list_test.dart

@@ -51,6 +59,58 @@ Future<void> setupMessageListPage(WidgetTester tester, {
 void main() {
  TestZulipBinding.ensureInitialized();

+  group('MessageWithSender', () {


nit: let's put this after the ScrollToBottomButton group, so the tests are in roughly the order of the code they respectively focus on

gnprice · 2023-08-02T23:30:36Z

test/widgets/message_list_test.dart

+      addTearDown(TestZulipBinding.instance.reset);
+
+      RealmContentNetworkImage? findAvatarImageWidget(WidgetTester tester) {
+        final firstMessageWithSender = tester.widgetList(find.byType(MessageWithSender)).first;


There's a tester.firstWidget; can this use that?

gnprice · 2023-08-02T23:35:19Z

test/widgets/message_list_test.dart

+        final firstMessageWithSender = tester.widgetList(find.byType(MessageWithSender)).first;
+        return tester.widgetList<RealmContentNetworkImage>(
+          find.descendant(
+            of: find.byWidget(firstMessageWithSender),


Also this could just be the find.byType(MessageWithSender) directly, and then firstWidget around the whole thing.

I think the difference in behavior would just be: if the first MessageWithSender has several RealmContentNetworkImage descendants, then this version would throw and that version wouldn't. Which… hmm, in fact it's bad to throw in that case, right? The message content could legitimately have an image, though it doesn't actually in this test.

In any event it doesn't seem helpful to throw in that case. If we have a bug where MessageWithSender shows the avatar twice or whatever, this isn't the test to detect that.

Whichever way it's organized, I don't love that we have to look for RealmContentNetworkImage here to find the avatar. That feels like a bit of an implementation detail of the message list, and also a bit fragile because that widget type could appear in the content. I'd rather have something we can look for that more specifically marks the avatar.

I guess with an Avatar widget that'd provide an answer. Though that also feels like a bit of an implementation detail.

Another version would be to look for a semantic label. That'd be appealing because it's not an implementation detail — it's something user-facing. Not sure what the right semantic label would be in this case, though. In fact I kind of think for the semantics tree we likely want to hide the avatar — there's nothing it communicates that isn't already covered by the sender name, other than the visual of the image itself.

Anyway, using RealmContentNetworkImage is fine for now. We can switch to the Avatar widget upon adding that, and possibly to semantics later.

Makes sense. Yeah, I felt kind of uneasy having it look for RealmContentNetworkImage too.

Also this could just be the find.byType(MessageWithSender) directly, and then firstWidget around the whole thing.

Ah: I tried this just now, but then a test failure reminded me that we'd need it to be firstWidgetOrNull, and that isn't offered.

gnprice · 2023-08-02T23:37:01Z

test/widgets/message_list_test.dart

+          case String(): {
+            check(findAvatarImageWidget(tester)).isNotNull().src.equals(resolveUrl(avatarUrl, eg.selfAccount));
+          }


No need for the braces:

Suggested change

case String(): {

check(findAvatarImageWidget(tester)).isNotNull().src.equals(resolveUrl(avatarUrl, eg.selfAccount));

}

case String():

check(findAvatarImageWidget(tester)).isNotNull().src.equals(resolveUrl(avatarUrl, eg.selfAccount));

In fact in Dart (unlike JS) one can even declare variables in a switch case, with no need for extra braces.

gnprice · 2023-08-03T00:05:19Z

test/widgets/message_list_test.dart

+        switch (avatarUrl) {
+          case String(): {


Also when the switch is just "null or not" it's probably simplest to just use an if/else 🙂

gnprice · 2023-08-03T00:05:45Z

test/widgets/message_list_test.dart

+        final avatarUrl = sender.avatarUrl;
+        switch (avatarUrl) {
+          case String(): {
+            check(findAvatarImageWidget(tester)).isNotNull().src.equals(resolveUrl(avatarUrl, eg.selfAccount));


nit:

Suggested change

check(findAvatarImageWidget(tester)).isNotNull().src.equals(resolveUrl(avatarUrl, eg.selfAccount));

check(findAvatarImageWidget(tester)).isNotNull()

.src.equals(resolveUrl(avatarUrl, eg.selfAccount));

gnprice · 2023-08-03T00:10:23Z

test/widgets/message_list_test.dart

+      // TODO only vary [avatarUrl], not other fields
+      checkResultForSender(eg.user(userId: eg.selfUser.userId, avatarUrl: '/foo.png'));


This checkResultForSender is a private helper for this test, right? And it only consults the avatarUrl.

So it seems like you could resolve this TODO by just passing the avatarUrl value itself.

chrisbobbe · 2023-08-03T00:40:04Z

Thanks for the review! Revision pushed.

Now, as demonstrated in the new test, if the author of a message changes their avatar, we'll see the update in the message list as soon as we get the event. While we're at it, comment out the property on [Message] to guide future consumers to [User]. Related: zulip#135

gnprice · 2023-08-03T20:20:13Z

Thanks! LGTM; merging.

chrisbobbe added a-msglist The message-list screen, except what's label:a-content a-model Implementing our data model (PerAccountStore, etc.) labels Aug 1, 2023

chrisbobbe requested a review from gnprice August 1, 2023 21:49

chrisbobbe commented Aug 1, 2023

View reviewed changes

chrisbobbe force-pushed the pr-msglist-avatar-live-update branch from a75fcb1 to 278acdc Compare August 1, 2023 21:55

chrisbobbe force-pushed the pr-msglist-avatar-live-update branch from 278acdc to 5f18921 Compare August 2, 2023 21:45

chrisbobbe mentioned this pull request Aug 2, 2023

RecentDmConversationsPage: Add #249

Merged

gnprice reviewed Aug 3, 2023

View reviewed changes

chrisbobbe force-pushed the pr-msglist-avatar-live-update branch from 5f18921 to 6141ee3 Compare August 3, 2023 00:39

chrisbobbe added 3 commits August 3, 2023 13:19

test: Have eg.user accept avatarUrl

feabdfe

test [nfc]: Move fake-image-request code to its own file

55cf074

gnprice force-pushed the pr-msglist-avatar-live-update branch from 6141ee3 to aade783 Compare August 3, 2023 20:20

gnprice merged commit aade783 into zulip:main Aug 3, 2023

chrisbobbe deleted the pr-msglist-avatar-live-update branch August 3, 2023 20:21

chrisbobbe mentioned this pull request Aug 3, 2023

urls [nfc]: Pass around Uri objects more (cut out some unneeded re-stringification) #247

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

msglist: Use [User.avatarUrl] instead of [Message.avatarUrl] #246

msglist: Use [User.avatarUrl] instead of [Message.avatarUrl] #246

chrisbobbe commented Aug 1, 2023

chrisbobbe Aug 1, 2023

gnprice Aug 2, 2023

chrisbobbe commented Aug 2, 2023 •

edited

Loading

chrisbobbe commented Aug 2, 2023

gnprice left a comment

gnprice Aug 2, 2023

gnprice Aug 2, 2023

gnprice Aug 2, 2023

gnprice Aug 2, 2023

gnprice Aug 2, 2023

gnprice Aug 3, 2023

chrisbobbe Aug 3, 2023

chrisbobbe Aug 3, 2023

gnprice Aug 2, 2023

gnprice Aug 3, 2023

gnprice Aug 3, 2023

gnprice Aug 3, 2023

chrisbobbe commented Aug 3, 2023

gnprice commented Aug 3, 2023

		import 'package:flutter_test/flutter_test.dart';


		class FakeHttpClient extends Fake implements HttpClient {

	import '../api/fake_api.dart' as fake_api;
	import '../api/fake_api.dart' show FakeApiConnection;

	check(findAvatarImageWidget(tester)).isNotNull().src.equals(resolveUrl(avatarUrl, eg.selfAccount));
	check(findAvatarImageWidget(tester)).isNotNull()
	.src.equals(resolveUrl(avatarUrl, eg.selfAccount));

		// TODO only vary [avatarUrl], not other fields
		checkResultForSender(eg.user(userId: eg.selfUser.userId, avatarUrl: '/foo.png'));

msglist: Use [User.avatarUrl] instead of [Message.avatarUrl] #246

msglist: Use [User.avatarUrl] instead of [Message.avatarUrl] #246

Conversation

chrisbobbe commented Aug 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Aug 2, 2023 • edited Loading

chrisbobbe commented Aug 2, 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

chrisbobbe commented Aug 3, 2023

gnprice commented Aug 3, 2023

chrisbobbe commented Aug 2, 2023 •

edited

Loading