Repository: zulip/zulip-terminal

Issue:
Improve algorithm for 'next unread topic' (n) to use current message state
This is intended as a first step to resolving #1332. Please focus on this element of that issue first, and if things go well, then extending the algorithm further to resolve the remaining elements would be great as followup PR(s).

The current message is part of the UI, so we likely want to try passing this information as an additional parameter to the `get_next_unread_topic` method. This seems likely to replace the persistent topic state stored in the model right now.

There are existing tests for this method, which will need to be adjusted to account for the change in method signature (new parameter) and possible change in behavior. Depending on the cases covered by the tests, this may also warrant additional cases to demonstrate the code is working well.

Discussion:
theViz343: @zulipbot add "PR needs review"
theViz343: 
> Initial testing is generally good in terms of the intended feature, though I have found it reliably crashes right now with use from a message which is not a stream message, eg. loading ZT, then `P`, `n`.
> 
> I picked up that crash test by noting the different types you're using, as well as that you're unconditionally accessing and using the stream_id and topic name, and wondering if that would cause a problem :) (see inline comments)

Thanks for informing me about this! I think that checking if the message is a valid stream message should solve this issue. Also, I'll test other unintented cases where the feature might potentially fail.

> However, my earlier points mean you'll need some extra logic, which could be at the calling point, but it may be clearer in the model instead, so you could pass in the message id instead of the topic? The id is not indexed in the unread counts, but the model can validate and look up everything as to what response is suitable.

That makes sense. I'll change it accordingly.

> Highlighting one element of a comment inline, for the commits that currently implement the new logic, I'm unsure from your initial notes and questions whether you've additional tests still left to add or not, to help validate and complement the new code. Certainly additional tests are of interest if they cover extra test cases.

I've added some additional testcases which seem to cover the algorithm for the next unread topic for intended use. However, I plan to add more tests after I finish some manual testing for the unexpected usage cases. Also, I've yet to add tests for the get_current_topic function.

> In terms of the current code, it seems that I can change the code to use a much simpler non-bisect approach and still pass the tests you've added, so I'd query if we benefit from that complexity.

I agree that the bisect approach is harder to read. While writing the logic, it seemed that bisect reduced a few lines of code. But considering that a lot of people might potentially end up reading this code, I agree that a simple approach should be used to improve readability. I'll make the changes.


neiljp: > I've added some additional testcases which seem to cover the algorithm for the next unread topic for intended use. However, I plan to add more tests after I finish some manual testing for the unexpected usage cases. Also, I've yet to add tests for the get_current_topic function.

If you pass the message id, note that function may end up not being necessary? (get_current_topic)
theViz343: Yeah, you're correct. I plan to move the get_current_topic function to model.py and then call it in the get_next_unread_topic function. Keeping the topic extraction logic (get_current_topic) separate from the rest of the get_next_unread_topic would make testing easier. So the get_current_topic would reside in model.py instead of views.py, with a slight difference being the extraction of topic of the message from message id instead of the current way of doing it.
zulipbot: Hello @theViz343, it seems like you have referenced #1333 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 #1333.`.

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](https://zulip.readthedocs.io/en/latest/contributing/version-control.html#commit-messages).

<!-- fixCommitWarning -->

theViz343: @zulipbot add "PR needs review" remove "PR awaiting update"
theViz343: @zulipbot remove "PR awaiting update"
theViz343: @zulipbot remove "PR awaiting update"
@zulipbot add "PR needs review"
zulipbot: Heads up @theViz343, we just merged some commits that conflict with the changes you 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 -->

zulipbot: Hello @theViz343, it seems like you have referenced #1333 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 #1333.`.

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](https://zulip.readthedocs.io/en/latest/contributing/version-control.html#commit-messages).

<!-- fixCommitWarning -->


Patch:
diff --git a/zulipterminal/model.py b/zulipterminal/model.py
index 82d809a..7073e4f 100644
--- a/zulipterminal/model.py
+++ b/zulipterminal/model.py
@@ -2,6 +2,7 @@
 Defines the `Model`, fetching and storing data retrieved from the Zulip server
 """
 
+import bisect
 import itertools
 import json
 import time
@@ -111,7 +112,6 @@ class Model:
         self.stream_id: Optional[int] = None
         self.recipients: FrozenSet[Any] = frozenset()
         self.index = initial_index
-        self._last_unread_topic = None
         self.last_unread_pm = None
 
         self.user_id = -1
@@ -886,23 +886,67 @@ class Model:
         topic_to_search = (stream_name, topic)
         return topic_to_search in self._muted_topics
 
-    def get_next_unread_topic(self) -> Optional[Tuple[int, str]]:
+    def stream_topic_from_message_id(
+        self, message_id: int
+    ) -> Optional[Tuple[int, str]]:
+        """
+        Returns the stream and topic of a message of a given message id.
+        If the message is not a stream message or if it is not present in the index,
+        None is returned.
+        """
+        message = self.index["messages"].get(message_id, None)
+        if message is not None and message["type"] == "stream":
+            stream_id = message["stream_id"]
+            topic = message["subject"]
+            return (stream_id, topic)
+        return None
+
+    def next_unread_topic_from_message_id(
+        self, current_message: Optional[int]
+    ) -> Optional[Tuple[int, str]]:
+        if current_message:
+            current_topic = self.stream_topic_from_message_id(current_message)
+        else:
+            current_topic = (
+                self.stream_id_from_name(self.narrow[0][1]),
+                self.narrow[1][1],
+            )
         unread_topics = sorted(self.unread_counts["unread_topics"].keys())
         next_topic = False
-        if self._last_unread_topic not in unread_topics:
+        stream_start: Optional[Tuple[int, str]] = None
+        if current_topic is None:
             next_topic = True
+        elif current_topic not in unread_topics:
+            # insert current_topic in list of unread_topics for the case where
+            # 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)
         # 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:
             stream_id, topic_name = unread_topic
-            if (
-                not self.is_muted_topic(stream_id, topic_name)
-                and not self.is_muted_stream(stream_id)
-                and next_topic
-            ):
-                self._last_unread_topic = unread_topic
-                return unread_topic
-            if unread_topic == self._last_unread_topic:
+            if not self.is_muted_topic(
+                stream_id, topic_name
+            ) and not self.is_muted_stream(stream_id):
+                if next_topic:
+                    if unread_topic == current_topic:
+                        return None
+                    if (
+                        current_topic is not None
+                        and unread_topic[0] != current_topic[0]
+                        and stream_start != current_topic
+                    ):
+                        return stream_start
+                    return unread_topic
+
+                if (
+                    stream_start is None
+                    and current_topic is not None
+                    and unread_topic[0] == current_topic[0]
+                ):
+                    stream_start = unread_topic
+            if unread_topic == current_topic:
                 next_topic = True
         return None
 
diff --git a/zulipterminal/ui_tools/views.py b/zulipterminal/ui_tools/views.py
index 64eebeb..6a1e4e6 100644
--- a/zulipterminal/ui_tools/views.py
+++ b/zulipterminal/ui_tools/views.py
@@ -584,9 +584,20 @@ class MiddleColumnView(urwid.Frame):
 
         elif is_command_key("NEXT_UNREAD_TOPIC", key):
             # narrow to next unread topic
-            stream_topic = self.model.get_next_unread_topic()
-            if stream_topic is None:
+            focus = self.view.message_view.focus
+            narrow = self.model.narrow
+            if focus:
+                current_msg_id = focus.original_widget.message["id"]
+                stream_topic = self.model.next_unread_topic_from_message_id(
+                    current_msg_id
+                )
+                if stream_topic is None:
+                    return key
+            elif narrow[0][0] == "stream" and narrow[1][0] == "topic":
+                stream_topic = self.model.next_unread_topic_from_message_id(None)
+            else:
                 return key
+
             stream_id, topic = stream_topic
             self.controller.narrow_to_topic(
                 stream_name=self.model.stream_dict[stream_id]["name"],


Test patch:
diff --git a/tests/model/test_model.py b/tests/model/test_model.py
index 2bf150a..fe2ba55 100644
--- a/tests/model/test_model.py
+++ b/tests/model/test_model.py
@@ -72,7 +72,6 @@ class TestModel:
         assert model.stream_dict == stream_dict
         assert model.recipients == frozenset()
         assert model.index == initial_index
-        assert model._last_unread_topic is None
         assert model.last_unread_pm is None
         model.get_messages.assert_called_once_with(
             num_before=30, num_after=10, anchor=None
@@ -3900,7 +3899,7 @@ class TestModel:
         assert return_value == is_muted
 
     @pytest.mark.parametrize(
-        "unread_topics, last_unread_topic, next_unread_topic",
+        "unread_topics, current_topic, next_unread_topic",
         [
             case(
                 {(1, "topic"), (2, "topic2")},
@@ -3920,10 +3919,10 @@ class TestModel:
                 (1, "topic"),
                 id="unread_present_before_previous_topic",
             ),
-            case(  # TODO Should be None? (2 other cases)
+            case(
                 {(1, "topic")},
                 (1, "topic"),
-                (1, "topic"),
+                None,
                 id="unread_still_present_in_topic",
             ),
             case(
@@ -3993,16 +3992,42 @@ class TestModel:
                 (2, "topic2"),
                 id="unread_present_after_previous_topic_muted",
             ),
+            case(
+                {(1, "topic1"), (2, "topic2"), (2, "topic2 muted")},
+                (2, "topic1"),
+                (2, "topic2"),
+                id="unmuted_unread_present_in_same_stream_as_current_topic_not_in_unread_list",
+            ),
+            case(
+                {(1, "topic1"), (2, "topic2 muted"), (4, "topic4")},
+                (2, "topic1"),
+                (4, "topic4"),
+                id="unmuted_unread_present_in_next_stream_as_current_topic_not_in_unread_list",
+            ),
+            case(
+                {(1, "topic1"), (2, "topic2 muted"), (3, "topic3")},
+                (2, "topic1"),
+                (1, "topic1"),
+                id="unmuted_unread_not_present_in_next_stream_as_current_topic_not_in_unread_list",
+            ),
+            case(
+                {(1, "topic1"), (1, "topic11"), (2, "topic2")},
+                (1, "topic11"),
+                (1, "topic1"),
+                id="unread_present_in_same_stream_wrap_around",
+            ),
         ],
     )
-    def test_get_next_unread_topic(
-        self, model, unread_topics, last_unread_topic, next_unread_topic
+    def test_next_unread_topic_from_message(
+        self, mocker, model, unread_topics, current_topic, next_unread_topic
     ):
         # NOTE Not important how many unreads per topic, so just use '1'
         model.unread_counts = {
             "unread_topics": {stream_topic: 1 for stream_topic in unread_topics}
         }
-        model._last_unread_topic = last_unread_topic
+
+        current_message_id = 10  # Arbitrary value due to mock below
+        model.stream_topic_from_message_id = mocker.Mock(return_value=current_topic)
 
         # Minimal extra streams for muted stream testing (should not exist otherwise)
         assert {3, 4} & set(model.stream_dict) == set()
@@ -4020,7 +4045,38 @@ class TestModel:
             ]
         }
 
-        unread_topic = model.get_next_unread_topic()
+        unread_topic = model.next_unread_topic_from_message_id(current_message_id)
+
+        assert unread_topic == next_unread_topic
+
+    @pytest.mark.parametrize(
+        "unread_topics, empty_narrow, narrow_stream_id, next_unread_topic",
+        [
+            case(
+                {(1, "topic1"), (1, "topic2"), (2, "topic3")},
+                [["stream", "Stream 1"], ["topic", "topic1.5"]],
+                1,
+                (1, "topic2"),
+            ),
+        ],
+    )
+    def test_next_unread_topic_from_message__empty_narrow(
+        self,
+        mocker,
+        model,
+        unread_topics,
+        empty_narrow,
+        narrow_stream_id,
+        next_unread_topic,
+    ):
+        # NOTE Not important how many unreads per topic, so just use '1'
+        model.unread_counts = {
+            "unread_topics": {stream_topic: 1 for stream_topic in unread_topics}
+        }
+        model.stream_id_from_name = mocker.Mock(return_value=narrow_stream_id)
+        model.narrow = empty_narrow
+
+        unread_topic = model.next_unread_topic_from_message_id(None)
 
         assert unread_topic == next_unread_topic
 
@@ -4043,6 +4099,21 @@ class TestModel:
         assert return_value is None
         assert model.last_unread_pm is None
 
+    @pytest.mark.parametrize(
+        "message_id, expected_value",
+        [
+            case(537286, (205, "Test"), id="stream_message"),
+            case(537287, None, id="direct_message"),
+            case(537289, None, id="non-existent message"),
+        ],
+    )
+    def test_stream_topic_from_message_id(
+        self, mocker, model, message_id, expected_value, empty_index
+    ):
+        model.index = empty_index
+        current_topic = model.stream_topic_from_message_id(message_id)
+        assert current_topic == expected_value
+
     @pytest.mark.parametrize(
         "stream_id, expected_response",
         [
diff --git a/tests/ui/test_ui_tools.py b/tests/ui/test_ui_tools.py
index c04c9c3..324ee04 100644
--- a/tests/ui/test_ui_tools.py
+++ b/tests/ui/test_ui_tools.py
@@ -887,9 +887,10 @@ class TestMiddleColumnView:
     ):
         size = widget_size(mid_col_view)
         mocker.patch(MIDCOLVIEW + ".focus_position")
+        mocker.patch.object(self.view, "message_view")
 
         mid_col_view.model.stream_dict = {1: {"name": "stream"}}
-        mid_col_view.model.get_next_unread_topic.return_value = (1, "topic")
+        mid_col_view.model.next_unread_topic_from_message_id.return_value = (1, "topic")
 
         return_value = mid_col_view.keypress(size, key)
 
@@ -904,7 +905,8 @@ class TestMiddleColumnView:
     ):
         size = widget_size(mid_col_view)
         mocker.patch(MIDCOLVIEW + ".focus_position")
-        mid_col_view.model.get_next_unread_topic.return_value = None
+        mocker.patch.object(self.view, "message_view")
+        mid_col_view.model.next_unread_topic_from_message_id.return_value = None
 
         return_value = mid_col_view.keypress(size, key)