model: Highlight stream name in reporting of moving/splitting topics. #1196

srdeotarse · 2022-04-11T19:00:37Z

What does this PR do?
This PR highlights stream names in reporting of moving/splitting topic in footer text.

Partially fixes #1172

CZO - https://chat.zulip.org/#narrow/stream/206-zulip-terminal/topic/.E2.9C.94.20Improve.20reporting.20on.20moving.2Fsplitting.20topics.20.23T1172.20.23T1178

New CZO - https://chat.zulip.org/#narrow/stream/206-zulip-terminal/topic/Highlight.20text.20of.20reporting.20on.20moving.2Fsplitting.20topics

Tested?

Manually
Existing tests (adapted, if necessary)
New tests added (for any new behavior)
Passed linting & tests (each commit)

Commit flow

Notes & Questions

Interactions

Visual changes

neiljp

@srdeotarse This achieves the formatting fine, but there are many ways to style text in urwid, so passing ["some string", ("named_style", " another string"), ...] is going to be simpler.

This has been discussed in the past in terms of better defining an actual type for this 'styled text' in other topics and open PRs, which you may find informative - it's rather complex due to what is possible!

One followup aspect to this is that we're casually passing in a list to report_success and this is not being flagged by mypy! We may well want to change the type of the function, but I'd like to know why mypy is not warning about this - tests appear to be passing.

zulipterminal/model.py

srdeotarse · 2022-04-14T14:29:50Z

@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 · 2022-04-14T17:45:51Z

@zulipbot add "PR needs review".

neiljp · 2022-04-15T17:38:24Z

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 · 2022-04-16T06:10:36Z

@zulipbot add "PR needs review".

neiljp · 2022-04-20T00:31:47Z

@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?

srdeotarse · 2022-04-20T01:19:49Z

@zulipbot add "PR needs review".

srdeotarse · 2022-04-20T15:54:14Z

@zulipbot add "PR needs review".

zulipbot · 2022-04-20T15:54:15Z

ERROR: Label "PR needs review" already exists and was thus not added to this pull request.

Change argument type of report_success from text: str to text: List[Union[str, Tuple[Literal["footer_contrast"], str]]]. Tests updated.

Change argument type of report_warning from text: str to text: List[Union[str, Tuple[Literal["footer_contrast"], str]]]. Tests updated.

Change argument type of report_error from text: str to text: List[Union[str, Tuple[Literal["footer_contrast"], str]]]. Tests updated.

Tests updated. Partial fix towards zulip#1172.

neiljp

@srdeotarse There were a few minor points here which I've addressed myself in a quick tidy and pushed back:

the blacken is in an incorrect commit (but passed overall)
some strings had been combined (and were overly long)
in one case the text was already a list, that was passed to report_error

As long as this passes, which I don't expect any problems with, I'll merge shortly after :)

neiljp · 2022-04-21T19:38:47Z

zulipterminal/ui_tools/boxes.py

@@ -329,7 +329,7 @@ def _tidy_valid_recipients_and_notify_invalid_ones(
                ),
                " to autocomplete.",
            ]
-            self.view.controller.report_error(invalid_recipients_error)
+            self.view.controller.report_error([invalid_recipients_error])


This is already a list.

neiljp · 2022-04-21T19:55:09Z

tests/helper/test_helper.py

-            [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."
+            ],


This is a blacken.

zulipbot · 2022-04-21T20:47:40Z

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!

neiljp · 2022-04-21T20:57:18Z

@srdeotarse Thanks for the followup here! 🎉

zulipbot added size: XL area: message editing area: UI General user interface update enhancement New feature or request labels Apr 11, 2022

srdeotarse force-pushed the issue-1172-followup-highlight-moving/spliting-topic-reporting branch from 2fba636 to e96594c Compare April 12, 2022 03:18

neiljp reviewed Apr 14, 2022

View reviewed changes

zulipterminal/model.py Outdated Show resolved Hide resolved

zulipterminal/model.py Outdated Show resolved Hide resolved

neiljp added the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label Apr 14, 2022

srdeotarse force-pushed the issue-1172-followup-highlight-moving/spliting-topic-reporting branch from e96594c to 32b0717 Compare April 14, 2022 12:09

zulipbot added size: L and removed size: XL labels Apr 14, 2022

srdeotarse force-pushed the issue-1172-followup-highlight-moving/spliting-topic-reporting branch from 32b0717 to 28b5f9f Compare April 14, 2022 14:34

zulipbot added the PR needs review PR requires feedback to proceed label Apr 14, 2022

neiljp removed the PR needs review PR requires feedback to proceed label Apr 15, 2022

srdeotarse force-pushed the issue-1172-followup-highlight-moving/spliting-topic-reporting branch from 28b5f9f to c0bbffd Compare April 16, 2022 06:07

zulipbot added the PR needs review PR requires feedback to proceed label Apr 16, 2022

srdeotarse force-pushed the issue-1172-followup-highlight-moving/spliting-topic-reporting branch from c0bbffd to b44090a Compare April 16, 2022 06:27

neiljp removed the PR needs review PR requires feedback to proceed label Apr 20, 2022

srdeotarse force-pushed the issue-1172-followup-highlight-moving/spliting-topic-reporting branch from b44090a to 8530f4e Compare April 20, 2022 01:15

zulipbot added size: XL and removed size: L labels Apr 20, 2022

srdeotarse force-pushed the issue-1172-followup-highlight-moving/spliting-topic-reporting branch from 8530f4e to c2e644b Compare April 20, 2022 01:17

zulipbot added the PR needs review PR requires feedback to proceed label Apr 20, 2022

srdeotarse force-pushed the issue-1172-followup-highlight-moving/spliting-topic-reporting branch 2 times, most recently from 2662ae7 to d14f51d Compare April 20, 2022 14:52

srdeotarse force-pushed the issue-1172-followup-highlight-moving/spliting-topic-reporting branch from d14f51d to 648b188 Compare April 20, 2022 15:52

srdeotarse added 4 commits April 21, 2022 13:10

refactor: core: Change report_success argument type to list.

be4fdec

Change argument type of report_success from text: str to text: List[Union[str, Tuple[Literal["footer_contrast"], str]]]. Tests updated.

refactor: core: Change report_warning argument type to list.

bf429d1

Change argument type of report_warning from text: str to text: List[Union[str, Tuple[Literal["footer_contrast"], str]]]. Tests updated.

refactor: core: Change report_error argument type to list.

81080b8

Change argument type of report_error from text: str to text: List[Union[str, Tuple[Literal["footer_contrast"], str]]]. Tests updated.

model: Highlight stream & topic in reporting of moving/spliting topics.

dc5ba6b

Tests updated. Partial fix towards zulip#1172.

neiljp reviewed Apr 21, 2022

View reviewed changes

neiljp force-pushed the issue-1172-followup-highlight-moving/spliting-topic-reporting branch from 648b188 to dc5ba6b Compare April 21, 2022 20:47

neiljp merged commit 5b43ca5 into zulip:main Apr 21, 2022

neiljp removed PR needs review PR requires feedback to proceed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Apr 21, 2022

neiljp added this to the Next Release milestone Apr 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

model: Highlight stream name in reporting of moving/splitting topics. #1196

model: Highlight stream name in reporting of moving/splitting topics. #1196

srdeotarse commented Apr 11, 2022 •

edited

neiljp left a comment

srdeotarse commented Apr 14, 2022

srdeotarse commented Apr 14, 2022

neiljp commented Apr 15, 2022 •

edited

srdeotarse commented Apr 16, 2022

neiljp commented Apr 20, 2022

srdeotarse commented Apr 20, 2022

srdeotarse commented Apr 20, 2022

zulipbot commented Apr 20, 2022

neiljp left a comment

neiljp Apr 21, 2022

neiljp Apr 21, 2022

zulipbot commented Apr 21, 2022

neiljp commented Apr 21, 2022

model: Highlight stream name in reporting of moving/splitting topics. #1196

model: Highlight stream name in reporting of moving/splitting topics. #1196

Conversation

srdeotarse commented Apr 11, 2022 • edited

neiljp left a comment

Choose a reason for hiding this comment

srdeotarse commented Apr 14, 2022

srdeotarse commented Apr 14, 2022

neiljp commented Apr 15, 2022 • edited

srdeotarse commented Apr 16, 2022

neiljp commented Apr 20, 2022

srdeotarse commented Apr 20, 2022

srdeotarse commented Apr 20, 2022

zulipbot commented Apr 20, 2022

neiljp left a comment

Choose a reason for hiding this comment

neiljp Apr 21, 2022

Choose a reason for hiding this comment

neiljp Apr 21, 2022

Choose a reason for hiding this comment

zulipbot commented Apr 21, 2022

neiljp commented Apr 21, 2022

srdeotarse commented Apr 11, 2022 •

edited

neiljp commented Apr 15, 2022 •

edited