@-mention autocomplete UI #207

chrisbobbe · 2023-06-28T21:47:39Z

(edited)

I'll follow up with specific issues this doesn't yet address, but here's a 1.0 of the @-mention autocomplete feature! 🎉

Fixes: #49
Fixes: #129

gnprice

Thanks for building this! The code all looks good, with small comments below.

As you mentioned in the office, the UI should get avatars too. (Though if that's complicated for some reason, it'd be fine to leave as a TODO.)

It'd be good to have some tests for this — doesn't need many, given the thorough unit tests on the several areas of underlying logic, but it'd be good to have a few to check that the UI still shows up and still does its thing when the user taps on it.

gnprice · 2023-06-28T22:15:24Z

lib/widgets/compose_box.dart

 class _ContentInputState extends State<_ContentInput> {
-  MentionAutocompleteView? _mentionAutocompleteView; // TODO different autocomplete view types


In this version, _ContentInputState no longer carries any state, and so could be folded back into _ContentInput as a stateless widget.

(Not sure whether that'll remain true in a future version.)

gnprice · 2023-06-28T22:25:19Z

lib/widgets/autocomplete.dart

+                shrinkWrap: true,
+                itemCount: _resultsToDisplay.length,
+                itemBuilder: (BuildContext context, int index) {
+                  final option = _resultsToDisplay.elementAt(index);


Suggested change

final option = _resultsToDisplay.elementAt(index);

final option = _resultsToDisplay[index];

For lists they're equivalent; but elementAt is present on Iterable, so seeing it makes it look like we're potentially working with a general Iterable (where it might be slow).

gnprice · 2023-06-28T22:26:59Z

lib/widgets/autocomplete.dart

+                    child: Builder(
+                      builder: (BuildContext context) {


Is this Builder doing something subtle I'm missing, or can it be simplified out?

Ah, nope, comes from copy-pasting from Flutter's Material Autocomplete then probably removing some stuff.

gnprice · 2023-06-28T22:29:20Z

lib/widgets/autocomplete.dart

+                        return Container(
+                          padding: const EdgeInsets.all(16.0),


nit:

Suggested change

return Container(

padding: const EdgeInsets.all(16.0),

return Padding(

padding: const EdgeInsets.all(16.0),

gnprice · 2023-06-28T22:29:54Z

lib/widgets/autocomplete.dart

+                      builder: (BuildContext context) {
+                        return Container(
+                          padding: const EdgeInsets.all(16.0),
+                          child: Text(label ?? 'unknown'));


nit:

Suggested change

child: Text(label ?? 'unknown'));

child: Text(label));

and String label; above

gnprice · 2023-06-28T22:31:24Z

lib/widgets/autocomplete.dart

+                padding: EdgeInsets.zero,
+                shrinkWrap: true,
+                itemCount: _resultsToDisplay.length,
+                itemBuilder: (BuildContext context, int index) {


This would probably be a good spot to split this code up 🙂, as it's a bit of a long widget-expression — so like itemBuilder: _buildItem.

chrisbobbe · 2023-07-08T01:06:47Z

Thanks for the review! Revision pushed, now with a commit that bumps the Flutter floor to get flutter/flutter#129802.

gnprice

Thanks! Generally this looks great; small comments below.

One commit-message nit:

autocomplete: Add basic UI for mention autocompletes

I'd say "@-mention autocompletes", or (a bit more specific) "user-mention autocompletes". I think you're intending "mention" here to mean an @-mention, a mention with a leading @; but I would call the stream-mention syntax #**general** a "mention" too, as well as topic mentions (like stream mentions but with > inside).

Let's also file follow-up issues for a couple of UX items:

Options should show the users' avatars.
Options should be sorted by name, rather than user ID.

(Or better yet something more relevant than name: some kind of relevance to search plus relevance to the context, like people subscribed to the stream or that have talked recently in the narrow. But sorting by user ID seems like it's just going to feel weird and arbitrary to people.)

gnprice · 2023-07-08T01:16:45Z

lib/api/route/messages.dart

+  /// An example empty result, for use in tests.
+  GetMessagesResult.empty() :


Let's keep this test data in the test code.

For now since it has just one call site, it can be a private helper in that test file. If we find ourselves needing it in a variety of test files, we'll have a better idea of the right way to generalize it after we see those.

gnprice · 2023-07-08T01:19:55Z

lib/api/route/messages.dart

+  GetMessagesResult.empty() :
+    // this `anchor` was observed empirically;
+    // also, it's not representative of all requests.
+    anchor = 10000000000000000,


formatting nit:

Suggested change

GetMessagesResult.empty() :

// this `anchor` was observed empirically;

// also, it's not representative of all requests.

anchor = 10000000000000000,

GetMessagesResult.empty()

: // this `anchor` was observed empirically;

// also, it's not representative of all requests.

anchor = 10000000000000000,

(Otherwise that line ending in a colon looks like it's some very different syntax — a label? a fragment of Python or YAML? I actually thought at first it was some other language, and had to reread the filename and the context.)

gnprice · 2023-07-08T01:26:55Z

test/widgets/autocomplete_test.dart

+  group('ComposeAutocomplete', () {
+    testWidgets('', (WidgetTester tester) async {


This empty name looks a bit odd.

If there's just one test in the group, and you don't feel there's an informative name to give it, probably better to just drop the group and say testWidgets('ComposeAutocomplete',.

Ah, I left that to come back to but forgot to come back to it :)

gnprice · 2023-07-08T01:32:49Z

test/widgets/autocomplete_test.dart

+      // BUG (RawAutocomplete): Options list can lag behind the
+      //   MentionAutocompleteView. RawAutocomplete updates the list with a
+      //   listener on the TextEditingValue, not on the MentionAutocompleteView.
+      //   So, this extra edit, which we should delete when the bug is fixed.
+      await tester.enterText(composeInputFinder, 'hello @user ');


Let's file an issue for this, so we have something this can link to (which in turn makes the reference here findable by grep). Can be a quick issue with not many more words than this comment; we can always expand it later.

Also let's stick to TODO, rather than sometimes writing BUG. A lot of TODO comments are bugs, and many others might be arguably bugs. If we had both, then when trying to find references to a given issue I don't think it'd be possible to be able to consistently predict which was used, and one would have to search for both.

gnprice · 2023-07-08T04:35:25Z

test/widgets/autocomplete_test.dart

+      await tester.enterText(composeInputFinder, 'hello @user t');
+      await tester.pumpAndSettle(); // async computation; options appear
+      check(tester.widgetList(find.text('User One'))).isEmpty();
+      tester.widget(find.text('User Two'));


Hmm, this line is awkward because it looks like it isn't doing anything, but it has an important point which is to check that this widget is present. (I know I've written some similar tester.widget lines.) I initially glossed over this line's checking role and figured it was part of setting up the next set of checks.

I think the typical flutter_test idiom would be expect(find.whatever(), findsOneWidget);. I guess we should find or write some comparable idiom for package:checks.

Anyway, for the moment, perhaps a quick comment to say this is checking that this option appears, or that these three lines are checking that there are these two options appearing.

Filed #232 so we don't lose track of this.

gnprice · 2023-07-08T04:36:34Z

test/widgets/autocomplete_test.dart

+      check(tester.widget<TextField>(composeInputFinder).controller!.text)
+        .contains(mention(user3, users: store.users));


Let's also check that the options have disappeared.

gnprice · 2023-07-10T18:15:09Z

lib/widgets/autocomplete.dart

+      _viewModel ??= (MentionAutocompleteView.init(store: store, narrow: widget.narrow)
+        ..addListener(_viewModelChanged));


Suggested change

_viewModel ??= (MentionAutocompleteView.init(store: store, narrow: widget.narrow)

..addListener(_viewModelChanged));

_viewModel ??= MentionAutocompleteView.init(store: store, narrow: widget.narrow)

..addListener(_viewModelChanged);

With that pair of parens, I was hoping to make it clear that the listener is only added once; that it doesn't behave like

(_viewModel ??= MentionAutocompleteView.init(store: store, narrow: widget.narrow)) ..addListener(_viewModelChanged);

which I think would add a new listener on every keystroke (!) when there's a newAutocompleteIntent.

I see.

I think the way to parse this statement as a reader is to treat ??= as if it were = — all the assignment operators have the same precedence (which in fact is the lowest, or loosest-binding, precedence of all operators). And then one routinely writes things like

foo = bar() ..baz() ..quux();

and those mean foo gets the value of that whole right-hand side. So that tells you that this works the same way.

chrisbobbe · 2023-07-19T20:55:51Z

Thanks for the review! Revision pushed.

Let's also file follow-up issues for a couple of UX items:

Options should show the users' avatars.

Options should be sorted by name, rather than user ID.

(Or better yet something more relevant than name: some kind of relevance to search plus relevance to the context, like people subscribed to the stream or that have talked recently in the narrow. But sorting by user ID seems like it's just going to feel weird and arbitrary to people.)

Filed:

Also:

autocomplete: Options list can persistently lag behind query #226

which I hope to resolve soon as a followup.

gnprice

Thanks for the revision! LGTM, with just nits remaining; see below and also #207 (comment) .

gnprice · 2023-07-19T23:50:19Z

test/widgets/autocomplete_test.dart

+      // TODO(#226): Remove one of these edits when this bug is fixed.
+      await tester.enterText(composeInputFinder, 'hello @user ');
+      await tester.enterText(composeInputFinder, 'hello @user t');
+


Are these lines intended? It doesn't seem like they're affecting the test.

I added these because I realized that the part that comes after wasn't doing quite what I'd intended:

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

I wanted to check that removing an AutocompleteIntent (e.g. by changing the text from 'hello @user two' to ' ') causes the options to disappear. If there were a bug and that didn't happen, then without the added lines, the test would have still passed, because the options had disappeared as a result of tapping one of them.

I'll add comments to try to clarify: there are two things that can cause the options to disappear, and we want to check them both:

Finishing the autocomplete by tapping an option

Removing the autocomplete intent (like by clearing out the input)

test/widgets/autocomplete_test.dart

chrisbobbe · 2023-07-20T19:21:43Z

Thanks for the review! Revision pushed.

Reading [Iterable.takeWhile] more closely than I did when writing this `mention` function in 6bd11b6, it turns out it's not what we want here: /// Creates a lazy iterable of the leading elements satisfying [test]. Note that it says "*leading* elements". [Iterable.where] also says it gives a lazy [Iterable], without the "leading" restriction. Use that, and change the test so it would fail if the bug were re-introduced.

This looks like something we just forgot to do.

It recently handed off its stateful logic to ComposeAutocomplete.

We're about to write a listener on a MentionAutocompleteView (the view-model), and we won't want that one to get confused with this.

This widget is all about managing autocompletes, so there isn't another kind of view model that this is likely to get confused with.

This *looks* like a likely-looking place where we might add this logic, but perhaps it belongs somewhere else? We'll take a closer look later.

The UI for now is kept simple; the optionsViewBuilder closely follows the one in the Material library's `Autocomplete`, which thinly wraps `RawAutocomplete`: https://api.flutter.dev/flutter/material/Autocomplete-class.html Fixes: zulip#49 Fixes: zulip#129

gnprice · 2023-07-20T22:48:45Z

Thanks! All looks good; merging.

chrisbobbe added the a-compose Compose box, autocomplete, attaching files/images label Jun 28, 2023

chrisbobbe requested a review from gnprice June 28, 2023 21:47

gnprice reviewed Jun 28, 2023

View reviewed changes

chrisbobbe force-pushed the pr-autocomplete-ui branch 4 times, most recently from deeba3d to 23df6ae Compare July 8, 2023 01:05

chrisbobbe marked this pull request as ready for review July 8, 2023 01:06

gnprice reviewed Jul 10, 2023

View reviewed changes

This was referenced Jul 19, 2023

autocomplete: Options list can persistently lag behind query #226

Open

autocomplete: Sort user-mention autocomplete results #228

Closed

chrisbobbe force-pushed the pr-autocomplete-ui branch from 23df6ae to 5605659 Compare July 19, 2023 20:54

gnprice approved these changes Jul 19, 2023

View reviewed changes

gnprice mentioned this pull request Jul 20, 2023

Find a package:checks analogue of flutter_test's widget-finder matchers #232

Closed

chrisbobbe force-pushed the pr-autocomplete-ui branch from 5605659 to 6e2f3e1 Compare July 20, 2023 19:21

chrisbobbe added 9 commits July 20, 2023 15:47

compose: Add missing dispose call for _mentionAutocompleteView

4b81b72

This looks like something we just forgot to do.

autocomplete [nfc]: Make MentionAutocompleteResult a sealed class

2c919c2

compose [nfc]: Factor out autocomplete view-model handling to new widget

1b808eb

compose [nfc]: Make _ContentInput a stateless widget

677d9df

It recently handed off its stateful logic to ComposeAutocomplete.

autocomplete [nfc]: Rename a private method to be more specific

de3255b

We're about to write a listener on a MentionAutocompleteView (the view-model), and we won't want that one to get confused with this.

autocomplete [nfc]: Shorten name of widget's MentionAutocompleteView

6a9139a

This widget is all about managing autocompletes, so there isn't another kind of view model that this is likely to get confused with.

autocomplete [nfc]: Add TODO about sorting results

a8be6bd

This *looks* like a likely-looking place where we might add this logic, but perhaps it belongs somewhere else? We'll take a closer look later.

gnprice force-pushed the pr-autocomplete-ui branch from 6e2f3e1 to 234b65d Compare July 20, 2023 22:48

gnprice merged commit 234b65d into zulip:main Jul 20, 2023

chrisbobbe deleted the pr-autocomplete-ui branch July 21, 2023 00:29

This was referenced Jul 21, 2023

autocomplete: Support user-group @-mentions #233

Open

autocomplete: Support wildcard @-mentions #234

Closed

gnprice mentioned this pull request Sep 18, 2024

Use flutter_test matchers where helpful, via flutter_checks #952

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

@-mention autocomplete UI #207

@-mention autocomplete UI #207

chrisbobbe commented Jun 28, 2023 •

edited

Loading

gnprice left a comment

gnprice Jun 28, 2023

gnprice Jun 28, 2023

gnprice Jun 28, 2023

chrisbobbe Jun 28, 2023 •

edited

Loading

gnprice Jun 28, 2023

gnprice Jun 28, 2023

gnprice Jun 28, 2023

chrisbobbe commented Jul 8, 2023

gnprice left a comment

gnprice Jul 8, 2023

gnprice Jul 8, 2023

gnprice Jul 8, 2023

chrisbobbe Jul 19, 2023

gnprice Jul 8, 2023

chrisbobbe Jul 19, 2023

gnprice Jul 8, 2023

gnprice Jul 20, 2023

gnprice Jul 8, 2023

gnprice Jul 10, 2023

chrisbobbe Jul 19, 2023 •

edited

Loading

gnprice Jul 19, 2023

chrisbobbe commented Jul 19, 2023

gnprice left a comment

gnprice Jul 19, 2023

chrisbobbe Jul 20, 2023

chrisbobbe commented Jul 20, 2023

gnprice commented Jul 20, 2023

		class _ContentInputState extends State<_ContentInput> {
		MentionAutocompleteView? _mentionAutocompleteView; // TODO different autocomplete view types

	final option = _resultsToDisplay.elementAt(index);
	final option = _resultsToDisplay[index];

		/// An example empty result, for use in tests.
		GetMessagesResult.empty() :

		group('ComposeAutocomplete', () {
		testWidgets('', (WidgetTester tester) async {

		check(tester.widget<TextField>(composeInputFinder).controller!.text)
		.contains(mention(user3, users: store.users));

		_viewModel ??= (MentionAutocompleteView.init(store: store, narrow: widget.narrow)
		..addListener(_viewModelChanged));

@-mention autocomplete UI #207

@-mention autocomplete UI #207

Conversation

chrisbobbe commented Jun 28, 2023 • edited Loading

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

chrisbobbe Jun 28, 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

chrisbobbe commented Jul 8, 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

chrisbobbe Jul 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Jul 19, 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

chrisbobbe commented Jul 20, 2023

gnprice commented Jul 20, 2023

chrisbobbe commented Jun 28, 2023 •

edited

Loading

chrisbobbe Jun 28, 2023 •

edited

Loading

chrisbobbe Jul 19, 2023 •

edited

Loading