Implement basic keyword search #1662

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

Stacked on #1658

Still TODO before shipping, but I wanted to push my progress before heading home:

Cut out match_content match_topic in message objects, for now
"There are no messages here" placeholder text -- probably isn't
ever right for the search page. Maybe we want one message for when
there isn't (yet) a search query, and another message for when
we've searched and there are no results.
Tests.
Screenshots, for Alya's review.

Fixes #252.

This code had a switch/case on the Narrow type, so I discovered it while implementing keyword-search narrows. We support Zulip Server 7 and later (see README) and refuse to connect to older servers. Since we haven't been using this protocol for servers FL 155+, this should be NFC for all servers we connect to, as long as we know their feature level accurately. Related: zulip#992

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 changes 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

Still TODO before shipping: - "There are no messages here" placeholder text -- probably isn't ever right for the search page. Maybe we want one message for when there isn't (yet) a search query, and another message for when we've searched and there are no results. - Tests, ideally - Cut out `match_content` `match_topic` in message objects, for now

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

@@ -281,8 +281,7 @@ class _MessageListPageState extends State<MessageListPage> implements MessageLis
    });
  }

-  @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.

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

@@ -245,3 +245,16 @@ class ApiNarrowMessageId extends ApiNarrowElement {
    negated: json['negated'] as bool? ?? false,
  );
 }
+
+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.)

chrisbobbe added 7 commits July 2, 2025 21:16

msglist [nfc]: Pull out _buildAppBar helper

87f7294

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

3d7fac1

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

3f21e9b

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

8caea0e

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Implement basic keyword search #1662

Implement basic keyword search #1662

Uh oh!

chrisbobbe commented Jul 3, 2025 •

edited

Loading

Uh oh!

gnprice left a comment

Uh oh!

gnprice Jul 3, 2025

Uh oh!

gnprice Jul 3, 2025

Uh oh!

gnprice Jul 3, 2025

Uh oh!

gnprice Jul 3, 2025

Uh oh!

gnprice Jul 3, 2025

Uh oh!

gnprice Jul 3, 2025

Uh oh!

gnprice Jul 3, 2025

Uh oh!

gnprice Jul 3, 2025

Uh oh!

gnprice Jul 3, 2025

Uh oh!

gnprice Jul 3, 2025

Uh oh!

gnprice commented Jul 3, 2025

Uh oh!

Uh oh!

		&& 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,

Implement basic keyword search #1662

Are you sure you want to change the base?

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!

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!

gnprice commented Jul 3, 2025

Uh oh!

Uh oh!

chrisbobbe commented Jul 3, 2025 •

edited

Loading