Disable cycling to recipients when editing PM #1504
Conversation
This test verifies the assumption that the to_write_box is editable by default. This assumption will change when we create a specific "editing private message" box soon. Co-authored-by: zormit <[email protected]>
Change the name to update_recipients_from_emails_in_widget, in order to be more specific in detail what this helper function is doing. This prepares for a change that will add another method on this class, that will update the recipient in a different way. Co-authored-by: zormit <[email protected]>
Pull out a helper method when composing the private_box_view, similar to _setup_common_stream_compose. The helper mainly takes care of preparing the msg and header write boxes. This common functionality will be needed soon, when we build a different view for reply and edit state. Co-authored-by: zormit <[email protected]>
This helper method to build the recipient list will also be needed in the upcoming change. Co-authored-by: zormit <[email protected]>
In other words: CYCLE_COMPOSE_FOCUS is now deactivated for the case of editing a private message. This is one core change of this PR and solves the issue that it doesn't particularly make sense to move the focus to the recipients. Note that we don't yet disallow *clicking* into the recpient list, which will be a later change. Co-authored-by: zormit <[email protected]>
zormit
force-pushed
the
774-disable-cycling-to-recipients-editing-pm
branch
2 times, most recently
from
c6dd9a5 to
bd20186
Compare
In order to fully disable editing (not only cycling to it, but e.g. prevent editing through mouse clicks) we need to make the editing of a message a different UI box than the usual reply. This new box uses a urwid.Text to display the recipient information rather than providing an editable field. Fixes zulip#774. Co-authored-by: zormit <[email protected]>
Co-authored-by: zormit <[email protected]>
zormit
force-pushed
the
774-disable-cycling-to-recipients-editing-pm
branch
from
bd20186 to
9b5ca86
Compare
|
|
||
| if not self.msg_body_edit_enabled: | ||
| return key | ||
| if self.focus_position == self.FOCUS_CONTAINER_HEADER: | ||
| self.focus_position = self.FOCUS_CONTAINER_MESSAGE | ||
| else: | ||
| self.focus_position = self.FOCUS_CONTAINER_HEADER | ||
| if self.compose_box_status == "open_with_stream": |
There was a problem hiding this comment.
This logic is a bit tricky to read. I was thinking about refactoring the whole CYCLE_COMPOSE_FOCUS section, but that might be overkill for this PR.
| @@ -1096,7 +1096,9 @@ def keypress(self, size: urwid_Size, key: str) -> Optional[str]: | |||
| ) | |||
|
|
|||
| if self.message["type"] == "private": | |||
| self.keypress(size, primary_key_for_command("REPLY_MESSAGE")) | |||
| self.model.controller.view.write_box.private_box_edit_view( | |||
There was a problem hiding this comment.
This change doesn't have a test. It didn't have a test in the PR that I refined, but maybe it's worth thinking about adding one? How would we test this? (didn't look into it yet)
There was a problem hiding this comment.
Apart from NITs, I couldn't really find anything to suggest improvements.
The PR looks well-organized to me, and is very readable.
The changes work.
There's a typo 'Recipient' in commit message of commit 6. Also the PR title, but that of course doesn't matter.
| ( | ||
| [11, 12], | ||
| "To: Human 1 <[email protected]>, Human 2 <[email protected]>", | ||
| ), |
There was a problem hiding this comment.
Test ids could be added, I assume.
| ), | ||
| ], | ||
| ) | ||
| def test_private_box_recipient_editing( |
There was a problem hiding this comment.
I learnt that sometimes using double underscores in the test function names helps, not sure about this test name in particular. Would test_private_box__recipient_editing be better? We don't have any other tests for private box, yet this is easier to read?
| @@ -265,7 +265,7 @@ def track_idleness_and_update_status() -> None: | |||
|
|
|||
| urwid.connect_signal(self.msg_write_box, "change", on_type_send_status) | |||
|
|
|||
| def update_recipients(self, write_box: ReadlineEdit) -> None: | |||
| def update_recipients_from_emails_in_widget(self, write_box: ReadlineEdit) -> None: | |||
There was a problem hiding this comment.
I noticed that this naming using 'in_widget' occurs only in this function in the codebase. Does it actually help us to differentiate it from valid emails, or is it redundant?
zormit
changed the title
zormit
added
PR awaiting update
What does this PR do, and why?
This is a rebase and simplification of #1280. It's the missing piece to resolve #774.
In comparison to the previous work
External discussion & connections
Disable PM recipient cycling on edit #T774 #T1280How did you test this?
Self-review checklist for each commit