zulip-terminal icon indicating copy to clipboard operation
zulip-terminal copied to clipboard

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

Open zee-bit opened this issue 4 years ago • 7 comments

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.

zee-bit avatar Jan 02 '21 14:01 zee-bit

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

neiljp avatar Jan 16 '21 07:01 neiljp

@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 avatar Jan 25 '21 13:01 zee-bit

@zulipbot add "PR needs review"

zee-bit avatar Jan 25 '21 13:01 zee-bit

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

neiljp avatar Feb 17 '21 07:02 neiljp

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

zee-bit avatar Mar 14 '21 20:03 zee-bit

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

zee-bit avatar Mar 28 '21 07:03 zee-bit

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 avatar Aug 30 '21 23:08 zulipbot