autocomplete: Add code to recognize @-mention syntax in the content input #154

chrisbobbe · 2023-06-07T01:58:42Z

At the tip of this branch is a DEV DEMO commit that lets you see mention-autocomplete results printed to the console as you type! 🎉 (The actual UI to select a result will come later; there's some complexity there as described in #49).

I'm particularly unsure about the amount of computation; whether it's getting to be too much for this performance-sensitive area. I've tried to keep it reasonably low but would be glad to know if there's a problem there. 🙂

Related: #49
Related: #129

gnprice

Thanks! Comments below. I quite appreciate the many test cases and the nice compact syntax to enable them, and the comments explaining the choices in the regexp.

To your question about the amount of computation, see comment below about checking up front for @.

gnprice · 2023-06-07T02:16:27Z

lib/model/autocomplete.dart

@@ -1,10 +1,105 @@
+import 'package:flutter/cupertino.dart';


nit:

Suggested change

import 'package:flutter/cupertino.dart';

import 'package:flutter/services.dart';

Cupertino is a widget library, so we shouldn't need to import it here.

(I found the more-specific import by commenting out this line, going to the error, then uncommenting and jumping to that identifier's definition. It was under packages/flutter/lib/src/services/, so it'll be exported by packages/flutter/lib/services.dart.)

gnprice · 2023-06-07T02:58:02Z

test/model/autocomplete_test.dart


 import '../example_data.dart' as eg;
 import 'test_store.dart';
 import 'autocomplete_checks.dart';

 void main() {
+  group('ContentTextEditingController.autocompleteIntent', () {


Suggested change

group('ContentTextEditingController.autocompleteIntent', () {

test('ContentTextEditingController.autocompleteIntent', () {

Or better yet, add some test calls grouping the doCheck calls below (and so that every doCheck is enclosed in a test call.)

Otherwise, if you alter the app code so that one of the tests fail, you get error output like this:

$ flutter test 00:04 +38 -1: loading /home/greg/z/flutterz/test/model/autocomplete_test.dart [E] Failed to load "/home/greg/z/flutterz/test/model/autocomplete_test.dart": Expected: a ContentTextEditingController that: has autocompleteIntent Actual: <ContentTextEditingController#80c8d(TextEditingValue(text: ┤hello @chris├, selection: TextSelection(baseOffset: 0, extentOffset: 12, isDirectional: false), composing: TextRange(start: -1, end: -1)))> Which: threw while trying to read autocompleteIntent: <'package:zulip/model/autocomplete.dart': Failed assertion: line 30 pos 16: 'syntaxStartIndex == position': is not true.> #0 _AssertionError._doThrowNew (dart:core-patch/errors_patch.dart:51:61) #1 _AssertionError._throwNew (dart:core-patch/errors_patch.dart:40:5) #2 Autocomplete.autocompleteIntent (package:zulip/model/autocomplete.dart:30:16) … #29 Timer._createTimer.<anonymous closure> (dart:async-patch/timer_patch.dart:18:15) #30 _Timer._runTimers (dart:isolate-patch/timer_impl.dart:398:19) #31 _Timer._handleMessage (dart:isolate-patch/timer_impl.dart:429:5) #32 _RawReceivePort._handleMessage (dart:isolate-patch/isolate_patch.dart:189:12)

rather than an error that's inside an identifiable test, like:

$ flutter test 00:03 +38 -1: /home/greg/z/flutterz/test/model/autocomplete_test.dart: ContentTextEditingController.autocompleteIntent [E] Expected: a ContentTextEditingController that: has autocompleteIntent Actual: <ContentTextEditingController#b6667(TextEditingValue(text: ┤hello @chris├, selection: TextSelection(baseOffset: 0, extentOffset: 12, isDirectional: false), composing: TextRange(start: -1, end: -1)))> Which: threw while trying to read autocompleteIntent: <'package:zulip/model/autocomplete.dart': Failed assertion: line 30 pos 16: 'syntaxStartIndex == position': is not true.> #0 _AssertionError._doThrowNew (dart:core-patch/errors_patch.dart:51:61) #1 _AssertionError._throwNew (dart:core-patch/errors_patch.dart:40:5) #2 Autocomplete.autocompleteIntent (package:zulip/model/autocomplete.dart:30:16) … #11 main.<anonymous closure> (file:///home/greg/z/flutterz/test/model/autocomplete_test.dart:123:5) #12 Declarer.test.<anonymous closure>.<anonymous closure> (package:test_api/src/backend/declarer.dart:215:19) <asynchronous suspension> #13 StackZoneSpecification._registerUnaryCallback.<anonymous closure> (package:stack_trace/src/stack_zone_specification.dart:124:15)

(As I understand it, this is superseded by your proposal at #154 (comment), which I'll implement in my next revision.)

gnprice · 2023-06-07T03:08:03Z

test/model/autocomplete_test.dart

+        expectedSyntaxStartIndex: expectedSyntaxStartIndex);
+    }
+
+    final controller = ContentTextEditingController();


Hmm, ideally this wouldn't be happening outside a test callback either, as it's invoking basically some of the code under test.

Certainly I appreciate the compactness of not having to pass the controller as an argument to doCheck in each of the test cases, though.

gnprice · 2023-06-07T03:18:39Z

test/model/autocomplete_test.dart

+    doCheck(String markedText, MentionAutocompleteQuery? expectedQuery) {
+      final parsed = parseMarkedText(markedText);


Here's another way to organize these test cases, then:

Suggested change

doCheck(String markedText, MentionAutocompleteQuery? expectedQuery) {

final parsed = parseMarkedText(markedText);

doTest(String markedText, MentionAutocompleteQuery? expectedQuery) {

final description = expectedQuery != null

? 'in ${jsonEncode(markedText)}, query ${jsonEncode(expectedQuery.raw)}'

: 'no query in ${jsonEncode(markedText)}';

test(description, () {

final controller = ContentTextEditingController();

final parsed = parseMarkedText(markedText);

I.e.,

have this helper function wrap all the work inside a test call;

rename it to doTest to match;

also move the controller initialization inside the test call.

(And then keep group at the outer level, rather than test. Optionally also put some group calls enclosing groups of related doTest calls below, but only where the descriptions add clarity.)

Probably the biggest thing that gets us is that if you then make a change that breaks some tests, you get a nice list of all the tests that are affected, not just the first one. That can be quite helpful for spotting patterns so as to more quickly diagnose the issue. (Can be extra helpful with flutter test -r expanded, so that you also see listed all the tests that passed, the better to spot patterns in the contrast.)

As a bonus, having a separate ContentTextEditingController for each test case is helpful for guaranteeing that there's no lingering state from one test case affecting the next one. State leakage is a nemesis of test suites, one that can in some places cause puzzling test failures, and then in other places cause spurious test successes that mask real failures in what the test was trying to test.

Awesome, thanks! 😅

gnprice · 2023-06-07T03:22:18Z

lib/model/autocomplete.dart

+  // whitespace, or punctuation. Letters are unlikely; in that case an email
+  // might be intended. (By punctuation, we mean *some* punctuation, like "(".
+  // We could refine this.)
+  const beforeAtSign = r'(?:^|\s|\p{Punctuation})';


Here's a fun regexp feature which can simplify how we use this regexp:

Suggested change

const beforeAtSign = r'(?:^|\s|\p{Punctuation})';

const beforeAtSign = r'(?<=^|\s|\p{Punctuation})';

And happily the JS regexp language, which Dart adopts, does have that feature:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Regular_expressions/Lookbehind_assertion

The effect of using that is that the start point of the match will now always be the @ sign: syntaxStartIndex above will always just equal position.

Cool! Thanks!

gnprice · 2023-06-07T03:40:57Z

lib/model/autocomplete.dart

+      position >= 0 && (selection.end - position <= 30);
+      position--
+    ) {
+      final match = mentionAutocompleteMarkerRegex.matchAsPrefix(textUntilCursor, position);


One thing that I think would help for letting us rest easy that this won't have too much performance impact on normal non-autocomplete editing is to take advantage of the fact that the syntax always involves an @, and use a simple check for that to gate getting into the more complex logic in the regexp.

This becomes easier to do after using lookbehind (?<=…) in the regexp, as in my comment just previous, so that position where we match the regexp is also where the @ sign should appear. Then we can say:

Suggested change

final match = mentionAutocompleteMarkerRegex.matchAsPrefix(textUntilCursor, position);

if (textUntilCursor[position] != '@') {

continue;

}

final match = mentionAutocompleteMarkerRegex.matchAsPrefix(textUntilCursor, position);

(The Dart VM's regexp implementation seems to be fairly clever, and it's quite possible that this optimization doesn't do anything useful — that the easy fast check for the literal @ is the first thing the compiled regexp would do anyway. In particular if there weren't that beforeAtSign part there, and the regexp just straightforwardly started with a literal @, then I'd definitely expect this optimization not to add any value. But it seems quite possible that with the beforeAtSign lookbehind coming first, it'd go check those conditions first before looking for the @.)

gnprice · 2023-06-07T03:50:53Z

test/model/autocomplete_test.dart

+    doCheck('@"^', null);      doCheck('@_"^', null);
+    doCheck('@\n^', null);     doCheck('@_\n^', null); // control character
+    doCheck('@\u0000^', null); doCheck('@_\u0000^', null); // control
+    doCheck('@\u061C^', null); doCheck('@_\u061C^', null); // control


This one seems to be format, not control:

$ unicode 061c --long U+061C ARABIC LETTER MARK UTF-8: d8 9c UTF-16BE: 061c Decimal: ؜ Octal: \03034 Category: Cf (Other, Format); East Asian width: N (neutral) Unicode block: 0600..06FF; Arabic Bidi: AL (Right-to-Left Arabic)

(The outcome is the same, though.)

gnprice · 2023-06-07T03:53:51Z

test/model/autocomplete_test.dart

+    doCheck('[email protected]^', null);
+    doCheck('email support@ with details of the issue^', null);
+    doCheck('email support@^ with details of the issue', null);


I like this compact syntax for expressing test cases!

gnprice · 2023-06-07T03:54:52Z

lib/model/autocomplete.dart

+  // [syntaxStartIndex], then the safe behavior would be accomplished more
+  // naturally, I think. But [TextEditingController] doesn't support subclasses
+  // that use a custom/subclassed [TextEditingValue], so that's not convenient.
+  final int syntaxStartIndex;


Perhaps just syntaxStart. That makes it analogous to the TextRange.start we work with on selections, and I think it basically carries all the relevant information.

gnprice · 2023-06-07T04:14:02Z

lib/widgets/compose_box.dart

+        _mentionAutocompleteView = MentionAutocompleteView.init(
+          store: widget.store, narrow: widget.narrow);


compose [nfc]: Have _StreamComposeBoxState provide store to children In particular, _StreamContentInput, so it can initialize [MentionAutocompleteView]s soon. But might as well pass it along to various other children so they don't have to get it themselves.

Hmm, this doesn't feel idiomatic to me. I think the usual thing would be to just have each widget get the store for itself, given that we've already put it on an InheritedWidget.

At this spot in particular, a key fact is that State has a context getter of its own. So we can write:

Suggested change

_mentionAutocompleteView = MentionAutocompleteView.init(

store: widget.store, narrow: widget.narrow);

final store = PerAccountStoreWidget.of(context);

_mentionAutocompleteView = MentionAutocompleteView.init(

store: store, narrow: widget.narrow);

chrisbobbe · 2023-06-07T20:20:13Z

Thanks for the review! Revision pushed. I've added one more commit here:

0adf9878c autocomplete [nfc]: Add `silent` to MentionAutocompleteQuery

gnprice

Looking good! Small comments below.

gnprice · 2023-06-07T22:35:50Z

test/model/autocomplete_test.dart

+    doTest('^@abc', null);             doTest('^@_abc', null);
+    doTest('@abc', null);              doTest('@_abc', null); // (no cursor)
+
+    doTest('@ ^', null);      // doCheck('@_ ^', null); // (would fail, but OK… technically "_" could start a word in full_name)


nit:

Suggested change

doTest('@ ^', null); // doCheck('@_ ^', null); // (would fail, but OK… technically "_" could start a word in full_name)

doTest('@ ^', null); // doTest('@_ ^', null); // (would fail, but OK… technically "_" could start a word in full_name)

gnprice · 2023-06-07T22:36:01Z

lib/model/autocomplete.dart

+      // We don't require [isCollapsed] to be true because we've seen that
+      // autocorrect and even backspace involve programmatically expanding the
+      // selection to the left. Once we know where the syntax starts, we can at
+      // least require that the selection doesn't extend leftword past that;


nit:

Suggested change

// least require that the selection doesn't extend leftword past that;

// least require that the selection doesn't extend leftward past that;

gnprice · 2023-06-07T22:52:50Z

lib/model/autocomplete.dart

+      if (selection.start < position) {
+        // See comment about [TextSelection.isCollapsed] above.
+        return null;
+      }


I think this part actually still belongs after the regexp match succeeds.

Otherwise we get a peculiar behavior where if you're in a situation like (in the tests' notation)
~@[email protected]^
and you start extending the selection left, the autocomplete state remains unchanged for a while:
~@someone@^example.com^
but then once you get past the @ that was in the middle of the query:
@someone^@example.com^
@^[email protected]^
the autocomplete goes away, even though you've still only selected text that was within the query.

Ah, right! Thanks for the catch; I'll fix this and add test cases from those helpful examples you gave.

chrisbobbe · 2023-06-07T23:46:03Z

Thanks for the review! Revision pushed, and this time I've chopped off the DEV DEMO commit and am un-marking the PR as a draft. In case you still want to use that demo, here's the diff:

diff --git lib/widgets/compose_box.dart lib/widgets/compose_box.dart
index 637cc777d..e6337faa9 100644
--- lib/widgets/compose_box.dart
+++ lib/widgets/compose_box.dart
@@ -173,6 +173,8 @@ class _StreamContentInput extends StatefulWidget {
 class _StreamContentInputState extends State<_StreamContentInput> {
   MentionAutocompleteView? _mentionAutocompleteView; // TODO different autocomplete view types
 
+  Iterable<MentionAutocompleteResult>? _devDemoResults;
 lib/widgets/compose_box.dart | 20 +++++++++++++++++---
 lib/widgets/compose_box.dart | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)


diff --git lib/widgets/compose_box.dart lib/widgets/compose_box.dart
index 637cc777d..e6337faa9 100644
--- lib/widgets/compose_box.dart
+++ lib/widgets/compose_box.dart
@@ -173,6 +173,8 @@ class _StreamContentInput extends StatefulWidget {
 class _StreamContentInputState extends State<_StreamContentInput> {
   MentionAutocompleteView? _mentionAutocompleteView; // TODO different autocomplete view types
 
+  Iterable<MentionAutocompleteResult>? _devDemoResults;
+
   late String _topicTextNormalized;
 
   _topicChanged() {
@@ -184,9 +186,20 @@ class _StreamContentInputState extends State<_StreamContentInput> {
   _changed() {
     final newAutocompleteIntent = widget.controller.autocompleteIntent();
     if (newAutocompleteIntent != null) {
-      final store = PerAccountStoreWidget.of(context);
-      _mentionAutocompleteView ??= MentionAutocompleteView.init(
-        store: store, narrow: widget.narrow);
+      if (_mentionAutocompleteView == null) {
+        final store = PerAccountStoreWidget.of(context);
+        _mentionAutocompleteView = MentionAutocompleteView.init(
+          store: store, narrow: widget.narrow);
+        _mentionAutocompleteView!.addListener(() {
+          if (_mentionAutocompleteView == null) {
+            _devDemoResults = [];
+            return;
+          }
+          setState(() {
+            _devDemoResults = _mentionAutocompleteView!.results;
+          });
+        });
+      }
       _mentionAutocompleteView!.query = newAutocompleteIntent.query;
     } else {
       if (_mentionAutocompleteView != null) {
@@ -214,6 +227,7 @@ class _StreamContentInputState extends State<_StreamContentInput> {
   @override
   Widget build(BuildContext context) {
     final store = PerAccountStoreWidget.of(context);
+    print('autocomplete suggestions (max 5 shown): ${_devDemoResults?.take(5).map((r) => (r as UserMentionAutocompleteResult).userId)}');
     final streamName = store.streams[widget.streamId]?.name ?? '(unknown stream)';
 
     ColorScheme colorScheme = Theme.of(context).colorScheme;

gnprice

LGTM!

Looks like it's still marked as draft. If you're happy with it, feel free to merge.

chrisbobbe · 2023-06-08T00:33:20Z

Ah oops! Thanks; merging.

For now, only to handle mention autocomplete syntax; others, including stream/topic and insert-a-time, will come later.

…ntent

As it does in the web app, we'll want this bit to control the suggestion filtering: if the user wants a silent mention, then it won't make sense to offer wildcard mentions in the list (@ALL, @everyone, etc.).

chrisbobbe requested a review from gnprice June 7, 2023 01:58

chrisbobbe force-pushed the pr-recognize-mention-syntax branch from 9724681 to cbb1b7d Compare June 7, 2023 02:04

gnprice reviewed Jun 7, 2023

View reviewed changes

chrisbobbe force-pushed the pr-recognize-mention-syntax branch from cbb1b7d to c392ec9 Compare June 7, 2023 20:19

gnprice reviewed Jun 7, 2023

View reviewed changes

chrisbobbe force-pushed the pr-recognize-mention-syntax branch from c392ec9 to b0d2b76 Compare June 7, 2023 23:45

gnprice approved these changes Jun 7, 2023

View reviewed changes

chrisbobbe added 6 commits June 7, 2023 17:33

autocomplete: Implement toString on MentionAutocompleteQuery

5a41d6e

autocomplete: Cancel in-progress computations after [dispose] is called

41e17ad

autocomplete: Extend content controller with autocompleteIntent method

3db7e9a

For now, only to handle mention autocomplete syntax; others, including stream/topic and insert-a-time, will come later.

compose [nfc]: Pass message list's narrow to StreamComposeBox

8aec8f7

compose: Maintain a MentionAutocompleteView when controller reports i…

466018f

…ntent

autocomplete [nfc]: Add silent to MentionAutocompleteQuery

519a6c5

As it does in the web app, we'll want this bit to control the suggestion filtering: if the user wants a silent mention, then it won't make sense to offer wildcard mentions in the list (@ALL, @everyone, etc.).

chrisbobbe force-pushed the pr-recognize-mention-syntax branch from b0d2b76 to 519a6c5 Compare June 8, 2023 00:33

chrisbobbe merged commit 519a6c5 into zulip:main Jun 8, 2023

chrisbobbe deleted the pr-recognize-mention-syntax branch June 8, 2023 00:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

autocomplete: Add code to recognize @-mention syntax in the content input #154

autocomplete: Add code to recognize @-mention syntax in the content input #154

chrisbobbe commented Jun 7, 2023

gnprice left a comment

gnprice Jun 7, 2023

gnprice Jun 7, 2023

chrisbobbe Jun 7, 2023

gnprice Jun 7, 2023

gnprice Jun 7, 2023

chrisbobbe Jun 7, 2023

gnprice Jun 7, 2023

chrisbobbe Jun 7, 2023

gnprice Jun 7, 2023

gnprice Jun 7, 2023

gnprice Jun 7, 2023

gnprice Jun 7, 2023

gnprice Jun 7, 2023

chrisbobbe commented Jun 7, 2023

gnprice left a comment

gnprice Jun 7, 2023

gnprice Jun 7, 2023

gnprice Jun 7, 2023

chrisbobbe Jun 7, 2023

chrisbobbe commented Jun 7, 2023

gnprice left a comment

chrisbobbe commented Jun 8, 2023

	import 'package:flutter/cupertino.dart';
	import 'package:flutter/services.dart';

	group('ContentTextEditingController.autocompleteIntent', () {
	test('ContentTextEditingController.autocompleteIntent', () {

		doCheck(String markedText, MentionAutocompleteQuery? expectedQuery) {
		final parsed = parseMarkedText(markedText);

-    doCheck(String markedText, MentionAutocompleteQuery? expectedQuery) {
-      final parsed = parseMarkedText(markedText);
+    doTest(String markedText, MentionAutocompleteQuery? expectedQuery) {
+      final description = expectedQuery != null
+        ? 'in ${jsonEncode(markedText)}, query ${jsonEncode(expectedQuery.raw)}'
+        : 'no query in ${jsonEncode(markedText)}';
+      test(description, () {
+        final controller = ContentTextEditingController();
+        final parsed = parseMarkedText(markedText);

	const beforeAtSign = r'(?:^\|\s\|\p{Punctuation})';
	const beforeAtSign = r'(?<=^\|\s\|\p{Punctuation})';

		_mentionAutocompleteView = MentionAutocompleteView.init(
		store: widget.store, narrow: widget.narrow);

	doTest('@ ^', null); // doCheck('@_ ^', null); // (would fail, but OK… technically "_" could start a word in full_name)
	doTest('@ ^', null); // doTest('@_ ^', null); // (would fail, but OK… technically "_" could start a word in full_name)

	// least require that the selection doesn't extend leftword past that;
	// least require that the selection doesn't extend leftward past that;

autocomplete: Add code to recognize @-mention syntax in the content input #154

autocomplete: Add code to recognize @-mention syntax in the content input #154

Conversation

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

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

chrisbobbe commented Jun 7, 2023

gnprice left a comment

Choose a reason for hiding this comment

chrisbobbe commented Jun 8, 2023