Compose box colored and further styled #865

Ezio-Sarthak · 2021-01-03T09:26:50Z

This PR is a follow-up PR from #852.
Visual improvements:

Stream and topic prefixes minimized.
Invalid stream handling.
Stream prefix color rendering.

Partially fixes #783.

neiljp

@Ezio-Sarthak This looks to work fairly well, and certainly with small adjustment we could grab the first few commits fairly cleanly. I left some commits in-line.

I think we may want to refine the stream-validation aspect:

this triggers fine via an invalid stream on tab (CYCLE_COMPOSE_FOCUS)
if autocompleting to a valid value, then returning to the original text always appears to show as invalid? This is noticeable not just if we have eg. a -> announce -> a, but also with mobile -> mobile -> mobile-team -> mobile.
the previous case is also a little strange with eg. clearing the box, where the stream character remains as before, but then changes if cycling as above

We can discuss the above points in-stream.

zulipterminal/config/symbols.py

zulipterminal/ui_tools/boxes.py

tests/ui_tools/test_boxes.py

zulipterminal/ui_tools/boxes.py

tests/ui_tools/test_boxes.py

neiljp · 2021-01-06T21:32:17Z

tests/ui_tools/test_boxes.py

+        def is_valid_stream(stream_name: str) -> bool:
+            for stream in stream_dict.values():
+                if stream['name'] == stream_name:
+                    return True
+            return False
+
+        def stream_id_from_name(stream_name: str) -> int:
+            for stream_id, stream in stream_dict.items():
+                if stream['name'] == stream_name:
+                    return stream_id
+            raise RuntimeError("Invalid stream name.")


This duplication of the model code seems overly complex. Can we just mock the behavior we expect for these cases, or parametrize the tests appropriately?

zulipterminal/ui_tools/boxes.py

Ezio-Sarthak · 2021-01-07T11:40:19Z

Thanks for suggesting the changes! They were really required to clean the messy code earlier 😅 .

I squashed the first commit because, as you said, the symbols need to be added whenever they are used in the code. So, making an explicit commit for that wasn't useful.
Re invalid stream marker I've improved the rendering of stream name (now by urwid.connect_signal()) which clears that issue :)
This PR might also improve the test coverage of WriteBox, as raised in Improve test coverage for WriteBox #739 :)

neiljp · 2021-01-07T15:08:57Z

Re the previous first commit, we already use P/# in one place for the stream list, so that's what I meant you could extract; let me know in the stream if this isn't clear.

neiljp

@Ezio-Sarthak This include the refactoring I expected, and the live validation of the stream works really well 👍

Most changes below are fairly small, though if we could use a method rather than directly accessing the stream_dict that would likely improve the tests, which might be easier once we have the general refactoring I've mentioned elsewhere too.

zulipterminal/ui_tools/boxes.py

tests/ui_tools/test_boxes.py

zulipterminal/ui_tools/buttons.py

zulipterminal/ui_tools/views.py

tests/ui_tools/test_boxes.py

neiljp · 2021-01-11T19:39:11Z

tests/ui_tools/test_boxes.py

+        write_box.model.stream_dict = stream_dict
+        write_box.stream_id = stream_id
+        write_box.model.is_valid_stream.return_value = is_valid_stream
+        write_box.model.stream_id_from_name.return_value = stream_id


Needing to set the first and last of these comes from how we're directly accessing the model. Improving the stream encapsulation seems upcoming with @Abhirup-99 's work on the subscription events and other interest in that direction.

tests/ui_tools/test_boxes.py

neiljp · 2021-01-11T19:43:55Z

tests/ui_tools/test_boxes.py

@@ -337,6 +341,7 @@ def test__set_stream_write_box_style_markers(self, write_box, stream_id,
        write_box._set_stream_write_box_style(write_box, stream_name)

        assert write_box.header_write_box[0].text == expected_marker
+        assert expected_color in str(write_box.header_write_box[0].get_text())


Is there a reason to use in rather than direct equality?

Yep. Actually the .get_text() attribute gives a tuple object ('#', [('color', 1)]), so I thought it would be clean to stringify it and search for the expected color. We could also use the .attrib to get display attributes in form [('color', 1)].

An alternative to this could be to directly use equality as follows:

assert expected_color == write_box.header_write_box[0].attrib[0][0]

which I tried earlier, but thought it would look messy. (but seems fine now :))

With this commit, the stream prefixes in stream list and stream info are refactored.

* Stream and topic box prefixes replaced with equivalent symbols. * Symbols now packed in the header column elements. * Focus values of header and message box elements updated. * Focus values of stream and topic prefixes added. Tests added (header contents).

* The function added currently handles only stream prefix markups. * Invalid stream name results in a standard wrong marker. Tests updated (the ones linked with stream autocomplete). Tests added (for the new styling function).

Style improvements: * Stream prefixes' color rendered. * The topic prefix is not rendered to make equivalence with the webapp. Tests updated and added (marker/color rendering). Partially fixes zulip#783.

Ezio-Sarthak · 2021-01-12T11:03:52Z

Thanks for the suggestions! Updated the PR :)

neiljp · 2021-01-12T21:23:54Z

@Ezio-Sarthak I've merged this with a few adjustments as the commit ending at e2a62a2 🎉

The adjustments were mainly small reformatting or text adjustments - including commit text which you'd not fully adjusted after the split of commits, perhaps? I also moved the two extra constants we discussed between commits ie. to where they were first used. The biggest change I noticed latterly was a bugfix - the stream marker was not set on setup, only on changes, so I added that (with test changes) and added myself as a co-author to avoid needing to start over.

As before, I'd recommend reading the final commits to understand the differences in style/text/code and using git range-diff.

zulipbot added the size: XL [Automatic label added by zulipbot] label Jan 3, 2021

Ezio-Sarthak force-pushed the re-enhance-compose-msg-area branch 2 times, most recently from e0c8921 to 895f79b Compare January 6, 2021 15:57

neiljp reviewed Jan 6, 2021

View reviewed changes

neiljp added the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label Jan 6, 2021

Ezio-Sarthak force-pushed the re-enhance-compose-msg-area branch 4 times, most recently from 2d7d535 to 437d7aa Compare January 7, 2021 11:26

Ezio-Sarthak requested a review from neiljp January 7, 2021 12:15

Ezio-Sarthak force-pushed the re-enhance-compose-msg-area branch from 437d7aa to e51d62c Compare January 7, 2021 13:35

Ezio-Sarthak force-pushed the re-enhance-compose-msg-area branch 4 times, most recently from cf0e4ed to 08f77e6 Compare January 7, 2021 16:36

neiljp added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Jan 8, 2021

neiljp reviewed Jan 11, 2021

View reviewed changes

neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs review PR requires feedback to proceed labels Jan 11, 2021

Ezio-Sarthak force-pushed the re-enhance-compose-msg-area branch from 08f77e6 to 79d0518 Compare January 12, 2021 10:53

Ezio-Sarthak added 4 commits January 12, 2021 16:24

refactor: Private and public stream prefixes added and refactored.

706368d

With this commit, the stream prefixes in stream list and stream info are refactored.

boxes: Private and public stream prefix markers rendered.

8b8b6f5

* The function added currently handles only stream prefix markups. * Invalid stream name results in a standard wrong marker. Tests updated (the ones linked with stream autocomplete). Tests added (for the new styling function).

enhancement: boxes: Re Stream marker colors rendered.

d0982ae

Style improvements: * Stream prefixes' color rendered. * The topic prefix is not rendered to make equivalence with the webapp. Tests updated and added (marker/color rendering). Partially fixes zulip#783.

Ezio-Sarthak force-pushed the re-enhance-compose-msg-area branch from 79d0518 to d0982ae Compare January 12, 2021 10:54

neiljp closed this Jan 12, 2021

neiljp added this to the Next Release milestone Jan 12, 2021

Ezio-Sarthak deleted the re-enhance-compose-msg-area branch March 26, 2021 04:29

Ezio-Sarthak mentioned this pull request May 2, 2021

Show placeholder hints for different entry boxes #1021

Open

Uh oh!

Compose box colored and further styled #865

Compose box colored and further styled #865

Uh oh!

Conversation

Ezio-Sarthak commented Jan 3, 2021

Uh oh!

neiljp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

neiljp Jan 6, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Ezio-Sarthak commented Jan 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neiljp commented Jan 7, 2021

Uh oh!

neiljp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

neiljp Jan 11, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

neiljp Jan 11, 2021

Choose a reason for hiding this comment

Uh oh!

Ezio-Sarthak Jan 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ezio-Sarthak commented Jan 12, 2021

Uh oh!

neiljp commented Jan 12, 2021

Uh oh!

Uh oh!

Ezio-Sarthak commented Jan 7, 2021 •

edited

Loading

Ezio-Sarthak Jan 12, 2021 •

edited

Loading