model: Alter message search for searching in current narrow. #430

sumanthvrao · 2019-07-10T17:12:12Z

The earlier logic of message search [/], searched for messages
in the default view by setting the narrow to [['search', text]].

We now alter this, to search for messages in current narrow
by extending the narrow before fetching messages from
the server. This is the syntax required by get_messages
for searching within narrow.

neiljp

@sumanthvrao I just tested this manually.

I started by searching in all-messages, and searching for 'RDS'. That gave various results across multiple streams/topics. I then went to a production-help topic (AWS Deployment) and used 'z' to jump to that topic. I then used '/' again, left the text as 'RDS' and hit enter, and I get a traceback. If I jumped out to the stream before searching again, then it gives zero results, which seems incorrect ;) The latter also seems to hide the cursor or stop it moving somehow? (I've not figured out quite what is happening there).

A few things to think about and try out :)

sumanthvrao · 2019-07-12T01:15:05Z

Ah! Thanks for the report. I'll check why this is happening.

sumanthvrao · 2019-07-13T15:49:10Z

@neiljp updated this. This turned out more intensive than the straightforward fix I initially assumed it would be. Hope this simplifies the searching process ;)

I have also added tests, which have some 'expected' behaviours of the 'search_message' logic.

neiljp

@sumanthvrao From manual testing again, this is definitely an improvement in the sense of searching over only narrows:

All messages -> / RDS -> multiple streams/topics with RDS in :)
Production help -> / RDS -> multiple topics with RDS in, in stream :)
Production help -> / AWS -> various topics with AWS in :)
Production help / topic -> / AWS -> shows same topic, but indicates it's in a stream? (doesn't show 'topic narrow', or anything indicating something similar)

However that last point, with the confusing stream/topic UI has me concerned right now, and also that it's not clear when the search text (which is not cleared unless explicitly using '/'+backspace?) is applied to the existing - or next/new - narrow.

I think we should clear the search when we change to some 'different enough' narrows. What conditions are 'enough' might be up for discussion, since perhaps the user wishes to keep searching until they clear the search box, even between narrows, or the exact opposite! We are also running into the issue that #278 tries to work around, but here with empty search narrows.

neiljp

@sumanthvrao I just had a quick manual test of this again.

Did we come to a decision on how to clarify the UI? (narrow + search-text when not in search narrow?) This seems to still rather unclear.
Searching in 'starred' does seem to show the expected results, but doesn't show the right narrow title (ie. starred)
Similarly, searching in 'all PMs' results in the narrow being shown as 'private conversation' narrow, even though it is clearly showing a search in all PMs

The last few issues appear to also cause the narrow to update whenever switching between messages, which is unexpected!

sumanthvrao · 2019-08-02T14:43:05Z

@neiljp @amanagr Have pushed an update for this. The behaviour is more what one expects when searching. The main changes include:

Clearly marking 'Search Results' from the current narrow. This handles the last few points mentioned in Neils comment above.
Clearing the search result when switching narrow to something else from the search result.

I leave you to explore more of how this feels. Any suggestions or improvements are welcome.

A few improvements which I can think of right now would be:

Not clearing search text for 'close enough' narrows. (In which case the search carries over)
Display a dummy message when no search hits are found after search.
Allowing the search to carry on across narrows and defining some key to exit search (To be discussed)

neiljp · 2019-08-04T17:30:36Z

@sumanthvrao This looks a lot cleaner than before in manual testing. The remaining issues I see are:

searching for an empty string seems to give no results? (regression? do we have tests for that?)
having 'Search Results' and 'Search [/]:' in the same bar is not a great use of space, or textually very good; adjusting that could be a follow-up PR as it's just UI (?) but perhaps either have "Search [/]:" in white, or "Search Results [/]: " all in red?

Other than the first point, practically speaking this looks like a good 'v2' of our search, ie. including the narrow.

neiljp

@sumanthvrao The code looks pretty good, though perhaps a few extra methods might make things clearer? I mentioned moving the first added test to near the start, since it might make the early changes more obvious in terms of the behavior changing as defined by the tests.

zulipterminal/core.py

zulipterminal/helper.py

neiljp · 2019-08-04T17:56:15Z

tests/core/test_core.py

+        ({100})
+    ])
+    def test_search_message(self, initial_narrow, final_narrow,
+                            controller, mocker, msg_ids):


I know that it might be less simple to write, but it would be informative of the changes you have made to search if this test existed at the start, and then was amended as you worked through the previous commits? Then you wouldn't need to say 'from the previous commits', either.

@neiljp I have managed to include the tests in this commit itself. It would benefit the flow of commits.

I think this test is fine as a separate commit. The intent of my original suggestion was that you move (and adjust) the test commit to be first/early in the list of commits. Then when you change/add the search-in-narrow behavior you can change/add the expected behaviors to the test.

Yes. The behaviour has been adjusted.

neiljp · 2019-08-04T18:04:39Z

zulipterminal/ui_tools/boxes.py

+        if 'search' in [ele[0] for ele in curr_narrow]:
+            is_search_narrow = True
+            curr_narrow = [sub_narrow for sub_narrow in curr_narrow
+                           if sub_narrow[0] != 'search']


This code seems similar to that which you added to the model (and what I hinted at for the is_search_narrow case). I think this comes down to the idea of some kind of 'base' narrow, combined with a filter on top (ie. the search). If thinking of it that way, maybe the model could provide some kind of base_narrow narrow, separate to a search filter (if necessary)?

Thanks for pointing this out @neiljp. I do like the idea of making it like a base narrow and the search being an add on top of the base narrow. I have used the is_search_narrow helper now and it has helped reduce complexity a little bit. I am planning on taking this up as a follow-up if its okay.

zulipterminal/ui_tools/boxes.py

sumanthvrao · 2019-08-05T10:42:36Z

@neiljp Thanks for the review. I have included the tests in the starting commit which introduces the set_search_narrow function. This helped reduce some code duplication to a good degree. Further, the base_narrow implementation is a good idea. I am okay to do this as a follow-up.

neiljp

@sumanthvrao I've left some comments which are mostly minor or regarding commit structuring thoughts. However, it does still seem to not 'reset' when returning back to an 'empty' search?

As I clarified in a comment, I think it's fine to keep the introduction of the search test in a separate commit and early to define the current state, before updating it as the behavior changes through this PR.

zulipterminal/model.py

neiljp · 2019-08-06T01:09:04Z

tests/core/test_core.py

+        ({100})
+    ])
+    def test_search_message(self, initial_narrow, final_narrow,
+                            controller, mocker, msg_ids):


I think this test is fine as a separate commit. The intent of my original suggestion was that you move (and adjust) the test commit to be first/early in the list of commits. Then when you change/add the search-in-narrow behavior you can change/add the expected behaviors to the test.

neiljp · 2019-08-06T01:13:10Z

zulipterminal/core.py

@@ -117,6 +117,7 @@ def show_msg_info(self, msg: Any) -> None:

    def search_messages(self, text: str) -> None:
        # Search for a text in messages
+        self.model.index['search'].clear()


Does the test need to be amended here? Shouldn't it need to be?

Rather 'added'. We didn't test for self.model.index contents after call to search_messages, but we can do that now.

Tests added involve a previous narrow being unset and a new search narrow being set based on search query. This reflects the old earch behaviour which will be changed in the subsequent commits.

Helper aded to check if a narrow is a result of searching. This can be determined by checking if the narrow containts ['search', text] at any position. We make use of the helper to now as the conditional to add and fetch searches messages in the index. Tests added for model.is_search_narrow().

We make use of two helper functions, one to set the search narrow based on searched text and another to unset search narrow after a search has completed. model.set_narrow(search=text) has been removed due to the current version of using set_search_narrow specifically for setting such type of narrows. Tests amended to remove setting search narrow for set_narrow. Tests amended for setting narrow with `set_search_narrow` method based on new behaviour.

To avoid displaying search results indexed away from a previous search result, we flush the search index at the start of each search. Tests added.

To easily distinguish between a search result and other narrows, we mark the top_search_bar with 'Search Result'. Navigating out of this view remove the indicator from the header impying that the search query does not hold to the current narrow. Tests added.

Before we use 'z' keypress in order for the conditionals to work, we unset the ['search', text] part of the narrow.

sumanthvrao · 2019-08-08T13:47:31Z

@neiljp Updated based on your feedback.

neiljp · 2019-08-13T01:02:40Z

@sumanthvrao Thanks for this - as discussed, merged with slight adjustments with some extra tests, but this is a really useful search improvement 🎉

zulipbot added the size: S label Jul 10, 2019

neiljp reviewed Jul 12, 2019

View reviewed changes

sumanthvrao force-pushed the zt-improve-search-in-stream branch from 3ffd545 to 81687d0 Compare July 13, 2019 15:45

zulipbot added size: L and removed size: S labels Jul 13, 2019

neiljp reviewed Jul 13, 2019

View reviewed changes

zulipbot added the has conflicts label Jul 14, 2019

sumanthvrao force-pushed the zt-improve-search-in-stream branch from 81687d0 to 51d79a5 Compare July 14, 2019 10:51

zulipbot removed the has conflicts label Jul 14, 2019

neiljp reviewed Jul 28, 2019

View reviewed changes

sumanthvrao force-pushed the zt-improve-search-in-stream branch 4 times, most recently from e756573 to a649818 Compare August 2, 2019 14:37

sumanthvrao force-pushed the zt-improve-search-in-stream branch from a649818 to f5b2ef8 Compare August 2, 2019 14:44

neiljp reviewed Aug 4, 2019

View reviewed changes

sumanthvrao force-pushed the zt-improve-search-in-stream branch 3 times, most recently from 0b50b64 to 00924c9 Compare August 5, 2019 10:25

neiljp reviewed Aug 6, 2019

View reviewed changes

sumanthvrao added 5 commits August 8, 2019 09:55

tests: core: Add tests for setting narrow through search_messages.

5e702ae

Tests added involve a previous narrow being unset and a new search narrow being set based on search query. This reflects the old earch behaviour which will be changed in the subsequent commits.

core: Clear search index list before searching.

1b2dbd2

To avoid displaying search results indexed away from a previous search result, we flush the search index at the start of each search. Tests added.

sumanthvrao added 2 commits August 8, 2019 18:03

boxes: Reset search text when changing to a different narrow.

a7bb116

boxes: Unset search narrow before using 'z' keypress.

630510f

Before we use 'z' keypress in order for the conditionals to work, we unset the ['search', text] part of the narrow.

sumanthvrao force-pushed the zt-improve-search-in-stream branch from 00924c9 to 630510f Compare August 8, 2019 12:34

zulipbot added size: XL and removed size: L labels Aug 8, 2019

neiljp closed this Aug 13, 2019

sumanthvrao deleted the zt-improve-search-in-stream branch August 13, 2019 01:07

sumanthvrao mentioned this pull request Aug 13, 2019

TRACKING: Message search followups to #430 #458

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

model: Alter message search for searching in current narrow. #430

model: Alter message search for searching in current narrow. #430

sumanthvrao commented Jul 10, 2019

neiljp left a comment

sumanthvrao commented Jul 12, 2019

sumanthvrao commented Jul 13, 2019 •

edited

neiljp left a comment

neiljp left a comment

sumanthvrao commented Aug 2, 2019

neiljp commented Aug 4, 2019

neiljp left a comment

neiljp Aug 4, 2019

sumanthvrao Aug 5, 2019

neiljp Aug 6, 2019

sumanthvrao Aug 8, 2019

neiljp Aug 4, 2019

sumanthvrao Aug 5, 2019

sumanthvrao commented Aug 5, 2019

neiljp left a comment

neiljp Aug 6, 2019

neiljp Aug 6, 2019

sumanthvrao Aug 8, 2019

sumanthvrao commented Aug 8, 2019

neiljp commented Aug 13, 2019

model: Alter message search for searching in current narrow. #430

model: Alter message search for searching in current narrow. #430

Conversation

sumanthvrao commented Jul 10, 2019

neiljp left a comment

Choose a reason for hiding this comment

sumanthvrao commented Jul 12, 2019

sumanthvrao commented Jul 13, 2019 • edited

neiljp left a comment

Choose a reason for hiding this comment

neiljp left a comment

Choose a reason for hiding this comment

sumanthvrao commented Aug 2, 2019

neiljp commented Aug 4, 2019

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

sumanthvrao commented Aug 5, 2019

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

sumanthvrao commented Aug 8, 2019

neiljp commented Aug 13, 2019

sumanthvrao commented Jul 13, 2019 •

edited