zulip-terminal
zulip-terminal copied to clipboard
zulip
Improve handling Esc during message compose
What does this PR do, and why?
Uses the existing confirmation popup to confirm whether user wants to exit the compose box upon pressing Esc, Enabling the user to go back and save their draft from compose.
Outstanding aspect(s)
- [ ]
External discussion & connections
- [x] Discussed in #zulip-terminal in
Improve handling Esc during msg compose #T1342 #T1362 #T1442 - [x] Fully fixes #1342
- [ ] Partially fixes issue #
- [x] Builds upon previous unmerged work in PR #1362
- [ ] Is a follow-up to work in PR #
- [ ] Requires merge of PR #
- [ ] Merge will enable work on #
How did you test this?
- [x] Manually - Behavioral changes
- [x] Manually - Visual changes
- [ ] Adapting existing automated tests
- [ ] Adding automated tests for new behavior (or missing tests)
- [ ] Existing automated tests should already cover this (only a refactor of tested code)
Self-review checklist for each commit
- [x] It is a minimal coherent idea
- [ ] It has a commit summary following the documented style (title & body)
- [ ] It has a commit summary describing the motivation and reasoning for the change
- [ ] It individually passes linting and tests
- [ ] It contains test additions for any new behavior
- [ ] It flows clearly from a previous branch commit, and/or prepares for the next commit
Visual changes
Hello @zulip/server-hotkeys members, this pull request was labeled with the "area: hotkeys" label, so you may want to check it out!
Updated this PR. Currently it uses 15 character length as the threshold to determine whether to trigger the popup or not.
@neiljp Thanks for the review :)
I'd need to examine the code again, but wonder if we should adjust the existing tests to only assert on the new function being called, and have a separate test for the new function which defines that behavior. Then changes to the internals which that new function may need to address won't need to be updated in these tests. If so, that would be a good refactor commit.
I'm not too sure if I understood your comment correctly but from what I've understood, we would only need to check for whether exit_compose_confirmation_popup is called and then make another test for the behavior when that function is called?
If I understood correctly, what about the case where the popup isn't activated? Shouldn't we retain the current assertions to cover that case as well?
assert write_box.to_write_box is None
assert write_box.msg_write_box.edit_text == ""
assert write_box.compose_box_status == "closed"
Updated this PR :)
@zulipbot remove "PR awaiting update" @zulipbot add "PR needs review"
This implementation LGTM. Those are some interesting edge cases indeed. Added some of my thoughts and questions regarding the decisions behind the implementation in the CZO topic. I've got to go apply that previous feedback about the splitting of tests to several of my commits :)
@neiljp @mounilKshah, does this require any additional changes? I have self-reviewed the code again and @Niloth-p had buddy-reviewed this before. I can't seem to find anything that still needs to be addressed, apart from handling the exit when in editing mode (which I will address in another PR).
@rsashank Other than resolving the expected conflicts you expected with the other PR, this looks essentially good to go except for a few tiny nits from previous comments :+1: (that other PR looks like it'll help the readability of this code too)
I did notice when I was manually testing to confirm the autocomplete aspect we discussed, that the recovery of the autocomplete after cancelling exit of compose doesn't appear to quite work properly - the footer is present, and we're presumably still 'in autocomplete mode' via that bool, but it has '[No matches found]'? I'm not sure how easy that is to resolve at this point?
If you can resolve that fairly easily, that'd be great, but if not then I'd suggest adding a call to that helper function with a comment regarding this problem, so we can simply reset it for now but it's noted in the code for future reference.
It'll be useful to have this merged - I lost a composition just recently...!
@rsashank Other than resolving the expected conflicts you expected with the other PR, this looks essentially good to go except for a few tiny nits from previous comments 👍 (that other PR looks like it'll help the readability of this code too)
I did notice when I was manually testing to confirm the autocomplete aspect we discussed, that the recovery of the autocomplete after cancelling exit of compose doesn't appear to quite work properly - the footer is present, and we're presumably still 'in autocomplete mode' via that bool, but it has '[No matches found]'? I'm not sure how easy that is to resolve at this point?
If you can resolve that fairly easily, that'd be great, but if not then I'd suggest adding a call to that helper function with a comment regarding this problem, so we can simply reset it for now but it's noted in the code for future reference.
It'll be useful to have this merged - I lost a composition just recently...!
In that case, could we maybe have this merged? I'll look into this after I tidy the polls and spoilers PR. I'll address this in a separate PR.
Or should we wait on fixing this before merging this PR?
@rsashank Thanks for keeping this work going - this is going to save many lost compositions, I suspect! 👍
I just pushed back here with a removal of the EXIT_COMPOSE branch near the top of the keypress() method, and a corresponding comment with TODO there, matching my last note about autocomplete not yet resuming correctly.
You noted the early push I made had a test issue - that was due to the footer being reset twice in some cases, which I don't think will be an issue, but I've added a comment in any case, as well as migrating that change across the commits.
I did want to get the follow-up points noted, but I'm going to merge this first :tada: