Skip to content

autocomplete: Add user avatars to user-mention autocompletes #590

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 27, 2024
Merged

autocomplete: Add user avatars to user-mention autocompletes #590

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ec07043
autocomplete [nfc]: Delete wildcard and user-group result subclasses
sm-sayedi Mar 25, 2024
4ab1a82
content test [nfc]: Pull `prepareBoringImageHttpClient` out to toplevel
sm-sayedi Mar 26, 2024
919cf12
autocomplete: Add user avatars to user-mention autocompletes
sm-sayedi Mar 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 2 additions & 17 deletions lib/model/autocomplete.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,21 +342,6 @@ class UserMentionAutocompleteResult extends MentionAutocompleteResult {
final int userId;
}

enum WildcardMentionType {
all,
everyone,
stream,
}

class WildcardMentionAutocompleteResult extends MentionAutocompleteResult {
WildcardMentionAutocompleteResult({required this.type});

final WildcardMentionType type;
}
// TODO(#233): // class UserGroupMentionAutocompleteResult extends MentionAutocompleteResult {


class UserGroupMentionAutocompleteResult extends MentionAutocompleteResult {
UserGroupMentionAutocompleteResult({required this.userGroupId});

final int userGroupId;
}
// TODO(#234): // class WildcardMentionAutocompleteResult extends MentionAutocompleteResult {
21 changes: 10 additions & 11 deletions lib/widgets/autocomplete.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import 'package:flutter/material.dart';

import 'content.dart';
import 'store.dart';
import '../model/autocomplete.dart';
import '../model/compose.dart';
Expand Down Expand Up @@ -107,10 +108,6 @@ class _ComposeAutocompleteState extends State<ComposeAutocomplete> with PerAccou
// TODO(i18n) language-appropriate space character; check active keyboard?
// (maybe handle centrally in `widget.controller`)
replacementString = '${mention(store.users[userId]!, silent: intent.query.silent, users: store.users)} ';
case WildcardMentionAutocompleteResult():
replacementString = '[unimplemented]'; // TODO(#234)
case UserGroupMentionAutocompleteResult():
replacementString = '[unimplemented]'; // TODO(#233)
}

widget.controller.value = intent.textEditingValue.replaced(
Expand All @@ -123,23 +120,25 @@ class _ComposeAutocompleteState extends State<ComposeAutocomplete> with PerAccou

Widget _buildItem(BuildContext _, int index) {
final option = _resultsToDisplay[index];
Widget avatar;
String label;
switch (option) {
case UserMentionAutocompleteResult(:var userId):
// TODO(#227) avatar
avatar = Avatar(userId: userId, size: 32, borderRadius: 3);
label = PerAccountStoreWidget.of(context).users[userId]!.fullName;
case WildcardMentionAutocompleteResult():
label = '[unimplemented]'; // TODO(#234)
case UserGroupMentionAutocompleteResult():
label = '[unimplemented]'; // TODO(#233)
}
return InkWell(
onTap: () {
_onTapOption(option);
},
child: Padding(
padding: const EdgeInsets.all(16.0),
child: Text(label)));
padding: const EdgeInsets.symmetric(horizontal: 16.0, vertical: 8.0),
child: Row(
children: [
avatar,
const SizedBox(width: 8),
Text(label),
])));
}

@override
Expand Down
54 changes: 41 additions & 13 deletions test/widgets/autocomplete_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,25 @@ import 'package:zulip/api/model/model.dart';
import 'package:zulip/api/route/messages.dart';
import 'package:zulip/model/compose.dart';
import 'package:zulip/model/narrow.dart';
import 'package:zulip/model/store.dart';
import 'package:zulip/widgets/message_list.dart';
import 'package:zulip/widgets/store.dart';

import '../api/fake_api.dart';
import '../example_data.dart' as eg;
import '../model/binding.dart';
import '../model/test_store.dart';
import 'content_test.dart';

/// Simulates loading a [MessageListPage] and tapping to focus the compose input.
///
/// Also adds [users] to the [PerAccountStore],
/// so they can show up in autocomplete.
///
/// Also sets [debugNetworkImageHttpClientProvider] to return a constant image.
///
/// The caller must set [debugNetworkImageHttpClientProvider] back to null
/// before the end of the test.
Future<Finder> setupToComposeInput(WidgetTester tester, {
required List<User> users,
}) async {
Expand All @@ -39,6 +46,8 @@ Future<Finder> setupToComposeInput(WidgetTester tester, {
messages: [message],
).toJson());

prepareBoringImageHttpClient();
Copy link
Member

Choose a reason for hiding this comment

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

With this call, it becomes part of the interface of this setupToComposeInput function that its caller needs to set debugNetworkImageHttpClientProvider back to null before the end of the test. So this function should gain a note to that effect in its dartdoc, just like prepareBoringImageHttpClient has.

(It's regrettable that that reset can't be done using addTearDown, so that callers and callers' callers wouldn't have to care like this. There's an open issue upstream about that, relating to debugNetworkImageHttpClientProvider and a bunch of other Flutter test knobs of a similar flavor. I don't have the link offhand, but it'd be good to track it down again and then put a comment on prepareBoringImageHttpClient pointing to that.)

Copy link
Collaborator Author

@sm-sayedi sm-sayedi Mar 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I found a closed issue upstream. This one also seemed closer to this, but wasn't sure to add it to the prepareBoringImageHttpClient comment.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for looking! Found the one I was thinking of (which it turns out is one I filed 🙂), and added a comment: 68116a2

Looks like there was a backlink to it in the second issue you found:
image
as that issue is basically a different specific case of the issue I had in mind.


await tester.pumpWidget(
MaterialApp(
localizationsDelegates: ZulipLocalizations.localizationsDelegates,
Expand All @@ -65,10 +74,26 @@ void main() {
TestZulipBinding.ensureInitialized();

group('ComposeAutocomplete', () {

Finder findNetworkImage(String url) {
return find.byWidgetPredicate((widget) => switch(widget) {
Image(image: NetworkImage(url: var imageUrl)) when imageUrl == url
=> true,
_ => false,
});
}

void checkUserShown(User user, PerAccountStore store, {required bool expected}) {
check(find.text(user.fullName).evaluate().length).equals(expected ? 1 : 0);
final avatarFinder =
findNetworkImage(store.tryResolveUrl(user.avatarUrl!).toString());
check(avatarFinder.evaluate().length).equals(expected ? 1 : 0);
}

testWidgets('options appear, disappear, and change correctly', (WidgetTester tester) async {
final user1 = eg.user(userId: 1, fullName: 'User One');
final user2 = eg.user(userId: 2, fullName: 'User Two');
final user3 = eg.user(userId: 3, fullName: 'User Three');
final user1 = eg.user(userId: 1, fullName: 'User One', avatarUrl: 'user1.png');
final user2 = eg.user(userId: 2, fullName: 'User Two', avatarUrl: 'user2.png');
final user3 = eg.user(userId: 3, fullName: 'User Three', avatarUrl: 'user3.png');
final composeInputFinder = await setupToComposeInput(tester, users: [user1, user2, user3]);
final store = await testBinding.globalStore.perAccount(eg.selfAccount.id);

Expand All @@ -77,34 +102,37 @@ void main() {
await tester.enterText(composeInputFinder, 'hello @user ');
await tester.enterText(composeInputFinder, 'hello @user t');
await tester.pumpAndSettle(); // async computation; options appear

// "User Two" and "User Three" appear, but not "User One"
check(tester.widgetList(find.text('User One'))).isEmpty();
tester.widget(find.text('User Two'));
tester.widget(find.text('User Three'));
checkUserShown(user1, store, expected: false);
Copy link
Member

Choose a reason for hiding this comment

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

This version is fine. But for other situations where the shared context is more verbose than just store, and so it's more of a pain to have to pass it each time, there are two solutions we often use:

  • The helper function can be local within the test function, so that it sees the same store as a local.
  • The context like store can be a late variable that's declared up at the top of a test group, or of the whole test main, and that gets initialized afresh by a helper function that each test case calls. Those helpers are often named prepare or of the form prepareFoo.

checkUserShown(user2, store, expected: true);
checkUserShown(user3, store, expected: true);

// Finishing autocomplete updates compose box; causes options to disappear
await tester.tap(find.text('User Three'));
await tester.pump();
check(tester.widget<TextField>(composeInputFinder).controller!.text)
.contains(mention(user3, users: store.users));
check(tester.widgetList(find.text('User One'))).isEmpty();
check(tester.widgetList(find.text('User Two'))).isEmpty();
check(tester.widgetList(find.text('User Three'))).isEmpty();
checkUserShown(user1, store, expected: false);
checkUserShown(user2, store, expected: false);
checkUserShown(user3, store, expected: false);

// Then a new autocomplete intent brings up options again
// TODO(#226): Remove this extra edit when this bug is fixed.
await tester.enterText(composeInputFinder, 'hello @user tw');
await tester.enterText(composeInputFinder, 'hello @user two');
await tester.pumpAndSettle(); // async computation; options appear
tester.widget(find.text('User Two'));
checkUserShown(user2, store, expected: true);

// Removing autocomplete intent causes options to disappear
// TODO(#226): Remove one of these edits when this bug is fixed.
await tester.enterText(composeInputFinder, '');
await tester.enterText(composeInputFinder, ' ');
check(tester.widgetList(find.text('User One'))).isEmpty();
check(tester.widgetList(find.text('User Two'))).isEmpty();
check(tester.widgetList(find.text('User Three'))).isEmpty();
checkUserShown(user1, store, expected: false);
checkUserShown(user2, store, expected: false);
checkUserShown(user3, store, expected: false);

debugNetworkImageHttpClientProvider = null;
});
});
}
30 changes: 15 additions & 15 deletions test/widgets/content_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,21 @@ import 'dialog_checks.dart';
import 'message_list_checks.dart';
import 'page_checks.dart';

/// Set [debugNetworkImageHttpClientProvider] to return a constant image.
///
/// Returns the [FakeImageHttpClient] that handles the requests.
///
/// The caller must set [debugNetworkImageHttpClientProvider] back to null
/// before the end of the test.
FakeImageHttpClient prepareBoringImageHttpClient() {
Copy link
Member

Choose a reason for hiding this comment

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

nit: prefix for this commit:

f3aebc9 autocomplete [nfc]: Pull prepareBoringImageHttpClient out to toplevel

should be autocomplete test [nfc]:, to indicate that it's only touching the test code

(And then the "nfc" part means that on top of that, it has only an NFC effect on even the test code.)

Copy link
Member

Choose a reason for hiding this comment

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

Oh and looking again, it's not really about the autocomplete tests in particular, so autocomplete isn't quite right.

I'll make it content test [nfc], since it is in that file. Could also have made it just test [nfc], since the point is it's now exporting this for other tests.

final httpClient = FakeImageHttpClient();
debugNetworkImageHttpClientProvider = () => httpClient;
httpClient.request.response
..statusCode = HttpStatus.ok
..content = kSolidBlueAvatar;
return httpClient;
}

void main() {
// For testing a new content feature:
//
Expand Down Expand Up @@ -64,21 +79,6 @@ void main() {
});
}

/// Set [debugNetworkImageHttpClientProvider] to return a constant image.
///
/// Returns the [FakeImageHttpClient] that handles the requests.
///
/// The caller must set [debugNetworkImageHttpClientProvider] back to null
/// before the end of the test.
FakeImageHttpClient prepareBoringImageHttpClient() {
final httpClient = FakeImageHttpClient();
debugNetworkImageHttpClientProvider = () => httpClient;
httpClient.request.response
..statusCode = HttpStatus.ok
..content = kSolidBlueAvatar;
return httpClient;
}

group('Heading', () {
testWidgets('plain h6', (tester) async {
await prepareContentBare(tester,
Expand Down