Repository: zulip/zulip-terminal
Pull request #1206 (2022-04-18 03:01:14)

bugfix: core: Do not exit editor mode when narrowing.

<!-- Please see https://github.com/zulip/zulip-terminal#contributor-guidelines ! -->

**What does this PR do?**  <!-- Overall description goes here -->

These unrelated aspects being tied together was not an issue previously,
when narrowing was always done outside of compose. However, with
integration of #1194 as 2195904dd this is no longer the case, so this is
required to ensure that narrowing does not prematurely end editor mode,
since otherwise container widgets would steal keypresses.

This may be improved later through further refactoring of the keypress
handling.

**Tested?** <!-- Fine to leave some of these unchecked if this is a draft/work-in-progress -->
- [x] Manually
- [x] Existing tests (adapted, if necessary)
- [ ] New tests added (for any new behavior)
- [x] Passed linting & tests (each commit)
<!-- Code must pass CI (GitHub Actions) before merging - look for the green tick! -->

**Notes & Questions** <!-- if any; add/delete/fill-in as appropriate -->

Everything appears to work fine; as per commit description, we should fix this more cleanly with the keypress refactoring. See #**zulip-terminal > Keypress refactoring**

**Interactions** <!-- if any; add/delete/fill-in as appropriate -->

This resolves an issue triggered by #1194, but which was present previously and only triggered by this situation.


Patch:
diff --git a/zulipterminal/core.py b/zulipterminal/core.py
index bde77a4..1a049ba 100644
--- a/zulipterminal/core.py
+++ b/zulipterminal/core.py
@@ -8,7 +8,7 @@ from collections import OrderedDict
 from functools import partial
 from platform import platform
 from types import TracebackType
-from typing import Any, Dict, List, Optional, Tuple, Type
+from typing import Any, Dict, List, Optional, Tuple, Type, Union
 
 import pyperclip
 import urwid
@@ -400,8 +400,10 @@ class Controller:
             and os.environ.get("TERM")
         ):
             self.report_error(
-                "No DISPLAY environment variable specified. This could "
-                "likely mean the ZT host is running without a GUI."
+                [
+                    "No DISPLAY environment variable specified. This could "
+                    "likely mean the ZT host is running without a GUI."
+                ]
             )
             return
         try:
@@ -412,11 +414,13 @@ class Controller:
             with suppress_output():
                 browser_controller.open(url)
                 self.report_success(
-                    f"The link was successfully opened using {browser_controller.name}"
+                    [
+                        f"The link was successfully opened using {browser_controller.name}"
+                    ]
                 )
         except webbrowser.Error as e:
             # Set a footer text if no runnable browser is located
-            self.report_error(f"ERROR: {e}")
+            self.report_error([f"ERROR: {e}"])
 
     @asynch
     def show_typing_notification(self) -> None:
@@ -437,19 +441,31 @@ class Controller:
         self.is_typing_notification_in_progress = False
         self.view.set_footer_text()
 
-    def report_error(self, text: str, duration: int = 3) -> None:
+    def report_error(
+        self,
+        text: List[Union[str, Tuple[Literal["footer_contrast"], str]]],
+        duration: int = 3,
+    ) -> None:
         """
         Helper to show an error message in footer
         """
         self.view.set_footer_text(text, "task:error", duration)
 
-    def report_success(self, text: str, duration: int = 3) -> None:
+    def report_success(
+        self,
+        text: List[Union[str, Tuple[Literal["footer_contrast"], str]]],
+        duration: int = 3,
+    ) -> None:
         """
         Helper to show a success message in footer
         """
         self.view.set_footer_text(text, "task:success", duration)
 
-    def report_warning(self, text: str, duration: int = 3) -> None:
+    def report_warning(
+        self,
+        text: List[Union[str, Tuple[Literal["footer_contrast"], str]]],
+        duration: int = 3,
+    ) -> None:
         """
         Helper to show a warning message in footer
         """
@@ -494,10 +510,10 @@ class Controller:
             pyperclip.copy(text)
             clipboard_text = pyperclip.paste()
             if clipboard_text == text:
-                self.report_success(f"{text_category} copied successfully")
+                self.report_success([f"{text_category} copied successfully"])
             else:
                 self.report_warning(
-                    f"{text_category} copied, but the clipboard text does not match"
+                    [f"{text_category} copied, but the clipboard text does not match"]
                 )
         except pyperclip.PyperclipException:
             body = [
@@ -542,8 +558,6 @@ class Controller:
         else:
             self.view.message_view.log.extend(w_list)
 
-        self.exit_editor_mode()
-
     def narrow_to_stream(
         self, *, stream_name: str, contextual_message_id: Optional[int] = None
     ) -> None:
diff --git a/zulipterminal/helper.py b/zulipterminal/helper.py
index de980f2..8e18695 100644
--- a/zulipterminal/helper.py
+++ b/zulipterminal/helper.py
@@ -640,7 +640,7 @@ def canonicalize_color(color: str) -> str:
 
 def display_error_if_present(response: Dict[str, Any], controller: Any) -> None:
     if response["result"] == "error" and hasattr(controller, "view"):
-        controller.report_error(response["msg"])
+        controller.report_error([response["msg"]])
 
 
 def check_narrow_and_notify(
@@ -656,7 +656,9 @@ def check_narrow_and_notify(
         key = primary_key_for_command("NARROW_MESSAGE_RECIPIENT")
 
         controller.report_success(
-            f"Message is sent outside of current narrow. Press [{key}] to narrow to conversation.",
+            [
+                f"Message is sent outside of current narrow. Press [{key}] to narrow to conversation."
+            ],
             duration=6,
         )
 
diff --git a/zulipterminal/model.py b/zulipterminal/model.py
index ab5db50..d9452ac 100644
--- a/zulipterminal/model.py
+++ b/zulipterminal/model.py
@@ -36,6 +36,7 @@ from zulipterminal.api_types import (
     Subscription,
 )
 from zulipterminal.config.keys import primary_key_for_command
+from zulipterminal.config.symbols import STREAM_TOPIC_SEPARATOR
 from zulipterminal.config.ui_mappings import ROLE_BY_ID, StreamAccessType
 from zulipterminal.helper import (
     Message,
@@ -481,7 +482,7 @@ class Model:
 
     def save_draft(self, draft: Composition) -> None:
         self._draft = deepcopy(draft)
-        self.controller.report_success("Saved message as draft")
+        self.controller.report_success(["Saved message as draft"])
 
     @asynch
     def toggle_message_star_status(self, message: Message) -> None:
@@ -580,7 +581,12 @@ class Model:
             stream_name = message.get("display_recipient", None)
             old_topic = message.get("subject", None)
             new_topic = request["topic"]
-
+            stream_name_markup = (
+                "footer_contrast",
+                f" {stream_name} {STREAM_TOPIC_SEPARATOR} ",
+            )
+            old_topic_markup = ("footer_contrast", f" {old_topic} ")
+            new_topic_markup = ("footer_contrast", f" {new_topic} ")
             if old_topic != new_topic:
                 if propagate_mode == "change_one":
                     messages_changed = "one message's"
@@ -589,9 +595,15 @@ class Model:
                 else:  # propagate_mode == "change_later":
                     messages_changed = "some messages'"
                 self.controller.report_success(
-                    f"You changed {messages_changed} topic"
-                    + f" from #{stream_name} > {old_topic}"
-                    + f" to #{stream_name} > {new_topic}.",
+                    [
+                        f"You changed {messages_changed} topic from ",
+                        stream_name_markup,
+                        old_topic_markup,
+                        " to ",
+                        stream_name_markup,
+                        new_topic_markup,
+                        " .",
+                    ],
                     duration=6,
                 )
 
diff --git a/zulipterminal/ui.py b/zulipterminal/ui.py
index 544f9d7..574f65b 100644
--- a/zulipterminal/ui.py
+++ b/zulipterminal/ui.py
@@ -296,7 +296,7 @@ class View(urwid.WidgetWrap):
                 self.middle_column.set_focus("footer")
             else:
                 self.controller.report_error(
-                    "No draft message was saved in this session."
+                    ["No draft message was saved in this session."]
                 )
             return key
         elif is_command_key("ABOUT", key):
diff --git a/zulipterminal/ui_tools/boxes.py b/zulipterminal/ui_tools/boxes.py
index 46a3475..42bcb41 100644
--- a/zulipterminal/ui_tools/boxes.py
+++ b/zulipterminal/ui_tools/boxes.py
@@ -765,7 +765,7 @@ class WriteBox(urwid.Pile):
                         )
                     else:
                         self.view.controller.report_error(
-                            "Cannot send message without specifying recipients."
+                            ["Cannot send message without specifying recipients."]
                         )
                         success = None
             if success:
@@ -851,7 +851,7 @@ class WriteBox(urwid.Pile):
                                     primary_key_for_command("AUTOCOMPLETE_REVERSE"),
                                 )
                             )
-                            self.view.controller.report_error(invalid_stream_error)
+                            self.view.controller.report_error([invalid_stream_error])
                             return key
                         user_ids = self.model.get_other_subscribers_in_stream(
                             stream_name=stream_name
@@ -1865,17 +1865,21 @@ class MessageBox(urwid.Pile):
             ):
                 if self.message["type"] == "stream":
                     self.model.controller.report_error(
-                        " You can't edit messages sent by other users that"
-                        " already have a topic."
+                        [
+                            " You can't edit messages sent by other users that"
+                            " already have a topic."
+                        ]
                     )
                 else:
                     self.model.controller.report_error(
-                        " You can't edit private messages sent by other users."
+                        [" You can't edit private messages sent by other users."]
                     )
                 return key
             # Check if editing is allowed in the realm
             elif not self.model.initial_data["realm_allow_message_editing"]:
-                self.model.controller.report_error(" Editing sent message is disabled.")
+                self.model.controller.report_error(
+                    [" Editing sent message is disabled."]
+                )
                 return key
             # Check if message is still editable, i.e. within
             # the time limit. A limit of 0 signifies no limit
@@ -1891,28 +1895,36 @@ class MessageBox(urwid.Pile):
                     if time_since_msg_sent >= edit_time_limit:
                         if self.message["type"] == "private":
                             self.model.controller.report_error(
-                                " Time Limit for editing the message has been exceeded."
+                                [
+                                    " Time Limit for editing the message has been exceeded."
+                                ]
                             )
                             return key
                         elif self.message["type"] == "stream":
                             self.model.controller.report_warning(
-                                " Only topic editing allowed."
-                                " Time Limit for editing the message body"
-                                " has been exceeded."
+                                [
+                                    " Only topic editing allowed."
+                                    " Time Limit for editing the message body"
+                                    " has been exceeded."
+                                ]
                             )
                             msg_body_edit_enabled = False
                 elif self.message["type"] == "stream":
                     # Allow editing topic if the message has "(no topic)" subject
                     if self.message["subject"] == "(no topic)":
                         self.model.controller.report_warning(
-                            " Only topic editing is allowed."
-                            " This is someone else's message but with (no topic)."
+                            [
+                                " Only topic editing is allowed."
+                                " This is someone else's message but with (no topic)."
+                            ]
                         )
                         msg_body_edit_enabled = False
                     else:
                         self.model.controller.report_error(
-                            " You can't edit messages sent by other users that"
-                            " already have a topic."
+                            [
+                                " You can't edit messages sent by other users that"
+                                " already have a topic."
+                            ]
                         )
                         return key
                 else:
diff --git a/zulipterminal/ui_tools/buttons.py b/zulipterminal/ui_tools/buttons.py
index 9611fd5..3f7d819 100644
--- a/zulipterminal/ui_tools/buttons.py
+++ b/zulipterminal/ui_tools/buttons.py
@@ -607,7 +607,7 @@ class MessageLinkButton(urwid.Button):
         error = self._validate_narrow_link(parsed_link)
 
         if error:
-            self.controller.report_error(f" {error}")
+            self.controller.report_error([f" {error}"])
         else:
             self._switch_narrow_to(parsed_link)
 


Test patch:
diff --git a/tests/core/test_core.py b/tests/core/test_core.py
index d5c68f3..beac0d1 100644
--- a/tests/core/test_core.py
+++ b/tests/core/test_core.py
@@ -370,12 +370,14 @@ class TestController:
         controller.copy_to_clipboard(text_to_copy, text_category)
 
         if expected_result == "success":
-            mock_success.assert_called_once_with(f"{text_category} copied successfully")
+            mock_success.assert_called_once_with(
+                [f"{text_category} copied successfully"]
+            )
             mock_warning.assert_not_called()
         else:
             mock_success.assert_not_called()
             mock_warning.assert_called_once_with(
-                f"{text_category} copied, but the clipboard text does not match"
+                [f"{text_category} copied, but the clipboard text does not match"]
             )
 
     def test_copy_to_clipboard_exception(
@@ -415,7 +417,7 @@ class TestController:
 
         mock_open.assert_called_once_with(url)
         mocked_report_success.assert_called_once_with(
-            f"The link was successfully opened using {mock_get.return_value.name}"
+            [f"The link was successfully opened using {mock_get.return_value.name}"]
         )
 
     def test_open_in_browser_fail__no_browser_controller(
@@ -428,7 +430,7 @@ class TestController:
 
         controller.open_in_browser("https://chat.zulip.org/#narrow/stream/test")
 
-        mocked_report_error.assert_called_once_with(f"ERROR: {error}")
+        mocked_report_error.assert_called_once_with([f"ERROR: {error}"])
 
     def test_main(self, mocker: MockerFixture, controller: Controller) -> None:
         controller.view.palette = {"default": "theme_properties"}
diff --git a/tests/helper/test_helper.py b/tests/helper/test_helper.py
index a9def5b..1a6a24d 100644
--- a/tests/helper/test_helper.py
+++ b/tests/helper/test_helper.py
@@ -328,7 +328,7 @@ def test_display_error_if_present(
     display_error_if_present(response, controller)
 
     if footer_updated:
-        report_error.assert_called_once_with(response["msg"])
+        report_error.assert_called_once_with([response["msg"]])
     else:
         report_error.assert_not_called()
 
@@ -431,7 +431,9 @@ def test_notify_if_message_sent_outside_narrow(
     if footer_updated:
         key = primary_key_for_command("NARROW_MESSAGE_RECIPIENT")
         report_success.assert_called_once_with(
-            f"Message is sent outside of current narrow. Press [{key}] to narrow to conversation.",
+            [
+                f"Message is sent outside of current narrow. Press [{key}] to narrow to conversation."
+            ],
             duration=6,
         )
     else:
diff --git a/tests/model/test_model.py b/tests/model/test_model.py
index dab8f7a..43fe7f1 100644
--- a/tests/model/test_model.py
+++ b/tests/model/test_model.py
@@ -7,6 +7,7 @@ import pytest
 from pytest import param as case
 from zulip import Client, ZulipError
 
+from zulipterminal.config.symbols import STREAM_TOPIC_SEPARATOR
 from zulipterminal.helper import initial_index, powerset
 from zulipterminal.model import (
     MAX_MESSAGE_LENGTH,
@@ -853,7 +854,15 @@ class TestModel:
                     "topic": "Topic change",
                 },
                 "Old topic",
-                "You changed one message's topic from #stream > Old topic to #stream > Topic change.",
+                [
+                    "You changed one message's topic from ",
+                    ("footer_contrast", f" stream {STREAM_TOPIC_SEPARATOR} "),
+                    ("footer_contrast", " Old topic "),
+                    " to ",
+                    ("footer_contrast", f" stream {STREAM_TOPIC_SEPARATOR} "),
+                    ("footer_contrast", " Topic change "),
+                    " .",
+                ],
             ),
             case(
                 {
@@ -882,7 +891,15 @@ class TestModel:
                     "topic": "new_terminal",
                 },
                 "old_terminal",
-                "You changed some messages' topic from #stream > old_terminal to #stream > new_terminal.",
+                [
+                    "You changed some messages' topic from ",
+                    ("footer_contrast", f" stream {STREAM_TOPIC_SEPARATOR} "),
+                    ("footer_contrast", " old_terminal "),
+                    " to ",
+                    ("footer_contrast", f" stream {STREAM_TOPIC_SEPARATOR} "),
+                    ("footer_contrast", " new_terminal "),
+                    " .",
+                ],
             ),
             case(
                 {
@@ -892,7 +909,15 @@ class TestModel:
                     "topic": "grett",
                 },
                 "greet",
-                "You changed one message's topic from #stream > greet to #stream > grett.",
+                [
+                    "You changed one message's topic from ",
+                    ("footer_contrast", f" stream {STREAM_TOPIC_SEPARATOR} "),
+                    ("footer_contrast", " greet "),
+                    " to ",
+                    ("footer_contrast", f" stream {STREAM_TOPIC_SEPARATOR} "),
+                    ("footer_contrast", " grett "),
+                    " .",
+                ],
             ),
             case(
                 {
@@ -902,7 +927,15 @@ class TestModel:
                     "topic": "party",
                 },
                 "lets_party",
-                "You changed all messages' topic from #stream > lets_party to #stream > party.",
+                [
+                    "You changed all messages' topic from ",
+                    ("footer_contrast", f" stream {STREAM_TOPIC_SEPARATOR} "),
+                    ("footer_contrast", " lets_party "),
+                    " to ",
+                    ("footer_contrast", f" stream {STREAM_TOPIC_SEPARATOR} "),
+                    ("footer_contrast", " party "),
+                    " .",
+                ],
             ),
         ],
     )
diff --git a/tests/ui/test_ui.py b/tests/ui/test_ui.py
index 7e50281..80ce5ff 100644
--- a/tests/ui/test_ui.py
+++ b/tests/ui/test_ui.py
@@ -459,7 +459,7 @@ class TestView:
             view.middle_column.set_focus.assert_called_once_with("footer")
         else:
             view.controller.report_error.assert_called_once_with(
-                "No draft message was saved in this session."
+                ["No draft message was saved in this session."]
             )
 
     @pytest.mark.parametrize("key", keys_for_command("SEARCH_PEOPLE"))
diff --git a/tests/ui/test_ui_tools.py b/tests/ui/test_ui_tools.py
index 1c3a7df..81e8c29 100644
--- a/tests/ui/test_ui_tools.py
+++ b/tests/ui/test_ui_tools.py
@@ -2564,9 +2564,11 @@ class TestMessageBox:
             write_box.msg_write_box.set_edit_text.assert_not_called()
         if expect_footer_text[message_type]:
             if expect_editing_to_succeed[message_type]:
-                report_warning.assert_called_once_with(expect_footer_text[message_type])
+                report_warning.assert_called_once_with(
+                    [expect_footer_text[message_type]]
+                )
             else:
-                report_error.assert_called_once_with(expect_footer_text[message_type])
+                report_error.assert_called_once_with([expect_footer_text[message_type]])
 
     @pytest.mark.parametrize(
         "raw_html, expected_content",