From f6e7e3cb5c7d66b9a8b4859917105862992e2732 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 18 Nov 2024 14:12:11 -0800 Subject: [PATCH 01/21] autocomplete test: Fix not-starve test, using awaitFakeAsync Using just `fakeAsync`, when this hit an `await` it just stopped and didn't finish the remainder of the test, so didn't get to the point of testing what it's meant to test. I believe the test worked correctly when first committed, as it had no `await` of its own; but later was accidentally defeated by eca33f982 introducing an `await` for `store.addUsers`. Using our `awaitFakeAsync` fixes the problem. This was the only call to `fakeAsync` in our codebase, so I believe this commit fixes the whole problem. --- test/model/autocomplete_test.dart | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/model/autocomplete_test.dart b/test/model/autocomplete_test.dart index 9830b2a734..926eea75fb 100644 --- a/test/model/autocomplete_test.dart +++ b/test/model/autocomplete_test.dart @@ -2,7 +2,6 @@ import 'dart:async'; import 'dart:convert'; import 'package:checks/checks.dart'; -import 'package:fake_async/fake_async.dart'; import 'package:flutter/widgets.dart'; import 'package:test/scaffolding.dart'; import 'package:zulip/api/model/initial_snapshot.dart'; @@ -15,6 +14,7 @@ import 'package:zulip/widgets/compose_box.dart'; import '../api/fake_api.dart'; import '../example_data.dart' as eg; +import '../fake_async.dart'; import 'test_store.dart'; import 'autocomplete_checks.dart'; @@ -189,7 +189,7 @@ void main() { }); test('MentionAutocompleteView not starve timers', () { - fakeAsync((binding) async { + return awaitFakeAsync((binding) async { const narrow = ChannelNarrow(1); final store = eg.store(); await store.addUsers([eg.selfUser, eg.otherUser, eg.thirdUser]); From e561f3aff07dad6738c6ce69aeccce08c815b1e4 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 18 Nov 2024 15:45:16 -0800 Subject: [PATCH 02/21] autocomplete [nfc]: Document how query, view-model, and results classes relate --- lib/model/autocomplete.dart | 49 +++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/lib/model/autocomplete.dart b/lib/model/autocomplete.dart index 9222a817a5..1ef32a7764 100644 --- a/lib/model/autocomplete.dart +++ b/lib/model/autocomplete.dart @@ -177,6 +177,13 @@ class AutocompleteViewManager { /// A view-model for an autocomplete interaction. /// +/// Subclasses correspond to subclasses of [AutocompleteQuery], +/// i.e. different types of autocomplete interaction initiated by the user. +/// Each subclass specifies the corresponding [AutocompleteQuery] subclass +/// as `QueryT`, +/// and the [AutocompleteResult] subclass in `ResultT` describes the +/// possible results that the user might choose in the autocomplete interaction. +/// /// The owner of one of these objects must call [dispose] when the object /// will no longer be used, in order to free resources on the [PerAccountStore]. /// @@ -284,6 +291,7 @@ abstract class AutocompleteView { MentionAutocompleteView._({ required super.store, @@ -493,11 +501,29 @@ class MentionAutocompleteView extends AutocompleteView _lowercaseWords; /// Whether all of this query's words have matches in [words] that appear in order. @@ -523,6 +549,7 @@ abstract class AutocompleteQuery { } } +/// A @-mention autocomplete query, used by [MentionAutocompleteView]. class MentionAutocompleteQuery extends AutocompleteQuery { MentionAutocompleteQuery(super.raw, {this.silent = false}); @@ -555,6 +582,10 @@ class MentionAutocompleteQuery extends AutocompleteQuery { int get hashCode => Object.hash('MentionAutocompleteQuery', raw, silent); } +/// Cached data that is used for autocomplete +/// but kept around in between autocomplete interactions. +/// +/// An instance of this class is managed by [AutocompleteViewManager]. class AutocompleteDataCache { final Map _normalizedNamesByUser = {}; @@ -575,10 +606,22 @@ class AutocompleteDataCache { } } +/// A result the user chose, or might choose, from an autocomplete interaction. +/// +/// Different subclasses of [AutocompleteView], +/// representing different types of autocomplete interaction, +/// have corresponding subclasses of [AutocompleteResult] they might produce. class AutocompleteResult {} +/// A result from an @-mention autocomplete interaction, +/// managed by a [MentionAutocompleteView]. +/// +/// This is abstract because there are several kinds of result +/// that can all be offered in the same @-mention autocomplete interaction: +/// a user, a wildcard, or a user group. sealed class MentionAutocompleteResult extends AutocompleteResult {} +/// An autocomplete result for an @-mention of an individual user. class UserMentionAutocompleteResult extends MentionAutocompleteResult { UserMentionAutocompleteResult({required this.userId}); @@ -589,6 +632,7 @@ class UserMentionAutocompleteResult extends MentionAutocompleteResult { // TODO(#234): // class WildcardMentionAutocompleteResult extends MentionAutocompleteResult { +/// An autocomplete interaction for choosing a topic for a message. class TopicAutocompleteView extends AutocompleteView { TopicAutocompleteView._({required super.store, required this.streamId}); @@ -599,7 +643,9 @@ class TopicAutocompleteView extends AutocompleteView _topics = []; bool _isFetching = false; @@ -642,6 +688,8 @@ class TopicAutocompleteView extends AutocompleteView Object.hash('TopicAutocompleteQuery', raw); } +/// A topic chosen in an autocomplete interaction, via a [TopicAutocompleteView]. class TopicAutocompleteResult extends AutocompleteResult { final String topic; From 777245f7ed385a680422aaf5d47e727cf9325221 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 18 Nov 2024 14:50:46 -0800 Subject: [PATCH 03/21] autocomplete [nfc]: Make query non-nullable on AutocompleteView At first glance this appears to make a functional change: it causes the search to begin as soon as the AutocompleteView is constructed, rather than later when `query` is set to non-null. But in fact the only times (outside tests) that we ultimately construct an AutocompleteView are by calling an implementation of AutocompleteField.initViewModel... and those call sites are immediately followed by setting a non-null query. So this is an NFC commit after all. --- lib/model/autocomplete.dart | 52 +++++++++++++++++---------- lib/widgets/autocomplete.dart | 24 ++++++------- test/model/autocomplete_test.dart | 59 ++++++++++++++++--------------- 3 files changed, 77 insertions(+), 58 deletions(-) diff --git a/lib/model/autocomplete.dart b/lib/model/autocomplete.dart index 1ef32a7764..f1b3eb8c86 100644 --- a/lib/model/autocomplete.dart +++ b/lib/model/autocomplete.dart @@ -184,37 +184,42 @@ class AutocompleteViewManager { /// and the [AutocompleteResult] subclass in `ResultT` describes the /// possible results that the user might choose in the autocomplete interaction. /// +/// When an [AutocompleteView] is created, its constructor begins the search +/// for results corresponding to the initial [query]. +/// The query may later be updated, causing a new search. +/// /// The owner of one of these objects must call [dispose] when the object /// will no longer be used, in order to free resources on the [PerAccountStore]. /// /// Lifecycle: -/// * Create an instance of a concrete subtype. +/// * Create an instance of a concrete subtype, beginning a search /// * Add listeners with [addListener]. -/// * Use the [query] setter to start a search for a query. +/// * When the user edits the query, use the [query] setter to update the search. /// * On reassemble, call [reassemble]. /// * When the object will no longer be used, call [dispose] to free /// resources on the [PerAccountStore]. abstract class AutocompleteView extends ChangeNotifier { - AutocompleteView({required this.store}); + /// Construct a view-model for an autocomplete interaction, + /// and begin the search for the initial query. + AutocompleteView({required this.store, required QueryT query}) + : _query = query { + _startSearch(); + } final PerAccountStore store; - QueryT? get query => _query; - QueryT? _query; - set query(QueryT? query) { + QueryT get query => _query; + QueryT _query; + set query(QueryT query) { _query = query; - if (query != null) { - _startSearch(); - } + _startSearch(); } /// Called when the app is reassembled during debugging, e.g. for hot reload. /// /// This will redo the search from scratch for the current query, if any. void reassemble() { - if (_query != null) { - _startSearch(); - } + _startSearch(); } Iterable get results => _results; @@ -271,8 +276,7 @@ abstract class AutocompleteView candidates, required List results, }) async { - assert(_query != null); - final query = _query!; + final query = _query; final iterator = candidates.iterator; outer: while (true) { @@ -295,6 +299,7 @@ abstract class AutocompleteView { MentionAutocompleteView._({ required super.store, + required super.query, required this.narrow, required this.sortedUsers, }); @@ -302,9 +307,11 @@ class MentionAutocompleteView extends AutocompleteView { - TopicAutocompleteView._({required super.store, required this.streamId}); + TopicAutocompleteView._({ + required super.store, + required super.query, + required this.streamId, + }); - factory TopicAutocompleteView.init({required PerAccountStore store, required int streamId}) { - final view = TopicAutocompleteView._(store: store, streamId: streamId); + factory TopicAutocompleteView.init({ + required PerAccountStore store, + required int streamId, + required TopicAutocompleteQuery query, + }) { + final view = TopicAutocompleteView._( + store: store, streamId: streamId, query: query); store.autocompleteViewManager.registerTopicAutocomplete(view); view._fetch(); return view; @@ -661,7 +677,7 @@ class TopicAutocompleteView extends AutocompleteView e.name); _isFetching = false; - if (_query != null) return _startSearch(); + return _startSearch(); } @override diff --git a/lib/widgets/autocomplete.dart b/lib/widgets/autocomplete.dart index 277048f6a9..979feb68b4 100644 --- a/lib/widgets/autocomplete.dart +++ b/lib/widgets/autocomplete.dart @@ -23,7 +23,7 @@ abstract class AutocompleteField initViewModel(BuildContext context); + AutocompleteView initViewModel(BuildContext context, QueryT query); @override State> createState() => _AutocompleteFieldState(); @@ -32,18 +32,18 @@ abstract class AutocompleteField extends State> with PerAccountStoreAwareStateMixin> { AutocompleteView? _viewModel; - void _initViewModel() { - _viewModel = widget.initViewModel(context) + void _initViewModel(QueryT query) { + _viewModel = widget.initViewModel(context, query) ..addListener(_viewModelChanged); } void _handleControllerChange() { - final newAutocompleteIntent = widget.autocompleteIntent(); - if (newAutocompleteIntent != null) { + final newQuery = widget.autocompleteIntent()?.query; + if (newQuery != null) { if (_viewModel == null) { - _initViewModel(); + _initViewModel(newQuery); } - _viewModel!.query = newAutocompleteIntent.query; + _viewModel!.query = newQuery; } else { if (_viewModel != null) { _viewModel!.dispose(); // removes our listener @@ -64,7 +64,7 @@ class _AutocompleteFieldState? autocompleteIntent() => controller.autocompleteIntent(); @override - MentionAutocompleteView initViewModel(BuildContext context) { + MentionAutocompleteView initViewModel(BuildContext context, MentionAutocompleteQuery query) { final store = PerAccountStoreWidget.of(context); - return MentionAutocompleteView.init(store: store, narrow: narrow); + return MentionAutocompleteView.init(store: store, narrow: narrow, query: query); } void _onTapOption(BuildContext context, MentionAutocompleteResult option) { @@ -238,9 +238,9 @@ class TopicAutocomplete extends AutocompleteField? autocompleteIntent() => controller.autocompleteIntent(); @override - TopicAutocompleteView initViewModel(BuildContext context) { + TopicAutocompleteView initViewModel(BuildContext context, TopicAutocompleteQuery query) { final store = PerAccountStoreWidget.of(context); - return TopicAutocompleteView.init(store: store, streamId: streamId); + return TopicAutocompleteView.init(store: store, streamId: streamId, query: query); } void _onTapOption(BuildContext context, TopicAutocompleteResult option) { diff --git a/test/model/autocomplete_test.dart b/test/model/autocomplete_test.dart index 926eea75fb..2919c6fe7d 100644 --- a/test/model/autocomplete_test.dart +++ b/test/model/autocomplete_test.dart @@ -175,11 +175,11 @@ void main() { const narrow = ChannelNarrow(1); final store = eg.store(); await store.addUsers([eg.selfUser, eg.otherUser, eg.thirdUser]); - final view = MentionAutocompleteView.init(store: store, narrow: narrow); + final view = MentionAutocompleteView.init(store: store, narrow: narrow, + query: MentionAutocompleteQuery('Third')); bool done = false; view.addListener(() { done = true; }); - view.query = MentionAutocompleteQuery('Third'); await Future(() {}); await Future(() {}); check(done).isTrue(); @@ -193,12 +193,8 @@ void main() { const narrow = ChannelNarrow(1); final store = eg.store(); await store.addUsers([eg.selfUser, eg.otherUser, eg.thirdUser]); - final view = MentionAutocompleteView.init(store: store, narrow: narrow); bool searchDone = false; - view.addListener(() { - searchDone = true; - }); // Schedule a timer event with zero delay. // This stands in for a user interaction, or frame rendering timer, @@ -210,9 +206,11 @@ void main() { check(searchDone).isFalse(); }); - view.query = MentionAutocompleteQuery('Third'); - check(timerDone).isFalse(); - check(searchDone).isFalse(); + final view = MentionAutocompleteView.init(store: store, narrow: narrow, + query: MentionAutocompleteQuery('Third')); + view.addListener(() { + searchDone = true; + }); binding.elapse(const Duration(seconds: 1)); @@ -230,11 +228,11 @@ void main() { for (int i = 1; i <= 2500; i++) { await store.addUser(eg.user(userId: i, email: 'user$i@example.com', fullName: 'User $i')); } - final view = MentionAutocompleteView.init(store: store, narrow: narrow); bool done = false; + final view = MentionAutocompleteView.init(store: store, narrow: narrow, + query: MentionAutocompleteQuery('User 2222')); view.addListener(() { done = true; }); - view.query = MentionAutocompleteQuery('User 2222'); await Future(() {}); check(done).isFalse(); @@ -253,11 +251,11 @@ void main() { for (int i = 1; i <= 1500; i++) { await store.addUser(eg.user(userId: i, email: 'user$i@example.com', fullName: 'User $i')); } - final view = MentionAutocompleteView.init(store: store, narrow: narrow); bool done = false; + final view = MentionAutocompleteView.init(store: store, narrow: narrow, + query: MentionAutocompleteQuery('User 1111')); view.addListener(() { done = true; }); - view.query = MentionAutocompleteQuery('User 1111'); await Future(() {}); check(done).isFalse(); @@ -288,11 +286,11 @@ void main() { for (int i = 1; i <= 2500; i++) { await store.addUser(eg.user(userId: i, email: 'user$i@example.com', fullName: 'User $i')); } - final view = MentionAutocompleteView.init(store: store, narrow: narrow); bool done = false; + final view = MentionAutocompleteView.init(store: store, narrow: narrow, + query: MentionAutocompleteQuery('User 110')); view.addListener(() { done = true; }); - view.query = MentionAutocompleteQuery('User 110'); await Future(() {}); check(done).isFalse(); @@ -544,7 +542,8 @@ void main() { group('ranking across signals', () { void checkPrecedes(Narrow narrow, User userA, Iterable usersB) { - final view = MentionAutocompleteView.init(store: store, narrow: narrow); + final view = MentionAutocompleteView.init(store: store, narrow: narrow, + query: MentionAutocompleteQuery('')); for (final userB in usersB) { check(view.debugCompareUsers(userA, userB)).isLessThan(0); check(view.debugCompareUsers(userB, userA)).isGreaterThan(0); @@ -552,7 +551,8 @@ void main() { } void checkRankEqual(Narrow narrow, List users) { - final view = MentionAutocompleteView.init(store: store, narrow: narrow); + final view = MentionAutocompleteView.init(store: store, narrow: narrow, + query: MentionAutocompleteQuery('')); for (int i = 0; i < users.length; i++) { for (int j = i + 1; j < users.length; j++) { check(view.debugCompareUsers(users[i], users[j])).equals(0); @@ -669,21 +669,24 @@ void main() { test('CombinedFeedNarrow gives error', () async { await prepare(users: [eg.user(), eg.user()], messages: []); const narrow = CombinedFeedNarrow(); - check(() => MentionAutocompleteView.init(store: store, narrow: narrow)) + check(() => MentionAutocompleteView.init(store: store, narrow: narrow, + query: MentionAutocompleteQuery(''))) .throws(); }); test('MentionsNarrow gives error', () async { await prepare(users: [eg.user(), eg.user()], messages: []); const narrow = MentionsNarrow(); - check(() => MentionAutocompleteView.init(store: store, narrow: narrow)) + check(() => MentionAutocompleteView.init(store: store, narrow: narrow, + query: MentionAutocompleteQuery(''))) .throws(); }); test('StarredMessagesNarrow gives error', () async { await prepare(users: [eg.user(), eg.user()], messages: []); const narrow = StarredMessagesNarrow(); - check(() => MentionAutocompleteView.init(store: store, narrow: narrow)) + check(() => MentionAutocompleteView.init(store: store, narrow: narrow, + query: MentionAutocompleteQuery(''))) .throws(); }); }); @@ -692,9 +695,9 @@ void main() { Future> getResults( Narrow narrow, MentionAutocompleteQuery query) async { bool done = false; - final view = MentionAutocompleteView.init(store: store, narrow: narrow); + final view = MentionAutocompleteView.init(store: store, narrow: narrow, + query: query); view.addListener(() { done = true; }); - view.query = query; await Future(() {}); check(done).isTrue(); final results = view.results @@ -777,13 +780,14 @@ void main() { final third = eg.getStreamTopicsEntry(maxId: 3, name: 'Third Topic'); connection.prepare(json: GetStreamTopicsResult( topics: [first, second, third]).toJson()); + final view = TopicAutocompleteView.init( store: store, - streamId: eg.stream().streamId); - + streamId: eg.stream().streamId, + query: TopicAutocompleteQuery('Third')); bool done = false; view.addListener(() { done = true; }); - view.query = TopicAutocompleteQuery('Third'); + // those are here to wait for topics to be loaded await Future(() {}); await Future(() {}); @@ -802,11 +806,10 @@ void main() { final view = TopicAutocompleteView.init( store: store, - streamId: eg.stream().streamId); - + streamId: eg.stream().streamId, + query: TopicAutocompleteQuery('te')); bool done = false; view.addListener(() { done = true; }); - view.query = TopicAutocompleteQuery('te'); check(done).isFalse(); await Future(() {}); From 13385fe1e0ddb2e5ec2789498a5f38573b49fe8d Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 18 Nov 2024 17:14:23 -0800 Subject: [PATCH 04/21] autocomplete [nfc]: Document semantics of `query` and `results` on view-model --- lib/model/autocomplete.dart | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/lib/model/autocomplete.dart b/lib/model/autocomplete.dart index f1b3eb8c86..598bb9f67d 100644 --- a/lib/model/autocomplete.dart +++ b/lib/model/autocomplete.dart @@ -208,6 +208,15 @@ abstract class AutocompleteView _query; QueryT _query; set query(QueryT query) { @@ -222,6 +231,13 @@ abstract class AutocompleteView get results => _results; List _results = []; From 726ec5aea030b869da54cdd5e346edb3dd05a53f Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 18 Nov 2024 13:20:00 -0800 Subject: [PATCH 05/21] autocomplete [nfc]: Separate ComposeAutocompleteQuery/Result from Mention-etc. The two concepts have meant the same set of possible values so far, because @-mentions are the only type of autocomplete we've had so far in the compose box's content input. As a result we've taken some shortcuts by conflating them. But as we introduce other types of autocomplete in the content input, like for emoji and #-mentions, we have some places that will need to refer to the more general concept while others refer to the more specific one. So separate them out. --- lib/model/autocomplete.dart | 36 ++++++++++++-- lib/widgets/autocomplete.dart | 77 +++++++++++++++++++---------- test/model/autocomplete_checks.dart | 2 +- 3 files changed, 84 insertions(+), 31 deletions(-) diff --git a/lib/model/autocomplete.dart b/lib/model/autocomplete.dart index 598bb9f67d..3a643211d3 100644 --- a/lib/model/autocomplete.dart +++ b/lib/model/autocomplete.dart @@ -9,7 +9,7 @@ import 'narrow.dart'; import 'store.dart'; extension ComposeContentAutocomplete on ComposeContentController { - AutocompleteIntent? autocompleteIntent() { + AutocompleteIntent? autocompleteIntent() { if (!selection.isValid || !selection.isNormalized) { // We don't require [isCollapsed] to be true because we've seen that // autocorrect and even backspace involve programmatically expanding the @@ -208,6 +208,10 @@ abstract class AutocompleteView query is QueryT; + /// The last query this [AutocompleteView] was asked to perform for the user. /// /// If this view-model is currently performing a search, @@ -311,7 +315,12 @@ abstract class AutocompleteView; + +/// An [AutocompleteView] for an @-mention autocomplete interaction, +/// an example of a [ComposeAutocompleteView]. class MentionAutocompleteView extends AutocompleteView { MentionAutocompleteView._({ required super.store, @@ -572,13 +581,27 @@ abstract class AutocompleteQuery { } } +/// Any autocomplete query in the compose box's content input. +abstract class ComposeAutocompleteQuery extends AutocompleteQuery { + ComposeAutocompleteQuery(super.raw); + + /// Construct an [AutocompleteView] initialized with this query + /// and ready to handle queries of the same type. + ComposeAutocompleteView initViewModel(PerAccountStore store, Narrow narrow); +} + /// A @-mention autocomplete query, used by [MentionAutocompleteView]. -class MentionAutocompleteQuery extends AutocompleteQuery { +class MentionAutocompleteQuery extends ComposeAutocompleteQuery { MentionAutocompleteQuery(super.raw, {this.silent = false}); /// Whether the user wants a silent mention (@_query, vs. @query). final bool silent; + @override + MentionAutocompleteView initViewModel(PerAccountStore store, Narrow narrow) { + return MentionAutocompleteView.init(store: store, narrow: narrow, query: this); + } + bool testUser(User user, AutocompleteDataCache cache) { // TODO(#236) test email too, not just name @@ -636,13 +659,18 @@ class AutocompleteDataCache { /// have corresponding subclasses of [AutocompleteResult] they might produce. class AutocompleteResult {} +/// A result from some autocomplete interaction in +/// the compose box's content input, initiated by a [ComposeAutocompleteQuery] +/// and managed by some [ComposeAutocompleteView]. +sealed class ComposeAutocompleteResult extends AutocompleteResult {} + /// A result from an @-mention autocomplete interaction, /// managed by a [MentionAutocompleteView]. /// /// This is abstract because there are several kinds of result /// that can all be offered in the same @-mention autocomplete interaction: /// a user, a wildcard, or a user group. -sealed class MentionAutocompleteResult extends AutocompleteResult {} +sealed class MentionAutocompleteResult extends ComposeAutocompleteResult {} /// An autocomplete result for an @-mention of an individual user. class UserMentionAutocompleteResult extends MentionAutocompleteResult { diff --git a/lib/widgets/autocomplete.dart b/lib/widgets/autocomplete.dart index 979feb68b4..9abe289920 100644 --- a/lib/widgets/autocomplete.dart +++ b/lib/widgets/autocomplete.dart @@ -39,16 +39,23 @@ class _AutocompleteFieldState { +class ComposeAutocomplete extends AutocompleteField { const ComposeAutocomplete({ super.key, required this.narrow, @@ -159,15 +166,15 @@ class ComposeAutocomplete extends AutocompleteField super.controller as ComposeContentController; @override - AutocompleteIntent? autocompleteIntent() => controller.autocompleteIntent(); + AutocompleteIntent? autocompleteIntent() => controller.autocompleteIntent(); @override - MentionAutocompleteView initViewModel(BuildContext context, MentionAutocompleteQuery query) { + ComposeAutocompleteView initViewModel(BuildContext context, ComposeAutocompleteQuery query) { final store = PerAccountStoreWidget.of(context); - return MentionAutocompleteView.init(store: store, narrow: narrow, query: query); + return query.initViewModel(store, narrow); } - void _onTapOption(BuildContext context, MentionAutocompleteResult option) { + void _onTapOption(BuildContext context, ComposeAutocompleteResult option) { // Probably the same intent that brought up the option that was tapped. // If not, it still shouldn't be off by more than the time it takes // to compute the autocomplete results, which we do asynchronously. @@ -175,14 +182,18 @@ class ComposeAutocomplete extends AutocompleteField _MentionAutocompleteItem(option: option), + }; + return InkWell( + onTap: () { + _onTapOption(context, option); + }, + child: child); + } +} + +class _MentionAutocompleteItem extends StatelessWidget { + const _MentionAutocompleteItem({required this.option}); + + final MentionAutocompleteResult option; + + @override + Widget build(BuildContext context) { Widget avatar; String label; switch (option) { @@ -202,18 +231,14 @@ class ComposeAutocomplete extends AutocompleteField { - Subject?> get autocompleteIntent => has((c) => c.autocompleteIntent(), 'autocompleteIntent'); + Subject?> get autocompleteIntent => has((c) => c.autocompleteIntent(), 'autocompleteIntent'); } extension ComposeTopicControllerChecks on Subject { From 0b28227d750523372d55b79ed8f47c92c93cd13f Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 18 Nov 2024 20:00:52 -0800 Subject: [PATCH 06/21] autocomplete [nfc]: Simplify intent-finding loop slightly --- lib/model/autocomplete.dart | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/lib/model/autocomplete.dart b/lib/model/autocomplete.dart index 3a643211d3..db47746c28 100644 --- a/lib/model/autocomplete.dart +++ b/lib/model/autocomplete.dart @@ -1,3 +1,5 @@ +import 'dart:math'; + import 'package:flutter/foundation.dart'; import 'package:flutter/services.dart'; @@ -18,12 +20,13 @@ extension ComposeContentAutocomplete on ComposeContentController { // see below. return null; } + + // To avoid spending a lot of time searching for autocomplete intents + // in long messages, we bound how far back we look for the intent's start. + final earliest = max(0, selection.end - 30); + final textUntilCursor = text.substring(0, selection.end); - for ( - int position = selection.end - 1; - position >= 0 && (selection.end - position <= 30); - position-- - ) { + for (int position = selection.end - 1; position >= earliest; position--) { if (textUntilCursor[position] != '@') { continue; } From 1c01908624d205967751d6b573333bb0eedf4b18 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 18 Nov 2024 20:14:44 -0800 Subject: [PATCH 07/21] autocomplete [nfc]: Short-circuit intent-finding on long selection This is NFC because when this condition is true, the only return statements below that can actually be reached are the ones that return null. In the code as it is, this makes a small optimization. But it also will help simplify an upcoming refactor. --- lib/model/autocomplete.dart | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lib/model/autocomplete.dart b/lib/model/autocomplete.dart index db47746c28..9c7cb66328 100644 --- a/lib/model/autocomplete.dart +++ b/lib/model/autocomplete.dart @@ -25,6 +25,12 @@ extension ComposeContentAutocomplete on ComposeContentController { // in long messages, we bound how far back we look for the intent's start. final earliest = max(0, selection.end - 30); + if (selection.start < earliest) { + // The selection extends to before any position we'd consider + // for the start of the intent. So there can't be a match. + return null; + } + final textUntilCursor = text.substring(0, selection.end); for (int position = selection.end - 1; position >= earliest; position--) { if (textUntilCursor[position] != '@') { From ce5beaaf7b5d72777204ccfa4fcfa40457b1c169 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 18 Nov 2024 20:22:10 -0800 Subject: [PATCH 08/21] autocomplete [nfc]: Shorten local names in intent-finding In particular the regex is effectively private to this logic, so we can make it a private variable and then simplify its name. --- lib/model/autocomplete.dart | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/model/autocomplete.dart b/lib/model/autocomplete.dart index 9c7cb66328..6d97bce98f 100644 --- a/lib/model/autocomplete.dart +++ b/lib/model/autocomplete.dart @@ -32,20 +32,20 @@ extension ComposeContentAutocomplete on ComposeContentController { } final textUntilCursor = text.substring(0, selection.end); - for (int position = selection.end - 1; position >= earliest; position--) { - if (textUntilCursor[position] != '@') { + for (int pos = selection.end - 1; pos >= earliest; pos--) { + if (textUntilCursor[pos] != '@') { continue; } - final match = mentionAutocompleteMarkerRegex.matchAsPrefix(textUntilCursor, position); + final match = _mentionIntentRegex.matchAsPrefix(textUntilCursor, pos); if (match == null) { continue; } - if (selection.start < position) { + if (selection.start < pos) { // See comment about [TextSelection.isCollapsed] above. return null; } return AutocompleteIntent( - syntaxStart: position, + syntaxStart: pos, query: MentionAutocompleteQuery(match[2]!, silent: match[1]! == '_'), textEditingValue: value); } @@ -62,7 +62,7 @@ extension ComposeTopicAutocomplete on ComposeTopicController { } } -final RegExp mentionAutocompleteMarkerRegex = (() { +final RegExp _mentionIntentRegex = (() { // What's likely to come before an @-mention: the start of the string, // whitespace, or punctuation. Letters are unlikely; in that case an email // might be intended. (By punctuation, we mean *some* punctuation, like "(". From 6b3d426a2930a51f2d7d5d7014929b7d48964e63 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 18 Nov 2024 17:59:41 -0800 Subject: [PATCH 09/21] autocomplete [nfc]: Generalize find-intent loop slightly This makes room for this loop to look for other autocomplete markers besides `@`. --- lib/model/autocomplete.dart | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/lib/model/autocomplete.dart b/lib/model/autocomplete.dart index 6d97bce98f..93dacce650 100644 --- a/lib/model/autocomplete.dart +++ b/lib/model/autocomplete.dart @@ -33,21 +33,21 @@ extension ComposeContentAutocomplete on ComposeContentController { final textUntilCursor = text.substring(0, selection.end); for (int pos = selection.end - 1; pos >= earliest; pos--) { - if (textUntilCursor[pos] != '@') { - continue; + final charAtPos = textUntilCursor[pos]; + if (charAtPos == '@') { + final match = _mentionIntentRegex.matchAsPrefix(textUntilCursor, pos); + if (match == null) { + continue; + } + if (selection.start < pos) { + // See comment about [TextSelection.isCollapsed] above. + return null; + } + return AutocompleteIntent( + syntaxStart: pos, + query: MentionAutocompleteQuery(match[2]!, silent: match[1]! == '_'), + textEditingValue: value); } - final match = _mentionIntentRegex.matchAsPrefix(textUntilCursor, pos); - if (match == null) { - continue; - } - if (selection.start < pos) { - // See comment about [TextSelection.isCollapsed] above. - return null; - } - return AutocompleteIntent( - syntaxStart: pos, - query: MentionAutocompleteQuery(match[2]!, silent: match[1]! == '_'), - textEditingValue: value); } return null; } From 54b1add012deef3ea646998b3326670c072eddc8 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 18 Nov 2024 20:11:00 -0800 Subject: [PATCH 10/21] autocomplete [nfc]: Split intent-finding loop to within/before selection This puts much less control flow inside the `@` case. That will help simplify things as we add more kinds of autocomplete beyond @-mentions. --- lib/model/autocomplete.dart | 34 ++++++++++++++++++++++------------ 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/lib/model/autocomplete.dart b/lib/model/autocomplete.dart index 93dacce650..4671bf80bf 100644 --- a/lib/model/autocomplete.dart +++ b/lib/model/autocomplete.dart @@ -32,23 +32,33 @@ extension ComposeContentAutocomplete on ComposeContentController { } final textUntilCursor = text.substring(0, selection.end); - for (int pos = selection.end - 1; pos >= earliest; pos--) { + int pos; + for (pos = selection.end - 1; pos > selection.start; pos--) { final charAtPos = textUntilCursor[pos]; if (charAtPos == '@') { final match = _mentionIntentRegex.matchAsPrefix(textUntilCursor, pos); - if (match == null) { - continue; - } - if (selection.start < pos) { - // See comment about [TextSelection.isCollapsed] above. - return null; - } - return AutocompleteIntent( - syntaxStart: pos, - query: MentionAutocompleteQuery(match[2]!, silent: match[1]! == '_'), - textEditingValue: value); + if (match == null) continue; + } else { + continue; } + // See comment about [TextSelection.isCollapsed] above. + return null; + } + + for (; pos >= earliest; pos--) { + final charAtPos = textUntilCursor[pos]; + final ComposeAutocompleteQuery query; + if (charAtPos == '@') { + final match = _mentionIntentRegex.matchAsPrefix(textUntilCursor, pos); + if (match == null) continue; + query = MentionAutocompleteQuery(match[2]!, silent: match[1]! == '_'); + } else { + continue; + } + return AutocompleteIntent(syntaxStart: pos, textEditingValue: value, + query: query); } + return null; } } From ecd313803fdf6a29b384782973a7510b530da2c3 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 19 Nov 2024 13:36:49 -0800 Subject: [PATCH 11/21] emoji [nfc]: Factor out ImageEmojiWidget --- lib/widgets/emoji.dart | 55 +++++++++++++++++++++++++++++++++ lib/widgets/emoji_reaction.dart | 30 ++++-------------- 2 files changed, 61 insertions(+), 24 deletions(-) create mode 100644 lib/widgets/emoji.dart diff --git a/lib/widgets/emoji.dart b/lib/widgets/emoji.dart new file mode 100644 index 0000000000..cd8c7347d2 --- /dev/null +++ b/lib/widgets/emoji.dart @@ -0,0 +1,55 @@ +import 'package:flutter/foundation.dart'; +import 'package:flutter/widgets.dart'; + +import '../model/emoji.dart'; +import 'content.dart'; + +class ImageEmojiWidget extends StatelessWidget { + const ImageEmojiWidget({ + super.key, + required this.emojiDisplay, + required this.size, + this.textScaler = TextScaler.noScaling, + this.errorBuilder, + }); + + final ImageEmojiDisplay emojiDisplay; + + /// The base width and height to use for the emoji. + /// + /// This will be scaled by [textScaler]. + final double size; + + /// The text scaler to apply to [size]. + /// + /// Defaults to [TextScaler.noScaling]. + final TextScaler textScaler; + + final ImageErrorWidgetBuilder? errorBuilder; + + @override + Widget build(BuildContext context) { + // Some people really dislike animated emoji. + final doNotAnimate = + // From reading code, this doesn't actually get set on iOS: + // https://github.com/zulip/zulip-flutter/pull/410#discussion_r1408522293 + MediaQuery.disableAnimationsOf(context) + || (defaultTargetPlatform == TargetPlatform.iOS + // TODO(upstream) On iOS 17+ (new in 2023), there's a more closely + // relevant setting than "reduce motion". It's called "auto-play + // animated images", and we should file an issue to expose it. + // See GitHub comment linked above. + && WidgetsBinding.instance.platformDispatcher.accessibilityFeatures.reduceMotion); + + final size = textScaler.scale(this.size); + + final resolvedUrl = doNotAnimate + ? (emojiDisplay.resolvedStillUrl ?? emojiDisplay.resolvedUrl) + : emojiDisplay.resolvedUrl; + + return RealmContentNetworkImage( + width: size, height: size, + errorBuilder: errorBuilder, + resolvedUrl); + } +} diff --git a/lib/widgets/emoji_reaction.dart b/lib/widgets/emoji_reaction.dart index d92a050b05..be1f145571 100644 --- a/lib/widgets/emoji_reaction.dart +++ b/lib/widgets/emoji_reaction.dart @@ -5,7 +5,7 @@ import '../api/model/model.dart'; import '../api/route/messages.dart'; import '../model/emoji.dart'; import 'color.dart'; -import 'content.dart'; +import 'emoji.dart'; import 'store.dart'; import 'text.dart'; @@ -361,29 +361,11 @@ class _ImageEmoji extends StatelessWidget { @override Widget build(BuildContext context) { - // Some people really dislike animated emoji. - final doNotAnimate = - // From reading code, this doesn't actually get set on iOS: - // https://github.com/zulip/zulip-flutter/pull/410#discussion_r1408522293 - MediaQuery.disableAnimationsOf(context) - || (defaultTargetPlatform == TargetPlatform.iOS - // TODO(upstream) On iOS 17+ (new in 2023), there's a more closely - // relevant setting than "reduce motion". It's called "auto-play - // animated images", and we should file an issue to expose it. - // See GitHub comment linked above. - && WidgetsBinding.instance.platformDispatcher.accessibilityFeatures.reduceMotion); - - final resolvedUrl = doNotAnimate - ? (emojiDisplay.resolvedStillUrl ?? emojiDisplay.resolvedUrl) - : emojiDisplay.resolvedUrl; - - // Unicode and text emoji get scaled; it would look weird if image emoji didn't. - final size = _squareEmojiScalerClamped(context).scale(_squareEmojiSize); - - return RealmContentNetworkImage( - resolvedUrl, - width: size, - height: size, + return ImageEmojiWidget( + size: _squareEmojiSize, + // Unicode and text emoji get scaled; it would look weird if image emoji didn't. + textScaler: _squareEmojiScalerClamped(context), + emojiDisplay: emojiDisplay, errorBuilder: (context, _, __) => _TextEmoji( emojiDisplay: TextEmojiDisplay(emojiName: emojiName), selected: selected), ); From 1485720e497669a32e4ca24c6f7e18ead51f76bc Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 19 Nov 2024 13:53:49 -0800 Subject: [PATCH 12/21] emoji [nfc]: Factor out UnicodeEmojiWidget --- lib/widgets/emoji.dart | 76 +++++++++++++++++++++++++++++++++ lib/widgets/emoji_reaction.dart | 47 +++----------------- 2 files changed, 81 insertions(+), 42 deletions(-) diff --git a/lib/widgets/emoji.dart b/lib/widgets/emoji.dart index cd8c7347d2..d8af9827d7 100644 --- a/lib/widgets/emoji.dart +++ b/lib/widgets/emoji.dart @@ -4,6 +4,82 @@ import 'package:flutter/widgets.dart'; import '../model/emoji.dart'; import 'content.dart'; +class UnicodeEmojiWidget extends StatelessWidget { + const UnicodeEmojiWidget({ + super.key, + required this.emojiDisplay, + required this.size, + required this.notoColorEmojiTextSize, + this.textScaler = TextScaler.noScaling, + }); + + final UnicodeEmojiDisplay emojiDisplay; + + /// The base width and height to use for the emoji. + /// + /// This will be scaled by [textScaler]. + final double size; + + /// A font size that, with Noto Color Emoji and our line-height config, + /// causes a Unicode emoji to occupy a square of size [size] in the layout. + /// + /// This has to be determined experimentally, as far as we know. + final double notoColorEmojiTextSize; + + /// The text scaler to apply to [size]. + /// + /// Defaults to [TextScaler.noScaling]. + final TextScaler textScaler; + + @override + Widget build(BuildContext context) { + switch (defaultTargetPlatform) { + case TargetPlatform.android: + case TargetPlatform.fuchsia: + case TargetPlatform.linux: + case TargetPlatform.windows: + return Text( + textScaler: textScaler, + style: TextStyle( + fontFamily: 'Noto Color Emoji', + fontSize: notoColorEmojiTextSize, + ), + strutStyle: StrutStyle( + fontSize: notoColorEmojiTextSize, forceStrutHeight: true), + emojiDisplay.emojiUnicode); + + case TargetPlatform.iOS: + case TargetPlatform.macOS: + // We expect the font "Apple Color Emoji" to be used. There are some + // surprises in how Flutter ends up rendering emojis in this font: + // - With a font size of 17px, the emoji visually seems to be about 17px + // square. (Unlike on Android, with Noto Color Emoji, where a 14.5px font + // size gives an emoji that looks 17px square.) See: + // + // - The emoji doesn't fill the space taken by the [Text] in the layout. + // There's whitespace above, below, and on the right. See: + // + // + // That extra space would be problematic, except we've used a [Stack] to + // make the [Text] "positioned" so the space doesn't add margins around the + // visible part. Key points that enable the [Stack] workaround: + // - The emoji seems approximately vertically centered (this is + // accomplished with help from a [StrutStyle]; see below). + // - There seems to be approximately no space on its left. + final boxSize = textScaler.scale(size); + return Stack(alignment: Alignment.centerLeft, clipBehavior: Clip.none, children: [ + SizedBox(height: boxSize, width: boxSize), + PositionedDirectional(start: 0, child: Text( + textScaler: textScaler, + style: TextStyle(fontSize: size), + strutStyle: StrutStyle(fontSize: size, forceStrutHeight: true), + emojiDisplay.emojiUnicode)), + ]); + } + } +} + + class ImageEmojiWidget extends StatelessWidget { const ImageEmojiWidget({ super.key, diff --git a/lib/widgets/emoji_reaction.dart b/lib/widgets/emoji_reaction.dart index be1f145571..7d4375ba46 100644 --- a/lib/widgets/emoji_reaction.dart +++ b/lib/widgets/emoji_reaction.dart @@ -1,4 +1,3 @@ -import 'package:flutter/foundation.dart'; import 'package:flutter/material.dart'; import '../api/model/model.dart'; @@ -304,47 +303,11 @@ class _UnicodeEmoji extends StatelessWidget { @override Widget build(BuildContext context) { - switch (defaultTargetPlatform) { - case TargetPlatform.android: - case TargetPlatform.fuchsia: - case TargetPlatform.linux: - case TargetPlatform.windows: - return Text( - textScaler: _squareEmojiScalerClamped(context), - style: const TextStyle( - fontFamily: 'Noto Color Emoji', - fontSize: _notoColorEmojiTextSize, - ), - strutStyle: const StrutStyle(fontSize: _notoColorEmojiTextSize, forceStrutHeight: true), - emojiDisplay.emojiUnicode); - case TargetPlatform.iOS: - case TargetPlatform.macOS: - // We expect the font "Apple Color Emoji" to be used. There are some - // surprises in how Flutter ends up rendering emojis in this font: - // - With a font size of 17px, the emoji visually seems to be about 17px - // square. (Unlike on Android, with Noto Color Emoji, where a 14.5px font - // size gives an emoji that looks 17px square.) See: - // - // - The emoji doesn't fill the space taken by the [Text] in the layout. - // There's whitespace above, below, and on the right. See: - // - // - // That extra space would be problematic, except we've used a [Stack] to - // make the [Text] "positioned" so the space doesn't add margins around the - // visible part. Key points that enable the [Stack] workaround: - // - The emoji seems approximately vertically centered (this is - // accomplished with help from a [StrutStyle]; see below). - // - There seems to be approximately no space on its left. - final boxSize = _squareEmojiScalerClamped(context).scale(_squareEmojiSize); - return Stack(alignment: Alignment.centerLeft, clipBehavior: Clip.none, children: [ - SizedBox(height: boxSize, width: boxSize), - PositionedDirectional(start: 0, child: Text( - textScaler: _squareEmojiScalerClamped(context), - style: const TextStyle(fontSize: _squareEmojiSize), - strutStyle: const StrutStyle(fontSize: _squareEmojiSize, forceStrutHeight: true), - emojiDisplay.emojiUnicode)), - ]); - } + return UnicodeEmojiWidget( + size: _squareEmojiSize, + notoColorEmojiTextSize: _notoColorEmojiTextSize, + textScaler: _squareEmojiScalerClamped(context), + emojiDisplay: emojiDisplay); } } From 8127b0a3821d209ec58080f82d2188dde49b3ce3 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 19 Nov 2024 13:55:38 -0800 Subject: [PATCH 13/21] emoji [nfc]: Cut disused `selected` field on _UnicodeEmoji This field is no longer used, since 4f6ad02f6. --- lib/widgets/emoji_reaction.dart | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/lib/widgets/emoji_reaction.dart b/lib/widgets/emoji_reaction.dart index 7d4375ba46..39264eb3b6 100644 --- a/lib/widgets/emoji_reaction.dart +++ b/lib/widgets/emoji_reaction.dart @@ -182,7 +182,7 @@ class ReactionChip extends StatelessWidget { final emoji = switch (emojiDisplay) { UnicodeEmojiDisplay() => _UnicodeEmoji( - emojiDisplay: emojiDisplay, selected: selfVoted), + emojiDisplay: emojiDisplay), ImageEmojiDisplay() => _ImageEmoji( emojiDisplay: emojiDisplay, emojiName: emojiName, selected: selfVoted), TextEmojiDisplay() => _TextEmoji( @@ -293,13 +293,9 @@ TextScaler _labelTextScalerClamped(BuildContext context) => MediaQuery.textScalerOf(context).clamp(maxScaleFactor: 2); class _UnicodeEmoji extends StatelessWidget { - const _UnicodeEmoji({ - required this.emojiDisplay, - required this.selected, - }); + const _UnicodeEmoji({required this.emojiDisplay}); final UnicodeEmojiDisplay emojiDisplay; - final bool selected; @override Widget build(BuildContext context) { From 0abe2b7eb8472f98ab3d92e41e597fd0f04a45a1 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 19 Nov 2024 14:44:46 -0800 Subject: [PATCH 14/21] autocomplete test [nfc]: Prepare autocompleteIntent tests to handle non-mentions --- test/model/autocomplete_test.dart | 80 +++++++++++++++---------------- 1 file changed, 40 insertions(+), 40 deletions(-) diff --git a/test/model/autocomplete_test.dart b/test/model/autocomplete_test.dart index 2919c6fe7d..762a0c8a6d 100644 --- a/test/model/autocomplete_test.dart +++ b/test/model/autocomplete_test.dart @@ -68,7 +68,7 @@ void main() { /// /// For example, "~@chris^" means the text is "@chris", the selection is /// collapsed at index 6, and we expect the syntax to start at index 0. - void doTest(String markedText, MentionAutocompleteQuery? expectedQuery) { + void doTest(String markedText, ComposeAutocompleteQuery? expectedQuery) { final description = expectedQuery != null ? 'in ${jsonEncode(markedText)}, query ${jsonEncode(expectedQuery.raw)}' : 'no query in ${jsonEncode(markedText)}'; @@ -87,8 +87,8 @@ void main() { }); } - MentionAutocompleteQuery queryOf(String raw) => MentionAutocompleteQuery(raw, silent: false); - MentionAutocompleteQuery silentQueryOf(String raw) => MentionAutocompleteQuery(raw, silent: true); + MentionAutocompleteQuery mention(String raw) => MentionAutocompleteQuery(raw, silent: false); + MentionAutocompleteQuery silentMention(String raw) => MentionAutocompleteQuery(raw, silent: true); doTest('', null); doTest('^', null); @@ -125,48 +125,48 @@ void main() { doTest('`@chris^', null); doTest('`@_chris^', null); - doTest('~@^_', queryOf('')); // Odd/unlikely, but should not crash + doTest('~@^_', mention('')); // Odd/unlikely, but should not crash - doTest('~@__^', silentQueryOf('_')); + doTest('~@__^', silentMention('_')); - doTest('~@^abc^', queryOf('abc')); doTest('~@_^abc^', silentQueryOf('abc')); - doTest('~@a^bc^', queryOf('abc')); doTest('~@_a^bc^', silentQueryOf('abc')); - doTest('~@ab^c^', queryOf('abc')); doTest('~@_ab^c^', silentQueryOf('abc')); - doTest('~^@^', queryOf('')); doTest('~^@_^', silentQueryOf('')); + doTest('~@^abc^', mention('abc')); doTest('~@_^abc^', silentMention('abc')); + doTest('~@a^bc^', mention('abc')); doTest('~@_a^bc^', silentMention('abc')); + doTest('~@ab^c^', mention('abc')); doTest('~@_ab^c^', silentMention('abc')); + doTest('~^@^', mention('')); doTest('~^@_^', silentMention('')); // but: doTest('^hello @chris^', null); doTest('^hello @_chris^', null); - doTest('~@me@zulip.com^', queryOf('me@zulip.com')); doTest('~@_me@zulip.com^', silentQueryOf('me@zulip.com')); - doTest('~@me@^zulip.com^', queryOf('me@zulip.com')); doTest('~@_me@^zulip.com^', silentQueryOf('me@zulip.com')); - doTest('~@me^@zulip.com^', queryOf('me@zulip.com')); doTest('~@_me^@zulip.com^', silentQueryOf('me@zulip.com')); - doTest('~@^me@zulip.com^', queryOf('me@zulip.com')); doTest('~@_^me@zulip.com^', silentQueryOf('me@zulip.com')); - - doTest('~@abc^', queryOf('abc')); doTest('~@_abc^', silentQueryOf('abc')); - doTest(' ~@abc^', queryOf('abc')); doTest(' ~@_abc^', silentQueryOf('abc')); - doTest('(~@abc^', queryOf('abc')); doTest('(~@_abc^', silentQueryOf('abc')); - doTest('—~@abc^', queryOf('abc')); doTest('—~@_abc^', silentQueryOf('abc')); - doTest('"~@abc^', queryOf('abc')); doTest('"~@_abc^', silentQueryOf('abc')); - doTest('“~@abc^', queryOf('abc')); doTest('“~@_abc^', silentQueryOf('abc')); - doTest('。~@abc^', queryOf('abc')); doTest('。~@_abc^', silentQueryOf('abc')); - doTest('«~@abc^', queryOf('abc')); doTest('«~@_abc^', silentQueryOf('abc')); - - doTest('~@ab^c', queryOf('ab')); doTest('~@_ab^c', silentQueryOf('ab')); - doTest('~@a^bc', queryOf('a')); doTest('~@_a^bc', silentQueryOf('a')); - doTest('~@^abc', queryOf('')); doTest('~@_^abc', silentQueryOf('')); - doTest('~@^', queryOf('')); doTest('~@_^', silentQueryOf('')); - - doTest('~@abc ^', queryOf('abc ')); doTest('~@_abc ^', silentQueryOf('abc ')); - doTest('~@abc^ ^', queryOf('abc ')); doTest('~@_abc^ ^', silentQueryOf('abc ')); - doTest('~@ab^c ^', queryOf('abc ')); doTest('~@_ab^c ^', silentQueryOf('abc ')); - doTest('~@^abc ^', queryOf('abc ')); doTest('~@_^abc ^', silentQueryOf('abc ')); - - doTest('Please ask ~@chris^', queryOf('chris')); doTest('Please ask ~@_chris^', silentQueryOf('chris')); - doTest('Please ask ~@chris bobbe^', queryOf('chris bobbe')); doTest('Please ask ~@_chris bobbe^', silentQueryOf('chris bobbe')); - - doTest('~@Rodion Romanovich Raskolnikov^', queryOf('Rodion Romanovich Raskolnikov')); - doTest('~@_Rodion Romanovich Raskolniko^', silentQueryOf('Rodion Romanovich Raskolniko')); - doTest('~@Родион Романович Раскольников^', queryOf('Родион Романович Раскольников')); - doTest('~@_Родион Романович Раскольнико^', silentQueryOf('Родион Романович Раскольнико')); + doTest('~@me@zulip.com^', mention('me@zulip.com')); doTest('~@_me@zulip.com^', silentMention('me@zulip.com')); + doTest('~@me@^zulip.com^', mention('me@zulip.com')); doTest('~@_me@^zulip.com^', silentMention('me@zulip.com')); + doTest('~@me^@zulip.com^', mention('me@zulip.com')); doTest('~@_me^@zulip.com^', silentMention('me@zulip.com')); + doTest('~@^me@zulip.com^', mention('me@zulip.com')); doTest('~@_^me@zulip.com^', silentMention('me@zulip.com')); + + doTest('~@abc^', mention('abc')); doTest('~@_abc^', silentMention('abc')); + doTest(' ~@abc^', mention('abc')); doTest(' ~@_abc^', silentMention('abc')); + doTest('(~@abc^', mention('abc')); doTest('(~@_abc^', silentMention('abc')); + doTest('—~@abc^', mention('abc')); doTest('—~@_abc^', silentMention('abc')); + doTest('"~@abc^', mention('abc')); doTest('"~@_abc^', silentMention('abc')); + doTest('“~@abc^', mention('abc')); doTest('“~@_abc^', silentMention('abc')); + doTest('。~@abc^', mention('abc')); doTest('。~@_abc^', silentMention('abc')); + doTest('«~@abc^', mention('abc')); doTest('«~@_abc^', silentMention('abc')); + + doTest('~@ab^c', mention('ab')); doTest('~@_ab^c', silentMention('ab')); + doTest('~@a^bc', mention('a')); doTest('~@_a^bc', silentMention('a')); + doTest('~@^abc', mention('')); doTest('~@_^abc', silentMention('')); + doTest('~@^', mention('')); doTest('~@_^', silentMention('')); + + doTest('~@abc ^', mention('abc ')); doTest('~@_abc ^', silentMention('abc ')); + doTest('~@abc^ ^', mention('abc ')); doTest('~@_abc^ ^', silentMention('abc ')); + doTest('~@ab^c ^', mention('abc ')); doTest('~@_ab^c ^', silentMention('abc ')); + doTest('~@^abc ^', mention('abc ')); doTest('~@_^abc ^', silentMention('abc ')); + + doTest('Please ask ~@chris^', mention('chris')); doTest('Please ask ~@_chris^', silentMention('chris')); + doTest('Please ask ~@chris bobbe^', mention('chris bobbe')); doTest('Please ask ~@_chris bobbe^', silentMention('chris bobbe')); + + doTest('~@Rodion Romanovich Raskolnikov^', mention('Rodion Romanovich Raskolnikov')); + doTest('~@_Rodion Romanovich Raskolniko^', silentMention('Rodion Romanovich Raskolniko')); + doTest('~@Родион Романович Раскольников^', mention('Родион Романович Раскольников')); + doTest('~@_Родион Романович Раскольнико^', silentMention('Родион Романович Раскольнико')); doTest('If @chris is around, please ask him.^', null); // @ sign is too far away from cursor doTest('If @_chris is around, please ask him.^', null); // @ sign is too far away from cursor }); From a2399a8c40dbdc0674628b0bafd51dc3f4594d3a Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 20 Nov 2024 16:51:13 -0800 Subject: [PATCH 15/21] autocomplete test [nfc]: Move TypingNotifier disable to setup helper This wasn't needed in the topic-autocomplete test -- we only send typing notifications when editing the content input, not the topic. --- test/widgets/autocomplete_test.dart | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/test/widgets/autocomplete_test.dart b/test/widgets/autocomplete_test.dart index 34d0885967..c2de951aad 100644 --- a/test/widgets/autocomplete_test.dart +++ b/test/widgets/autocomplete_test.dart @@ -30,6 +30,9 @@ import 'test_app.dart'; Future setupToComposeInput(WidgetTester tester, { required List users, }) async { + TypingNotifier.debugEnable = false; + addTearDown(TypingNotifier.debugReset); + addTearDown(testBinding.reset); await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); final store = await testBinding.globalStore.perAccount(eg.selfAccount.id); @@ -132,9 +135,6 @@ void main() { final composeInputFinder = await setupToComposeInput(tester, users: [user1, user2, user3]); final store = await testBinding.globalStore.perAccount(eg.selfAccount.id); - TypingNotifier.debugEnable = false; - addTearDown(TypingNotifier.debugReset); - // Options are filtered correctly for query // TODO(#226): Remove this extra edit when this bug is fixed. await tester.enterText(composeInputFinder, 'hello @user '); @@ -186,9 +186,6 @@ void main() { final topicInputFinder = await setupToTopicInput(tester, topics: [topic1, topic2, topic3]); final store = await testBinding.globalStore.perAccount(eg.selfAccount.id); - TypingNotifier.debugEnable = false; - addTearDown(TypingNotifier.debugReset); - // Options are filtered correctly for query // TODO(#226): Remove this extra edit when this bug is fixed. await tester.enterText(topicInputFinder, 'Topic'); From ecd2cb5d4490ffb7b90fb526d2b0fce5b0feb6d8 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 20 Nov 2024 17:08:10 -0800 Subject: [PATCH 16/21] emoji: Make list of emoji to consider for autocomplete or emoji picker This leaves the emojiDisplay field of these objects untested. I skipped that because it seems like pretty boring low-risk code, just invoking emojiDisplayFor. (And emojiDisplayFor has its own tests.) But included a TODO comment for completeness in thinking about what logic there is to test here. Fixes: #669 --- lib/model/emoji.dart | 106 ++++++++++++++++++++++++++++ lib/model/store.dart | 3 + test/model/emoji_test.dart | 137 +++++++++++++++++++++++++++++++++++++ 3 files changed, 246 insertions(+) diff --git a/lib/model/emoji.dart b/lib/model/emoji.dart index 2eea7fcde2..0da76e0e60 100644 --- a/lib/model/emoji.dart +++ b/lib/model/emoji.dart @@ -1,3 +1,5 @@ +import 'package:collection/collection.dart'; + import '../api/model/events.dart'; import '../api/model/initial_snapshot.dart'; import '../api/model/model.dart'; @@ -51,6 +53,37 @@ class TextEmojiDisplay extends EmojiDisplay { TextEmojiDisplay({required super.emojiName}); } +/// An emoji that might be offered in an emoji picker UI. +final class EmojiCandidate { + /// The Zulip "emoji type" for this emoji. + final ReactionType emojiType; + + /// The Zulip "emoji code" for this emoji. + /// + /// This is the value that would appear in [Reaction.emojiCode]. + final String emojiCode; + + /// The Zulip "emoji name" to use for this emoji. + /// + /// This might not be the only name this emoji has; see [aliases]. + final String emojiName; + + /// Additional Zulip "emoji name" values for this emoji, + /// to show in the emoji picker UI. + Iterable get aliases => _aliases ?? const []; + final List? _aliases; + + final EmojiDisplay emojiDisplay; + + EmojiCandidate({ + required this.emojiType, + required this.emojiCode, + required this.emojiName, + required List? aliases, + required this.emojiDisplay, + }) : _aliases = aliases; +} + /// The portion of [PerAccountStore] describing what emoji exist. mixin EmojiStore { /// The realm's custom emoji (for [ReactionType.realmEmoji], @@ -63,6 +96,8 @@ mixin EmojiStore { required String emojiName, }); + Iterable allEmojiCandidates(); + // TODO cut debugServerEmojiData once we can query for lists of emoji; // have tests make those queries end-to-end Map>? get debugServerEmojiData; @@ -148,12 +183,83 @@ class EmojiStoreImpl with EmojiStore { /// retrieving the data. Map>? _serverEmojiData; + List? _allEmojiCandidates; + + EmojiCandidate _emojiCandidateFor({ + required ReactionType emojiType, + required String emojiCode, + required String emojiName, + required List? aliases, + }) { + return EmojiCandidate( + emojiType: emojiType, emojiCode: emojiCode, emojiName: emojiName, + aliases: aliases, + emojiDisplay: emojiDisplayFor( + emojiType: emojiType, emojiCode: emojiCode, emojiName: emojiName)); + } + + List _generateAllCandidates() { + final results = []; + + final namesOverridden = { + for (final emoji in realmEmoji.values) emoji.name, + 'zulip', + }; + // TODO(log) if _serverEmojiData missing + for (final entry in (_serverEmojiData ?? {}).entries) { + final allNames = entry.value; + final String emojiName; + final List? aliases; + if (allNames.any(namesOverridden.contains)) { + final names = allNames.whereNot(namesOverridden.contains).toList(); + if (names.isEmpty) continue; + emojiName = names.removeAt(0); + aliases = names; + } else { + // Most emoji aren't overridden, so avoid copying the list. + emojiName = allNames.first; + aliases = allNames.length > 1 ? allNames.sublist(1) : null; + } + results.add(_emojiCandidateFor( + emojiType: ReactionType.unicodeEmoji, + emojiCode: entry.key, emojiName: emojiName, + aliases: aliases)); + } + + for (final entry in realmEmoji.entries) { + final emojiName = entry.value.name; + if (emojiName == 'zulip') { + // TODO does 'zulip' really override realm emoji? + // (This is copied from zulip-mobile's behavior.) + continue; + } + results.add(_emojiCandidateFor( + emojiType: ReactionType.realmEmoji, + emojiCode: entry.key, emojiName: emojiName, + aliases: null)); + } + + results.add(_emojiCandidateFor( + emojiType: ReactionType.zulipExtraEmoji, + emojiCode: 'zulip', emojiName: 'zulip', + aliases: null)); + + return results; + } + + @override + Iterable allEmojiCandidates() { + return _allEmojiCandidates ??= _generateAllCandidates(); + } + @override void setServerEmojiData(ServerEmojiData data) { _serverEmojiData = data.codeToNames; + _allEmojiCandidates = null; } void handleRealmEmojiUpdateEvent(RealmEmojiUpdateEvent event) { realmEmoji = event.realmEmoji; + _allEmojiCandidates = null; } } diff --git a/lib/model/store.dart b/lib/model/store.dart index 7e230a6fa9..05178142e3 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -408,6 +408,9 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, ChannelStore, Mess notifyListeners(); } + @override + Iterable allEmojiCandidates() => _emoji.allEmojiCandidates(); + EmojiStoreImpl _emoji; //////////////////////////////// diff --git a/test/model/emoji_test.dart b/test/model/emoji_test.dart index f3fdbf3b0a..f89bde205f 100644 --- a/test/model/emoji_test.dart +++ b/test/model/emoji_test.dart @@ -1,7 +1,10 @@ import 'package:checks/checks.dart'; import 'package:test/scaffolding.dart'; +import 'package:zulip/api/model/events.dart'; import 'package:zulip/api/model/model.dart'; +import 'package:zulip/api/route/realm.dart'; import 'package:zulip/model/emoji.dart'; +import 'package:zulip/model/store.dart'; import '../example_data.dart' as eg; @@ -73,6 +76,132 @@ void main() { ..resolvedStillUrl.isNull(); }); }); + + Condition isUnicodeCandidate(String? emojiCode, List? names) { + return (it_) { + final it = it_.isA(); + it.emojiType.equals(ReactionType.unicodeEmoji); + if (emojiCode != null) it.emojiCode.equals(emojiCode); + if (names != null) { + it.emojiName.equals(names.first); + it.aliases.deepEquals(names.sublist(1)); + } + }; + } + + Condition isRealmCandidate({String? emojiCode, String? emojiName}) { + return (it_) { + final it = it_.isA(); + it.emojiType.equals(ReactionType.realmEmoji); + if (emojiCode != null) it.emojiCode.equals(emojiCode); + if (emojiName != null) it.emojiName.equals(emojiName); + it.aliases.isEmpty(); + }; + } + + Condition isZulipCandidate() { + return (it) => it.isA() + ..emojiType.equals(ReactionType.zulipExtraEmoji) + ..emojiCode.equals('zulip') + ..emojiName.equals('zulip') + ..aliases.isEmpty(); + } + + group('allEmojiCandidates', () { + // TODO test emojiDisplay of candidates matches emojiDisplayFor + + PerAccountStore prepare({ + Map realmEmoji = const {}, + Map>? unicodeEmoji, + }) { + final store = eg.store( + initialSnapshot: eg.initialSnapshot(realmEmoji: realmEmoji)); + if (unicodeEmoji != null) { + store.setServerEmojiData(ServerEmojiData(codeToNames: unicodeEmoji)); + } + return store; + } + + test('realm emoji overrides Unicode emoji', () { + final store = prepare(realmEmoji: { + '1': eg.realmEmojiItem(emojiCode: '1', emojiName: 'smiley'), + }, unicodeEmoji: { + '1f642': ['smile'], + '1f603': ['smiley'], + }); + check(store.allEmojiCandidates()).deepEquals([ + isUnicodeCandidate('1f642', ['smile']), + isRealmCandidate(emojiCode: '1', emojiName: 'smiley'), + isZulipCandidate(), + ]); + }); + + test('Unicode emoji with overridden aliases survives with remaining names', () { + final store = prepare(realmEmoji: { + '1': eg.realmEmojiItem(emojiCode: '1', emojiName: 'tangerine'), + }, unicodeEmoji: { + '1f34a': ['orange', 'tangerine', 'mandarin'], + }); + check(store.allEmojiCandidates()).deepEquals([ + isUnicodeCandidate('1f34a', ['orange', 'mandarin']), + isRealmCandidate(emojiCode: '1', emojiName: 'tangerine'), + isZulipCandidate(), + ]); + }); + + test('Unicode emoji with overridden primary name survives with remaining names', () { + final store = prepare(realmEmoji: { + '1': eg.realmEmojiItem(emojiCode: '1', emojiName: 'orange'), + }, unicodeEmoji: { + '1f34a': ['orange', 'tangerine', 'mandarin'], + }); + check(store.allEmojiCandidates()).deepEquals([ + isUnicodeCandidate('1f34a', ['tangerine', 'mandarin']), + isRealmCandidate(emojiCode: '1', emojiName: 'orange'), + isZulipCandidate(), + ]); + }); + + test('updates on setServerEmojiData', () { + final store = prepare(); + check(store.allEmojiCandidates()).deepEquals([ + isZulipCandidate(), + ]); + + store.setServerEmojiData(ServerEmojiData(codeToNames: { + '1f642': ['smile'], + })); + check(store.allEmojiCandidates()).deepEquals([ + isUnicodeCandidate('1f642', ['smile']), + isZulipCandidate(), + ]); + }); + + test('updates on RealmEmojiUpdateEvent', () { + final store = prepare(); + check(store.allEmojiCandidates()).deepEquals([ + isZulipCandidate(), + ]); + + store.handleEvent(RealmEmojiUpdateEvent(id: 1, realmEmoji: { + '1': eg.realmEmojiItem(emojiCode: '1', emojiName: 'happy'), + })); + check(store.allEmojiCandidates()).deepEquals([ + isRealmCandidate(emojiCode: '1', emojiName: 'happy'), + isZulipCandidate(), + ]); + }); + + test('memoizes result', () { + final store = prepare(realmEmoji: { + '1': eg.realmEmojiItem(emojiCode: '1', emojiName: 'happy'), + }, unicodeEmoji: { + '1f642': ['smile'], + }); + final candidates = store.allEmojiCandidates(); + check(store.allEmojiCandidates()).identicalTo(candidates); + }); + }); } extension EmojiDisplayChecks on Subject { @@ -87,3 +216,11 @@ extension ImageEmojiDisplayChecks on Subject { Subject get resolvedUrl => has((x) => x.resolvedUrl, 'resolvedUrl'); Subject get resolvedStillUrl => has((x) => x.resolvedStillUrl, 'resolvedStillUrl'); } + +extension EmojiCandidateChecks on Subject { + Subject get emojiType => has((x) => x.emojiType, 'emojiType'); + Subject get emojiCode => has((x) => x.emojiCode, 'emojiCode'); + Subject get emojiName => has((x) => x.emojiName, 'emojiName'); + Subject> get aliases => has((x) => x.aliases, 'aliases'); + Subject get emojiDisplay => has((x) => x.emojiDisplay, 'emojiDisplay'); +} From cc41c082f20159b641b66e2047071aa681e71088 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 20 Nov 2024 17:08:50 -0800 Subject: [PATCH 17/21] emoji: Define which emoji match a given autocomplete query The initViewModel method can't yet be called, because these EmojiAutocompleteQuery objects aren't yet ever constructed in the actual app. So for the moment it throws UnimplementedError, letting us save for a later commit the implementation of the class it should return an instance of. --- lib/model/emoji.dart | 61 ++++++++++++++++ test/model/emoji_test.dart | 145 +++++++++++++++++++++++++++++++++++++ 2 files changed, 206 insertions(+) diff --git a/lib/model/emoji.dart b/lib/model/emoji.dart index 0da76e0e60..b90af72e8e 100644 --- a/lib/model/emoji.dart +++ b/lib/model/emoji.dart @@ -1,9 +1,13 @@ import 'package:collection/collection.dart'; +import 'package:flutter/foundation.dart'; import '../api/model/events.dart'; import '../api/model/initial_snapshot.dart'; import '../api/model/model.dart'; import '../api/route/realm.dart'; +import 'autocomplete.dart'; +import 'narrow.dart'; +import 'store.dart'; /// An emoji, described by how to display it in the UI. sealed class EmojiDisplay { @@ -263,3 +267,60 @@ class EmojiStoreImpl with EmojiStore { _allEmojiCandidates = null; } } + +class EmojiAutocompleteQuery extends ComposeAutocompleteQuery { + EmojiAutocompleteQuery(super.raw) + : _adjusted = _adjustQuery(raw); + + final String _adjusted; + + static String _adjustQuery(String raw) { + return raw.toLowerCase().replaceAll(' ', '_'); // TODO(#1067) remove diacritics too + } + + @override + ComposeAutocompleteView initViewModel(PerAccountStore store, Narrow narrow) { + throw UnimplementedError(); // TODO(#670) + } + + // Compare get_emoji_matcher in Zulip web:shared/src/typeahead.ts . + bool matches(EmojiCandidate candidate) { + if (candidate.emojiDisplay case UnicodeEmojiDisplay(:var emojiUnicode)) { + if (_adjusted == emojiUnicode) return true; + } + return _nameMatches(candidate.emojiName) + || candidate.aliases.any((alias) => _nameMatches(alias)); + } + + // Compare query_matches_string_in_order in Zulip web:shared/src/typeahead.ts . + bool _nameMatches(String emojiName) { + // TODO(#1067) this assumes emojiName is already lower-case (and no diacritics) + const String separator = '_'; + + if (!_adjusted.contains(separator)) { + // If the query is a single token (doesn't contain a separator), + // the match can be anywhere in the string. + return emojiName.contains(_adjusted); + } + + // If there is a separator in the query, then we + // require the match to start at the start of a token. + // (E.g. for 'ab_cd_ef', query could be 'ab_c' or 'cd_ef', + // but not 'b_cd_ef'.) + return emojiName.startsWith(_adjusted) + || emojiName.contains(separator + _adjusted); + } + + @override + String toString() { + return '${objectRuntimeType(this, 'EmojiAutocompleteQuery')}($raw)'; + } + + @override + bool operator ==(Object other) { + return other is EmojiAutocompleteQuery && other.raw == raw; + } + + @override + int get hashCode => Object.hash('EmojiAutocompleteQuery', raw); +} diff --git a/test/model/emoji_test.dart b/test/model/emoji_test.dart index f89bde205f..6e923d9355 100644 --- a/test/model/emoji_test.dart +++ b/test/model/emoji_test.dart @@ -202,6 +202,151 @@ void main() { check(store.allEmojiCandidates()).identicalTo(candidates); }); }); + + group('EmojiAutocompleteQuery.matches', () { + EmojiCandidate unicode(List names, {String? emojiCode}) { + emojiCode ??= '10ffff'; + return EmojiCandidate(emojiType: ReactionType.unicodeEmoji, + emojiCode: emojiCode, + emojiName: names.first, aliases: names.sublist(1), + emojiDisplay: UnicodeEmojiDisplay( + emojiName: names.first, + emojiUnicode: tryParseEmojiCodeToUnicode(emojiCode)!)); + } + + bool matchesName(String query, String emojiName) { + return EmojiAutocompleteQuery(query).matches(unicode([emojiName])); + } + + test('one-word query matches anywhere in name', () { + check(matchesName('', 'smile')).isTrue(); + check(matchesName('s', 'smile')).isTrue(); + check(matchesName('sm', 'smile')).isTrue(); + check(matchesName('smile', 'smile')).isTrue(); + check(matchesName('m', 'smile')).isTrue(); + check(matchesName('mile', 'smile')).isTrue(); + check(matchesName('e', 'smile')).isTrue(); + + check(matchesName('smiley', 'smile')).isFalse(); + check(matchesName('a', 'smile')).isFalse(); + + check(matchesName('o', 'open_book')).isTrue(); + check(matchesName('open', 'open_book')).isTrue(); + check(matchesName('pe', 'open_book')).isTrue(); + check(matchesName('boo', 'open_book')).isTrue(); + check(matchesName('ok', 'open_book')).isTrue(); + }); + + test('multi-word query matches from start of a word', () { + check(matchesName('open_', 'open_book')).isTrue(); + check(matchesName('open_b', 'open_book')).isTrue(); + check(matchesName('open_book', 'open_book')).isTrue(); + + check(matchesName('pen_', 'open_book')).isFalse(); + check(matchesName('n_b', 'open_book')).isFalse(); + + check(matchesName('blue_dia', 'large_blue_diamond')).isTrue(); + }); + + test('spaces in query behave as underscores', () { + check(matchesName('open ', 'open_book')).isTrue(); + check(matchesName('open b', 'open_book')).isTrue(); + check(matchesName('open book', 'open_book')).isTrue(); + + check(matchesName('pen ', 'open_book')).isFalse(); + check(matchesName('n b', 'open_book')).isFalse(); + + check(matchesName('blue dia', 'large_blue_diamond')).isTrue(); + }); + + test('query is lower-cased', () { + check(matchesName('Smi', 'smile')).isTrue(); + }); + + test('query matches aliases same way as primary name', () { + bool matchesNames(String query, List names) { + return EmojiAutocompleteQuery(query).matches(unicode(names)); + } + + check(matchesNames('a', ['a', 'b'])).isTrue(); + check(matchesNames('b', ['a', 'b'])).isTrue(); + check(matchesNames('c', ['a', 'b'])).isFalse(); + + check(matchesNames('pe', ['x', 'open_book'])).isTrue(); + check(matchesNames('ok', ['x', 'open_book'])).isTrue(); + + check(matchesNames('open_', ['x', 'open_book'])).isTrue(); + check(matchesNames('open b', ['x', 'open_book'])).isTrue(); + check(matchesNames('pen_', ['x', 'open_book'])).isFalse(); + + check(matchesNames('Smi', ['x', 'smile'])).isTrue(); + }); + + test('query matches literal Unicode value', () { + bool matchesLiteral(String query, String emojiCode, {required String aka}) { + assert(aka == query); + return EmojiAutocompleteQuery(query) + .matches(unicode(['asdf'], emojiCode: emojiCode)); + } + + // Matching the code, in hex, doesn't count. + check(matchesLiteral('1f642', aka: '1f642', '1f642')).isFalse(); + + // Matching the Unicode value the code describes does count… + check(matchesLiteral('🙂', aka: '\u{1f642}', '1f642')).isTrue(); + // … and failing to match it doesn't make a match. + check(matchesLiteral('🙁', aka: '\u{1f641}', '1f642')).isFalse(); + + // Multi-code-point emoji work fine. + check(matchesLiteral('🏳‍🌈', aka: '\u{1f3f3}\u{200d}\u{1f308}', + '1f3f3-200d-1f308')).isTrue(); + // Only exact matches count; no partial matches. + check(matchesLiteral('🏳', aka: '\u{1f3f3}', + '1f3f3-200d-1f308')).isFalse(); + check(matchesLiteral('‍🌈', aka: '\u{200d}\u{1f308}', + '1f3f3-200d-1f308')).isFalse(); + check(matchesLiteral('🏳‍🌈', aka: '\u{1f3f3}\u{200d}\u{1f308}', + '1f3f3')).isFalse(); + }); + + test('can match realm emoji', () { + EmojiCandidate realmCandidate(String emojiName) { + return EmojiCandidate( + emojiType: ReactionType.realmEmoji, + emojiCode: '1', emojiName: emojiName, aliases: null, + emojiDisplay: ImageEmojiDisplay( + emojiName: emojiName, + resolvedUrl: eg.realmUrl.resolve('/emoji/1.png'), + resolvedStillUrl: eg.realmUrl.resolve('/emoji/1-still.png'))); + } + + check(EmojiAutocompleteQuery('eqeq') + .matches(realmCandidate('eqeq'))).isTrue(); + check(EmojiAutocompleteQuery('open_') + .matches(realmCandidate('open_book'))).isTrue(); + check(EmojiAutocompleteQuery('n_b') + .matches(realmCandidate('open_book'))).isFalse(); + check(EmojiAutocompleteQuery('blue dia') + .matches(realmCandidate('large_blue_diamond'))).isTrue(); + check(EmojiAutocompleteQuery('Smi') + .matches(realmCandidate('smile'))).isTrue(); + }); + + test('can match Zulip extra emoji', () { + final store = eg.store(); + final zulipCandidate = EmojiCandidate( + emojiType: ReactionType.zulipExtraEmoji, + emojiCode: 'zulip', emojiName: 'zulip', aliases: null, + emojiDisplay: store.emojiDisplayFor( + emojiType: ReactionType.zulipExtraEmoji, + emojiCode: 'zulip', emojiName: 'zulip')); + + check(EmojiAutocompleteQuery('z').matches(zulipCandidate)).isTrue(); + check(EmojiAutocompleteQuery('Zulip').matches(zulipCandidate)).isTrue(); + check(EmojiAutocompleteQuery('p').matches(zulipCandidate)).isTrue(); + check(EmojiAutocompleteQuery('x').matches(zulipCandidate)).isFalse(); + }); + }); } extension EmojiDisplayChecks on Subject { From 31d71225370dfefe2a6e8b8f90e232b5413342e7 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 20 Nov 2024 17:09:06 -0800 Subject: [PATCH 18/21] emoji: Add view-model for autocomplete, EmojiAutocompleteView As of this commit, it's not yet possible in the app to initiate an emoji autocomplete interaction. So in the widgets code that would consume the results of such an interaction, we just throw for now, leaving that to be implemented in a later commit. --- lib/model/autocomplete.dart | 19 +++++++++ lib/model/emoji.dart | 28 ++++++++++++++ lib/widgets/autocomplete.dart | 3 ++ test/model/emoji_test.dart | 73 +++++++++++++++++++++++++++++++++++ 4 files changed, 123 insertions(+) diff --git a/lib/model/autocomplete.dart b/lib/model/autocomplete.dart index 4671bf80bf..5f3726b2cc 100644 --- a/lib/model/autocomplete.dart +++ b/lib/model/autocomplete.dart @@ -7,6 +7,7 @@ import '../api/model/events.dart'; import '../api/model/model.dart'; import '../api/route/channels.dart'; import '../widgets/compose_box.dart'; +import 'emoji.dart'; import 'narrow.dart'; import 'store.dart'; @@ -142,6 +143,7 @@ class AutocompleteIntent { class AutocompleteViewManager { final Set _mentionAutocompleteViews = {}; final Set _topicAutocompleteViews = {}; + final Set _emojiAutocompleteViews = {}; AutocompleteDataCache autocompleteDataCache = AutocompleteDataCache(); @@ -165,6 +167,16 @@ class AutocompleteViewManager { assert(removed); } + void registerEmojiAutocomplete(EmojiAutocompleteView view) { + final added = _emojiAutocompleteViews.add(view); + assert(added); + } + + void unregisterEmojiAutocomplete(EmojiAutocompleteView view) { + final removed = _emojiAutocompleteViews.remove(view); + assert(removed); + } + void handleRealmUserRemoveEvent(RealmUserRemoveEvent event) { autocompleteDataCache.invalidateUser(event.userId); } @@ -683,6 +695,13 @@ class AutocompleteResult {} /// and managed by some [ComposeAutocompleteView]. sealed class ComposeAutocompleteResult extends AutocompleteResult {} +/// An emoji chosen in an autocomplete interaction, via [EmojiAutocompleteView]. +class EmojiAutocompleteResult extends ComposeAutocompleteResult { + EmojiAutocompleteResult(this.candidate); + + final EmojiCandidate candidate; +} + /// A result from an @-mention autocomplete interaction, /// managed by a [MentionAutocompleteView]. /// diff --git a/lib/model/emoji.dart b/lib/model/emoji.dart index b90af72e8e..3a92c7373d 100644 --- a/lib/model/emoji.dart +++ b/lib/model/emoji.dart @@ -268,6 +268,34 @@ class EmojiStoreImpl with EmojiStore { } } +class EmojiAutocompleteView extends AutocompleteView { + EmojiAutocompleteView._({required super.store, required super.query}); + + factory EmojiAutocompleteView.init({ + required PerAccountStore store, + required EmojiAutocompleteQuery query, + }) { + final view = EmojiAutocompleteView._(store: store, query: query); + store.autocompleteViewManager.registerEmojiAutocomplete(view); + return view; + } + + @override + Future?> computeResults() async { + // TODO(#1068): rank emoji results (popular, realm, other; exact match, prefix, other) + final results = []; + if (await filterCandidates(filter: _testCandidate, + candidates: store.allEmojiCandidates(), results: results)) { + return null; + } + return results; + } + + EmojiAutocompleteResult? _testCandidate(EmojiAutocompleteQuery query, EmojiCandidate candidate) { + return query.matches(candidate) ? EmojiAutocompleteResult(candidate) : null; + } +} + class EmojiAutocompleteQuery extends ComposeAutocompleteQuery { EmojiAutocompleteQuery(super.raw) : _adjusted = _adjustQuery(raw); diff --git a/lib/widgets/autocomplete.dart b/lib/widgets/autocomplete.dart index 9abe289920..76a83a3c0e 100644 --- a/lib/widgets/autocomplete.dart +++ b/lib/widgets/autocomplete.dart @@ -187,6 +187,8 @@ class ComposeAutocomplete extends AutocompleteField _MentionAutocompleteItem(option: option), + EmojiAutocompleteResult() => throw UnimplementedError(), // TODO(#670) }; return InkWell( onTap: () { diff --git a/test/model/emoji_test.dart b/test/model/emoji_test.dart index 6e923d9355..3a0ddc5e95 100644 --- a/test/model/emoji_test.dart +++ b/test/model/emoji_test.dart @@ -3,6 +3,7 @@ import 'package:test/scaffolding.dart'; import 'package:zulip/api/model/events.dart'; import 'package:zulip/api/model/model.dart'; import 'package:zulip/api/route/realm.dart'; +import 'package:zulip/model/autocomplete.dart'; import 'package:zulip/model/emoji.dart'; import 'package:zulip/model/store.dart'; @@ -203,6 +204,74 @@ void main() { }); }); + group('EmojiAutocompleteView', () { + Condition isUnicodeResult({String? emojiCode, List? names}) { + return (it) => it.isA().candidate.which( + isUnicodeCandidate(emojiCode, names)); + } + + Condition isRealmResult({String? emojiCode, String? emojiName}) { + return (it) => it.isA().candidate.which( + isRealmCandidate(emojiCode: emojiCode, emojiName: emojiName)); + } + + Condition isZulipResult() { + return (it) => it.isA().candidate.which( + isZulipCandidate()); + } + + PerAccountStore prepare({ + Map realmEmoji = const {}, + Map>? unicodeEmoji, + }) { + final store = eg.store( + initialSnapshot: eg.initialSnapshot(realmEmoji: { + for (final MapEntry(:key, :value) in realmEmoji.entries) + key: eg.realmEmojiItem(emojiCode: key, emojiName: value), + })); + if (unicodeEmoji != null) { + store.setServerEmojiData(ServerEmojiData(codeToNames: unicodeEmoji)); + } + return store; + } + + test('results can include all three emoji types', () async { + final store = prepare( + realmEmoji: {'1': 'happy'}, unicodeEmoji: {'1f642': ['smile']}); + final view = EmojiAutocompleteView.init(store: store, + query: EmojiAutocompleteQuery('')); + bool done = false; + view.addListener(() { done = true; }); + await Future(() {}); + check(done).isTrue(); + check(view.results).deepEquals([ + isUnicodeResult(names: ['smile']), + isRealmResult(emojiName: 'happy'), + isZulipResult(), + ]); + }); + + test('results update after query change', () async { + final store = prepare( + realmEmoji: {'1': 'happy'}, unicodeEmoji: {'1f642': ['smile']}); + final view = EmojiAutocompleteView.init(store: store, + query: EmojiAutocompleteQuery('h')); + bool done = false; + view.addListener(() { done = true; }); + await Future(() {}); + check(done).isTrue(); + check(view.results).single.which( + isRealmResult(emojiName: 'happy')); + + done = false; + view.query = EmojiAutocompleteQuery('s'); + await Future(() {}); + check(done).isTrue(); + check(view.results).single.which( + isUnicodeResult(names: ['smile'])); + }); + }); + group('EmojiAutocompleteQuery.matches', () { EmojiCandidate unicode(List names, {String? emojiCode}) { emojiCode ??= '10ffff'; @@ -369,3 +438,7 @@ extension EmojiCandidateChecks on Subject { Subject> get aliases => has((x) => x.aliases, 'aliases'); Subject get emojiDisplay => has((x) => x.emojiDisplay, 'emojiDisplay'); } + +extension EmojiAutocompleteResultChecks on Subject { + Subject get candidate => has((x) => x.candidate, 'candidate'); +} From 2673b6a6171d6dba28497051fb44f09f1da6fad4 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 19 Nov 2024 15:56:09 -0800 Subject: [PATCH 19/21] autocomplete: Identify when the user intends an emoji autocomplete The "forbid preceding ':'" wrinkle is one I discovered because of writing tests: I wrote down the '::^' test, was surprised to find that it failed, and then went back and extended the regexp to make it pass. For this commit we temporarily intercept the query at the AutocompleteField widget, to avoid invoking the widgets that are still unimplemented. That lets us defer those widgets' logic to a separate later commit. --- lib/model/autocomplete.dart | 31 ++++++++++++++ lib/widgets/autocomplete.dart | 4 +- test/model/autocomplete_test.dart | 70 +++++++++++++++++++++++++++++++ 3 files changed, 104 insertions(+), 1 deletion(-) diff --git a/lib/model/autocomplete.dart b/lib/model/autocomplete.dart index 5f3726b2cc..7a59f636e3 100644 --- a/lib/model/autocomplete.dart +++ b/lib/model/autocomplete.dart @@ -39,6 +39,9 @@ extension ComposeContentAutocomplete on ComposeContentController { if (charAtPos == '@') { final match = _mentionIntentRegex.matchAsPrefix(textUntilCursor, pos); if (match == null) continue; + } else if (charAtPos == ':') { + final match = _emojiIntentRegex.matchAsPrefix(textUntilCursor, pos); + if (match == null) continue; } else { continue; } @@ -53,6 +56,10 @@ extension ComposeContentAutocomplete on ComposeContentController { final match = _mentionIntentRegex.matchAsPrefix(textUntilCursor, pos); if (match == null) continue; query = MentionAutocompleteQuery(match[2]!, silent: match[1]! == '_'); + } else if (charAtPos == ':') { + final match = _emojiIntentRegex.matchAsPrefix(textUntilCursor, pos); + if (match == null) continue; + query = EmojiAutocompleteQuery(match[1]!); } else { continue; } @@ -98,6 +105,30 @@ final RegExp _mentionIntentRegex = (() { unicode: true); })(); +final RegExp _emojiIntentRegex = (() { + // Similar reasoning as in _mentionIntentRegex. + // Specifically forbid a preceding ":", though, to make "::" not a query. + const before = r'(?<=^|\s|\p{Punctuation})(? { AutocompleteIntent({ diff --git a/lib/widgets/autocomplete.dart b/lib/widgets/autocomplete.dart index 76a83a3c0e..407be7f3f2 100644 --- a/lib/widgets/autocomplete.dart +++ b/lib/widgets/autocomplete.dart @@ -1,5 +1,6 @@ import 'package:flutter/material.dart'; +import '../model/emoji.dart'; import 'content.dart'; import 'store.dart'; import '../model/autocomplete.dart'; @@ -38,7 +39,8 @@ class _AutocompleteFieldState MentionAutocompleteQuery(raw, silent: false); MentionAutocompleteQuery silentMention(String raw) => MentionAutocompleteQuery(raw, silent: true); + EmojiAutocompleteQuery emoji(String raw) => EmojiAutocompleteQuery(raw); doTest('', null); doTest('^', null); doTest('!@#\$%&*()_+', null); + // @-mentions. + doTest('^@', null); doTest('^@_', null); doTest('^@abc', null); doTest('^@_abc', null); doTest('@abc', null); doTest('@_abc', null); // (no cursor) @@ -169,6 +173,72 @@ void main() { doTest('~@_Родион Романович Раскольнико^', silentMention('Родион Романович Раскольнико')); doTest('If @chris is around, please ask him.^', null); // @ sign is too far away from cursor doTest('If @_chris is around, please ask him.^', null); // @ sign is too far away from cursor + + // Emoji (":smile:"). + + // Basic positive examples, to contrast with all the negative examples below. + doTest('~:^', emoji('')); + doTest('~:a^', emoji('a')); + doTest('~:a ^', emoji('a ')); + doTest('~:a_^', emoji('a_')); + doTest('~:a b^', emoji('a b')); + doTest('ok ~:s^', emoji('s')); + doTest('this: ~:s^', emoji('s')); + + doTest('^:', null); + doTest('^:abc', null); + doTest(':abc', null); // (no cursor) + + // Avoid interpreting colons in normal prose as queries. + doTest(': ^', null); + doTest(':\n^', null); + doTest('this:^', null); + doTest('this: ^', null); + doTest('là ~:^', emoji('')); // ambiguous in French prose, tant pis + doTest('là : ^', null); + doTest('8:30^', null); + + // Avoid interpreting already-entered `:foo:` syntax as queries. + doTest(':smile:^', null); + + // Avoid interpreting emoticons as queries. + doTest(':-^', null); + doTest(':)^', null); doTest(':-)^', null); + doTest(':(^', null); doTest(':-(^', null); + doTest(':/^', null); doTest(':-/^', null); + doTest('~:p^', emoji('p')); // ambiguously an emoticon + doTest(':-p^', null); + + // Avoid interpreting as queries some ways colons appear in source code. + doTest('::^', null); + doTest(':<^', null); + doTest(':=^', null); + + // Emoji names may have letters and numbers in various scripts. + // (A few appear in the server's list of Unicode emoji; + // many more might be in a given realm's custom emoji.) + doTest('~:コ^', emoji('コ')); + doTest('~:空^', emoji('空')); + doTest('~:φ^', emoji('φ')); + doTest('~:100^', emoji('100')); + doTest('~:１^', emoji('１')); // U+FF11 FULLWIDTH DIGIT ONE + doTest('~:٢^', emoji('٢')); // U+0662 ARABIC-INDIC DIGIT TWO + + // Accept punctuation before the emoji: opening… + doTest('(~:^', emoji('')); doTest('(~:a^', emoji('a')); + doTest('[~:^', emoji('')); doTest('[~:a^', emoji('a')); + doTest('«~:^', emoji('')); doTest('«~:a^', emoji('a')); + doTest('（~:^', emoji('')); doTest('（~:a^', emoji('a')); + // … closing… + doTest(')~:^', emoji('')); doTest(')~:a^', emoji('a')); + doTest(']~:^', emoji('')); doTest(']~:a^', emoji('a')); + doTest('»~:^', emoji('')); doTest('»~:a^', emoji('a')); + doTest('）~:^', emoji('')); doTest('）~:a^', emoji('a')); + // … and other. + doTest('.~:^', emoji('')); doTest('.~:a^', emoji('a')); + doTest(',~:^', emoji('')); doTest(',~:a^', emoji('a')); + doTest('，~:^', emoji('')); doTest('，~:a^', emoji('a')); + doTest('。~:^', emoji('')); doTest('。~:a^', emoji('a')); }); test('MentionAutocompleteView misc', () async { From 8d84178c10516f8c9dbcb90cfde8e7a00b74f78f Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 25 Nov 2024 00:09:43 -0800 Subject: [PATCH 20/21] autocomplete: Accept emoji queries for :+1: and with dashes This makes this logic a bit more complicated, but also more explicitly related to the set of possible emoji names, and more fully aligned with that set. --- lib/model/autocomplete.dart | 38 +++++++++++++++++++++++++++---- test/model/autocomplete_test.dart | 12 ++++++++++ 2 files changed, 46 insertions(+), 4 deletions(-) diff --git a/lib/model/autocomplete.dart b/lib/model/autocomplete.dart index 7a59f636e3..6007dd6730 100644 --- a/lib/model/autocomplete.dart +++ b/lib/model/autocomplete.dart @@ -114,18 +114,48 @@ final RegExp _emojiIntentRegex = (() { // meaning "whitespace and punctuation, except not `:`": // r'(?<=^|[[\s\p{Punctuation}]--[:]])' - /// Characters that might be meant as part of (a query for) an emoji's name, - /// other than whitespace. + // What possible emoji queries do we want to anticipate? + // + // First, look only for queries aimed at emoji names (and aliases); + // there's little point in searching by literal emoji here, because once the + // user has entered a literal emoji they can simply leave it in. + // (Searching by literal emoji is useful, by contrast, for adding a reaction.) + // + // Then, what are the possible names (including aliases)? + // For custom emoji, the names the server allows are r'^[0-9a-z_-]*[0-9a-z]$'; + // see check_valid_emoji_name in zerver/lib/emoji.py. + // So only ASCII lowercase alnum, underscore, and dash. + // A few Unicode emoji have more general names in the server's list: + // Latin letters with diacritics, a few kana and kanji, and the name "+1". + // (And the only "Zulip extra emoji" has one name, "zulip".) + // Details: https://github.com/zulip/zulip-flutter/pull/1069#discussion_r1855964953 + // + // We generalize [0-9a-z] to "any letter or number". + // That handles the existing names except "+1", plus a potential future + // loosening of the constraints on custom emoji's names. + // + // Then "+1" we take as a special case, without generalizing, + // in order to recognize that name without adding false positives. + // + // Even though there could be a custom emoji whose name begins with "-", + // we reject queries that begin that way: ":-" is much more likely to be + // the start of an emoticon. + + /// Characters that might be meant as part of (a query for) an emoji's name + /// at any point in the query. const nameCharacters = r'_\p{Letter}\p{Number}'; return RegExp(unicode: true, before + r':' + r'(|' + // Recognize '+' only as part of '+1', the only emoji name that has it. + + r'\+1?|' // Reject on whitespace right after ':'; interpret that // as the user choosing to get out of the emoji autocomplete. - + r'[' + nameCharacters + r']' - + r'[\s' + nameCharacters + r']*' + // Similarly reject starting with ':-', which is common for emoticons. + + r'[' + nameCharacters + r']' + + r'[-\s' + nameCharacters + r']*' + r')$'); })(); diff --git a/test/model/autocomplete_test.dart b/test/model/autocomplete_test.dart index 6f3870509f..9d6667ca3f 100644 --- a/test/model/autocomplete_test.dart +++ b/test/model/autocomplete_test.dart @@ -224,6 +224,18 @@ void main() { doTest('~:１^', emoji('１')); // U+FF11 FULLWIDTH DIGIT ONE doTest('~:٢^', emoji('٢')); // U+0662 ARABIC-INDIC DIGIT TWO + // Emoji names may have dashes '-'. + doTest('~:e-m^', emoji('e-m')); + doTest('~:jack-o-l^', emoji('jack-o-l')); + + // Just one emoji has a '+' in its name, namely ':+1:'. + doTest('~:+^', emoji('+')); + doTest('~:+1^', emoji('+1')); + doTest(':+2^', null); + doTest(':+100^', null); + doTest(':+1 ^', null); + doTest(':1+1^', null); + // Accept punctuation before the emoji: opening… doTest('(~:^', emoji('')); doTest('(~:a^', emoji('a')); doTest('[~:^', emoji('')); doTest('[~:a^', emoji('a')); From 0fd1d648bc08c708c8f18872f495f5877aac61b6 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 20 Nov 2024 17:04:36 -0800 Subject: [PATCH 21/21] emoji: Finish emoji autocomplete for compose box Fixes: #670 --- lib/model/emoji.dart | 4 +- lib/widgets/autocomplete.dart | 54 +++++++++++-- test/widgets/autocomplete_test.dart | 119 +++++++++++++++++++++++++--- 3 files changed, 159 insertions(+), 18 deletions(-) diff --git a/lib/model/emoji.dart b/lib/model/emoji.dart index 3a92c7373d..d3fcaad67f 100644 --- a/lib/model/emoji.dart +++ b/lib/model/emoji.dart @@ -307,8 +307,8 @@ class EmojiAutocompleteQuery extends ComposeAutocompleteQuery { } @override - ComposeAutocompleteView initViewModel(PerAccountStore store, Narrow narrow) { - throw UnimplementedError(); // TODO(#670) + EmojiAutocompleteView initViewModel(PerAccountStore store, Narrow narrow) { + return EmojiAutocompleteView.init(store: store, query: this); } // Compare get_emoji_matcher in Zulip web:shared/src/typeahead.ts . diff --git a/lib/widgets/autocomplete.dart b/lib/widgets/autocomplete.dart index 407be7f3f2..ba0d003a8f 100644 --- a/lib/widgets/autocomplete.dart +++ b/lib/widgets/autocomplete.dart @@ -2,6 +2,7 @@ import 'package:flutter/material.dart'; import '../model/emoji.dart'; import 'content.dart'; +import 'emoji.dart'; import 'store.dart'; import '../model/autocomplete.dart'; import '../model/compose.dart'; @@ -39,8 +40,7 @@ class _AutocompleteFieldState _MentionAutocompleteItem(option: option), - EmojiAutocompleteResult() => throw UnimplementedError(), // TODO(#670) + EmojiAutocompleteResult() => _EmojiAutocompleteItem(option: option), }; return InkWell( onTap: () { @@ -247,6 +247,50 @@ class _MentionAutocompleteItem extends StatelessWidget { } } +class _EmojiAutocompleteItem extends StatelessWidget { + const _EmojiAutocompleteItem({required this.option}); + + final EmojiAutocompleteResult option; + + static const _size = 32.0; + static const _notoColorEmojiTextSize = 25.7; + + @override + Widget build(BuildContext context) { + final store = PerAccountStoreWidget.of(context); + final candidate = option.candidate; + + final emojiDisplay = candidate.emojiDisplay.resolve(store.userSettings); + final Widget? glyph = switch (emojiDisplay) { + ImageEmojiDisplay() => + ImageEmojiWidget(size: _size, emojiDisplay: emojiDisplay), + UnicodeEmojiDisplay() => + UnicodeEmojiWidget( + size: _size, notoColorEmojiTextSize: _notoColorEmojiTextSize, + emojiDisplay: emojiDisplay), + TextEmojiDisplay() => null, // The text is already shown separately. + }; + + final label = candidate.aliases.isEmpty + ? candidate.emojiName + : [candidate.emojiName, ...candidate.aliases].join(", "); // TODO(#1080) + + return Padding( + padding: const EdgeInsets.symmetric(horizontal: 16.0, vertical: 8.0), + child: Row(children: [ + if (glyph != null) ...[ + glyph, + const SizedBox(width: 8), + ], + Expanded( + child: Text( + maxLines: 2, + overflow: TextOverflow.ellipsis, + label)), + ])); + } +} + class TopicAutocomplete extends AutocompleteField { const TopicAutocomplete({ super.key, diff --git a/test/widgets/autocomplete_test.dart b/test/widgets/autocomplete_test.dart index c2de951aad..24ae2dba83 100644 --- a/test/widgets/autocomplete_test.dart +++ b/test/widgets/autocomplete_test.dart @@ -1,10 +1,14 @@ import 'package:checks/checks.dart'; import 'package:flutter/material.dart'; +import 'package:flutter_checks/flutter_checks.dart'; import 'package:flutter_test/flutter_test.dart'; +import 'package:zulip/api/model/events.dart'; import 'package:zulip/api/model/model.dart'; import 'package:zulip/api/route/messages.dart'; import 'package:zulip/api/route/channels.dart'; +import 'package:zulip/api/route/realm.dart'; import 'package:zulip/model/compose.dart'; +import 'package:zulip/model/emoji.dart'; import 'package:zulip/model/localizations.dart'; import 'package:zulip/model/narrow.dart'; import 'package:zulip/model/store.dart'; @@ -28,7 +32,7 @@ import 'test_app.dart'; /// The caller must set [debugNetworkImageHttpClientProvider] back to null /// before the end of the test. Future setupToComposeInput(WidgetTester tester, { - required List users, + List users = const [], }) async { TypingNotifier.debugEnable = false; addTearDown(TypingNotifier.debugReset); @@ -108,19 +112,20 @@ Future setupToTopicInput(WidgetTester tester, { return finder; } -void main() { - TestZulipBinding.ensureInitialized(); +Finder findNetworkImage(String url) { + return find.byWidgetPredicate((widget) => switch(widget) { + Image(image: NetworkImage(url: var imageUrl)) when imageUrl == url + => true, + _ => false, + }); +} - group('ComposeAutocomplete', () { +typedef ExpectedEmoji = (String label, EmojiDisplay display); - Finder findNetworkImage(String url) { - return find.byWidgetPredicate((widget) => switch(widget) { - Image(image: NetworkImage(url: var imageUrl)) when imageUrl == url - => true, - _ => false, - }); - } +void main() { + TestZulipBinding.ensureInitialized(); + group('@-mentions', () { void checkUserShown(User user, PerAccountStore store, {required bool expected}) { check(find.text(user.fullName).evaluate().length).equals(expected ? 1 : 0); final avatarFinder = @@ -174,6 +179,98 @@ void main() { }); }); + group('emoji', () { + void checkEmojiShown(ExpectedEmoji option, {required bool expected}) { + final (label, display) = option; + final labelSubject = check(find.text(label)); + expected ? labelSubject.findsOne() : labelSubject.findsNothing(); + + final Subject displaySubject; + switch (display) { + case UnicodeEmojiDisplay(): + displaySubject = check(find.text(display.emojiUnicode)); + case ImageEmojiDisplay(): + displaySubject = check(findNetworkImage(display.resolvedUrl.toString())); + case TextEmojiDisplay(): + // We test this case in the "text emoji" test below, + // but that doesn't use this helper method. + throw UnimplementedError(); + } + expected ? displaySubject.findsOne(): displaySubject.findsNothing(); + } + + testWidgets('show, update, choose', (tester) async { + final composeInputFinder = await setupToComposeInput(tester); + final store = await testBinding.globalStore.perAccount(eg.selfAccount.id); + store.setServerEmojiData(ServerEmojiData(codeToNames: { + '1f4a4': ['zzz', 'sleepy'], // (just 'zzz' in real data) + })); + await store.handleEvent(RealmEmojiUpdateEvent(id: 1, realmEmoji: { + '1': eg.realmEmojiItem(emojiCode: '1', emojiName: 'buzzing'), + })); + + final zulipOption = ('zulip', store.emojiDisplayFor( + emojiType: ReactionType.zulipExtraEmoji, + emojiCode: 'zulip', emojiName: 'zulip')); + final buzzingOption = ('buzzing', store.emojiDisplayFor( + emojiType: ReactionType.realmEmoji, + emojiCode: '1', emojiName: 'buzzing')); + final zzzOption = ('zzz, sleepy', store.emojiDisplayFor( + emojiType: ReactionType.unicodeEmoji, + emojiCode: '1f4a4', emojiName: 'zzz')); + + // Enter a query; options appear, of all three emoji types. + // TODO(#226): Remove this extra edit when this bug is fixed. + await tester.enterText(composeInputFinder, 'hi :'); + await tester.enterText(composeInputFinder, 'hi :z'); + await tester.pump(); + checkEmojiShown(expected: true, zzzOption); + checkEmojiShown(expected: true, buzzingOption); + checkEmojiShown(expected: true, zulipOption); + + // Edit query; options change. + await tester.enterText(composeInputFinder, 'hi :zz'); + await tester.pump(); + checkEmojiShown(expected: true, zzzOption); + checkEmojiShown(expected: true, buzzingOption); + checkEmojiShown(expected: false, zulipOption); + + // Choosing an option enters result and closes autocomplete. + await tester.tap(find.text('buzzing')); + await tester.pump(); + check(tester.widget(composeInputFinder).controller!.text) + .equals('hi :buzzing:'); + checkEmojiShown(expected: false, zzzOption); + checkEmojiShown(expected: false, buzzingOption); + checkEmojiShown(expected: false, zulipOption); + + debugNetworkImageHttpClientProvider = null; + }); + + testWidgets('text emoji means just show text', (tester) async { + final composeInputFinder = await setupToComposeInput(tester); + final store = await testBinding.globalStore.perAccount(eg.selfAccount.id); + await store.handleEvent(UserSettingsUpdateEvent(id: 1, + property: UserSettingName.emojiset, value: Emojiset.text)); + + // TODO(#226): Remove this extra edit when this bug is fixed. + await tester.enterText(composeInputFinder, 'hi :'); + await tester.enterText(composeInputFinder, 'hi :z'); + await tester.pump(); + + // The emoji's name appears. (And only once.) + check(find.text('zulip')).findsOne(); + + // But no emoji image appears. + check(find.byWidgetPredicate((widget) => switch(widget) { + Image(image: NetworkImage()) => true, + _ => false, + })).findsNothing(); + + debugNetworkImageHttpClientProvider = null; + }); + }); + group('TopicAutocomplete', () { void checkTopicShown(GetStreamTopicsEntry topic, PerAccountStore store, {required bool expected}) { check(find.text(topic.name).evaluate().length).equals(expected ? 1 : 0);