autocomplete: Introduce topic input autocomplete #627

Khader-1 · 2024-04-23T08:16:34Z

Fixes: #310

Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-07-11.at.10.38.47.mp4

gnprice

Thanks @Khader-1! Generally this strategy looks good, modulo the comment already made in the chat thread. A few other high-level comments below.

lib/model/stream_topics.dart

lib/widgets/compose_box.dart

lib/model/stream_topics.dart

lib/api/route/streams.dart

Khader-1

Thanks @gnprice! I have made some changes. Yet this still needs some tests. Some comments about things I am not so sure about are below. Any other notes or suggestions are welcome.

lib/widgets/compose_box.dart

lib/model/autocomplete.dart

gnprice

Cool, thanks for the revision. Some high-level comments below, but broadly the strategy looks good — I think the main next step is to write tests for this, and to otherwise get it into shape for a non-draft PR.

One theme I notice in several places is that there's code that looks to have been copy-pasted from some similar existing code, but that hasn't been fully edited to reflect the differences. This occurs particularly in comments. I figure that's part of the draft state of this PR — but as part of revising this into non-draft status, please do read through your changes again to look for anything that doesn't fit what's happening around it, particularly comments, and update those.

Similarly, there are several places with excess indentation, double blank lines, or other formatting nits; so please give the code a reread looking for those.

lib/model/autocomplete.dart

lib/widgets/compose_box.dart

sm-sayedi

Thank you @Khader-1 for all the work on this. The generic approach seems promising. Added a few comments, mostly small. And, it would be better to see this PR rebased on the main for the next revision as some changes happened in MentionAutocompleteView in #693 being merged.

Also, the following commits fail the tests (need to pass):

e955ef9: autocomplete [nfc]: Remove unused narrow in MentionAutocompleteView
3f55c7b: autocomplete[nfc]: Use generic fields in AutocompleteViewManager

lib/model/autocomplete.dart

test/model/autocomplete_test.dart

lib/widgets/autocomplete.dart

lib/widgets/compose_box.dart

Khader-1 · 2024-06-30T10:14:36Z

Thanks @sm-sayedi! you're comments are really helpful!
I've pushed a new revision PTAL and please refer to this discussion regarding naming (streams / topics).

sm-sayedi

Thanks for the revision. Two comments below.

lib/model/autocomplete.dart

Khader-1 · 2024-07-01T12:55:51Z

Thanks @sm-sayedi! I've pushed a new revision PTAL.

sm-sayedi

Thanks @Khader-1 for all the work on this. LGTM.

Let's move on to the mentor review with @kenclary!

kenclary

good stuff

chrisbobbe · 2024-07-26T21:17:47Z

Thanks! Looks like there's a conflict from the just-merged #805. I think you can resolve by skipping this commit:

autocomplete test [nfc]: Refactor widget setup in `setupToComposeInput`

and using TestZulipApp instead of ZulipApp in test/widgets/autocomplete_test.dart in the main commit.

Separately, if you get some test failures about TextDirection.ltr and TextDirection.rtl, run flutter run and rerun the tests, and they should succeed. (There's an autogenerated file that should get updated; let me know if that doesn't work.) Discussion: #805 (comment)

Khader-1 · 2024-07-27T08:52:34Z

Thanks @chrisbobbe! I've resolved with a new revision. PTAL

chrisbobbe · 2024-07-29T22:32:44Z

Thanks! Marking as ready for Greg's review.

gnprice

Thanks @Khader-1, and thanks @sm-sayedi and @chrisbobbe for the previous reviews!

For this round, I've read the first two commits:
c6d3ae1 autocomplete [nfc]: Rename Autocomplete to ComposeContentAutocomplete
28c2694 autocomplete [nfc]: Factor out generic AutocompleteView

Comments on those below. I think some of the comments are likely to apply also to the third commit:
b2f9e5c autocomplete [nfc]: Factor out generic AutocompleteField

so please try to revise that as well where these comments relate to it.

lib/model/autocomplete.dart

test/model/autocomplete_checks.dart

gnprice

Thanks for the revision! Comments below.

For this round, I've read through these three commits:
265ad97 autocomplete [nfc]: Rename Autocomplete to ComposeContentAutocomplete
7d9cc85 autocomplete [nfc]: Factor out generic AutocompleteView
beaf4e2 autocomplete: Setup stream topics API binding

and part of these two:
7c24173 autocomplete [nfc]: Factor out generic AutocompleteField
b34f5c4 autocomplete: Implement targeted topic autocomplete

I think those latter two will be easier to read after acting on a couple of my comments below.

lib/model/autocomplete.dart

lib/widgets/autocomplete.dart

lib/api/route/streams.dart

lib/model/autocomplete.dart

Khader-1 · 2024-08-07T20:11:00Z

Thanks @gnprice! I've pushed a new revision PTAL.

gnprice

Thanks for the revision! Comments below. In this round I got through the whole PR.

lib/widgets/autocomplete.dart

lib/api/route/channels.dart

lib/model/autocomplete.dart

lib/widgets/autocomplete.dart

gnprice

Thanks for the revision! This is quite close now. Just small comments below, and a couple of subthreads remain open from above — #627 (comment) as noted below, and #627 (comment) where I replied above.

gnprice · 2024-08-11T21:51:45Z

lib/api/route/channels.dart

+import 'package:json_annotation/json_annotation.dart';
+
+import '../core.dart';
+part 'channels.g.dart';


Cool, this is a lot closer:

api: Set up stream topics API binding

Let's specifically match previous commits that added new endpoint bindings, though. Here's a command that shows those (as well as a few other matches):
$ git log --oneline lib/api/route | grep -vw nfc

So in particular
6d9f22d api: Add markAllAsRead, markStreamAsRead, and markTopicAsRead routes
dfcc6ca api: Add updateMessageFlags and updateMessageFlagsForNarrow routes
b8290cd api: Add routes registerFcmToken, registerApnsToken

For this, let's then say "api: Add route getStreamTopics".

lib/widgets/autocomplete.dart

gnprice · 2024-08-11T22:03:20Z

lib/model/autocomplete.dart

+  }
+
+  @override
+  Iterable<String> getSortedItemsToTest(TopicAutocompleteQuery query) => _topics;


(I think you already know this is still open, but bumping just so this remains visible in the new review)

lib/widgets/autocomplete.dart

Khader-1 · 2024-08-12T09:51:10Z

Thanks @gnprice! I've pushed a new revision PTAL.

The name Autocomplete is too generic and it collides with flutter's material Autocomplete widget name. So this uses the more specific name 'ComposeContentAutocomplete'.

The mechanism of query and results is generic to the idea of autocomplete in general, it's not specific to autocomplete on @-mentions vs. topics vs. anything else.

Most of the logic in `ComposeAutocomplete` is not specific to the content input it self rather it is general logic that applies to any autocomplete field.

The filtering logic (in [TopicAutocompleteQuery.testTopic]) differs from zulip-mobile but agrees with web. Details at: zulip#627 (comment) Fixes: zulip#310

gnprice

Thanks for the revision! Everything looks good now, with a couple of nits below; I'll fix those and merge.

I also tweaked the main commit's summary line:

-    autocomplete: Implement targeted topic autocomplete
+    autocomplete: Add autocomplete for topic to send to

because "targeted" sounded to me like it was a kind of autocomplete that was somehow "targeted". I think what you intended was instead that the topic was "targeted", as in being the topic the draft message is aimed toward, so reworded to hopefully make that clearer.

gnprice · 2024-08-13T04:45:22Z

lib/model/autocomplete.dart

+  }
+
+  @override
+  Iterable<String> getSortedItemsToTest(TopicAutocompleteQuery query) => _topics;


Cool. Verifying the existing server's behavior isn't enough in itself, because that doesn't guarantee the server won't change behavior later… but that thread now also has Tim's agreement that we intend that as part of the API, and an intention to write it down in the docs. So that means we can rely on it for the future too.

gnprice · 2024-08-13T04:46:42Z

lib/model/autocomplete.dart

+  TopicAutocompleteQuery(super.raw);
+
+  bool testTopic(String topic) {
+    return topic != raw && topic.toLowerCase().contains(raw.toLowerCase());


bump on one part of #627 (comment) : adding a TODO(#881) comment

Also I added a few words to the commit message saying we picked web's behavior over zulip-mobile's, and linking to this subthread.

gnprice reviewed Apr 24, 2024

View reviewed changes

lib/model/stream_topics.dart Outdated Show resolved Hide resolved

lib/widgets/compose_box.dart Outdated Show resolved Hide resolved

lib/model/stream_topics.dart Outdated Show resolved Hide resolved

lib/api/route/streams.dart Outdated Show resolved Hide resolved

Khader-1 force-pushed the compose-topic_autocomplete branch 2 times, most recently from 9b465c3 to f0b4a3c Compare April 30, 2024 10:15

Khader-1 requested a review from gnprice April 30, 2024 10:19

Khader-1 commented Apr 30, 2024

View reviewed changes

lib/widgets/compose_box.dart Outdated Show resolved Hide resolved

lib/widgets/compose_box.dart Outdated Show resolved Hide resolved

lib/model/autocomplete.dart Outdated Show resolved Hide resolved

gnprice reviewed Apr 30, 2024

View reviewed changes

Khader-1 force-pushed the compose-topic_autocomplete branch 3 times, most recently from fa64db7 to b6c64d4 Compare May 27, 2024 17:22

Khader-1 changed the title ~~wip: Introduce Topic Autocomplete~~ autocomplete: Introduce topic input autocomplete May 27, 2024

Khader-1 marked this pull request as ready for review May 27, 2024 17:27

Khader-1 added the buddy review GSoC buddy review needed. label May 28, 2024

Khader-1 requested a review from sm-sayedi May 28, 2024 09:21

Khader-1 force-pushed the compose-topic_autocomplete branch 2 times, most recently from 964eb9f to e3d5933 Compare May 29, 2024 21:56

sm-sayedi reviewed Jun 12, 2024

View reviewed changes

Khader-1 force-pushed the compose-topic_autocomplete branch 2 times, most recently from 4a34f33 to bb33514 Compare June 30, 2024 10:10

Khader-1 requested a review from sm-sayedi June 30, 2024 10:15

Khader-1 force-pushed the compose-topic_autocomplete branch from bb33514 to a33ea7b Compare June 30, 2024 10:17

sm-sayedi reviewed Jul 1, 2024

View reviewed changes

lib/model/autocomplete.dart Outdated Show resolved Hide resolved

lib/model/autocomplete.dart Outdated Show resolved Hide resolved

Khader-1 force-pushed the compose-topic_autocomplete branch from a33ea7b to dfcf5af Compare July 1, 2024 12:27

Khader-1 requested a review from sm-sayedi July 1, 2024 12:55

sm-sayedi approved these changes Jul 1, 2024

View reviewed changes

sm-sayedi added mentor review GSoC mentor review needed. and removed buddy review GSoC buddy review needed. labels Jul 1, 2024

sm-sayedi requested a review from kenclary July 1, 2024 13:16

kenclary approved these changes Jul 1, 2024

View reviewed changes

Khader-1 requested a review from chrisbobbe July 26, 2024 07:16

Khader-1 force-pushed the compose-topic_autocomplete branch from 591b2e1 to 71f1128 Compare July 27, 2024 08:50

chrisbobbe added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Jul 29, 2024

gnprice reviewed Jul 30, 2024

View reviewed changes

Khader-1 force-pushed the compose-topic_autocomplete branch from 71f1128 to b34f5c4 Compare July 30, 2024 19:30

gnprice reviewed Jul 31, 2024

View reviewed changes

Khader-1 force-pushed the compose-topic_autocomplete branch from b34f5c4 to 223e3fe Compare August 7, 2024 20:10

gnprice reviewed Aug 9, 2024

View reviewed changes

Khader-1 force-pushed the compose-topic_autocomplete branch 2 times, most recently from 7cb5a30 to ee3f0fa Compare August 10, 2024 14:49

gnprice mentioned this pull request Aug 11, 2024

In topic autocomplete, rank results by match relevance #881

Open

gnprice reviewed Aug 11, 2024

View reviewed changes

Khader-1 force-pushed the compose-topic_autocomplete branch from ee3f0fa to 21f4cd0 Compare August 12, 2024 09:47

Khader-1 force-pushed the compose-topic_autocomplete branch 2 times, most recently from 93457aa to b3f111e Compare August 12, 2024 18:01

Khader-1 added 5 commits August 12, 2024 21:48

autocomplete [nfc]: Rename Autocomplete to ComposeContentAutocomplete

ababf79

The name Autocomplete is too generic and it collides with flutter's material Autocomplete widget name. So this uses the more specific name 'ComposeContentAutocomplete'.

autocomplete [nfc]: Factor out generic AutocompleteView

0ce17ad

The mechanism of query and results is generic to the idea of autocomplete in general, it's not specific to autocomplete on @-mentions vs. topics vs. anything else.

autocomplete [nfc]: Factor out generic AutocompleteField

fad368c

Most of the logic in `ComposeAutocomplete` is not specific to the content input it self rather it is general logic that applies to any autocomplete field.

api: Add route getStreamTopics

50a7609

autocomplete: Add autocomplete for topic to send to

046ceab

The filtering logic (in [TopicAutocompleteQuery.testTopic]) differs from zulip-mobile but agrees with web. Details at: zulip#627 (comment) Fixes: zulip#310

gnprice reviewed Aug 13, 2024

View reviewed changes

gnprice force-pushed the compose-topic_autocomplete branch from b3f111e to 046ceab Compare August 13, 2024 04:52

gnprice merged commit 046ceab into zulip:main Aug 13, 2024

Khader-1 deleted the compose-topic_autocomplete branch August 13, 2024 09:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

autocomplete: Introduce topic input autocomplete #627

autocomplete: Introduce topic input autocomplete #627

Khader-1 commented Apr 23, 2024 •

edited

Loading

gnprice left a comment

Khader-1 left a comment

gnprice left a comment

sm-sayedi left a comment

Khader-1 commented Jun 30, 2024

sm-sayedi left a comment

Khader-1 commented Jul 1, 2024

sm-sayedi left a comment

kenclary left a comment

chrisbobbe commented Jul 26, 2024 •

edited

Loading

Khader-1 commented Jul 27, 2024

chrisbobbe commented Jul 29, 2024

gnprice left a comment

gnprice left a comment

Khader-1 commented Aug 7, 2024

gnprice left a comment

gnprice left a comment

gnprice Aug 11, 2024

gnprice Aug 11, 2024

Khader-1 commented Aug 12, 2024

gnprice left a comment

gnprice Aug 13, 2024

gnprice Aug 13, 2024

gnprice Aug 13, 2024

autocomplete: Introduce topic input autocomplete #627

autocomplete: Introduce topic input autocomplete #627

Conversation

Khader-1 commented Apr 23, 2024 • edited Loading

gnprice left a comment

Choose a reason for hiding this comment

Khader-1 left a comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

sm-sayedi left a comment

Choose a reason for hiding this comment

Khader-1 commented Jun 30, 2024

sm-sayedi left a comment

Choose a reason for hiding this comment

Khader-1 commented Jul 1, 2024

sm-sayedi left a comment

Choose a reason for hiding this comment

kenclary left a comment

Choose a reason for hiding this comment

chrisbobbe commented Jul 26, 2024 • edited Loading

Khader-1 commented Jul 27, 2024

chrisbobbe commented Jul 29, 2024

gnprice left a comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Khader-1 commented Aug 7, 2024

gnprice left a comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

gnprice Aug 11, 2024

Choose a reason for hiding this comment

gnprice Aug 11, 2024

Choose a reason for hiding this comment

Khader-1 commented Aug 12, 2024

gnprice left a comment

Choose a reason for hiding this comment

gnprice Aug 13, 2024

Choose a reason for hiding this comment

gnprice Aug 13, 2024

Choose a reason for hiding this comment

gnprice Aug 13, 2024

Choose a reason for hiding this comment

Khader-1 commented Apr 23, 2024 •

edited

Loading

chrisbobbe commented Jul 26, 2024 •

edited

Loading