Repository: zulip/zulip-terminal
Pull request #1393 (2023-05-04 23:35:29)

Refactoring before further work on message-focus solution
<!-- See README for documentation, or ask in #zulip-terminal if unclear -->
### What does this PR do, and why?

This is pure refactoring work that became commits to improve the code, while exploring how best to fix the message focus issue (message id vs list id).

### External discussion & connections
<!-- [x] all that apply, specifying topic and adding numbers after # for issues/PRs -->
- [ ] Discussed in **#zulip-terminal** in `topic`
- [ ] Fully fixes #
- [ ] Partially fixes issue #
- [ ] 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 -->
- [ ] Manually - Behavioral changes
- [ ] Manually - Visual changes
- [ ] Adapting existing automated tests
- [ ] Adding automated tests for new behavior (or missing tests)
- [x] 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
- [ ] It contains test additions for any new behavior
- [x] It flows clearly from a previous branch commit, and/or prepares for the next commit


zulipbot: Hello @zulip/server-refactoring members, this pull request was labeled with the "area: refactoring" label, so you may want to check it out!

<!-- areaLabelAddition -->


Patch:
diff --git a/zulipterminal/core.py b/zulipterminal/core.py
index 8ee293d..f1f32e4 100644
--- a/zulipterminal/core.py
+++ b/zulipterminal/core.py
@@ -571,9 +571,9 @@ class Controller:
         w_list = create_msg_box_list(self.model, msg_id_list, focus_msg_id=anchor)
 
         focus_position = self.model.get_focus_in_current_narrow()
-        if focus_position == set():  # No available focus; set to end
+        if focus_position is None:  # No available focus; set to end
             focus_position = len(w_list) - 1
-        assert not isinstance(focus_position, set)
+        assert focus_position is not None
 
         self.view.message_view.log.clear()
         if 0 <= focus_position < len(w_list):
diff --git a/zulipterminal/helper.py b/zulipterminal/helper.py
index 12f402b..0b7f59b 100644
--- a/zulipterminal/helper.py
+++ b/zulipterminal/helper.py
@@ -25,7 +25,6 @@ from typing import (
     Set,
     Tuple,
     TypeVar,
-    Union,
 )
 from urllib.parse import unquote
 
@@ -79,7 +78,7 @@ class TidiedUserInfo(TypedDict):
 
 
 class Index(TypedDict):
-    pointer: Dict[str, Union[int, Set[None]]]  # narrow_str, message_id
+    pointer: Dict[str, Optional[int]]  # narrow_str, message_id (or no data)
     # Various sets of downloaded message ids (all, starred, ...)
     all_msg_ids: Set[int]
     starred_msg_ids: Set[int]
@@ -97,7 +96,7 @@ class Index(TypedDict):
 
 
 initial_index = Index(
-    pointer=defaultdict(set),
+    pointer=dict(),
     all_msg_ids=set(),
     starred_msg_ids=set(),
     mentioned_msg_ids=set(),
diff --git a/zulipterminal/model.py b/zulipterminal/model.py
index 9166e30..68b7850 100644
--- a/zulipterminal/model.py
+++ b/zulipterminal/model.py
@@ -264,12 +264,12 @@ class Model:
             )
             self.cached_retention_text[stream["stream_id"]] = message_retention_response
 
-    def get_focus_in_current_narrow(self) -> Union[int, Set[None]]:
+    def get_focus_in_current_narrow(self) -> Optional[int]:
         """
         Returns the focus in the current narrow.
-        For no existing focus this returns {}, otherwise the message ID.
+        For no existing focus this returns None, otherwise the message ID.
         """
-        return self.index["pointer"][repr(self.narrow)]
+        return self.index["pointer"].get(repr(self.narrow), None)
 
     def set_focus_in_current_narrow(self, focus_message: int) -> None:
         self.index["pointer"][repr(self.narrow)] = focus_message
diff --git a/zulipterminal/ui_tools/messages.py b/zulipterminal/ui_tools/messages.py
index 1911dc5..583b9c5 100644
--- a/zulipterminal/ui_tools/messages.py
+++ b/zulipterminal/ui_tools/messages.py
@@ -186,11 +186,11 @@ class MessageBox(urwid.Pile):
         header.markup = title_markup
         return header
 
-    def top_header_bar(self, message_view: Any) -> Any:
+    def recipient_header(self) -> Any:
         if self.message["type"] == "stream":
-            return message_view.stream_header()
+            return self.stream_header()
         else:
-            return message_view.private_header()
+            return self.private_header()
 
     def top_search_bar(self) -> Any:
         curr_narrow = self.model.narrow
@@ -622,10 +622,7 @@ class MessageBox(urwid.Pile):
     def main_view(self) -> List[Any]:
         # Recipient Header
         if self.need_recipient_header():
-            if self.message["type"] == "stream":
-                recipient_header = self.stream_header()
-            else:
-                recipient_header = self.private_header()
+            recipient_header = self.recipient_header()
         else:
             recipient_header = None
 
diff --git a/zulipterminal/ui_tools/views.py b/zulipterminal/ui_tools/views.py
index a4b673d..b59533d 100644
--- a/zulipterminal/ui_tools/views.py
+++ b/zulipterminal/ui_tools/views.py
@@ -64,12 +64,16 @@ SIDE_PANELS_MOUSE_SCROLL_LINES = 5
 
 
 class ModListWalker(urwid.SimpleFocusListWalker):
+    def __init__(self, *, contents: List[Any], action: Callable[[], None]) -> None:
+        self._action = action
+        super().__init__(contents)
+
     def set_focus(self, position: int) -> None:
         # When setting focus via set_focus method.
         self.focus = position
         self._modified()
-        if hasattr(self, "read_message"):
-            self.read_message()
+
+        self._action()
 
     def _set_focus(self, index: int) -> None:
         # This method is called when directly setting focus via
@@ -85,8 +89,8 @@ class ModListWalker(urwid.SimpleFocusListWalker):
         if index != self._focus:
             self._focus_changed(index)
         self._focus = index
-        if hasattr(self, "read_message"):
-            self.read_message()
+
+        self._action()
 
     def extend(self, items: List[Any], focus_position: Optional[int] = None) -> int:
         if focus_position is None:
@@ -106,8 +110,7 @@ class MessageView(urwid.ListBox):
         self.view = view
         # Initialize for reference
         self.focus_msg = 0
-        self.log = ModListWalker(self.main_view())
-        self.log.read_message = self.read_message
+        self.log = ModListWalker(contents=self.main_view(), action=self.read_message)
 
         super().__init__(self.log)
         self.set_focus(self.focus_msg)
@@ -241,7 +244,7 @@ class MessageView(urwid.ListBox):
             return
         # if view is ready display current narrow
         # at the bottom of the view.
-        recipient_bar = message_view.top_header_bar(message_view)
+        recipient_bar = message_view.recipient_header()
         top_header = message_view.top_search_bar()
         self.model.controller.view.search_box.conversation_focus.set_text(
             top_header.markup


Test patch:
diff --git a/tests/conftest.py b/tests/conftest.py
index 8ab80ac..3e77d51 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -820,7 +820,7 @@ def empty_index(
 ) -> Index:
     return deepcopy(
         Index(
-            pointer=defaultdict(set, {}),
+            pointer=dict(),
             all_msg_ids=set(),
             starred_msg_ids=set(),
             mentioned_msg_ids=set(),
diff --git a/tests/model/test_model.py b/tests/model/test_model.py
index 7d858b8..b1cbc1d 100644
--- a/tests/model/test_model.py
+++ b/tests/model/test_model.py
@@ -330,7 +330,7 @@ class TestModel:
                 == expect_msg_retention_text[stream_id]
             )
 
-    @pytest.mark.parametrize("msg_id", [1, 5, set()])
+    @pytest.mark.parametrize("msg_id", [1, 5, None])
     @pytest.mark.parametrize(
         "narrow",
         [
@@ -362,9 +362,7 @@ class TestModel:
         ],
     )
     def test_set_focus_in_current_narrow(self, mocker, model, narrow, msg_id):
-        from collections import defaultdict
-
-        model.index = dict(pointer=defaultdict(set))
+        model.index = dict(pointer=dict())
         model.narrow = narrow
         model.set_focus_in_current_narrow(msg_id)
         assert model.index["pointer"][str(narrow)] == msg_id
diff --git a/tests/ui/test_ui_tools.py b/tests/ui/test_ui_tools.py
index bb71340..3c4edf6 100644
--- a/tests/ui/test_ui_tools.py
+++ b/tests/ui/test_ui_tools.py
@@ -29,8 +29,9 @@ MIDCOLVIEW = VIEWS + ".MiddleColumnView"
 
 class TestModListWalker:
     @pytest.fixture
-    def mod_walker(self):
-        return ModListWalker([list(range(1))])
+    def mod_walker(self, mocker):
+        read_message = mocker.Mock(spec=lambda: None)
+        return ModListWalker(contents=[list(range(1))], action=read_message)
 
     @pytest.mark.parametrize(
         "num_items, focus_position",
@@ -46,14 +47,14 @@ class TestModListWalker:
         mod_walker._set_focus.assert_called_once_with(focus_position)
 
     def test__set_focus(self, mod_walker, mocker):
-        mod_walker.read_message = mocker.Mock()
+        mod_walker._action.assert_not_called()
         mod_walker._set_focus(0)
-        mod_walker.read_message.assert_called_once_with()
+        mod_walker._action.assert_called_once_with()
 
     def test_set_focus(self, mod_walker, mocker):
-        mod_walker.read_message = mocker.Mock()
+        mod_walker._action.assert_not_called()
         mod_walker.set_focus(0)
-        mod_walker.read_message.assert_called_once_with()
+        mod_walker._action.assert_called_once_with()
 
 
 class TestMessageView:
@@ -386,8 +387,7 @@ class TestMessageView:
         msg_w = mocker.MagicMock()
         msg_w.original_widget.message = {"id": 1}
         msg_view.update_search_box_narrow(msg_w.original_widget)
-        msg_w.original_widget.top_header_bar.assert_called_once_with
-        (msg_w.original_widget)
+        msg_w.original_widget.recipient_header.assert_called_once_with()
         msg_w.original_widget.top_search_bar.assert_called_once_with()
 
     def test_read_message_no_msgw(self, mocker, msg_view):
diff --git a/tests/ui_tools/test_messages.py b/tests/ui_tools/test_messages.py
index 78d9d60..db22da3 100644
--- a/tests/ui_tools/test_messages.py
+++ b/tests/ui_tools/test_messages.py
@@ -962,7 +962,7 @@ class TestMessageBox:
         current_message = messages[msg_type]
         msg_box = MessageBox(current_message, self.model, messages[0])
         search_bar = msg_box.top_search_bar()
-        header_bar = msg_box.top_header_bar(msg_box)
+        header_bar = msg_box.recipient_header()
 
         assert header_bar[0].text.startswith(assert_header_bar)
         assert search_bar.text_to_fill == assert_search_bar