Repository: zulip/zulip-terminal
Pull request #1160 (2022-03-05 02:07:37)

Support upper-case theme color spec & Clarify 16-color names
<!-- Please see https://github.com/zulip/zulip-terminal#contributor-guidelines ! -->

**What does this PR do?**  <!-- Overall description goes here -->

The documentation for urwid generally refers to lower case as the valid
format, but it lowers the barrier to entry for theme authors if we allow
upper case and explicitly convert ourselves.

For 16 color names, our color specification uses underscores to enable styling within a
single string (more generally) and make it simpler to separate color variations (eg.
"light gray") within a longer space-separated string.

**Tested?** <!-- Fine to leave some of these unchecked if this is a draft/work-in-progress -->
- [x] Manually
- [x] Existing tests (adapted, if necessary)

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/README.md b/README.md
index daf2217..2fff3c3 100644
--- a/README.md
+++ b/README.md
@@ -49,6 +49,23 @@ our open [Issues](https://github.com/zulip/zulip-terminal/issues/), or sign up o
 
 The minimum server version that Zulip Terminal supports is [`2.1.0`](https://zulip.readthedocs.io/en/latest/overview/changelog.html#id7). It may still work with earlier versions.
 
+### Supported Python Versions
+
+Version 0.6.0 was the last release with support for Python 3.5.
+
+Later releases and the main development branch are currently tested (on Ubuntu) with:
+- CPython 3.6-3.10
+- PyPy 3.6-3.9
+
+Since our automated testing does not cover interactive testing of the UI, there
+may be issues with some Python versions, though generally we have not found
+this to be the case.
+
+Please note that generally we limit each release to between a lower and upper
+Python version, so it is possible that for example if you have a newer version
+of Python installed, then some releases (or `main`) may not install correctly.
+In some cases this can give rise to the symptoms in issue #1145.
+
 ## Installation
 
 We recommend installing in a dedicated python virtual environment (see below) or using an automated option such as [pipx](https://pypi.python.org/pypi/pipx)
diff --git a/zulipterminal/api_types.py b/zulipterminal/api_types.py
index a47d5ed..55237c4 100644
--- a/zulipterminal/api_types.py
+++ b/zulipterminal/api_types.py
@@ -151,7 +151,9 @@ class UpdateMessageEvent(TypedDict):
     rendered_content: str
     # B: Subject of these message ids needs updating?
     message_ids: List[int]
+    orig_subject: str
     subject: str
+    propagate_mode: EditPropagateMode
     stream_id: int
 
 
diff --git a/zulipterminal/config/themes.py b/zulipterminal/config/themes.py
index 4a8c1e2..c6cfdab 100644
--- a/zulipterminal/config/themes.py
+++ b/zulipterminal/config/themes.py
@@ -156,13 +156,13 @@ def parse_themefile(
             new_style = (style_name, fg, bg)
 
         elif color_depth == 256:
-            fg = " ".join([fg_code256] + fg_props)
-            bg = " ".join([bg_code256] + bg_props)
+            fg = " ".join([fg_code256] + fg_props).lower()
+            bg = " ".join([bg_code256] + bg_props).lower()
             new_style = (style_name, "", "", "", fg, bg)
 
         elif color_depth == 2**24:
-            fg = " ".join([fg_code24] + fg_props)
-            bg = " ".join([bg_code24] + bg_props)
+            fg = " ".join([fg_code24] + fg_props).lower()
+            bg = " ".join([bg_code24] + bg_props).lower()
             new_style = (style_name, "", "", "", fg, bg)
 
         urwid_theme.append(new_style)
diff --git a/zulipterminal/model.py b/zulipterminal/model.py
index 142a41a..a53d2a6 100644
--- a/zulipterminal/model.py
+++ b/zulipterminal/model.py
@@ -1384,9 +1384,26 @@ class Model:
         if "subject" in event:
             new_subject = event["subject"]
             stream_id = event["stream_id"]
+            old_subject = event["orig_subject"]
+            msg_ids_by_topic = self.index["topic_msg_ids"][stream_id]
 
-            # Update any indexed messages & re-render them
+            # Remove each message_id from the old topic's `topic_msg_ids` set
+            # if it exists, and update & re-render the message if it is indexed.
             for msg_id in event["message_ids"]:
+                # Ensure that the new topic is not the same as the old one
+                # (no-op topic edit).
+                if new_subject != old_subject:
+                    # Remove the msg_id from the relevant `topic_msg_ids` set,
+                    # if that topic's set has already been initiated.
+                    if old_subject in msg_ids_by_topic:
+                        msg_ids_by_topic[old_subject].discard(msg_id)
+
+                    # Add the msg_id to the new topic's set, if the set has
+                    # already been initiated.
+                    if new_subject in msg_ids_by_topic:
+                        msg_ids_by_topic[new_subject].add(msg_id)
+
+                # Update and re-render indexed messages.
                 indexed_msg = self.index["messages"].get(msg_id)
                 if indexed_msg:
                     indexed_msg["subject"] = new_subject
diff --git a/zulipterminal/themes/THEME_CONTRIBUTING.md b/zulipterminal/themes/THEME_CONTRIBUTING.md
index 563ce5f..00fbc3e 100644
--- a/zulipterminal/themes/THEME_CONTRIBUTING.md
+++ b/zulipterminal/themes/THEME_CONTRIBUTING.md
@@ -45,8 +45,8 @@ color codes:
 
 * **16code**: Terminal's default 16 color aliases.
   * Only those used in DefaultColor are acceptable.
-  * Acceptable names: `default`, `black`, `dark red`, `dark green`, `brown`, `dark blue`, `dark magenta`, `dark cyan`, `dark gray`, `light red`, `light green`, `yellow`, `light blue`, `light magenta`, `light cyan`, `light gray`, `white`
-  * Reference link to urwid documentation: https://urwid.org/manual/displayattributes.html#standard-foreground 
+  * Acceptable names: `default`, `black`, `dark_red`, `dark_green`, `brown`, `dark_blue`, `dark_magenta`, `dark_cyan`, `dark_gray`, `light_red`, `light_green`, `yellow`, `light_blue`, `light_magenta`, `light_cyan`, `light_gray`, `white`
+  * Reference link to urwid documentation: https://urwid.org/manual/displayattributes.html#standard-foreground (**note our use of underscores**)
 * **256code**: High color, 256 color code.
   * Acceptable formats: `#000-#fff`, `h0-h255`, `g0-g100`, `g#00-g#ff`
   * Eg: `#rgb or h255 or g19`
@@ -54,6 +54,9 @@ color codes:
   * Similar to HTML colors.
   * Eg: `#rrggbb`
 
+> Note that `urwid` documentation suggests colors should be lower case, but we
+> explicitly support upper case in color specifications, eg. #FF5
+
 `'default'` is a special alias which uses the default
 foreground or background.
 
@@ -62,20 +65,20 @@ The following table consists of the 16code equivalents of 256 color codes-
 | 256 color codes                                                                                                                                                                                                                                                                      | Corresponding 16 color codes | 
 | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | ---------------------------- | 
 | `h0`, `h16`, `h232`, `h233`, `h234`, `h235`, `h236`, `h237`                                                                                                                                                                                                                          | `black`                      | 
-| `h1`, `h52`, `h88`, `h124`                                                                                                                                                                                                                                                           | `dark red`                   | 
-| `h2`, `h22`, `h28`, `h34`, `h72`                                                                                                                                                                                                                                                     | `dark green`                 | 
+| `h1`, `h52`, `h88`, `h124`                                                                                                                                                                                                                                                           | `dark_red`                   | 
+| `h2`, `h22`, `h28`, `h34`, `h72`                                                                                                                                                                                                                                                     | `dark_green`                 | 
 | `h3`, `h58`, `h64`, `h70`, `h94`, `h100`, `h106`, `h130`, `h136`, `h142`                                                                                                                                                                                                             | `brown`                      | 
-| `h4`, `h17`, `h18`, `h19`, `h60`, `h61`, `h67`, `h97`                                                                                                                                                                                                                                | `dark blue`                  | 
-| `h5`, `h53`, `h54`, `h55`, `h89`, `h90`, `h91`, `h96`, `h125`, `h126`, `h127`, `h132`, `h133`, `h139`                                                                                                                                                                                | `dark magenta`               | 
-| `h6`, `h23`, `h24`, `h25`, `h29`, `h30`, `h31`, `h35`, `h36`, `h37`, `h73`                                                                                                                                                                                                           | `dark cyan`                  | 
-| `h7`, `h71`, `h103`, `h107`, `h108`, `h109`, `h114`, `h115`, `h116`, `h131`, `h137`, `h138`, `h143`, `h144`, `h145`, `h146`, `h150`, `h151`, `h152`, `h174`, `h180`, `h181`, `h186`, `h187`, `h188`, `h248`, `h249`, `h250`, `h251`, `h252`, `h253`                                  | `light gray`                 | 
-| `h8`, `h59`, `h65`, `h66`, `h95`, `h101`, `h102`, `h238`, `h239`, `h240`, `h241`, `h242`, `h243`, `h244`, `h245`, `h246`, `h247`                                                                                                                                                     | `dark gray`                  | 
-| `h9`, `h160`, `h166`, `h167`, `h173`, `h196`, `h202`, `h203`, `h209`, `h215`                                                                                                                                                                                                         | `light red`                  | 
-| `h10`, `h40`, `h41`, `h46`, `h47`, `h76`, `h77`, `h78`, `h82`, `h83`, `h84`, `h85`, `h113`, `h119`, `h120`, `h121`, `h155`, `h156`                                                                                                                                                   | `light green`                | 
+| `h4`, `h17`, `h18`, `h19`, `h60`, `h61`, `h67`, `h97`                                                                                                                                                                                                                                | `dark_blue`                  | 
+| `h5`, `h53`, `h54`, `h55`, `h89`, `h90`, `h91`, `h96`, `h125`, `h126`, `h127`, `h132`, `h133`, `h139`                                                                                                                                                                                | `dark_magenta`               | 
+| `h6`, `h23`, `h24`, `h25`, `h29`, `h30`, `h31`, `h35`, `h36`, `h37`, `h73`                                                                                                                                                                                                           | `dark_cyan`                  | 
+| `h7`, `h71`, `h103`, `h107`, `h108`, `h109`, `h114`, `h115`, `h116`, `h131`, `h137`, `h138`, `h143`, `h144`, `h145`, `h146`, `h150`, `h151`, `h152`, `h174`, `h180`, `h181`, `h186`, `h187`, `h188`, `h248`, `h249`, `h250`, `h251`, `h252`, `h253`                                  | `light_gray`                 | 
+| `h8`, `h59`, `h65`, `h66`, `h95`, `h101`, `h102`, `h238`, `h239`, `h240`, `h241`, `h242`, `h243`, `h244`, `h245`, `h246`, `h247`                                                                                                                                                     | `dark_gray`                  | 
+| `h9`, `h160`, `h166`, `h167`, `h173`, `h196`, `h202`, `h203`, `h209`, `h215`                                                                                                                                                                                                         | `light_red`                  | 
+| `h10`, `h40`, `h41`, `h46`, `h47`, `h76`, `h77`, `h78`, `h82`, `h83`, `h84`, `h85`, `h113`, `h119`, `h120`, `h121`, `h155`, `h156`                                                                                                                                                   | `light_green`                | 
 | `h11`, `h112`, `h118`, `h148`, `h149`, `h154`, `h172`, `h178`, `h179`, `h184`, `h185`, `h190`, `h191`, `h208`, `h214`, `h220`, `h221`, `h226`, `h227`                                                                                                                                | `yellow`                     | 
-| `h12`, `h20`, `h21`, `h26`, `h27`, `h56`, `h57`, `h62`, `h63`, `h68`, `h69`, `h75`, `h98`, `h99`, `h104`, `h105`, `h110`, `h111`, `h135`, `h140`, `h141`, `h147`                                                                                                                     | `light blue`                 | 
-| `h13`, `h92`, `h93`, `h128`, `h134`, `h129`, `h161`, `h162`, `h163`, `h164`, `h165`, `h168`, `h169`, `h170`, `h171`, `h175`, `h176`, `h177`, `h182`, `h183`, `h197`, `h198`, `h199`, `h200`, `h201`, `h204`, `h205`, `h206`, `h207`, `h211`, `h212`, `h213`, `h218`, `h219`, `h225`  | `light magenta`              | 
-| `h14`, `h32`, `h33`, `h38`, `h39`, `h42`, `h43`, `h44`, `h45`, `h48`, `h49`, `h50`, `h51`, `h74`, `h79`, `h80`, `h81`, `h86`, `h87`, `h117`, `h122`, `h123`                                                                                                                          | `light cyan`                 | 
+| `h12`, `h20`, `h21`, `h26`, `h27`, `h56`, `h57`, `h62`, `h63`, `h68`, `h69`, `h75`, `h98`, `h99`, `h104`, `h105`, `h110`, `h111`, `h135`, `h140`, `h141`, `h147`                                                                                                                     | `light_blue`                 | 
+| `h13`, `h92`, `h93`, `h128`, `h134`, `h129`, `h161`, `h162`, `h163`, `h164`, `h165`, `h168`, `h169`, `h170`, `h171`, `h175`, `h176`, `h177`, `h182`, `h183`, `h197`, `h198`, `h199`, `h200`, `h201`, `h204`, `h205`, `h206`, `h207`, `h211`, `h212`, `h213`, `h218`, `h219`, `h225`  | `light_magenta`              | 
+| `h14`, `h32`, `h33`, `h38`, `h39`, `h42`, `h43`, `h44`, `h45`, `h48`, `h49`, `h50`, `h51`, `h74`, `h79`, `h80`, `h81`, `h86`, `h87`, `h117`, `h122`, `h123`                                                                                                                          | `light_cyan`                 | 
 | `h15`, `h153`, `h157`, `h158`, `h159`, `h189`, `h192`, `h193`, `h194`, `h195`, `h210`, `h216`, `h217`, `h222`, `h223`, `h224`, `h228`, `h229`, `h230`, `h231`, `h254`, `h255`                                                                                                        | `white`                      | 
 
 ### COLOR PROPERTIES


Test patch:
diff --git a/.github/workflows/lint-and-test.yml b/.github/workflows/lint-and-test.yml
index fbcb032..f5063b7 100644
--- a/.github/workflows/lint-and-test.yml
+++ b/.github/workflows/lint-and-test.yml
@@ -70,6 +70,7 @@ jobs:
           - {PYTHON: 'pypy-3.6', OS: ubuntu-latest, NAME: "PyPy 3.6 (ubuntu)"}
           - {PYTHON: 'pypy-3.7', OS: ubuntu-latest, NAME: "PyPy 3.7 (ubuntu)"}
           - {PYTHON: 'pypy-3.8', OS: ubuntu-latest, NAME: "PyPy 3.8 (ubuntu)"}
+          - {PYTHON: 'pypy-3.9', OS: ubuntu-latest, NAME: "PyPy 3.9 (ubuntu)"}
           - {PYTHON: 3.9, OS: macos-latest, NAME: "CPython 3.9 (macos)"}
     runs-on: ${{ matrix.env.OS }}
     name: Install & test - ${{ matrix.env.NAME}}
@@ -83,8 +84,8 @@ jobs:
         run: python --version
       - name: Upgrade pip
         run: python -m pip install --upgrade pip
-      - name: Ensure libxml-related libraries are installed (pypy-3.6 only)
-        if: matrix.env.PYTHON == 'pypy-3.6'
+      - name: Ensure libxml-related libraries are installed (some pypy versions don't have wheels)
+        if: startsWith(matrix.env.PYTHON, 'pypy-')
         run: sudo apt install libxml2-dev libxslt1-dev
       - name: Ensure regular package installs from checkout
         run: pip install .
diff --git a/tests/config/test_themes.py b/tests/config/test_themes.py
index 040f0a4..29edef7 100644
--- a/tests/config/test_themes.py
+++ b/tests/config/test_themes.py
@@ -26,23 +26,25 @@ expected_complete_themes = {
     "zt_light",
     "zt_blue",
 }
-aliases_16_color = [
+
+# These are urwid color names with underscores instead of spaces
+valid_16_color_codes = [
     "default",
     "black",
-    "dark red",
-    "dark green",
+    "dark_red",
+    "dark_green",
     "brown",
-    "dark blue",
-    "dark magenta",
-    "dark cyan",
-    "dark gray",
-    "light red",
-    "light green",
+    "dark_blue",
+    "dark_magenta",
+    "dark_cyan",
+    "dark_gray",
+    "light_red",
+    "light_green",
     "yellow",
-    "light blue",
-    "light magenta",
-    "light cyan",
-    "light gray",
+    "light_blue",
+    "light_magenta",
+    "light_cyan",
+    "light_gray",
     "white",
 ]
 
@@ -78,10 +80,10 @@ def test_builtin_theme_completeness(theme_name: str) -> None:
         if "__" in color.name:
             continue
 
-        codes = color.value.split()
+        codes = color.value.lower().split()
         assert len(codes) == 3
         # Check if 16-color alias is correct
-        assert codes[0].replace("_", " ") in aliases_16_color
+        assert codes[0] in valid_16_color_codes
         # Check if 24-bit and 256 color is any of the valid color codes
         pattern = re.compile(REGEX_COLOR_VALID_FORMATS)
         for code in [codes[1], codes[2]]:
diff --git a/tests/model/test_model.py b/tests/model/test_model.py
index bf0f887..7aab54f 100644
--- a/tests/model/test_model.py
+++ b/tests/model/test_model.py
@@ -1624,6 +1624,7 @@ class TestModel:
             case(
                 {  # Only subject of 1 message is updated.
                     "message_id": 1,
+                    "orig_subject": "old subject",
                     "subject": "new subject",
                     "stream_id": 10,
                     "message_ids": [1],
@@ -1644,6 +1645,9 @@ class TestModel:
                             "subject": "old subject",
                         },
                     },
+                    "topic_msg_ids": {
+                        10: {"new subject": {1}, "old subject": {2}},
+                    },
                     "edited_messages": {1},
                     "topics": {10: []},
                 },
@@ -1653,6 +1657,7 @@ class TestModel:
             case(
                 {  # Subject of 2 messages is updated
                     "message_id": 1,
+                    "orig_subject": "old subject",
                     "subject": "new subject",
                     "stream_id": 10,
                     "message_ids": [1, 2],
@@ -1673,6 +1678,9 @@ class TestModel:
                             "subject": "new subject",
                         },
                     },
+                    "topic_msg_ids": {
+                        10: {"new subject": {1, 2}, "old subject": set()},
+                    },
                     "edited_messages": {1},
                     "topics": {10: []},
                 },
@@ -1701,8 +1709,11 @@ class TestModel:
                             "subject": "old subject",
                         },
                     },
+                    "topic_msg_ids": {
+                        10: {"new subject": set(), "old subject": {1, 2}},
+                    },
                     "edited_messages": {1},
-                    "topics": {10: ["old subject"]},
+                    "topics": {10: ["new subject", "old subject"]},
                 },
                 False,
                 id="Message content is updated",
@@ -1711,6 +1722,7 @@ class TestModel:
                 {  # Both message content and subject is updated.
                     "message_id": 1,
                     "rendered_content": "<p>new content</p>",
+                    "orig_subject": "old subject",
                     "subject": "new subject",
                     "stream_id": 10,
                     "message_ids": [1],
@@ -1731,6 +1743,9 @@ class TestModel:
                             "subject": "old subject",
                         },
                     },
+                    "topic_msg_ids": {
+                        10: {"new subject": {1}, "old subject": {2}},
+                    },
                     "edited_messages": {1},
                     "topics": {10: []},
                 },
@@ -1758,8 +1773,11 @@ class TestModel:
                             "subject": "old subject",
                         },
                     },
+                    "topic_msg_ids": {
+                        10: {"new subject": set(), "old subject": {1, 2}},
+                    },
                     "edited_messages": {1},
-                    "topics": {10: ["old subject"]},
+                    "topics": {10: ["new subject", "old subject"]},
                 },
                 False,
                 id="Some new type of update which we don't handle yet",
@@ -1768,6 +1786,7 @@ class TestModel:
                 {  # message_id not present in index, topic view closed.
                     "message_id": 3,
                     "rendered_content": "<p>new content</p>",
+                    "orig_subject": "old subject",
                     "subject": "new subject",
                     "stream_id": 10,
                     "message_ids": [3],
@@ -1788,6 +1807,9 @@ class TestModel:
                             "subject": "old subject",
                         },
                     },
+                    "topic_msg_ids": {
+                        10: {"new subject": {3}, "old subject": {1, 2}},
+                    },
                     "edited_messages": set(),
                     "topics": {10: []},  # This resets the cache
                 },
@@ -1798,6 +1820,7 @@ class TestModel:
                 {  # message_id not present in index, topic view is enabled.
                     "message_id": 3,
                     "rendered_content": "<p>new content</p>",
+                    "orig_subject": "old subject",
                     "subject": "new subject",
                     "stream_id": 10,
                     "message_ids": [3],
@@ -1818,6 +1841,9 @@ class TestModel:
                             "subject": "old subject",
                         },
                     },
+                    "topic_msg_ids": {
+                        10: {"new subject": {3}, "old subject": {1, 2}},
+                    },
                     "edited_messages": set(),
                     "topics": {10: ["new subject", "old subject"]},
                 },
@@ -1828,6 +1854,7 @@ class TestModel:
                 {  # Message content is updated and topic view is enabled.
                     "message_id": 1,
                     "rendered_content": "<p>new content</p>",
+                    "orig_subject": "old subject",
                     "subject": "new subject",
                     "stream_id": 10,
                     "message_ids": [1],
@@ -1848,6 +1875,9 @@ class TestModel:
                             "subject": "old subject",
                         },
                     },
+                    "topic_msg_ids": {
+                        10: {"new subject": {1}, "old subject": {2}},
+                    },
                     "edited_messages": {1},
                     "topics": {10: ["new subject", "old subject"]},
                 },
@@ -1877,8 +1907,11 @@ class TestModel:
                 }
                 for message_id in [1, 2]
             },
+            "topic_msg_ids": {  # FIXME? consider test for eg. absence of empty set
+                10: {"new subject": set(), "old subject": {1, 2}},
+            },
             "edited_messages": set(),
-            "topics": {10: ["old subject"]},
+            "topics": {10: ["new subject", "old subject"]},
         }
         mocker.patch(MODEL + "._update_rendered_view")