refactor/bugfix: Migrate keypress to using commands instead of hardcoded keys. #953

zee-bit · 2021-03-18T11:49:49Z

This PR aims to replace all hard-coded keys in keypress to use their corresponding commands, to make them future-proof.
Commit flow:

The first commit is borrowed from @preetmishra's Migrate mouse_event(s) to use keys_for_command. #539, which parametrizes mouse_event calls and adds mouse_event test for TopicsView.
The second commit migrates all instances of hard-coded keys in mouse_events to use primary_key_for_commands.
The third commit replaces all other instances of hard-coded keys in keypress to use their corresponding commands so that they are future-proof.
The fourth commit is a bugfix that makes r for replying to message independent of ENTER key.

Our codebase would have no instances of hard-coded keys now. Can be checked using: git grep keypress\(size

Fixes #469 and #533.

zee-bit · 2021-03-29T16:05:58Z

@zulipbot add "PR needs review"

zee-bit · 2021-06-26T16:30:09Z

Not sure if I should call the 4th commit a bugfix, since there is no bug currently but there might be if we tried to customize the keys, which sounds like an (indirect) bug??

neiljp

@zee-bit I had this half-reviewed before I was away but apparently didn't send it, so here's some feedback - this will tidy up the last of the keypress refactoring 👍

I think my only concern here is potential interactions with how we're handling standard movements that urwid expects, which @Rohitth007 is looking into with the urwid command-map. We can discuss and check again before merging in any case.

neiljp · 2021-06-03T20:58:46Z

tests/ui/test_ui_tools.py

+            "scroll_wheel_down",
+        ],
+    )
+    def test_mouse_event(self, mocker, topic_view, button, key, widget_size):


We could add this in a separate commit - otherwise this isn't a pure refactor. Will that leave it the same as Preet's original commit?

Yes, adding that in a separate commit would leave it the same as Preet's commit. Do you want me to put that in a separate commit or just reword the commit message to not indicate it's a pure refactor?

Ok, added it as a separate commit. :)

tests/ui/test_ui_tools.py

zulipterminal/ui_tools/views.py

neiljp · 2021-06-03T21:21:19Z

zulipterminal/ui_tools/views.py

                for _ in range(5):
-                    self.keypress(size, "up")
+                    self.keypress(size, primary_key_for_command("GO_UP"))
                return True
            elif button == 5:
                for _ in range(5):
-                    self.keypress(size, "down")
+                    self.keypress(size, primary_key_for_command("GO_DOWN"))


I agree with this refactor, but while mouse access is not a priority the 5x multiplier isn't consistent with the other boxes. I'd like this to be consistent, but equally I don't use a scroll-wheel mouse right now, so we can discuss in the stream.

I think this might have been brought up recently in discussion, but I'm not sure we reached a conclusion?

neiljp · 2021-06-03T21:35:44Z

zulipterminal/ui_tools/boxes.py

@@ -1516,15 +1516,15 @@ def keypress(self, size: urwid_Size, key: str) -> Optional[str]:
                recipient_user_ids=[self.message["sender_id"]],
            )
        elif is_command_key("MENTION_REPLY", key):
-            self.keypress(size, "enter")
+            self.keypress(size, primary_key_for_command("REPLY_MESSAGE"))


These changes in this file likely work in practice, but mainly since we going through the intermediary of a str. At least, that's my perception of what's happening - can you see what I mean? We may want to make another change in this file to give these changes proper meaning.

I am really sorry, but I don't get this. Could you please explain a bit more?

This actually was broken at this commit - there was no REPLY_MESSAGE handler for this and manual testing confirmed issues. Compare the difference between your local version and the version I pushed.

neiljp · 2021-07-05T00:38:14Z

zulipterminal/ui_tools/views.py

        elif is_command_key("REPLY_MESSAGE", key):
-            self.body.keypress(size, "enter")
+            self.body.keypress(size, key)


I assume this is the start point you're referring to in the commit text?
So REPLY_MESSAGE here -> "enter" on box -> behavior like REPLY_MESSAGE via "ENTER" filter ?

This is a good find, but the commit text could be improved to make this clearer - what file/class source triggers what other ones?

Thanks for updating this, but as noted I adjusted code between the last two commits to make this a little more accurate.

Rohitth007 · 2021-07-06T05:05:00Z

I don't think these changes would have interfere with the keypress PR. It's just that some of these keypress calls are no longer needed. I could rebase on top of these changes.

This adds a fixture that generates the required parameters for mouse scroll actions. We return the type of event i.e. mouse press, along with the button and the expected key to be triggered for this mouse_event. Introducing the mouse_scroll_event fixture in mouse_event tests for several *Views classes reduces code duplication - essentially extracting duplicated test code and parametrizes into one fixture, making the tests more compact and readable. Co-authored-by: Preet Mishra <ipreetmishra@gmail.com>

Adds missing mouse_event test in TestTopicView class to increase our scope of testing and maintain consistency with other similar classes.

This commit migrates mouse_event functions to use primary_key_for_commands instead of hard-coded keys to ensure that they are resistant to breakage if navigational keys are updated in future. Tests amended.

The 'r' key that is used to reply to a message was actually depending on 'enter' key for its functionality. When replying to a message, the keypress is first intercepted by MiddleColumnView as part of REPLY_MESSAGE command, which is passed to the corresponding MessageBox as "enter" keypress. The MessageBox grabs this key and executes message reply but via the ENTER filter. This commit makes 'r' key for replying to message independent of 'enter' by passing the command 'REPLY_MESSAGE' instead of the hardcoded 'enter' key to MessageBox. Tests amended. Balance of code between commits adjusted slightly for correctness by neiljp.

This commit replaces all instances of hard-coded keys passed to keypress that posed a risk of breaking tests if in future we tried to customize the keys. Instead of passing keys we pass their corresponding commands that is more flexible and resistant to breakage on changing keys. Tests amended.

zulipbot · 2021-07-15T01:57:25Z

Hello @zee-bit, it seems like you have referenced #469 in your pull request description, but you have not referenced them in your commit message description(s). When you reference an issue in a commit message, it automatically closes the corresponding issue when the commit is merged.

Please run git commit --amend in your command line client to amend your commit message description with Fixes #469..

An example of a correctly-formatted commit:

commit fabd5e450374c8dde65ec35f02140383940fe146
Author: zulipbot
Date:   Sat Mar 18 13:42:40 2017 -0700

    pull requests: Check PR commits reference when issue is referenced.

    Fixes #51.

Thank you for your contributions to Zulip!

neiljp · 2021-07-15T02:08:57Z

@zee-bit This was essentially in a mergeable state except for a few points - I squashed the first two commits together (they make a refactor together), moved & slightly adjusted some code between the last two commits for correctness, and reordered them so that the "replace hardcoded key" statement remains correct - now to merge 🎉

zulipbot · 2021-07-15T02:27:02Z

Hello @zulip/server-refactoring members, this pull request was labeled with the "area: refactoring" label, so you may want to check it out!

zulipbot added the size: L [Automatic label added by zulipbot] label Mar 18, 2021

zulipbot added the PR needs review PR requires feedback to proceed label Mar 29, 2021

zee-bit force-pushed the fix-hardcoded-keys branch from 5c3c996 to 6166a46 Compare April 1, 2021 14:09

zulipbot added size: XL [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] labels Apr 1, 2021

zee-bit mentioned this pull request Apr 1, 2021

TRACKING: Migrate tests using hard-coded keys to use keys_for_command. #533

Closed

2 tasks

zee-bit force-pushed the fix-hardcoded-keys branch from 6166a46 to 9b40ecb Compare May 17, 2021 22:02

zee-bit force-pushed the fix-hardcoded-keys branch from 9b40ecb to 519be4d Compare June 2, 2021 21:35

zee-bit force-pushed the fix-hardcoded-keys branch 2 times, most recently from a72b18e to a0d2a70 Compare June 25, 2021 21:21

zee-bit force-pushed the fix-hardcoded-keys branch from a0d2a70 to 6aebb49 Compare July 4, 2021 07:25

neiljp reviewed Jul 5, 2021

View reviewed changes

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 Jul 5, 2021

zee-bit force-pushed the fix-hardcoded-keys branch from 6aebb49 to 368f03e Compare July 7, 2021 23:10

zee-bit 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 Jul 7, 2021

zee-bit force-pushed the fix-hardcoded-keys branch from 368f03e to 7058835 Compare July 8, 2021 14:15

zee-bit and others added 5 commits July 14, 2021 17:26

tests: ui_tools: Add mouse_event test for TopicsView.

7f29955

Adds missing mouse_event test in TestTopicView class to increase our scope of testing and maintain consistency with other similar classes.

refactor: views: Migrate mouse_events to use primary_key_for_command.

858bf4c

This commit migrates mouse_event functions to use primary_key_for_commands instead of hard-coded keys to ensure that they are resistant to breakage if navigational keys are updated in future. Tests amended.

neiljp force-pushed the fix-hardcoded-keys branch from 7058835 to b810ef7 Compare July 15, 2021 01:57

neiljp merged commit 9212920 into zulip:main Jul 15, 2021

neiljp mentioned this pull request Jul 15, 2021

Migrate mouse_event(s) to use keys_for_command. #539

Closed

neiljp added this to the Next Release milestone Jul 15, 2021

neiljp added area: refactoring area: tests and removed PR needs review PR requires feedback to proceed labels Jul 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor/bugfix: Migrate keypress to using commands instead of hardcoded keys. #953

refactor/bugfix: Migrate keypress to using commands instead of hardcoded keys. #953

zee-bit commented Mar 18, 2021 •

edited

Loading

zee-bit commented Mar 29, 2021

zee-bit commented Jun 26, 2021 •

edited

Loading

neiljp left a comment

neiljp Jun 3, 2021

zee-bit Jul 6, 2021

zee-bit Jul 8, 2021

neiljp Jun 3, 2021

neiljp Jun 3, 2021

zee-bit Jul 7, 2021

neiljp Jul 15, 2021

neiljp Jul 5, 2021

neiljp Jul 15, 2021

Rohitth007 commented Jul 6, 2021 •

edited

Loading

zulipbot commented Jul 15, 2021

neiljp commented Jul 15, 2021

zulipbot commented Jul 15, 2021

refactor/bugfix: Migrate keypress to using commands instead of hardcoded keys. #953

refactor/bugfix: Migrate keypress to using commands instead of hardcoded keys. #953

Conversation

zee-bit commented Mar 18, 2021 • edited Loading

zee-bit commented Mar 29, 2021

zee-bit commented Jun 26, 2021 • edited Loading

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

Rohitth007 commented Jul 6, 2021 • edited Loading

zulipbot commented Jul 15, 2021

neiljp commented Jul 15, 2021

zulipbot commented Jul 15, 2021

zee-bit commented Mar 18, 2021 •

edited

Loading

zee-bit commented Jun 26, 2021 •

edited

Loading

Rohitth007 commented Jul 6, 2021 •

edited

Loading