Skip to content

autocomplete: Add and test MentionAutocompleteView #102

New issue

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

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

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
May 25, 2023

Conversation

chrisbobbe
Copy link
Collaborator

@chrisbobbe chrisbobbe commented May 20, 2023

Thanks @gnprice for demonstrating tests for this tricky async logic (and also guiding its implementation)! I've added some more tests in this revision:

test('MentionAutocompleteView yield between batches of 1000', () async {
test('MentionAutocompleteView new query during computation replaces old', () async {
test('MentionAutocompleteView mutating store.users while in progress causes retry', () async {

and kept the "DEV DEMO" commit from my earlier draft in case that's convenient for more manual testing. (Marking as a draft just because of that commit.)

@chrisbobbe chrisbobbe requested a review from gnprice May 20, 2023 00:48
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

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

Thanks! Small comments below, mostly about the tests.

I think after those are resolved, it'd make sense to merge this (sans the demo commit). Then probably next is to build a UI for it, and that'll be enough for #49.

We can then file a followup issue for coming back to support wildcards and user groups, as well as other ways of matching queries to users or ranking matches.

Comment on lines 216 to 220
final Map<int, List<String>> _nameWordsByUser = {};
List<String> nameWordsForUser(User user) {
return _nameWordsByUser[user.userId] ??= user.fullName.toLowerCase().split(' ');
}
invalidateUser(int userId) {
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
final Map<int, List<String>> _nameWordsByUser = {};
List<String> nameWordsForUser(User user) {
return _nameWordsByUser[user.userId] ??= user.fullName.toLowerCase().split(' ');
}
invalidateUser(int userId) {
final Map<int, List<String>> _nameWordsByUser = {};
List<String> nameWordsForUser(User user) {
return _nameWordsByUser[user.userId] ??= user.fullName.toLowerCase().split(' ');
}
invalidateUser(int userId) {

Comment on lines 220 to 223
invalidateUser(int userId) {
_nameWordsByUser.remove(userId);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
invalidateUser(int userId) {
_nameWordsByUser.remove(userId);
void invalidateUser(int userId) {
_nameWordsByUser.remove(userId);

.equals(eg.thirdUser.userId);
});

test('MentionAutocompleteView not starve timers', () {
Copy link
Member

Choose a reason for hiding this comment

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

Co-authored-by: Greg Price <[email protected]>

should be greg@, not gnprice@ :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, haha yeah that's right! I've sometimes wondered whether it would have been neater if I took chris@, especially if I was the first to break the pattern of using just a first name.

check(done).isTrue();
check(view.results).single
.isA<UserMentionAutocompleteResult>()
.has((r) => r.userId, 'userId').equals(2999);
Copy link
Member

Choose a reason for hiding this comment

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

should pull these .has calls out into a little extension, similar to #94 (comment)

Here, as a bonus that will dedupe them.

test('MentionAutocompleteView yield between batches of 1000', () async {
const narrow = AllMessagesNarrow();
final store = eg.store();
for (int i = 0; i < 3000; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's make this a less round number, like 2500. That will make it easier to reason through why the number of yields below is what it is, because it will avoid any fencepost questions — questions like whether another yield becomes involved when the number of users reaches 3000, or when it exceeds 3000.

(The answer turns out to be "reaches" — so if this becomes "< 2999", then we need one fewer yield below.)

test('MentionAutocompleteView new query during computation replaces old', () async {
const narrow = AllMessagesNarrow();
final store = eg.store();
for (int i = 0; i < 2000; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

similarly, make this 1500 or 2500

@chrisbobbe chrisbobbe force-pushed the pr-mention-autocomplete-view branch from 368f3fb to f8d95ff Compare May 24, 2023 22:35
@chrisbobbe
Copy link
Collaborator Author

Thanks for the review! Revision pushed.

@chrisbobbe chrisbobbe marked this pull request as ready for review May 24, 2023 23:15
@gnprice
Copy link
Member

gnprice commented May 25, 2023

Thanks! Looks good; merging.

@gnprice gnprice force-pushed the pr-mention-autocomplete-view branch from f8d95ff to d21886a Compare May 25, 2023 02:44
@gnprice gnprice merged commit d21886a into zulip:main May 25, 2023
import 'package:checks/checks.dart';
import 'package:zulip/model/autocomplete.dart';

extension UserMentionAutocompleteResultChecks on Subject<UserMentionAutocompleteResult> {
Copy link
Member

Choose a reason for hiding this comment

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

(FWIW I'd be inclined to put this one inside the individual test file, down at the end after main, because it seems likely we'll only want it within the one file and because it'd be easy to move later if we want it in several files after all. But in a separate file is fine too.)

@chrisbobbe chrisbobbe deleted the pr-mention-autocomplete-view branch May 25, 2023 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants