Repository: zulip/zulip-terminal
Pull request #1386 (2023-04-24 13:20:49)

Adding prefix icons to current message headers
<!-- See README for documentation, or ask in #zulip-terminal if unclear -->
### What does this PR do, and why?
Adds prefix icons to currents message headers


### Outstanding aspect(s)    <!-- DELETE SECTION IF EMPTY -->
<!-- In what ways is this not fully implemented/functioning? Compared to a discussion/issue? -->
There are other parts of the ui that also needs the prefix icons but the person who opened the issue said that each section could be done with multiple pull request so that is how I chose to do it.
<!-- 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 -->
- [ ] Discussed in **#zulip-terminal** in `topic`
- [ ] Fully fixes #
- [x] Partially fixes issue #1348 
- [ ] 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
- [x] Manually - Visual changes
- [ ] Adapting existing automated tests
- [ ] 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)
- [ ] It has a commit summary following the [documented style](https://github.com/zulip/zulip-terminal#structuring-commits-
--speeding-up-reviews-merging--development) (title & body)
- [ ] It has a commit summary describing the  motivation and reasoning for the change
- [ ] It individually passes linting and tests
- [ ] It contains test additions for any new behavior
- [ ] 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
[![yourtitle](https://asciinema.org/a/<id>.png)](https://asciinema.org/a/<id>)
-->
<img width="321" alt="Screenshot 2023-04-24 at 16 18 38" src="https://user-images.githubusercontent.com/77788985/234008283-aafe4a28-6751-48ac-88cd-1e0b0253f742.png">

<img width="321" alt="Screenshot 2023-04-24 at 16 18 38" src="https://user-images.githubusercontent.com/77788985/234008463-02b53520-c849-4ccb-9aad-5c88e4198c84.png">

<img width="1020" alt="Screenshot 2023-04-24 at 16 20 30" src="https://user-images.githubusercontent.com/77788985/234008657-bf0d4c51-54a9-437c-bba7-27b2ed0fbd22.png">


neiljp: @supascooopa Thanks for working on this, it looks like a good start on the feature! :+1:

I suspect it would be clearer visually if we style the icon as part of the name, so that it looks like part of the 'bar' and doesn't look too similar to the user presence icons - particular with web-public streams.

Keeping the commits separate is fine for now, though the changes may well be small enough to squash together.

However, we will need to fix the test failure - are you planning to look at that next?

Please do drop into the stream on chat.zulip.org and ask if you have questions :)
supascooopa: I was not sure where to put the icon and was torn between it being inside the rectangle/bar or out. So I could do that switch no problem. 

I'll keep in mind the size of the commits for next time.

I'd like to resolve the test failure but I'm not sure what's causing it and I'd appreciate some help resolving it. 
neiljp: @supascooopa You may want to wait until you've made the changes before looking at the test, in case the fix is different.

Debugging the test failure may be easier if you're doing it locally? Did you use `make check`, `pytest` or similar? It's difficult to know what you're wanting help with without more context.
supascooopa: Before I committed any changes I wanted to make sure that the following is what you @neiljp  meant, please have a look:
<img width="102" alt="Screenshot 2023-04-25 at 11 20 20" src="https://user-images.githubusercontent.com/77788985/234217923-78ee5653-7171-4b5b-b4b4-52315d387bf8.png">

I've very little experience with pytest, and don't understand why the test are failing. I've ran the pytest after I've added the icon into the rectangle but the problem is persisting.
supascooopa: Hello, I have found a way to pass the tests. The problem was a invalid key being passed in STREAM_ACCESS_TYPE[stream_access_type]["icon"] by MagicMock object. I've looked through  TestMessageBox class to fix the failed tests but wasn't able to make much sense of it. Instead I was able to clear this issue by writing a try except block in stream_header method where the key error was coming from. Which cleared test error but others popped up. but I was quickly able to identify which test were failing and made adjustment to them and now the test all run without a hitch. 
neiljp: > Before I committed any changes I wanted to make sure that the following is what you @neiljp meant, please have a look: <img alt="Screenshot 2023-04-25 at 11 20 20" width="102" src="https://user-images.githubusercontent.com/77788985/234217923-78ee5653-7171-4b5b-b4b4-52315d387bf8.png">

This looks better :+1: We could also consider an additional space before this, what do you think?

supascooopa: Apologize for the late reply, I had some issues launching ZT but I managed to solve it. Yes I think adding a space before the icon looks better. Before it was pressing up against the left most border of the rectangle which gave it a claustrophobic feeling. please have a look below and let me know what you think:
Before:
<img width="120" alt="Screenshot 2023-04-28 at 18 23 49" src="https://user-images.githubusercontent.com/77788985/235188669-b6cb421c-24a6-494a-a2ae-d2f5843863e2.png">


After:
<img width="131" alt="Screenshot 2023-04-28 at 18 17 03" src="https://user-images.githubusercontent.com/77788985/235188355-a0994fb6-dbbb-4ff8-8ca3-1c668259f1b5.png">

supascooopa: I'm not sure why the linting test is failing now, any ideas @neiljp?
neiljp: @supascooopa The failing part is 'Ensure isolated PR commits', which as per the name, implies that each commit should individually pass linting/tests. So if you have added a commit to fix a failing earlier commit, that earlier commit will still fail this part of the automated testing. The typical solution is to combine (squash) the commits that 'fix' a previous commit with that earlier commit, using an interactive rebase. You can then force push to the PR with the revised branch.
supascooopa: @neiljp  Thank you for the suggestion. I'm working on it locally and it looks like I'll be left with 2 commits. Because the cause of the error is pretty early on the commit history. I'd have to squash 457761b to 95c6c3f and then 9776416 to 64b0018. If that's okay with you I'll do it this way. If you have any other idea's please let me know.
supascooopa: I'm currently looking through the git log and reading up on the commit message styles. I'll come up with a coherent commit message that adheres to those styles and squash all the commits soon.
supascooopa: My commit message would be something like the following, if @neiljp you think it's good I'll go ahead with it:

messages: Adding stream markers to message headers

This commit aims to improve the current UI, by adding prefix stream markers to message headers in messages view. This is achieved by the following changes made in stream_header method:
- Calling the stream access type from the model using the stream_id.
- Using that access type to call the kind of icon from the STREAM_ACCESS_TYPE dictionary. 
- Finally adding the icon to stream_title_markup.

tests and fixtures are updated accordingly.
neiljp: @supascooopa It's fine to go with something like that - note that you can update commit messages in your branch at any time using the `--amend` I noted above, or specific commits by changing the 'pick' in an interactive rebase to `r` (or `reword`). The necessary force-push to this feature branch (not `main`) after doing the squash would be just the same as if you adjusted the commit text repeatedly and pushed each time :)

In terms of the text specifically, you might try installing and setting up gitlint in your development environment (see the README) for some automated feedback, particularly if you intend to continue contributing.

Typically the body of a commit message does not need to repeat the logic of the code in text form, but specific notes on the reasoning for the change in behavior, or why a certain implementation was chosen.

Note that someone merging code may adjust the text/code slightly before merging in any case :)
supascooopa: @neiljp Can I help to resolve the conflicts? Would you like for me to squash the last two commits to 594808507a9d45a8d2e128e68046960aac6bfcc4.  Any feed back is welcome! :)
neiljp: @supascooopa Are you asking for help resolving the conflicts?

The last two commits should be squashed together, since the second is a refinement of the first.

I'm not suggesting you do the following, it is an example. If you had made the addition to symbols.py in one commit, and then used it in the next commit, this wouldn't have been a refinement in the same way. The next would have depended on the earlier commit, but was a separate change. The documentation tries to describe this in the 'Structuring commits' section.
supascooopa: @neiljp Yes I'd like some help resolving the conflicts. Should I rebase my branch to the latest commit on the main branch and resolve the conflicts that way?
 I will squash the last two commits together.
 Thank you for the suggestion, I try to follow the rules/documentation but keep forgetting some while following others...

Patch:
diff --git a/zulipterminal/config/symbols.py b/zulipterminal/config/symbols.py
index 4782f4c..153e76c 100644
--- a/zulipterminal/config/symbols.py
+++ b/zulipterminal/config/symbols.py
@@ -8,6 +8,7 @@ Terminal characters used to mark particular elements of the user interface
 # Suffix comments indicate: unicode name, codepoint (unicode block, version if not v1.1)
 
 INVALID_MARKER = "✗"  # BALLOT X, U+2717 (Dingbats)
+DIRECT_MESSAGE_MARKER = "§"  # SECTION SIGN, U+00A7 (Latin-1 supplement)
 STREAM_MARKER_PRIVATE = "P"
 STREAM_MARKER_PUBLIC = "#"
 STREAM_MARKER_WEB_PUBLIC = "⊚"  # CIRCLED RING OPERATOR, U+229A (Mathematical operators)
diff --git a/zulipterminal/ui_tools/messages.py b/zulipterminal/ui_tools/messages.py
index e73143a..47600b4 100644
--- a/zulipterminal/ui_tools/messages.py
+++ b/zulipterminal/ui_tools/messages.py
@@ -18,13 +18,14 @@ from tzlocal import get_localzone
 from zulipterminal.api_types import Message
 from zulipterminal.config.keys import is_command_key, primary_key_for_command
 from zulipterminal.config.symbols import (
+    DIRECT_MESSAGE_MARKER,
     MESSAGE_CONTENT_MARKER,
     MESSAGE_HEADER_DIVIDER,
     QUOTED_TEXT_MARKER,
     STREAM_TOPIC_SEPARATOR,
     TIME_MENTION_MARKER,
 )
-from zulipterminal.config.ui_mappings import STATE_ICON
+from zulipterminal.config.ui_mappings import STATE_ICON, STREAM_ACCESS_TYPE
 from zulipterminal.helper import get_unused_fence
 from zulipterminal.server_url import near_message_url
 from zulipterminal.ui_tools.tables import render_table
@@ -153,9 +154,12 @@ class MessageBox(urwid.Pile):
         assert self.stream_id is not None
         color = self.model.stream_dict[self.stream_id]["color"]
         bar_color = f"s{color}"
+        stream_access_type = self.model.stream_access_type(self.stream_id)
+        stream_icon = STREAM_ACCESS_TYPE[stream_access_type]["icon"]
         stream_title_markup = (
             "bar",
             [
+                (bar_color, f" {stream_icon} "),
                 (bar_color, f"{self.stream_name} {STREAM_TOPIC_SEPARATOR} "),
                 ("title", f" {self.topic_name}"),
             ],
@@ -174,7 +178,11 @@ class MessageBox(urwid.Pile):
     def private_header(self) -> Any:
         title_markup = (
             "header",
-            [("general_narrow", "You and "), ("general_narrow", self.recipients_names)],
+            [
+                ("general_narrow", f" {DIRECT_MESSAGE_MARKER} "),
+                ("general_narrow", "You and "),
+                ("general_narrow", self.recipients_names),
+            ],
         )
         title = urwid.Text(title_markup)
         header = urwid.Columns(


Test patch:
diff --git a/tests/conftest.py b/tests/conftest.py
index b42e955..fe771d0 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -119,9 +119,11 @@ def msg_box(
     """
     Mocked MessageBox with stream message
     """
+    model_mock = mocker.patch("zulipterminal.model.Model")
+    model_mock.stream_access_type.return_value = "public"
     return MessageBox(
         messages_successful_response["messages"][0],
-        mocker.patch("zulipterminal.model.Model"),
+        model_mock,
         None,
     )
 
diff --git a/tests/ui_tools/test_messages.py b/tests/ui_tools/test_messages.py
index db22da3..c87cc07 100644
--- a/tests/ui_tools/test_messages.py
+++ b/tests/ui_tools/test_messages.py
@@ -9,8 +9,10 @@ from urwid import Columns, Divider, Padding, Text
 
 from zulipterminal.config.keys import keys_for_command
 from zulipterminal.config.symbols import (
+    DIRECT_MESSAGE_MARKER,
     QUOTED_TEXT_MARKER,
     STATUS_INACTIVE,
+    STREAM_MARKER_PUBLIC,
     STREAM_TOPIC_SEPARATOR,
     TIME_MENTION_MARKER,
 )
@@ -28,6 +30,7 @@ class TestMessageBox:
     def mock_external_classes(self, mocker, initial_index):
         self.model = mocker.MagicMock()
         self.model.index = initial_index
+        self.model.stream_access_type.return_value = "public"
 
     @pytest.mark.parametrize(
         "message_type, set_fields",
@@ -893,54 +896,104 @@ class TestMessageBox:
     @pytest.mark.parametrize(
         "msg_narrow, msg_type, assert_header_bar, assert_search_bar",
         [
-            ([], 0, f"PTEST {STREAM_TOPIC_SEPARATOR} ", "All messages"),
-            ([], 1, "You and ", "All messages"),
-            ([], 2, "You and ", "All messages"),
+            (
+                [],
+                0,
+                f" {STREAM_MARKER_PUBLIC} PTEST {STREAM_TOPIC_SEPARATOR} ",
+                "All messages",
+            ),
+            ([], 1, f" {DIRECT_MESSAGE_MARKER} You and ", "All messages"),
+            ([], 2, f" {DIRECT_MESSAGE_MARKER} You and ", "All messages"),
             (
                 [["stream", "PTEST"]],
                 0,
-                f"PTEST {STREAM_TOPIC_SEPARATOR} ",
+                f" {STREAM_MARKER_PUBLIC} PTEST {STREAM_TOPIC_SEPARATOR} ",
                 ("bar", [("s#bd6", "PTEST")]),
             ),
             (
                 [["stream", "PTEST"], ["topic", "b"]],
                 0,
-                f"PTEST {STREAM_TOPIC_SEPARATOR}",
+                f" {STREAM_MARKER_PUBLIC} PTEST {STREAM_TOPIC_SEPARATOR}",
                 ("bar", [("s#bd6", "PTEST"), ("s#bd6", ": topic narrow")]),
             ),
-            ([["is", "private"]], 1, "You and ", "All direct messages"),
-            ([["is", "private"]], 2, "You and ", "All direct messages"),
+            (
+                [["is", "private"]],
+                1,
+                f" {DIRECT_MESSAGE_MARKER} You and ",
+                "All direct messages",
+            ),
+            (
+                [["is", "private"]],
+                2,
+                f" {DIRECT_MESSAGE_MARKER} You and ",
+                "All direct messages",
+            ),
             (
                 [["pm-with", "boo@zulip.com"]],
                 1,
-                "You and ",
+                f" {DIRECT_MESSAGE_MARKER} You and ",
                 "Direct message conversation",
             ),
             (
                 [["pm-with", "boo@zulip.com, bar@zulip.com"]],
                 2,
-                "You and ",
+                f" {DIRECT_MESSAGE_MARKER} You and ",
                 "Group direct message conversation",
             ),
             (
                 [["is", "starred"]],
                 0,
-                f"PTEST {STREAM_TOPIC_SEPARATOR} ",
+                f" {STREAM_MARKER_PUBLIC} PTEST {STREAM_TOPIC_SEPARATOR} ",
+                "Starred messages",
+            ),
+            (
+                [["is", "starred"]],
+                1,
+                f" {DIRECT_MESSAGE_MARKER} You and ",
+                "Starred messages",
+            ),
+            (
+                [["is", "starred"]],
+                2,
+                f" {DIRECT_MESSAGE_MARKER} You and ",
+                "Starred messages",
+            ),
+            (
+                [["is", "starred"], ["search", "FOO"]],
+                1,
+                f" {DIRECT_MESSAGE_MARKER} You and ",
                 "Starred messages",
             ),
-            ([["is", "starred"]], 1, "You and ", "Starred messages"),
-            ([["is", "starred"]], 2, "You and ", "Starred messages"),
-            ([["is", "starred"], ["search", "FOO"]], 1, "You and ", "Starred messages"),
             (
                 [["search", "FOO"]],
                 0,
-                f"PTEST {STREAM_TOPIC_SEPARATOR} ",
+                f" {STREAM_MARKER_PUBLIC} PTEST {STREAM_TOPIC_SEPARATOR} ",
                 "All messages",
             ),
-            ([["is", "mentioned"]], 0, f"PTEST {STREAM_TOPIC_SEPARATOR} ", "Mentions"),
-            ([["is", "mentioned"]], 1, "You and ", "Mentions"),
-            ([["is", "mentioned"]], 2, "You and ", "Mentions"),
-            ([["is", "mentioned"], ["search", "FOO"]], 1, "You and ", "Mentions"),
+            (
+                [["is", "mentioned"]],
+                0,
+                f" {STREAM_MARKER_PUBLIC} PTEST {STREAM_TOPIC_SEPARATOR} ",
+                "Mentions",
+            ),
+            (
+                [["is", "mentioned"]],
+                1,
+                f" {DIRECT_MESSAGE_MARKER} You and ",
+                "Mentions",
+            ),
+            (
+                [["is", "mentioned"]],
+                2,
+                f" {DIRECT_MESSAGE_MARKER} You and ",
+                "Mentions",
+            ),
+            (
+                [["is", "mentioned"], ["search", "FOO"]],
+                1,
+                f" {DIRECT_MESSAGE_MARKER} You and ",
+                "Mentions",
+            ),
         ],
     )
     def test_msg_generates_search_and_header_bar(