Repository: zulip/zulip-terminal
Pull request #1186 (2022-04-04 18:14:08)

ui/boxes: Improve stream sorting for stream typeahead in compose box.
<!-- Please see https://github.com/zulip/zulip-terminal#contributor-guidelines ! -->

**What does this PR do?**  <!-- Overall description goes here -->
This PR improves stream sorting for stream typeahead in compose box stream results in footer in the following order -

1) current stream (if matching)
2) pinned and unmuted
3) unpinned and unmuted
4) pinned and muted
5) unpinned and muted

<!-- If fixing a filed bug or new feature, add 'Fixes #<issue>' or 'Partial fix for #<issue>' -->

<!-- Add a link to a discussion on chat.zulip.org, if relevant -->
CZO - https://chat.zulip.org/#narrow/stream/206-zulip-terminal/topic/Improve.20stream.20sorting.20for.20stream.20typeahead.20in.20compose.20box.2E

**Tested?** <!-- Fine to leave some of these unchecked if this is a draft/work-in-progress -->
- [x] Manually
- [ ] Existing tests (adapted, if necessary)
- [ ] New tests added (for any new behavior)
- [ ] Passed linting & tests (each commit)
<!-- Code must pass CI (GitHub Actions) before merging - look for the green tick! -->
Passing all lint tests.

``test_generic_autocomplete_streams`` test to be updated in ``test_boxes.py``
<!-- See https://github.com/zulip/zulip-terminal#commit-style -->
**Commit flow** <!-- if more than one commit; add/delete/fill-in as appropriate -->
<!-- For example:
- first commit doing some thing
- maybe multiple commits doing similar things
-->

**Notes & Questions** <!-- if any; add/delete/fill-in as appropriate -->
<!-- For example:
- this doesn't include feature X (yet?)
- unsure about Y
- should this do Z?
-->

**Interactions** <!-- if any; add/delete/fill-in as appropriate -->
<!-- eg.
- Waiting on #<PR>
- Blocks #<PR>
-->

**Visual changes** <!-- if any; add/delete/fill-in with screenshot/diagram as appropriate -->
Stream sorting for stream typeahead before -
![image](https://user-images.githubusercontent.com/53873549/162564960-639f268c-b160-426d-8ab6-e4309806a59a.png)

Stream sorting for stream typeahead after -
![image](https://user-images.githubusercontent.com/53873549/162964508-31b452df-8ffd-4d3d-b4e7-a847d8f92f6c.png)

neiljp: Just to note here that as per discussion in the stream, the intention is to leave the search order unaltered during filtering in the left panel (as in the web app zulip/zulip#20618) and adjust the order for other cases such as the stream-box and stream/topic mentions in the main compose box.
srdeotarse: @neiljp as per your note, I have changed the PR topic and updated the code to improve stream sorting in compose box stream typeahead. Recent commit only handles stream typeahead sorting in compose box message area. 
srdeotarse: Reopening the PR
srdeotarse: @zulipbot add "PR needs review'.
srdeotarse: @zulipbot add "PR needs review".
zulipbot: Heads up @srdeotarse, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's [recent commits](https://github.com/zulip/zulip-terminal/commits/main) to see where the conflicts occur. Please rebase your feature branch against the `upstream/main` branch and [resolve](https://zulip.readthedocs.io/en/latest/git/troubleshooting.html#recover-from-a-git-rebase-failure) your pull request's merge conflicts accordingly.

<!-- mergeConflictWarning -->

neiljp: @srdeotarse I've just pushed the first 3 commits here, with some changes to the commit text reasoning as those ending in https://github.com/zulip/zulip-terminal/commit/898bb2e3722567f6a4f4bcc0730f2ad77cfa9d9f. Rebasing should clear any conflict.

I've not looked at more at this point, as the corresponding tests for the first code change fail, so there is some mismatch between test expectations and code behavior.
neiljp: @srdeotarse As per our discussion in the stream, I squashed your feature code+test commits together, and added some adjustments as per the extra commits, resulting in the branch with separated commits at https://github.com/neiljp/zulip-terminal/tree/review-original-1186-6-with-adjust

I did a final tidy of the commit text, to emphasize the action associated with the feature commits, ie. what changed/why, which you can see by comparison with your original commits in the link above or that you may have locally. This kind of detail can be helpful not just at this stage for merging and reading later, but also for reviewers wanting to know "What's in this commit?" and whether it matches the description.

Thanks for pushing this to completion - as I say, merging now! 🎉 
(with those fixup commits squashed into yours)

Patch:
diff --git a/zulipterminal/ui_tools/boxes.py b/zulipterminal/ui_tools/boxes.py
index 12f8a53..1abf753 100644
--- a/zulipterminal/ui_tools/boxes.py
+++ b/zulipterminal/ui_tools/boxes.py
@@ -636,10 +636,39 @@ class WriteBox(urwid.Pile):
 
         prefix_length = len(prefix_string)
 
-        matched_data = match_stream(
+        _, matched_streams = match_stream(
             stream_data, text[prefix_length:], self.view.pinned_streams
         )
-        return matched_data
+
+        muted_streams = [
+            self.model.stream_dict[stream_id]["name"]
+            for stream_id in self.model.muted_streams
+        ]
+        matching_muted_streams = [
+            stream_name
+            for stream_name in matched_streams
+            if stream_name in muted_streams
+        ]
+        pinned_streams = [stream["name"] for stream in self.view.pinned_streams]
+        pinned_unpinned_separator = len(set(pinned_streams) & set(matched_streams))
+        for matching_muted_stream in matching_muted_streams:
+            matched_streams.remove(matching_muted_stream)
+            if matching_muted_stream in pinned_streams:
+                matched_streams.insert(
+                    pinned_unpinned_separator - 1, matching_muted_stream
+                )
+            else:
+                matched_streams.append(matching_muted_stream)
+
+        current_stream = self.model.stream_dict.get(self.stream_id, None)
+        if current_stream is not None:
+            current_stream_name = current_stream["name"]
+            if current_stream_name in matched_streams:
+                matched_streams.remove(current_stream_name)
+                matched_streams.insert(0, current_stream_name)
+
+        matched_stream_typeaheads = format_string(matched_streams, "#**{}**")
+        return matched_stream_typeaheads, matched_streams
 
     def autocomplete_stream_and_topic(
         self, text: str, prefix_string: str


Test patch:
diff --git a/tests/ui_tools/test_boxes.py b/tests/ui_tools/test_boxes.py
index 4b3cec8..cf38ddd 100644
--- a/tests/ui_tools/test_boxes.py
+++ b/tests/ui_tools/test_boxes.py
@@ -136,10 +136,12 @@ class TestWriteBox:
         is_valid_stream: bool,
         required_typeahead: Optional[str],
         topics: List[str],
+        stream_dict: Dict[int, Dict[str, Any]],
     ) -> None:
         write_box.model.topics_in_stream.return_value = topics
         write_box.model.is_valid_stream.return_value = is_valid_stream
-
+        write_box.model.stream_dict = stream_dict
+        write_box.model.muted_streams = set()
         typeahead_string = write_box.generic_autocomplete(text, state)
 
         assert typeahead_string == required_typeahead
@@ -555,10 +557,13 @@ class TestWriteBox:
         state: Optional[int],
         footer_text: List[Any],
         text: str,
+        stream_dict: Dict[int, Dict[str, Any]],
     ) -> None:
         write_box.view.set_typeahead_footer = mocker.patch(
             "zulipterminal.ui.View.set_typeahead_footer"
         )
+        write_box.model.stream_dict = stream_dict
+        write_box.model.muted_streams = set()
         write_box.generic_autocomplete(text, state)
 
         write_box.view.set_typeahead_footer.assert_called_once_with(
@@ -814,6 +819,100 @@ class TestWriteBox:
                 },
                 {"pinned": ["Secret stream", "Stream 1"]},
             ),
+            # With 'Secret stream' muted.
+            (
+                "#Stream",
+                {
+                    0: "#**Stream 1**",
+                    1: "#**Stream 2**",
+                    2: "#**Some general stream**",
+                    3: "#**Web public stream**",
+                    4: "#**Secret stream**",
+                },
+                {"muted": ["Secret stream"]},
+            ),
+            # With 'Stream 1' and 'Secret stream' muted.
+            (
+                "#Stream",
+                {
+                    0: "#**Stream 2**",
+                    1: "#**Some general stream**",
+                    2: "#**Web public stream**",
+                    3: "#**Stream 1**",
+                    4: "#**Secret stream**",
+                },
+                {"muted": ["Secret stream", "Stream 1"]},
+            ),
+            # With 'Stream 1' and 'Secret stream' pinned, 'Secret stream' muted.
+            (
+                "#Stream",
+                {
+                    0: "#**Stream 1**",
+                    1: "#**Secret stream**",
+                    2: "#**Stream 2**",
+                    3: "#**Some general stream**",
+                    4: "#**Web public stream**",
+                },
+                {"pinned": ["Secret stream", "Stream 1"], "muted": ["Secret stream"]},
+            ),
+            # With 'Stream 1' and 'Secret stream' pinned, 'Some general stream' and 'Stream 2' muted.
+            (
+                "#Stream",
+                {
+                    0: "#**Stream 1**",
+                    1: "#**Secret stream**",
+                    2: "#**Web public stream**",
+                    3: "#**Stream 2**",
+                    4: "#**Some general stream**",
+                },
+                {
+                    "pinned": ["Secret stream", "Stream 1"],
+                    "muted": ["Some general stream", "Stream 2"],
+                },
+            ),
+            # With 'Stream 1' and 'Secret stream' pinned, 'Secret stream' and 'Stream 2' muted.
+            (
+                "#Stream",
+                {
+                    0: "#**Stream 1**",
+                    1: "#**Secret stream**",
+                    2: "#**Some general stream**",
+                    3: "#**Web public stream**",
+                    4: "#**Stream 2**",
+                },
+                {
+                    "pinned": ["Secret stream", "Stream 1"],
+                    "muted": ["Secret stream", "Stream 2"],
+                },
+            ),
+            # With 'Stream 1' as current stream.
+            (
+                "#S",
+                {
+                    0: "#**Stream 1**",
+                    1: "#**Secret stream**",
+                    2: "#**Some general stream**",
+                    3: "#**Stream 2**",
+                    4: "#**Web public stream**",
+                },
+                {"current_stream": 1},
+            ),
+            # With 'Stream 1' and 'Secret stream' pinned, 'Secret stream' and 'Stream 2' muted, 'Stream 2' as current stream.
+            (
+                "#Stream",
+                {
+                    0: "#**Stream 2**",
+                    1: "#**Stream 1**",
+                    2: "#**Secret stream**",
+                    3: "#**Some general stream**",
+                    4: "#**Web public stream**",
+                },
+                {
+                    "pinned": ["Secret stream", "Stream 1"],
+                    "muted": ["Secret stream", "Stream 2"],
+                    "current_stream": 2,
+                },
+            ),
         ],
     )
     def test_generic_autocomplete_streams(
@@ -822,6 +921,7 @@ class TestWriteBox:
         text: str,
         state_and_required_typeahead: Dict[int, Optional[str]],
         stream_categories: Dict[str, Any],
+        stream_dict: Dict[int, Dict[str, Any]],
     ) -> None:
         streams_to_pin = (
             [{"name": stream_name} for stream_name in stream_categories["pinned"]]
@@ -831,9 +931,19 @@ class TestWriteBox:
         for stream in streams_to_pin:
             write_box.view.unpinned_streams.remove(stream)
         write_box.view.pinned_streams = streams_to_pin
-        for state, required_typeahead in state_and_required_typeahead.items():
-            typeahead_string = write_box.generic_autocomplete(text, state)
-            assert typeahead_string == required_typeahead
+        write_box.stream_id = stream_categories.get("current_stream", None)
+        write_box.model.stream_dict = stream_dict
+        write_box.model.muted_streams = set(
+            stream["stream_id"]
+            for stream in stream_dict.values()
+            if stream["name"] in stream_categories.get("muted", set())
+        )
+        states = state_and_required_typeahead.keys()
+        required_typeaheads = list(state_and_required_typeahead.values())
+        typeahead_strings = [
+            write_box.generic_autocomplete(text, state) for state in states
+        ]
+        assert typeahead_strings == required_typeaheads
 
     @pytest.mark.parametrize(
         "text, state, required_typeahead",