Implement basic keyword search #1662

chrisbobbe · 2025-07-03T04:25:35Z

Fixes #252.

Stacked on #1658

Still TODO before shipping:

Tests.

Screenshots

Light	Dark

gnprice

Exciting! Here's a review of the current version, omitting the points you mentioned as TODOs above.

gnprice · 2025-07-03T05:00:02Z

lib/widgets/message_list.dart


-  @override
-  Widget build(BuildContext context) {
+  AppBar _buildAppBar(BuildContext context) {


msglist [nfc]: Pull out _buildAppBar helper

How about making it a separate widget entirely? Seems like that'd make the separation a bit crisper; and it doesn't look like this method is interacting with this widget's state beyond looking at narrow.

This might be feasible, but the reason I didn't is that Scaffold's appBar param is actually typed as PreferredSizeWidget?, not Widget?. Maybe I can subclass ZulipAppBar (which extends AppBar / PreferredSizeWidget) for this app bar?

Hmm I see, yeah. That bit of the Scaffold/AppBar API is kind of a pain.

I don't think there's a great way to do it by subclassing, either. Most of what this build-helper method is doing is computing values that get passed to the AppBar constructor, so those computations would have to happen in the subclass's constructor… and they're using the context, so that's no good.

I have an idea which I'll send as an added commit, with a comment.

ac6c750 msglist [nfc]: Move _buildAppBar to a widget-like class

Then I also went into a bit of a digression and sent a PR upstream that would clean up AppBar's API in this respect:

AppBar resists extending by composition, because of preferredSize flutter/flutter#171603

Add static for AppBar.preferredSize, to enable extending by composition flutter/flutter#171604

And I have a small draft commit that would use that to turn _MessageListAppBar into a proper widget class:
3ecadf2 wip use AppBar.preferredSizeFor to clean up _MessageListAppBar

Interesting, thanks for doing that!

gnprice · 2025-07-03T05:02:35Z

lib/model/message_list.dart

-      && narrow.containsMessage(outboxMessage)
+      && narrow.containsMessage(outboxMessage) == true


The app-code changes show just small behavior changes for a message-list page in a search narrow, which we'll implement soon: - Message events are ignored - Outbox messages never appear

nit: s/changes/choices/ — before this commit, there was no answer to the question of how these would behave on a null containsMessage result, because the types ruled that out

gnprice · 2025-07-03T05:05:43Z

lib/model/message_list.dart

+  /// Set [narrow] to [newNarrow], reset, [notifyListeners], and [fetchInitial].
+  void renarrowAndFetch(Narrow newNarrow) {


nit: mention this from the [narrow] doc, since it's a lot like a setter for that property

gnprice · 2025-07-03T05:09:00Z

lib/api/model/narrow.dart

+class ApiNarrowKeywordSearch extends ApiNarrowElement {
+  @override String get operator => 'search';


nit: let's call this ApiNarrowSearch, to keep it closely aligned with the underlying API

(unlike the Narrow subclass, which is more about how we organize the UI within the app)

gnprice · 2025-07-03T05:13:15Z

lib/api/model/narrow.dart

  );
 }
+
+class ApiNarrowKeywordSearch extends ApiNarrowElement {


nit: let's put this before with and id; those feel more specific than this because they point to an individual message, and naturally go at the end of a list of filters

(I guess that calls for with to go after is, too, next to id.)

gnprice · 2025-07-03T05:29:07Z

lib/model/narrow.dart

+  ///
+  /// Will be null for search narrows.
+  bool? containsMessage(MessageBase message);


I'd like this to give a bit more of a semantics for what it means for this to be null:

Suggested change

///

/// Will be null for search narrows.

bool? containsMessage(MessageBase message);

///

/// Null when the client is unable to predict whether the message

/// satisfies the filters of this narrow, e.g. when this is a search narrow.

bool? containsMessage(MessageBase message);

gnprice · 2025-07-03T05:41:48Z

lib/widgets/message_list.dart

+      controller: _controller,
+      autocorrect: false,
+
+      // Current servers seem to require straight quotes for the "exact match"-


gnprice · 2025-07-03T05:43:06Z

lib/widgets/message_list.dart

+      smartQuotesType: SmartQuotesType.disabled,
+
+      autofocus: true,
+      onSubmitted: (value) => widget.onSubmitted(_valueToNarrow(value)),


nit: maybe have one method do both these steps (since they always occur together):

Suggested change

onSubmitted: (value) => widget.onSubmitted(_valueToNarrow(value)),

onSubmitted: _handleSubmitted,

gnprice · 2025-07-03T05:46:34Z

lib/widgets/message_list.dart

+        border: OutlineInputBorder(
+          borderRadius: BorderRadius.circular(10),
+          borderSide: BorderSide.none),
+        hintStyle: TextStyle(color: designVariables.labelSearchPrompt)));


nit: should go next to hintText

gnprice · 2025-07-03T05:54:46Z

test/model/message_list_test.dart

+      case MentionsNarrow():
+      case StarredMessagesNarrow():
+        check(model.narrow.containsMessage(message)).isNotNull().isTrue();
+      case KeywordSearchNarrow():
+        check(model.narrow.containsMessage(message)).isNull();


If it's a KeywordSearchNarrow, then there's nothing that needs to be checked here; it's a fact about that narrow type, not about the list of messages, that containsMessage will always be null.

Then I think also this is one place where we don't need to be considering for each new narrow type how it should be handled. We can let the default be that the check is done; if at some point we add another narrow type where the check would fail, then we'll naturally notice at test time, and can fix it then. So this could look like:

if (model.narrow is! KeywordSearchNarrow) { check(model.narrow.containsMessage(message)).isNotNull().isTrue(); }

Hmm or maybe better yet: this could just check that containsMessage is either null or true. If it's null, we're unable to confirm whether the message really belongs here, so checkInvariants has to let it slide. What it's really looking for is cases where it's false, and therefore MessageListView really should have omitted the message.

gnprice · 2025-07-03T06:00:56Z

(All the comments above are small, except #1662 (comment) about narrowLink; and I believe that bit of code never actually gets exercised in the app, so it'd be fine to have it just throw UnimplementedError or something.

… Yeah, confirmed: the app only ever calls that function for generating a message link, so with a SendableNarrow.)

alya · 2025-07-03T17:43:39Z

Just a simple "No search results." is the default web app message for no results. We also show stopwords that were excluded, but I don't know if we exclude stopwords in mobile searches?

gnprice · 2025-07-03T18:02:25Z

The stopwords behavior is server-side (I believe it happens within the database, even), so it'll be the same for mobile as web.

As suggested by Greg: zulip#1662 (comment)

chrisbobbe · 2025-07-03T20:00:23Z

Thanks for the reviews! Revision pushed. I still intend to write some tests today, but I can do that while you're reviewing this revision. Also @alya I put screenshots in the issue description.

Just a simple "No search results." is the default web app message for no results.

Works for me for now, but I think ideally we'd show a different message (or no message?) if you haven't requested results yet, i.e. you haven't submitted a search query.

gnprice

Thanks for the revision! All looks good, modulo nits below, and it'd be good to have those tests you're working on.

I also haven't yet read the commit at the start which is #1658. I'll do that next.

gnprice · 2025-07-03T20:43:05Z

lib/widgets/message_list.dart

  Widget build(BuildContext context) {
-    final store = PerAccountStoreWidget.of(context);
-    final messageListTheme = MessageListTheme.of(context);
-    final zulipLocalizations = ZulipLocalizations.of(context);
-
-    final Color? appBarBackgroundColor;


nit: commit message needs edit after squashing

msglist [nfc]: Move _buildAppBar to a widget-like class Conceptually this should be a widget, but can't quite be, for the reasons explained in the comment. Still we can organize the code in nearly the same way as if it were a widget.

gnprice · 2025-07-03T20:48:41Z

lib/model/message_list.dart

-        // The messages were and remain in this narrow.
-        // TODO(#421): … except they may have become muted or not.
+        // The messages didn't enter or leave this narrow.
+        // TODO(#1255): … except they may have become muted or not.


msglist [nfc]: Renumber a TODO The UserTopic part of this is done, but handling channel muting/unmuting is still TODO.

Looks like there's a couple other TODO comments for this issue that should get the same update:

lib/model/channel.dart 325: // TODO(#421) update [MessageListView] if affected 357: // TODO(#421) update [MessageListView] if affected

… Or actually I guess only the first of those two is about channel mutes; the other is topic mutes. Is the latter resolved, then?

also nit: make the summary line a bit more specific, like "a TODO about muting"

gnprice · 2025-07-03T20:50:24Z

lib/generated/l10n/zulip_localizations.dart

+  /// Page title for the 'Search' message view.
+  ///
+  /// In en, this message translates to:
+  /// **'Search'**
+  String get searchMessagesPageTitle;


nit: these look to be happening in the wrong commit

gnprice · 2025-07-03T20:52:07Z

lib/api/model/model.dart

-  final String? matchContent;
-  @JsonKey(name: 'match_subject')
-  final String? matchTopic;
+  // TODO(#1663) matchContent and matchTopic


Let's leave some kind of breadcrumb in this TODO comment to say there's a commit we'll want to revert to restore these. (It has some useful bits in it beyond what one gets mechanically from the API docs: the rename, and the test for that.)

gnprice · 2025-07-03T20:56:17Z

lib/model/message_list.dart

+        _messagesMovedInternally(messageIds);
+
+
      case ChannelNarrow(:final streamId):


nit: double blank line

gnprice · 2025-07-03T20:58:06Z

lib/widgets/message_list.dart

+        fillColor: designVariables.bgSearchInput,
+        border: OutlineInputBorder(
+          borderRadius: BorderRadius.circular(10),
+          borderSide: BorderSide.none),));


nit:

Suggested change

borderSide: BorderSide.none),));

borderSide: BorderSide.none),

));

gnprice · 2025-07-03T20:59:15Z

lib/widgets/message_list.dart

+      final String message;
+
+      if (widget.narrow is KeywordSearchNarrow) {


nit: group declaration with initializing code

Suggested change

final String message;

if (widget.narrow is KeywordSearchNarrow) {

final String message;

if (widget.narrow is KeywordSearchNarrow) {

Conceptually this should be a widget, but can't quite be, for the reasons explained in the comment. Still we can organize the code in nearly the same way as if it were a widget. Co-authored-by: Greg Price <[email protected]>

Done as preparation to implement a new Narrow subclass for keyword search. We can't really implement a client-side "contains-message" predicate for search; it's up to the server to decide whether a message matches the search query. A simple path to representing that, done here, is to just change Narrow.containsMessage's contract to say that it always returns null if the narrow is a search narrow. The app-code changes show just small behavior choices for a message-list page in a search narrow, which we'll implement soon: - Message events are ignored - Outbox messages never appear Various TODOs in tests; we'll handle all of those as part of adding the keyword-search Narrow subclass.

…ning Seeing that the tests were expecting a notifyListeners call already, we found that one is already done, in MessageStore.handleUpdateMessageEvent, so this is redundant. But because the new call is done synchronously with the existing call, it doesn't trigger any extra rebuilds, and it helps the reader by making it clear locally that listeners do indeed get notified.

We'll use this in keyword search, coming up, for renarrowing when the user submits a new keyword.

Figma links: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=4031-32875&t=UsRz28UEu3IDeQQC-0 https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=10904-101398&t=UsRz28UEu3IDeQQC-0

As suggested by Greg: zulip#1662 (comment)

…ions It looks like this comment was written for CombinedFeedNarrow and not updated when the other two cases were added. (Unlike with CombinedFeedNarrow, it's possible that some of `messageIds` weren't starred or mentioned in the first place, so it was inaccurate to say they "were" in the narrow.)

We already handle muted topics, but we still need to update the message list on muting/unmuting a channel, which is zulip#1255.

We plan to revert this when we do zulip#1663.

Fixes zulip#252. For timeliness, tests are left as a followup: zulip#1667

chrisbobbe · 2025-07-03T23:53:09Z

Thanks! Revision pushed, and filed #1667 for tests as a followup, for timeliness.

gnprice · 2025-07-04T00:02:39Z

Thanks! Looks good; merging.

chrisbobbe requested a review from gnprice July 3, 2025 04:25

chrisbobbe assigned gnprice Jul 3, 2025

chrisbobbe added the integration review Added by maintainers when PR may be ready for integration label Jul 3, 2025

chrisbobbe force-pushed the pr-basic-search branch from ab2bdb6 to 30989af Compare July 3, 2025 04:27

gnprice reviewed Jul 3, 2025

View reviewed changes

gnprice mentioned this pull request Jul 3, 2025

settings: Migrate to new RadioGroup API; write widget tests for two just-added settings #1602

Merged

chrisbobbe mentioned this pull request Jul 3, 2025

api: Remove backward-compat code for mark-read protocol in FL <155 #1658

Merged

chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this pull request Jul 3, 2025

narrow [nfc]: Reorder ApiNarrowWith to be later, since more specific

82a8e45

As suggested by Greg: zulip#1662 (comment)

chrisbobbe force-pushed the pr-basic-search branch from ac6c750 to 0ef4fd6 Compare July 3, 2025 19:58

gnprice reviewed Jul 3, 2025

View reviewed changes

chrisbobbe mentioned this pull request Jul 3, 2025

Write tests for message search #1667

Open

chrisbobbe and others added 12 commits July 3, 2025 16:48

msglist [nfc]: Make a widget-like class for the app bar

55e14f4

Conceptually this should be a widget, but can't quite be, for the reasons explained in the comment. Still we can organize the code in nearly the same way as if it were a widget. Co-authored-by: Greg Price <[email protected]>

msglist [nfc]: Add public renarrowAndFetch method, and use internally

61a2319

We'll use this in keyword search, coming up, for renarrowing when the user submits a new keyword.

icons: Add remove and search, from Figma

3ecdd50

Figma links: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=4031-32875&t=UsRz28UEu3IDeQQC-0 https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=10904-101398&t=UsRz28UEu3IDeQQC-0

internal_link [nfc]: Renumber a TODO that isn't about basic search

6fcc009

narrow [nfc]: Reorder ApiNarrowWith to be later, since more specific

8d68939

As suggested by Greg: zulip#1662 (comment)

model [nfc]: Remove a TODO about topic muting, which is already resolved

8bf418e

msglist [nfc]: Renumber a TODO about muting

dd49897

We already handle muted topics, but we still need to update the message list on muting/unmuting a channel, which is zulip#1255.

model: Temporarily cut matchContent, matchTopic from message objects

ac7be16

We plan to revert this when we do zulip#1663.

msglist: Support basic keyword search

a6104aa

Fixes zulip#252. For timeliness, tests are left as a followup: zulip#1667

chrisbobbe force-pushed the pr-basic-search branch from 0ef4fd6 to a6104aa Compare July 3, 2025 23:52

chrisbobbe marked this pull request as ready for review July 3, 2025 23:52

gnprice merged commit a6104aa into zulip:main Jul 4, 2025
1 check passed

chrisbobbe deleted the pr-basic-search branch July 4, 2025 01:05

gnprice mentioned this pull request Jul 4, 2025

msglist: Offer search narrows #252

Closed

laurynmm mentioned this pull request Jul 8, 2025

help: Add mobile instructions in "Searching for messages" article. zulip/zulip#35172

Merged

12 tasks

		&& narrow.containsMessage(outboxMessage)
		&& narrow.containsMessage(outboxMessage) == true

		/// Set [narrow] to [newNarrow], reset, [notifyListeners], and [fetchInitial].
		void renarrowAndFetch(Narrow newNarrow) {

		class ApiNarrowKeywordSearch extends ApiNarrowElement {
		@override String get operator => 'search';

	onSubmitted: (value) => widget.onSubmitted(_valueToNarrow(value)),
	onSubmitted: _handleSubmitted,

		_messagesMovedInternally(messageIds);


		case ChannelNarrow(:final streamId):

	borderSide: BorderSide.none),));
	borderSide: BorderSide.none),
	));

		final String message;

		if (widget.narrow is KeywordSearchNarrow) {

Implement basic keyword search #1662

Implement basic keyword search #1662

Uh oh!

Conversation

chrisbobbe commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Screenshots

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice commented Jul 3, 2025

Uh oh!

alya commented Jul 3, 2025

Uh oh!

gnprice commented Jul 3, 2025

Uh oh!

chrisbobbe commented Jul 3, 2025

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Jul 3, 2025

Uh oh!

gnprice commented Jul 4, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

chrisbobbe commented Jul 3, 2025 •

edited

Loading