Add support for keywords in searchbar #1258

mounilKshah · 2022-09-18T19:46:35Z

What does this PR do?
This PR is to add support for keywords in search bar.

Fixes #638

Channel discussion:
Expand search support #T638 #T1258

Tested?

Manually
Existing tests (adapted, if necessary)
New tests added (for any new behavior)
Passed linting & tests (each commit)

Commit flow

One commit: Support only for stream and topic keywords

Notes & Questions

This is a WIP PR. It currently only has support for stream and topic name keywords.
As per the discussions, the plan ahead would be to:
- Extend support to other keywords for narrows supported by ZT
- Provide auto-populated text for the current narrow in the search bar
- Write tests

neiljp

@mounilKshah This does work better 👍

I've left feedback mainly on the second commit since it represents better the final state you have right now.

For tests on the current-narrow -> search-text function, the inputs for cases should be achievable from browsing how we handle non-search narrows.

I'd suggest you first split into commits to add each function, then use them together in core.py for 3 commits. Then some tests for each new function in the relevant commit. Covering streams/topics with tests would be good first. More narrow cases may flow quicker once you have the general code/test format :)

neiljp · 2022-09-28T02:47:27Z

zulipterminal/core.py

+        # if the user is currently inside a narrow, update search text to accomodate that
+        if len(self.model.narrow) > 0:
+            final_search_query = self.update_search_text_for_narrow(text)
+        search_query, search_narrow = self.check_for_prefixes(final_search_query)
        self.model.narrow = search_narrow


Note the conditional fails right now if in all messages - I know that's not covered yet :)

Your logic appears to be as follows, so let me know if there's a difference. The first method, which you've prefixed with update_, adds text onto the search query text and returns it. The second method extracts the special prefixes in the result, and converts to the tuple. Also, this PR force sets the narrow - so we need to be extra careful there, like in the previous iteration where it was set to all-messages if no stream (search) text was entered!

If the logic as described above is correct, it probably sounds rather complex, difficult to test, and maybe you even wonder why I suggested this approach?

The difference is that we don't need to pass the original search text between functions here. The fundamental operations are:

generate search text for current narrow (the model knows the current narrow, so no args)

combine that text with the (current) user-entered search text

translate total search text into <some format for doing search> (the tuple right now)

This means the first function can be tested very easily given a list of 'standard' non-search narrows we support, and the last with expected inputs from the first (along with various search strings). So depending how you express this, 3 commits might be appropriate in the end.

Later, the first function can be used to prefill the UI instead :)

neiljp · 2022-09-28T02:48:13Z

zulipterminal/core.py

+        # In cases where '+' is already present in stream and/or topic name, it is converted to '%2B'
+        # then the spaces in stream and topic names are converted to '+'
+        if self.model.narrow[0][0] == "stream":
+            search_query += f" {self.model.narrow[0][0]}:{self.model.narrow[0][1].replace('+', '%2B').replace(' ', '+')}"


The logic here may change, but note that due to the guaranteed equality, we can use "stream" instead to simplify.

It may also be clearer generally if you express what large expressions, here with the replace methods represent (ie. with a new variable) and why it's there (by name?).

neiljp · 2022-09-28T02:49:10Z

zulipterminal/core.py

+            search_query += f" {self.model.narrow[0][0]}:{self.model.narrow[0][1].replace('+', '%2B').replace(' ', '+')}"
+        if len(self.model.narrow) > 1:
+            if self.model.narrow[1][0] == "topic":
+                self.model.narrow[1][1].replace(" ", "+")


This seems to be a duplicate?

neiljp · 2022-09-28T03:04:37Z

zulipterminal/core.py

+                self.model.narrow[1][1].replace(" ", "+")
+                search_query += f" {self.model.narrow[1][0]}:{self.model.narrow[1][1].replace('+', '%2B').replace(' ', '+')}"
+        return search_query
+
    def check_for_prefixes(self, query: str) -> Tuple[str, List[List[str]]]:


The name while WIP doesn't matter too much, but now we're firming up the logic, let's make this name more expressive. If I see check_for_prefixes(something) in code, it doesn't tell me what it does (I can think of various meanings), or why it returns what it does do - I can look it up in an editor or IDE, but better to read right there :)

So, maybe think what goes in the blank:

search_query, search_narrow = self.<BLANK>(final_search_query)

While some/many ZT functions don't have docstrings (or comments) since they're older, some are very simple and ideally well-named that it may be sufficient to understand what's happening when they're used without them.

You might also rename the other function, if you change the logic of what it does.

neiljp · 2022-09-28T03:13:26Z

zulipterminal/core.py

-                new_narrow.append(word.split(":"))
+                new_narrow.append(
+                    [
+                        word.split(":")[0],


We're doing the same word.split operation a lot here, potentially repeatedly, and it likely will appear in other branches I suspect :)

neiljp

@mounilKshah This looks more readable and complete, including with the tests 👍

Once you finish the code and tests for the few existing narrows, along with the final (refactor) commit to replace the existing functions with the new functions, this will be mergeable. The functionality may not seem changed, but this refactor will enable supporting more search options in future :)

Note for tests, if you note the changes to tests (separate line) in the git log, there's no need to include it in the commit title.

zulipterminal/core.py

tests/core/test_core.py

zulipterminal/core.py

neiljp

@mounilKshah The extra commit means I can test this, and it appears to work fairly well from manual testing - and the test cases are reassuring too 👍

I left some final comments. This is essentially mergeable with some textual adjustments, it's only my concern re unexpectedly safely/unsafely changing the narrow or not, due to this likely being the end of the Summer work on this.

zulipterminal/core.py

neiljp · 2022-10-06T06:20:33Z

zulipterminal/core.py

+        text_without_keyword, search_narrow = self.extract_keywords_from_search_query(
+            modified_search_text
+        )
+        self.model.narrow = search_narrow


My primary remaining concern, given we're potentially taking a pause on this after your work, is this direct setting of the narrow here. Doing so means we potentially don't update other data which would otherwise be set, via eg. model.set_narrow or _narrow_to or something which also updated the UI.

Right now the positive is that the narrow shouldn't change, unless a user enters extra search text - and not all combinations work. For now I'd suggest we alert the user with a simple warning popup (popup_with_message), if the search looks like a complicated search which would require setting the narrow, ie. if the old and new narrows wouldn't match.

Examples of cases where this can happen is being in all messages, and adding eg. is:starred some search text, in which case the narrow changes. That 'works', but then adding stream:general or similar does not. So it'd be good to be consistent for now.

zulipterminal/model.py

tests/model/test_model.py

neiljp

@mounilKshah I'd apparently started a review for this but not finished, so just did so. Looks close to a safe refactor.

I thought we might be able to progress further with just search-specific narrow details. We may be able to and you're welcome to investigate, but we may need to refactor the narrow handling more first. Right now that's spread over many locations, possibly with duplicate logic.

neiljp · 2022-10-08T22:16:41Z

zulipterminal/model.py

+        for word in query.split(" "):
+            if word.startswith("stream:"):
+                stream_prefix_present_flag = True
+                stream = word.split(":")[1].replace("+", " ").replace("%2B", "+")
+                self.set_narrow(stream=stream)
+            elif word.startswith("topic:") and stream_prefix_present_flag:
+                topic = word.split(":")[1].replace("+", " ").replace("%2B", "+")
+                self.set_narrow(stream=stream, topic=topic)


This can lead to lead to two narrowing operations for a topic narrow. That's not necessarily bad, but it should be noted, or else worked around.

neiljp · 2022-10-08T22:24:29Z

zulipterminal/model.py

+    def extract_keywords_from_search_query(self, query: str) -> str:
+        """
+        This function checks for prefixes in the search string and returns
+        a newly created narrow as per the prefixes and the query without the prefixes.
+        """


Update this name given the new implementation.

neiljp · 2022-10-20T00:30:50Z

zulipterminal/model.py

@@ -285,6 +285,38 @@ def is_search_narrow(self) -> bool:
        """
        return "search" in [subnarrow[0] for subnarrow in self.narrow]

+    def extract_keywords_from_search_query(self, query: str) -> str:


This function no longer just extracts keywords, it also sets the base narrow.

neiljp · 2022-10-20T00:34:47Z

zulipterminal/model.py

@@ -278,6 +278,31 @@ def get_focus_in_current_narrow(self) -> Union[int, Set[None]]:
    def set_focus_in_current_narrow(self, focus_message: int) -> None:
        self.index["pointer"][repr(self.narrow)] = focus_message

+    def addtional_search_text_for_current_narrow(self) -> str:


neiljp · 2022-10-20T00:36:06Z

tests/core/test_core.py

+            ([["stream", "Stream 1"]], [["stream", "Stream 1"], ["search", "FOO"]]),
            (
-                [["pm_with", "foo@zulip.com"], ["search", "BOO"]],
-                [["pm_with", "foo@zulip.com"], ["search", "FOO"]],
+                [["pm_with", "person1@example.com"], ["search", "BOO"]],
+                [["pm_with", "person1@example.com"], ["search", "FOO"]],
            ),
            (
-                [["stream", "PTEST"], ["topic", "RDS"]],
-                [["stream", "PTEST"], ["topic", "RDS"], ["search", "FOO"]],
+                [["stream", "Stream 1"], ["topic", "RDS"]],
+                [["stream", "Stream 1"], ["topic", "RDS"], ["search", "FOO"]],


Are these changes just tidying, or required? If tidying, better to keep in a separate commit.

They are required changes. The final_narrow is created only when the narrow exists in the org, else the test case fails.

This commit contains Model.extract_keywords_from_search_query() which is used to extract keywords written in the search query and sets the narrow accordingly. Tests are also included in this commit.

This commit contains Model.addtional_search_text_for_current_narrow() which generates string from the current narrow. Tests for the function have also been added.

This commit contains function calls used to modify search text entered, as per the current narrow. Tests have also been updated accordingly.

zulipbot · 2022-11-02T23:28:44Z

Hello @mounilKshah, it seems like you have referenced #638 in your pull request description, but you have not referenced them in your commit message description(s). Referencing an issue in a commit message automatically closes the corresponding issue when the commit is merged, which makes the issue tracker easier to manage.

Please run git commit --amend in your command line client to amend your commit message description with Fixes #638..

An example of a correctly-formatted commit:

commit fabd5e450374c8dde65ec35f02140383940fe146
Author: zulipbot
Date:   Sat Mar 18 13:42:40 2017 -0700

    pull requests: Check PR commits reference when issue is referenced.

    Fixes #51.

To learn how to write a great commit message, please refer to our guide.

zulipbot · 2024-04-17T23:35:40Z

Heads up @mounilKshah, we just merged some commits that conflict with the changes you made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.

zulipbot added the size: S [Automatic label added by zulipbot] label Sep 18, 2022

mounilKshah force-pushed the support_keyword_in_search branch from d0a9d62 to 0074cb4 Compare September 28, 2022 00:13

zulipbot added size: L [Automatic label added by zulipbot] and removed size: S [Automatic label added by zulipbot] labels Sep 28, 2022

mounilKshah marked this pull request as ready for review September 28, 2022 00:14

neiljp reviewed Sep 28, 2022

View reviewed changes

mounilKshah force-pushed the support_keyword_in_search branch from 0074cb4 to 20011a7 Compare October 3, 2022 01:13

zulipbot added size: XL [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] labels Oct 3, 2022

neiljp reviewed Oct 5, 2022

View reviewed changes

neiljp added the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label Oct 5, 2022

mounilKshah force-pushed the support_keyword_in_search branch 2 times, most recently from b43fb25 to 1c8c832 Compare October 5, 2022 19:57

neiljp reviewed Oct 6, 2022

View reviewed changes

mounilKshah force-pushed the support_keyword_in_search branch 2 times, most recently from b830a53 to 4bba34d Compare October 8, 2022 21:40

mounilKshah added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Oct 8, 2022

mounilKshah changed the title ~~[WIP] Add support for keywords in searchbar~~ Add support for keywords in searchbar Oct 8, 2022

neiljp reviewed Oct 20, 2022

View reviewed changes

neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs review PR requires feedback to proceed labels Oct 20, 2022

mounilKshah added 2 commits November 1, 2022 03:29

model: Add function to extract keywords from search text.

c0ed5ac

This commit contains Model.extract_keywords_from_search_query() which is used to extract keywords written in the search query and sets the narrow accordingly. Tests are also included in this commit.

model: Add function to generate additional text for searches.

2a5531c

This commit contains Model.addtional_search_text_for_current_narrow() which generates string from the current narrow. Tests for the function have also been added.

mounilKshah force-pushed the support_keyword_in_search branch from 4bba34d to 3c73a6b Compare November 1, 2022 16:14

mounilKshah added PR needs review PR requires feedback to proceed area: message search and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Nov 1, 2022

core: Update search_messages() for current narrow.

912bcbc

This commit contains function calls used to modify search text entered, as per the current narrow. Tests have also been updated accordingly.

mounilKshah force-pushed the support_keyword_in_search branch from 3c73a6b to 912bcbc Compare November 2, 2022 23:28

zulipbot added the has conflicts label Apr 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for keywords in searchbar #1258

Add support for keywords in searchbar #1258

mounilKshah commented Sep 18, 2022 •

edited

Loading

neiljp left a comment

neiljp Sep 28, 2022

neiljp Sep 28, 2022

neiljp Sep 28, 2022

neiljp Sep 28, 2022

neiljp Sep 28, 2022

neiljp left a comment

neiljp left a comment

neiljp Oct 6, 2022

neiljp left a comment

neiljp Oct 8, 2022

neiljp Oct 8, 2022

neiljp Oct 20, 2022

neiljp Oct 20, 2022

neiljp Oct 20, 2022

mounilKshah Oct 31, 2022

zulipbot commented Nov 2, 2022

zulipbot commented Apr 17, 2024

Add support for keywords in searchbar #1258

Are you sure you want to change the base?

Add support for keywords in searchbar #1258

Conversation

mounilKshah commented Sep 18, 2022 • edited Loading

neiljp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neiljp left a comment

Choose a reason for hiding this comment

neiljp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neiljp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zulipbot commented Nov 2, 2022

zulipbot commented Apr 17, 2024

mounilKshah commented Sep 18, 2022 •

edited

Loading