Repository: zulip/zulip-terminal

Issue:
Improve reporting upon moving/splitting topics
Currently a message is given to the user when a topic is moved, but we could give more information about this:
- where from?
- where to?
- how many? (all? some?)

For the first version of this, updating the text we output would be sufficient.

As a follow-up, we could consider if there are other ways to indicate eg. split topics.

For comparison, the web app appears to intentionally not sort after splitting topics, to leave both parts in view, whereas ZT does so.

Discussion:
srdeotarse: @neiljp ``report_success`` function is passing the string list to ``set_footer_text`` whose argument type is ``Optional[List[Any]]``. Maybe that's why ``mypy`` is not showing type error.
Also, about changing the type of function ``report_success`` to ``Optional[List[Any]]``, is this what you intend?
Because passing ``stream_name`` and ``topic_name`` as a string doesn't display highlighting.
srdeotarse: @zulipbot add "PR needs review".
neiljp: > report_success function is passing the string list to set_footer_text whose argument type is Optional[List[Any]]
This doesn't explain why it wasn't showing the error previously. As per my stream comment just now, I suspect this is due to lack of typing between major modules, and we have a few options to what this 'should' be.
srdeotarse: @zulipbot add "PR needs review".
neiljp: @srdeotarse The second commit looks fine to merge, but I'm a little concerned the first does not take into account all the call sites?
zulipbot: Hello @srdeotarse, it seems like you have referenced #1172 in your pull request description, but you have not referenced them in your commit message description(s). When you reference an issue in a commit message, it automatically closes the corresponding issue when the commit is merged.

Please run `git commit --amend` in your command line client to amend your commit message description with `Fixes #1172.`.

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.
```

Thank you for your contributions to Zulip!

<!-- fixCommitWarning -->

srdeotarse: @zulipbot add "PR needs review".
srdeotarse: @zulipbot add "PR needs review".
zulipbot: **ERROR:** Label "PR needs review" already exists and was thus not added to this pull request.

zulipbot: Hello @srdeotarse, it seems like you have referenced #1172 in your pull request description, but you have not referenced them in your commit message description(s). When you reference an issue in a commit message, it automatically closes the corresponding issue when the commit is merged.

Please run `git commit --amend` in your command line client to amend your commit message description with `Fixes #1172.`.

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.
```

Thank you for your contributions to Zulip!

<!-- fixCommitWarning -->


Patch:
diff --git a/zulipterminal/core.py b/zulipterminal/core.py
index bde77a4..7c44f95 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 = [
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",