[WIP] Disable cycling while editing a PM #1280

Subhasish-Behera · 2023-01-04T14:51:58Z

What does this PR do?
-> Disables cycling during editing a PM by tab key .
-> Shows a warning if one uses tab during editing a PM.

Partial fix for #774

Tested?

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

Commit flow

commit 1: changes made to boxes.py
commit 2: changes made to test_boxes.py
Notes & Questions

Interactions

Visual changes

Subhasish-Behera · 2023-01-10T05:28:20Z

@zulipbot add "PR needs review"

neiljp

@Subhasish-Behera Thanks for working on this too 👍

I've tested this manually and it seems to function well, for use of cycling using TAB or similar.

In the version we merged for streams, it benefits from the stream being a non-editable text object, which also limits clicking into the box or other accidental cases where it receives focus. That is a small issue with the solution in this PR right now, since we have only one private compose setup function.

Taking an approach like for streams but for private messages, seems like it would also avoid the possible need for tracking the reply state. The branch where we reuse the reply mode would then use a dedicated setup function for private edit, like we already have for stream_box_edit_view.

We could still maintain an error message for tab (or up) when editing a PM, as a separate commit/feature, though I don't think it's as important as making the recipient(s) unchangeable.

How does that sound?

zulipterminal/ui_tools/boxes.py

zulipbot · 2023-02-20T21:55:06Z

Heads up @Subhasish-Behera, we just merged some commits that conflict with the changes you made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.

Subhasish-Behera · 2023-04-16T16:16:09Z

The new code dosen't need self.reply_keypressed or self.edit_keypressed to track which key triggered the private_box_view as you suggested.

Subhasish-Behera · 2023-04-16T21:02:53Z

@zulipbot add "PR needs review"

neiljp

@Subhasish-Behera This looks much different - and more detailed - and certainly seems to work in practice 👍

Your commit style is broadly OK, but there are a few elements that are throwing the style off:

there's no blank line between the title and summary (so tig is showing it all on one line for me!)
it's challenging, but it works best, including for github breaking the title into multiple lines, if you can keep the title to 72 characters
using gitlint as per the README should help prompt for these issues; if not then we can adjust the rules it uses perhaps
for referring to future work, it's useful to refer to the principle of why you're doing it, rather than eg. specific function names that don't yet exist :)

The tests are generally good to see, with a few points to bear in mind:

if adding a test for existing code, do it before modifying; you can then be more sure that you don't break anything
if adding a function, generally add the test in that commit too
it's not always necessary to test every underlying helper function
similarly, it's good to aim for the big picture behavior, though sometimes it can be challenging not to test the implementation

I like how you extracted the function to handle the common code, in addition to the one for the base PM/DM setup, though I do wonder if it would be simpler to incorporate it into the existing similar function.

I'll review the tests in more detail later, since we may wish to restructure again slightly before we're done, as per my comments - this is another reason that testing every little helper function can be a challenge, since it's testing the implementation in some sense!

neiljp · 2023-04-23T05:47:56Z

zulipterminal/ui_tools/boxes.py

-        self.to_write_box = ReadlineEdit("To: ", edit_text=recipient_info)
-        self.to_write_box.enable_autocomplete(
-            func=self._to_box_autocomplete,
-            key=primary_key_for_command("AUTOCOMPLETE"),
-            key_reverse=primary_key_for_command("AUTOCOMPLETE_REVERSE"),
-        )
-        self.to_write_box.set_completer_delims("")


Lines 202-208 definitely make sense to move out of the common method, based on the stream design.

Is the point here that the editing version will have typing updates of a different form, or something like that? So the other lines also move due to that reason?

This commit appears to be a refactor - please add that as an area if so - but my main query here is the order of operations from this refactoring. The deleted code is essentially moved to the top of private_box_view, so it would be worth mentioning that in the commit text.

If I understood correctly, you are asking about lines other than 202 to 208.
In the overall implementation(beyond this particular commit), the plan is to not send typing updates at all for the editing version. Because we don't want to send typing notifications for editing a pre-existing message. so earlier the typing updates was sent for every kind of private writebox(reply,edit etc). but now it's not a part of the editing version.

The same thing is done behaviour-wise in the web app. Like when you edit a PM you won't see typing updates being a recipient of that message(similar to this PR's behaviour and unlike current upstream ZT's behaviour).

If we shouldn't be sending typing updates for editing, I suspect that could be clearer in the API documentation. If you're sure this is the behavior we want, posting to that stream that you think that's the case would be useful, or even a quick PR for it.

Since we're splitting the private-compose and private-edit UI it certainly makes it simpler to separate this behavior in this way.

neiljp · 2023-04-23T06:05:37Z

zulipterminal/ui_tools/boxes.py

+    def convert_id_to_info(
+        self, recipient_user_ids: Optional[List[int]] = None


Would this be better incorporated into _set_regular_and_typing_recipient_user_ids?

It would be possible if we were only talking about convert_id_to_info but _set_regular_and_typing_recipient_user_ids is also used by update_recipients function .

OK, so

_set_regular_and_typing_recipient_user_ids does the following, dependent upon passed user_ids

sets recipient_user_ids (autocompletion)

sets typing_recipient_user_ids (not including user)

update_recipients (which appears to effectively be update_recipients_from_emails_in_widget or similar):

sets recipient_emails

calls _set_regular_and_typing_recipient_user_ids with calculated ids from recipient_emails

convert_id_to_info does the following, dependent upon the passed user_ids

calls _set_regular_and_typing_recipient_user_ids

sets recipient_emails

sets recipient_info

This pattern suggests that the last two methods have a similar intent and so should share a name prefix, and generally could benefit from renaming.

The latter sets an extra attribute, which seems to be for initialization only - can we make that a return value instead?

neiljp · 2023-04-23T06:12:13Z

zulipterminal/ui_tools/messages.py

+                self.model.controller.view.write_box.private_box_edit_view(
+                    recipient_user_ids=self.recipient_ids,
+                )


This is what enables the new edit-private-message functionality, correct?

Yep! Just like in the case of "REPLY_MESSAGE" keypress, but here calls to a private_box_edit_view function.

This is a valid commit, but we might squash with the previous one(s), as this change is small and straightforward now :)

zulipterminal/ui_tools/boxes.py

tests/ui_tools/test_boxes.py

neiljp · 2023-09-09T02:45:24Z

@Subhasish-Behera Is this ready for another review?

The new approach is similar to how streams setup the coompose box. Here _setup_common_private_compose setsup the common requirements of diffrent write_box view. Mainly the logic behind the self.to_write_box such as it's auto-complete and when to send typing event updates(yes in reply state and no in edit state) is not covered here as they are non-common.

Mainly tests if autocomplete has been called, connect_signal of urwid as msg_write_box is editable and changes are expected.

Subhasish-Behera · 2023-09-11T14:19:27Z

@Subhasish-Behera Is this ready for another review?

Yep.

This function takes in recipient_user_ids and sets the recipient info accordingly.This function ensures there will be no code repetition. Another change is making the recipinet_info a class attribute as it will be used in edit view, non edit view, converting id to info.

The test are based on size of recipient_user_ids list as well as if it is empty or not.Then the code uses user_id_email_dict and user_dict to create the end recipient_info.

It tests for autocomplete is called twice, once indirectly for msg_write_box and other directly for to_write_box.Also asserts if to_write_box is editable.

With this function, private messages compose boxes uses 2 different functions for WriteBox while editing a message and replying a message. Here the self.to_write_box is non-editable urwid.Text.Also as its not editable, there is no need for auto-complete(unlike private_box_view). Its also dosen't send typing event updates to other recipients as it should be done for new messages and not for editing old messages.

It tests if enable_autocomplete is just called once indirectly for msg_write_box (and not to_write_box).Also checks if to_write_box is non-editable.

Earlier editing was dependent on calling the reply_message keypress but now it calls a separate method custom to creating a WriteBox for editing private message.

The WriteBox has same keypress for the box created for editing as well as reply. There is no neet to check the validity of self.to_write_box in case of editing as it's non-editable.So now it checks the msg_edit_state . Also now if the focus_position is FOCUS_CONTAINER_MESSAGE then the tab will change the focus to FOCUS_CONTAINER_HEADER only if its a reply (msg_edit_state =None). Tests Adapted.

Change the name of update_recipients to update_recipients_from_emails_in_widget.

neiljp

@Subhasish-Behera This looks like it will be really useful! 👍

I did leave a lot of comments, but some relate to commit structure, commit text, and smaller changes or suggestions.

It seems like the functionality is essentially done, so this should be a case of moving things around and some final tidying, with some optional extensions.

Other than fixing the issue fully, this will be a good refactor to the compose code to have in place for future work! 🎉

neiljp · 2023-10-04T21:02:35Z

zulipterminal/ui_tools/boxes.py

+        *,
+        recipient_user_ids: Optional[List[int]] = None,
+    ) -> None:
+        self.recipient_info = self.update_recipients_from_user_ids(recipient_user_ids)


self.recipient_info doesn't appear to be used by any of your later commits, outside of the setup functions (this and the edit equivalent), so it seems like you can continue to use recipient_info instead?

neiljp · 2023-10-04T21:25:39Z

tests/ui_tools/test_boxes.py

+        write_box.recipient_info = write_box.update_recipients_from_user_ids(
+            recipient_user_ids
+        )
+
+        assert write_box.recipient_emails == expected_recipient_emails
+        assert write_box.recipient_info == expected_recipient_info


While this isn't too likely to break anything, the assignment to write_box.recipient_info is related to the current implementation of the function that uses this new function (currently private_box_view, if we make that change in the earlier commit). This test should only care about what the function should achieve internally, including what it returns. The return value can be set to a local name in this test function - it doesn't need to be write_box.recipient_info.

This can be one of the challenges of writing tests for helper functions.

neiljp · 2023-10-04T21:31:27Z

tests/ui_tools/test_boxes.py

+        write_box.model.user_id_email_dict = user_id_email_dict
+        write_box.model.user_dict = user_dict
+        connect_signal_mock = mocker.patch.object(urwid, "connect_signal")
+        write_box.private_box_view(recipient_user_ids=recipient_user_ids)


Nit: Unless the test is only a few lines, please separate the line being tested from the setup and assert lines by blank lines, to make it easier to pick out which lines are which (you did this in the previous commit!)

neiljp · 2023-10-04T21:42:55Z

tests/ui_tools/test_boxes.py

+        write_box.to_write_box = mocker.MagicMock(spec=ReadlineEdit)
+        enable_autocomplete = mocker.patch.object(ReadlineEdit, "enable_autocomplete")


to_write_box is already a mock, so is the enable_autocomplete line necessary? It seems like it patches one part of the mock, but not to anything in particular, and returns that part. You do test that mocked/patched part, but I'm not sure it would be any different than

enable_autocomplete = write_box.to_write_box.enable_autocomplete

or alternatively substituting that inline where you have the assert_has_calls, since you only use/assert enable_autocomplete once in the test.

It seems rather that the previous line isn't necessary, as the write_box.to_write_box will be completely replaced by a new ReadlineEdit in this line.

So we need to patch ReadlineEdit here, but not patch to_write_box initially.

neiljp · 2023-10-04T21:47:37Z

tests/ui_tools/test_boxes.py

+        user_dict: List[Dict[str, Any]],
+        user_id_email_dict: Dict[int, str],
+    ) -> None:
+        recipient_user_ids = [11]


I was going to suggest placing this as a default argument, but I'm not sure if this has the typical python issue with potential errors from persistent mutable default arguments. Typing it as a Mapping rather than Dict may help.

If we do this, that eases any parametrizing them later - and saves a blank line in this case ;)

neiljp · 2023-10-05T00:14:16Z

zulipterminal/ui_tools/boxes.py

-        ]
-        self.focus_position = self.FOCUS_CONTAINER_MESSAGE
-
+        self._setup_common_private_compose()


To improve readability later, it may be useful to regroup lines in this function, since we're eventually going to have

the new user setup function

the custom UI header, leading into setting up the common UI part using the header

setting up the typing status code

In particular, we have part of the last bullet oddly above this line (send_next_typing_update) and not with the related code. Once the code is simplified, it'd also be good if that was naturally further down, and each group separated by a blank line.

In the edit version you can do any grouping when you introduce it; for this function I'll leave you to decide where to add/leave blank lines and move the line I mentioned, but eg. you delete a blank line in this commit, that could separate where the new function is from the 'timedelta' lines below.

neiljp · 2023-10-05T00:39:11Z

zulipterminal/ui_tools/boxes.py

+                            "Recipient(s) can not be edited while editing a",
+                            "Direct Message.",


This is missing a space.

Also note that you don't need a comma (or 2) - the string literals will automatically join together.

neiljp · 2023-10-05T00:49:34Z

zulipterminal/ui_tools/boxes.py

-                    # that all the recipients are valid, to avoid including any
-                    # invalid ones.
-                    self.update_recipients(self.to_write_box)
+                    if self.msg_edit_state is None:


Without this commit, the previous commit - using the new edit DM box, with different widgets - results in a crash when cycling with Tab, since it tries to validate the text and it has a different widget structure.

Since this and the next commit affect the Tab behavior only, while the earlier commits handle the editable nature of the to-box and splitting out the typing notifications, I'd suggest moving these commits to the front of the PR - these are independent groups of changes that don't have to be done in a specific order. That will make the Tab fixes come in first, followed by fixing the clickable To box, and changing the notifications behavior.

neiljp · 2023-10-05T01:03:56Z

zulipterminal/ui_tools/messages.py

+                self.model.controller.view.write_box.private_box_edit_view(
+                    recipient_user_ids=self.recipient_ids,
+                )


This is a valid commit, but we might squash with the previous one(s), as this change is small and straightforward now :)

neiljp · 2023-10-05T01:20:22Z

tests/ui_tools/test_boxes.py

@@ -408,7 +408,7 @@ def test_footer_notification_on_invalid_recipients(
            ),
        ],
    )
-    def test_update_recipients(
+    def test_update_recipients_from_emails_in_widget(


Re commit title: good to see a refactor: here, but remember to include the file(s) as an area too. This should be clear from our README.

zulipbot · 2024-04-17T23:35:34Z

Heads up @Subhasish-Behera, we just merged some commits that conflict with the changes you made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.

zulipbot added the size: S [Automatic label added by zulipbot] label Jan 4, 2023

Subhasish-Behera changed the title ~~Disable cycling pm~~ Disable cycling while editing a PM Jan 4, 2023

Subhasish-Behera marked this pull request as ready for review January 4, 2023 14:58

zulipbot added the PR needs review PR requires feedback to proceed label Jan 10, 2023

neiljp reviewed Jan 16, 2023

View reviewed changes

zulipterminal/ui_tools/boxes.py Outdated Show resolved Hide resolved

zulipterminal/ui_tools/boxes.py Outdated Show resolved Hide resolved

neiljp added area: UI General user interface update 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 16, 2023

zulipbot added the has conflicts label Feb 20, 2023

Subhasish-Behera changed the title ~~Disable cycling while editing a PM~~ [WIP] Disable cycling while editing a PM Mar 16, 2023

Subhasish-Behera force-pushed the disable_cycling_PM branch from 2e1414a to 7016750 Compare April 16, 2023 14:26

zulipbot added size: XL [Automatic label added by zulipbot] and removed size: S [Automatic label added by zulipbot] labels Apr 16, 2023

Subhasish-Behera force-pushed the disable_cycling_PM branch 3 times, most recently from e304afe to f635f03 Compare April 16, 2023 16:06

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

neiljp reviewed Apr 23, 2023

View reviewed changes

neiljp removed PR needs review PR requires feedback to proceed has conflicts labels Apr 23, 2023

Subhasish-Behera force-pushed the disable_cycling_PM branch from f635f03 to cac1d35 Compare April 25, 2023 20:06

Subhasish-Behera force-pushed the disable_cycling_PM branch from 6165026 to cac1d35 Compare September 8, 2023 18:00

Subhasish-Behera added 2 commits September 9, 2023 19:08

tests: boxes: Add tests for _setup_common_private_compose.

7cfffe0

Mainly tests if autocomplete has been called, connect_signal of urwid as msg_write_box is editable and changes are expected.

Subhasish-Behera force-pushed the disable_cycling_PM branch from cac1d35 to bf7f923 Compare September 11, 2023 14:16

Subhasish-Behera added the PR needs review PR requires feedback to proceed label Sep 11, 2023

Subhasish-Behera and others added 9 commits September 11, 2023 20:06

tests: boxes: Add tests for update_recipients_from_user_ids.

415f31f

The test are based on size of recipient_user_ids list as well as if it is empty or not.Then the code uses user_id_email_dict and user_dict to create the end recipient_info.

tests: boxes: Add tests for private_box_view.

2a66b5d

It tests for autocomplete is called twice, once indirectly for msg_write_box and other directly for to_write_box.Also asserts if to_write_box is editable.

tests: boxes: Add tests for private_box_edit_view.

f1590cb

It tests if enable_autocomplete is just called once indirectly for msg_write_box (and not to_write_box).Also checks if to_write_box is non-editable.

messages: Call private_box_edit_view when editing private message.

ba32129

Earlier editing was dependent on calling the reply_message keypress but now it calls a separate method custom to creating a WriteBox for editing private message.

boxes: Footer message if tab key pessed while editing a private message.

ac20ad7

refactor: Change name of update_recipients function.

2bd5228

Change the name of update_recipients to update_recipients_from_emails_in_widget.

Subhasish-Behera force-pushed the disable_cycling_PM branch from bf7f923 to 2bd5228 Compare September 11, 2023 14:38

Subhasish-Behera added the PR needs mentor review label Sep 13, 2023

neiljp reviewed Oct 5, 2023

View reviewed changes

neiljp removed the PR needs review PR requires feedback to proceed label Oct 5, 2023

zulipbot added the has conflicts label Apr 17, 2024

neiljp added PR completion candidate PR with reviews that may unblock merging and removed PR needs mentor review labels May 8, 2024

Niloth-p assigned zormit and unassigned zormit May 20, 2024

zormit mentioned this pull request May 24, 2024

Disable cycling to recipients when editing PM #1504

Open

18 tasks

zormit added the PR replaced by another PR label May 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Disable cycling while editing a PM #1280

[WIP] Disable cycling while editing a PM #1280

Subhasish-Behera commented Jan 4, 2023 •

edited

Loading

Subhasish-Behera commented Jan 10, 2023

neiljp left a comment

zulipbot commented Feb 20, 2023

Subhasish-Behera commented Apr 16, 2023

Subhasish-Behera commented Apr 16, 2023

neiljp left a comment

neiljp Apr 23, 2023

Subhasish-Behera Apr 24, 2023

neiljp Apr 26, 2023

neiljp Apr 23, 2023

Subhasish-Behera Apr 25, 2023

neiljp Apr 27, 2023

neiljp Apr 23, 2023

Subhasish-Behera Apr 24, 2023

neiljp Oct 5, 2023

neiljp commented Sep 9, 2023

Subhasish-Behera commented Sep 11, 2023

neiljp left a comment

neiljp Oct 4, 2023

neiljp Oct 4, 2023

neiljp Oct 4, 2023

neiljp Oct 4, 2023

zormit May 22, 2024

neiljp Oct 4, 2023

neiljp Oct 5, 2023

neiljp Oct 5, 2023

neiljp Oct 5, 2023

neiljp Oct 5, 2023

neiljp Oct 5, 2023

zulipbot commented Apr 17, 2024

		def convert_id_to_info(
		self, recipient_user_ids: Optional[List[int]] = None

		write_box.to_write_box = mocker.MagicMock(spec=ReadlineEdit)
		enable_autocomplete = mocker.patch.object(ReadlineEdit, "enable_autocomplete")

		"Recipient(s) can not be edited while editing a",
		"Direct Message.",

[WIP] Disable cycling while editing a PM #1280

Are you sure you want to change the base?

[WIP] Disable cycling while editing a PM #1280

Conversation

Subhasish-Behera commented Jan 4, 2023 • edited Loading

Subhasish-Behera commented Jan 10, 2023

neiljp left a comment

Choose a reason for hiding this comment

zulipbot commented Feb 20, 2023

Subhasish-Behera commented Apr 16, 2023

Subhasish-Behera commented Apr 16, 2023

neiljp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neiljp commented Sep 9, 2023

Subhasish-Behera commented Sep 11, 2023

neiljp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zulipbot commented Apr 17, 2024

Subhasish-Behera commented Jan 4, 2023 •

edited

Loading