boxes: Disable cycling using arrow keys if recipient is invalid. #864

zee-bit · 2021-01-02T14:42:56Z

This PR disables cycling from the recipient box to any other box inside the WriteBox view using arrow keys, if the recipient name is invalid. This is very similar to CYCLE_COMPOSE_FOCUS using Tab key. Tests have also been updated and amended.

This PR fixes #779.

neiljp · 2021-01-16T07:02:29Z

@zee-bit Just some quick feedback:

is there a lot of duplication here? Could we reuse parts to make this a lot simpler?
the tests look large and complex too
is there some way we can trigger on 'losing focus', to avoid having all these cases? (that might cover the mouse as well, as a minor side-effect)

preetmishra

@zee-bit Hey, thanks for working on this. I pulled your changes locally and it seems to work well functionally. 👍

However, echoing what @neiljp mentioned, we could benefit from a general approach if feasible. In any case, we would want to reduce the apparent redundancy.

zee-bit · 2021-01-25T13:40:43Z

@neiljp Regarding your feedback:

I have made two functions(footer_notify_invalid_stream & footer_notify_invalid_emails), they have somewhat increased code reusability. The main logic, I think, could not be simplified further if I am to proceed with the current implementation.
The tests are very much related to that of CYCLE_COMPOSE_FOCUS as it is derived from there, but there were few places with unnecessary conditions and checks that I have removed.
I am not quite sure if there is any other way to trigger on 'loosing focus'(without all those conditions). If you or @preetmishra have some other implementation in mind, I would love to learn :). Re mouse event, I can try extending this for mouse events as well, if this implementation looks fine.

zee-bit · 2021-01-25T13:42:07Z

@zulipbot add "PR needs review"

neiljp

@zee-bit This needs rebasing to work around conflicts and potentially update for the latest improvements in the PM UI/data, but the implementation also demonstrates both the duplication and what look like issues due to not duplicating extra code. This could be refactored variously, but we could also "up" from the message content, which might affect how the refactoring could go.

zulipterminal/ui_tools/boxes.py

neiljp · 2021-02-17T07:17:55Z

I should say that it does seem to work OK 👍 I've not found a signal emitted within urwid that would solve this for us as it stands.

zee-bit · 2021-03-14T20:34:36Z

@zulipbot add "PR needs review"
@zulipbot remove "PR awaiting update"

prah23

@zee-bit Thanks for this!
This works as expected and restricts the user from bypassing the validation quite well. 👍
I've dropped a couple of comments inline, as some of these changes would also benefit #937.

zulipterminal/ui_tools/boxes.py

neiljp

@zee-bit Extracting the helper(s) for the footer simplifies things to some degree, but the overlap between tab and right/down is still very similar. I'm currently fairly open to this PR since you have a test. However, the code is highly duplicated and I'd much prefer a solution which combined the two branches if we are able, to avoid that. We can always refactor later, but then we accumulate "technical debt".

zulipterminal/ui_tools/boxes.py

zee-bit · 2021-03-28T07:19:48Z

@zulipbot add "PR needs review"
@zulipbot remove "PR awaiting update"

preetmishra

@zee-bit Thanks for your work. I pulled code locally and it seems to work well! 👍

While the PR is functional, we could simplify the code a bit. I have left a few in-line comments. Let me know if there's anything that needs clarification.

zulipterminal/ui_tools/boxes.py

preetmishra · 2021-05-26T17:49:14Z

tests/ui_tools/test_boxes.py

@@ -709,7 +737,8 @@ def test_keypress_CYCLE_COMPOSE_FOCUS(self, write_box, tab_key,
                write_box.stream_box_view(stream_id)
        else:
            write_box.private_box_view()
-        size = widget_size(write_box)
+        # FIXME: Use widget_size(), instead of hard-coded size.


Why aren't we using widget_size?

Well, I tried using widget_size, but for some cases, the write-box changes to box widget instead of flow and returns a (maxcol, maxrow) size instead of just (maxcol, ). This made the tests fail for such cases. Let me try to dig more into this and see if I can get a fix for this.

Sorry for the late reply. I got busy with other PR's since finding the root+fix of this particular issue was taking forever. :(
I have tried to dig more on this, but it seems like it's an internal urwid issue that I am unable to resolve. While looking at this, I realized that the whole write_box is sometimes recognized as "flow widget" for all tests (that's when all the tests pass), but sometimes the write_box is determined as "box widget" for all tests (2 tests fail in this case).

I also found that in the latter case, the tests only fail for "DOWN" keypresses, because it can't be handled properly by super() i.e. urwid's container.py. I can fix the whole issue by not letting the "DOWN" keypress being handled by urwid and returning early after manually handling, but it seems to have other side-effects and does not look clean.

zulipterminal/ui_tools/boxes.py

preetmishra · 2021-05-26T17:52:47Z

zulipterminal/ui_tools/boxes.py

+            ('code', primary_key_for_command('AUTOCOMPLETE_REVERSE')),
+            ' to autocomplete.'
+        ]
+        self.view.set_footer_text(footer_message, 3)


The method is calling an existing method, set_footer_text, with a particular error message. This could lead us to a state where we have multiple methods that essentially do the same thing with different arguments for different error types.

Another way could be to just have an ERROR_TEMPLATES data structure that could be used to pass an appropriate error message to the existing set_footer_text method.

Would it make sense to define the ERROR_TEMPLATES in a separate module?

This commit refactors code for footer notification in case of invalid recipients (stream/email) in the WriteBox to separate helper function. This prevents duplication of code and allows reusability. This function can now be called with a custom error message that will be displayed in the footer after appending it with a common typeahead suggestion. Tests amended.

This commit disables cycling from the recipient box (Stream/To) using arrow keys if recipient is invalid, similar to Tab. Cycling through the MessageBox now looks for arrow key events and checks for recipient name's validity when losing focus from the recipient box. This should prevent sending of messages to incorrect recipients. Currently, only RIGHT and DOWN keypress is monitored since these are the only keys that can cycle focus from the recipients box to some other box, according to our current MessageBox layout. Tests amended. Fixes zulip#779.

zulipbot · 2021-08-30T23:02:57Z

Heads up @zee-bit, we just merged some commits that conflict with the changes your 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 size: XL [Automatic label added by zulipbot] bug Something isn't working enhancement New feature or request labels Jan 2, 2021

zee-bit force-pushed the issue_779 branch 2 times, most recently from 834b1ae to 67d5df8 Compare January 6, 2021 18:13

neiljp added the PR needs review PR requires feedback to proceed label Jan 6, 2021

zee-bit force-pushed the issue_779 branch from 67d5df8 to 2ed90bf Compare January 8, 2021 18:30

preetmishra reviewed Jan 17, 2021

View reviewed changes

preetmishra removed the PR needs review PR requires feedback to proceed label Jan 17, 2021

zee-bit force-pushed the issue_779 branch from 2ed90bf to dd23c0d Compare January 25, 2021 12:56

zulipbot added the PR needs review PR requires feedback to proceed label Jan 25, 2021

neiljp added this to the Release after next milestone Jan 28, 2021

Base automatically changed from master to main January 30, 2021 20:31

neiljp reviewed Feb 17, 2021

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 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 Feb 17, 2021

zee-bit force-pushed the issue_779 branch from dd23c0d to 8d034ee Compare March 14, 2021 20:26

zulipbot 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 Mar 14, 2021

prah23 suggested changes Mar 14, 2021

View reviewed changes

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

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

zee-bit force-pushed the issue_779 branch 2 times, most recently from 20e7ef3 to fbf73c0 Compare March 15, 2021 18:58

neiljp added the PR blocks other PR label Mar 19, 2021

neiljp reviewed Mar 20, 2021

View reviewed changes

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

neiljp removed the PR needs review PR requires feedback to proceed label Mar 20, 2021

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

zee-bit force-pushed the issue_779 branch 3 times, most recently from 13b0a91 to e32c9d1 Compare March 23, 2021 11:42

zulipbot 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 Mar 28, 2021

preetmishra reviewed May 26, 2021

View reviewed changes

preetmishra 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 May 27, 2021

zee-bit force-pushed the issue_779 branch from e32c9d1 to 40ee48b Compare June 10, 2021 22:48

zee-bit force-pushed the issue_779 branch from 40ee48b to f55257f Compare June 27, 2021 14:54

zee-bit added 2 commits July 4, 2021 21:44

zee-bit force-pushed the issue_779 branch from f55257f to 6fca74f Compare July 4, 2021 16:21

zulipbot added the has conflicts label Aug 30, 2021

neiljp force-pushed the main branch from f9f483a to 793e73d Compare December 15, 2021 21:30

neiljp modified the milestones: Next Release, Release after next May 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

boxes: Disable cycling using arrow keys if recipient is invalid. #864

boxes: Disable cycling using arrow keys if recipient is invalid. #864

zee-bit commented Jan 2, 2021 •

edited

Loading

neiljp commented Jan 16, 2021

preetmishra left a comment

zee-bit commented Jan 25, 2021

zee-bit commented Jan 25, 2021

neiljp left a comment

neiljp commented Feb 17, 2021

zee-bit commented Mar 14, 2021

prah23 left a comment •

edited

Loading

neiljp left a comment

zee-bit commented Mar 28, 2021

preetmishra left a comment

preetmishra May 26, 2021

zee-bit May 30, 2021

zee-bit Jul 4, 2021

preetmishra May 26, 2021

zee-bit Jul 4, 2021

zulipbot commented Aug 30, 2021

boxes: Disable cycling using arrow keys if recipient is invalid. #864

Are you sure you want to change the base?

boxes: Disable cycling using arrow keys if recipient is invalid. #864

Conversation

zee-bit commented Jan 2, 2021 • edited Loading

neiljp commented Jan 16, 2021

preetmishra left a comment

Choose a reason for hiding this comment

zee-bit commented Jan 25, 2021

zee-bit commented Jan 25, 2021

neiljp left a comment

Choose a reason for hiding this comment

neiljp commented Feb 17, 2021

zee-bit commented Mar 14, 2021

prah23 left a comment • edited Loading

Choose a reason for hiding this comment

neiljp left a comment

Choose a reason for hiding this comment

zee-bit commented Mar 28, 2021

preetmishra left a comment

Choose a reason for hiding this comment

preetmishra May 26, 2021

Choose a reason for hiding this comment

zee-bit May 30, 2021

Choose a reason for hiding this comment

zee-bit Jul 4, 2021

Choose a reason for hiding this comment

preetmishra May 26, 2021

Choose a reason for hiding this comment

zee-bit Jul 4, 2021

Choose a reason for hiding this comment

zulipbot commented Aug 30, 2021

zee-bit commented Jan 2, 2021 •

edited

Loading

prah23 left a comment •

edited

Loading