Repository: zulip/zulip-terminal
Pull request #1412 (2023-06-23 12:10:44)

Use stream name instead of stream id to order unread topics in the next_unread_topic_from_message_id algorithm.
<!-- See README for documentation, or ask in #zulip-terminal if unclear -->
### What does this PR do, and why?

This PR sorts the list of unread topics using stream name instead of stream id. This makes the order of moving between streams similar to that in the left panel.

### Outstanding aspect(s)    <!-- DELETE SECTION IF EMPTY -->
<!-- In what ways is this not fully implemented/functioning? Compared to a discussion/issue? -->
<!-- Do you not understand something? Are you unsure about a certain approach? Want feedback? -->

### External discussion & connections
<!-- [x] all that apply, specifying topic and adding numbers after # for issues/PRs -->
- [x] Discussed in **#zulip-terminal** in [`Improve algorithm for 'next unread topic' (n)`](https://chat.zulip.org/#narrow/stream/206-zulip-terminal/topic/Improve.20algorithm.20for.20'next.20unread.20topic'.20.28n.29.20.23T1333.20.23T1356)
- [ ] Fully fixes #
- [x] Partially fixes issue #1332 
- [ ] Builds upon previous unmerged work in PR #
- [ ] Is a follow-up to work in PR #
- [ ] Requires merge of PR #
- [ ] Merge will enable work on #

### How did you test this?
<!-- [x] all that apply -->
- [x] Manually - Behavioral changes
- [ ] Manually - Visual changes
- [x] Adapting existing automated tests
- [x] Adding automated tests for new behavior (or missing tests)
- [ ] Existing automated tests should already cover this (*only a refactor of tested code*)

### Self-review checklist for each commit
- [x] It is a [minimal coherent idea](https://github.com/zulip/zulip-terminal#structuring-commits---speeding-up-reviews-merging--development)
- [x] It has a commit summary following the [documented style](https://github.com/zulip/zulip-terminal#structuring-commits---speeding-up-reviews-merging--development) (title & body)
- [x] It has a commit summary describing the  motivation and reasoning for the change
- [x] It individually passes linting and tests
- [x] It contains test additions for any new behavior
- [x] It flows clearly from a previous branch commit, and/or prepares for the next commit

### Visual changes    <!-- DELETE SECTION IF NO VISUAL CHANGE -->
<!-- Zulip tips at https://zulip.readthedocs.io/en/latest/tutorials/screenshot-and-gif-software.html -->
<!-- For video, try asciinema; after uploading, embed using
-->
<!-- NOTE: Attached videos/images will be clearer from smaller terminal windows -->
[![asciicast](https://asciinema.org/a/ML0Qj2WTSh5krcyCILzyzqdnv.svg)](https://asciinema.org/a/ML0Qj2WTSh5krcyCILzyzqdnv)


neiljp: @theViz343 Gave this a try - left more feedback in the stream :)
neiljp: @theViz343 I updated the labels since it doesn't appear to be in a state requiring feedback - the tests are failing?
neiljp: @theViz343 Thanks - this was good to go! :tada:

I only made a few very minor changes to the commit text and added an assert for the extra stream_dict data you added.

Patch:
diff --git a/zulipterminal/helper.py b/zulipterminal/helper.py
index ce77c52..52e4076 100644
--- a/zulipterminal/helper.py
+++ b/zulipterminal/helper.py
@@ -165,6 +165,20 @@ def asynch(func: Callable[ParamT, None]) -> Callable[ParamT, None]:
     return wrapper
 
 
+def sort_unread_topics(
+    unread_topics: Dict[Tuple[int, str], int], stream_list: List[int]
+) -> List[Tuple[int, str]]:
+    return sorted(
+        unread_topics.keys(),
+        key=lambda stream_topic: (
+            stream_list.index(stream_topic[0])
+            if stream_topic[0] in stream_list
+            else len(stream_list),
+            stream_topic[1],
+        ),
+    )
+
+
 def _set_count_in_model(
     new_count: int, changed_messages: List[Message], unread_counts: UnreadCounts
 ) -> None:
diff --git a/zulipterminal/model.py b/zulipterminal/model.py
index 7073e4f..9c6fce6 100644
--- a/zulipterminal/model.py
+++ b/zulipterminal/model.py
@@ -2,7 +2,6 @@
 Defines the `Model`, fetching and storing data retrieved from the Zulip server
 """
 
-import bisect
 import itertools
 import json
 import time
@@ -73,6 +72,7 @@ from zulipterminal.helper import (
     initial_index,
     notify_if_message_sent_outside_narrow,
     set_count,
+    sort_unread_topics,
 )
 from zulipterminal.platform_code import notify
 from zulipterminal.ui_tools.utils import create_msg_box_list
@@ -911,7 +911,13 @@ class Model:
                 self.stream_id_from_name(self.narrow[0][1]),
                 self.narrow[1][1],
             )
-        unread_topics = sorted(self.unread_counts["unread_topics"].keys())
+        left_panel_stream_list = [
+            stream["id"] for stream in (self.pinned_streams + self.unpinned_streams)
+        ]
+        unread_topics = sort_unread_topics(
+            self.unread_counts["unread_topics"],
+            left_panel_stream_list,
+        )
         next_topic = False
         stream_start: Optional[Tuple[int, str]] = None
         if current_topic is None:
@@ -921,7 +927,10 @@ class Model:
             # current_topic is not in unread_topics, and the next unmuted topic
             # is to be returned. This does not modify the original unread topics
             # data, and is just used to compute the next unmuted topic to be returned.
-            bisect.insort(unread_topics, current_topic)
+            unread_topics = sort_unread_topics(
+                {**self.unread_counts["unread_topics"], current_topic: 0},
+                left_panel_stream_list,
+            )
         # loop over unread_topics list twice for the case that last_unread_topic was
         # the last valid unread_topic in unread_topics list.
         for unread_topic in unread_topics * 2:


Test patch:
diff --git a/tests/helper/test_helper.py b/tests/helper/test_helper.py
index 3c23343..8a79359 100644
--- a/tests/helper/test_helper.py
+++ b/tests/helper/test_helper.py
@@ -19,6 +19,7 @@ from zulipterminal.helper import (
     open_media,
     powerset,
     process_media,
+    sort_unread_topics,
 )
 
 
@@ -244,6 +245,41 @@ def test_powerset(
     assert powerset(iterable, map_func) == expected_powerset
 
 
+@pytest.mark.parametrize(
+    "unread_topics, expected_value",
+    [
+        case({}, [], id="no_unread_topics"),
+        case(
+            {(99, "topic1"): 1},
+            [(99, "topic1")],
+            id="single_unread_topic",
+        ),
+        case(
+            {(999, "topic3"): 1, (1000, "topic2"): 1, (1, "topic1"): 1},
+            [(1000, "topic2"), (1, "topic1"), (999, "topic3")],
+            id="multiple_unread_topics",
+        ),
+        case(
+            {
+                (999, "topic3"): 1,
+                (1000, "topic2"): 1,
+                (1000, "topic4"): 3,
+                (1, "topic1"): 1,
+            },
+            [(1000, "topic2"), (1000, "topic4"), (1, "topic1"), (999, "topic3")],
+            id="multiple_unread_topics_in_same_stream",
+        ),
+    ],
+)
+def test_sort_unread_topics(
+    unread_topics: Dict[Tuple[int, str], int],
+    expected_value: List[Tuple[int, str]],
+    streams: List[Dict[str, Any]],
+) -> None:
+    stream_list = [stream["id"] for stream in streams]
+    assert sort_unread_topics(unread_topics, stream_list) == expected_value
+
+
 @pytest.mark.parametrize(
     "muted_streams, muted_topics, vary_in_unreads",
     [
diff --git a/tests/model/test_model.py b/tests/model/test_model.py
index fe2ba55..915621f 100644
--- a/tests/model/test_model.py
+++ b/tests/model/test_model.py
@@ -4016,6 +4016,12 @@ class TestModel:
                 (1, "topic1"),
                 id="unread_present_in_same_stream_wrap_around",
             ),
+            case(
+                {(1, "topic1"), (5, "topic5"), (2, "topic2")},
+                (2, "topic2"),
+                (5, "topic5"),
+                id="streams_sorted_according_to_left_panel",
+            ),
         ],
     )
     def test_next_unread_topic_from_message(
@@ -4035,6 +4041,20 @@ class TestModel:
         model.stream_dict[4] = {"name": "Stream 4"}
         model.muted_streams = {3}
 
+        # Extra stream for stream sort testing (should not exist otherwise)
+        assert 5 not in model.stream_dict
+        model.stream_dict[5] = {"name": "First Stream"}
+
+        model.pinned_streams = [
+            {"name": "First Stream", "id": 5},
+            {"name": "Stream 1", "id": 1},
+            {"name": "Stream 2", "id": 2},
+        ]
+        model.unpinned_streams = [
+            {"name": "Stream 3", "id": 3},
+            {"name": "Stream 4", "id": 4},
+        ]
+
         # date data unimportant (if present)
         model._muted_topics = {
             stream_topic: None
@@ -4073,6 +4093,15 @@ class TestModel:
         model.unread_counts = {
             "unread_topics": {stream_topic: 1 for stream_topic in unread_topics}
         }
+        model.pinned_streams = [
+            {"name": "Stream 1", "id": 1},
+            {"name": "Stream 2", "id": 2},
+        ]
+        model.unpinned_streams = [
+            {"name": "Stream 3", "id": 3},
+            {"name": "Stream 4", "id": 4},
+        ]
+
         model.stream_id_from_name = mocker.Mock(return_value=narrow_stream_id)
         model.narrow = empty_narrow