Migrate mouse_event(s) to use keys_for_command. #539

preetmishra · 2020-03-11T17:01:49Z

This migrates mouse_event(s) and the associated tests to use keys_for_command.
I have split the commit into four commits for making it easier to review.

Fixes #533 partially.

zulipbot · 2020-03-11T17:01:55Z

Hello @preetmishra, it seems like you have referenced #533 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 #533..

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!

preetmishra · 2020-03-11T17:17:48Z

More on the implementation: I have used next(iter()) to get a key from the set returned by keys_for_command. Other options to do this were:

Using a for loop
Converting the set to a list and then indexing it.

While the for loop would have been the fastest, the implementation would have added 2 more lines and a variable per keys_for_command and the latter option is expensive.
next(iter()) comes after for loop in terms of cost.

I had to add GO_UP/DOWN presses for Streams/Topics/UsersView views. They already support navigation through alternative navigation keys (this is defined in View class) but since mouse_event directly calls its keypress method, the addition was necessary to make it work.

neiljp

@preetmishra This makes things clearer and better tested 👍 I've left some feedback inline, though not on every instance, as there is duplication of similar changes between the commits.

Don't worry about having different commits, and improving code as you go, in a commit before the 'new' change.

zulipterminal/ui_tools/views.py

neiljp · 2020-03-20T00:05:47Z

tests/ui/test_ui_tools.py

-        ("mouse press", 4, "up"),
-        ("mouse press", 5, "down"),
+        ("mouse press", 4, next(iter(keys_for_command('GO_UP')))),
+        ("mouse press", 5, next(iter(keys_for_command('GO_DOWN')))),


Since these calls (or the pop I recommend elsewhere) are non-deterministic, we should probably take a different testing approach - we want to know that one of the valid up or down keypress keys was used, but that any should work. We could maybe parametrize further, or take another approach.

I could only validate the first bit (one of the valid up or down keypress keys was used). Could you assist me with how to approach the second bit (but any should work)?

To clarify, I mean by 'any should work', that any key extracted using pop should work, so you should be able to work through those in a test parametrization, or another similar approach.

neiljp · 2020-03-20T00:06:57Z

zulipterminal/ui_tools/boxes.py

@@ -652,7 +652,7 @@ def mouse_event(self, size: urwid_Size, event: str, button: int,
                    col: int, row: int, focus: bool) -> bool:
        if event == 'mouse press':
            if button == 1:
-                self.keypress(size, 'enter')
+                self.keypress(size, next(iter(keys_for_command('ENTER'))))


You don't have a connected test change for this?

My bad, we didn't have any test for this to change. I have added a test now.

tests/ui/test_ui_tools.py

neiljp · 2020-03-20T00:18:04Z

zulipterminal/ui_tools/views.py

+        elif is_command_key('GO_UP', key):
+            key = 'up'
+        elif is_command_key('GO_DOWN', key):
+            key = 'down'


One aspect which makes me wonder if we should approach this differently is that scrolling with the extra keys (vim-like) already works in the stream-view, from the base View? So it would be simpler if we could avoid adding extra code and tests just for this case.

I can follow why you've added this, but this and the associated test are separate code which are needed before the actual mouse_event is changed - so if we do take this route, I think we should put this in a separate commit, at least for now.

I had to add GO_UP/DOWN presses for Streams/Topics/UsersView views. They already support navigation through alternative navigation keys (this is defined in View class) but since mouse_event directly calls its keypress method, the addition was necessary to make it work.

I think it is necessary to add these to make this work. I have separated these changes in another commit 381908c with their tests.

tests/ui/test_ui_tools.py

zulipterminal/ui_tools/views.py

preetmishra · 2020-03-21T18:36:28Z

@neiljp Thanks. I had to close and reopen the PR in order to restart the build check, it was failing with 'An error occurred while generating the build script.' for Install & pytest - Python 3 (OS X; xcode10) even though it was passing all the tests locally.

I have made my remarks in the respective inline-comments.

preetmishra · 2020-03-22T19:48:59Z

@neiljp I restructured the order of commits and made a few changes that eliminate code duplication.

The first commit has a test for validating that each call to keys_for_command returns the original data in a new set.
The second commit is a refactor commit which reduces duplication in mouse_event tests for Streams/UsersView.
The third commit introduces key_for_command in 'simple' mouse_event functions to remove hard-coded keys. For the tests, I have added a fixture as we have a bunch of similar tests in ui_tools.
The fourth introduces extra navigation keys to get mouse_event ready for keys_for_command and the fifth commit has code for introducing keys_for_command in 'non-trivial' mouse_event functions and tests.

neiljp

@preetmishra I've merged the first commit just now since it was separate and independent 👍

The others seem a little mixed-up, since you introduce some scroll-wheel changes, then replace them in another commit? The 'invalid' mouse events and 'left click' seem clearly separate changes deserving of their own commit(s). Would it be clearer to adjust the mouse_event code to use keys_for_command first? If you can extract the separate changes (extra tests, split-out tests) into independent tests, then the 'flow' of the dependent commits may be clearer?

tests/ui/test_ui_tools.py

neiljp · 2020-04-09T04:04:19Z

tests/ui/test_ui_tools.py

+    ids=lambda param: 'scroll_wheel_' + ('up' if param[1] == 4 else 'down')
+                      + '-key:{}-button:{}'.format(*param)
+)
+def mouse_press_key_button_pair(request):


This is specifically for a mouse scrolling action, right? If so, maybe adjust the name accordingly.

Did you keep this out of conftest.py for a specific reason? (not that all fixtures have to be in there)

Yes, I have renamed the fixture.

I kept this here as it was only being used in ui_tools.py. Thanks for pointing this out; I have moved this to conftest.py as we could need it in other files as well.

preetmishra · 2020-04-11T14:42:00Z

@neiljp Thanks for the review. I have restructured the commits, to better convey the flow, and incorporated the suggestions that you left in the review.

The first two commits are refactoring commits. The first one extracts mouse_event_invalid and
the second one reduces code duplication.
The third one adds support for j/k keypresses in other views (MessageView already has the
support). After which the fourth commit migrates the mouse_event functions that we've in
views.py altogether.
The last one introduces keys_for_command for left click in the MessageBox.

Hopefully, this will be clearer to review. :)

neiljp · 2020-04-19T10:58:53Z

@preetmishra I just merged the first and last commits, as these are independent, with a few small changes. I held off on the second as it might be related to the others? In any case, clearer to review these related commits together.

preetmishra · 2020-04-19T15:32:17Z

@neiljp I have resolved the conflicts and rebased against master. Thanks for the refinements. 👍

This introduces parametrize calls in the mouse_event tests for StreamsView and UsersView to reduce code duplication.

The Streams/Topics/UsersView already support navigation using other navigation keys through View but this extension is necessary to get mouse_events ready for using keys_for_command. This also introduces a fixture, mouse_event_navigation_key_expected_key_pair, in conftest, to generate test cases for the keypress tests. Tests added.

* Updated mouse_event functions in MessageView, StreamsView, TopicsView and UsersView to use keys_for_command to ensure that they are future-proof. * Added a fixture, mouse_scroll_key_button_pair, to generate test cases for mouse_event tests. Test amended for MessageView, StreamsView and UsersView and added for TopicsView.

preetmishra · 2020-06-15T14:29:49Z

Updated to resolve conflicts.

zulipbot · 2021-01-30T20:34:52Z

Heads up @preetmishra, 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.

Ezio-Sarthak · 2021-03-18T13:23:34Z

This PR might close tracking issue #533. Are you planning to update/rebase this in near future?

preetmishra · 2021-03-30T15:04:40Z

Hi, @Ezio-Sarthak. Thanks for taking an interest! Feel free to take this further.

neiljp · 2021-07-15T02:20:54Z

@preetmishra #953 appears to cover the first commit here, and replaces the last commit; I'm not sure about the middle commit?

It'd be good to close this out, so if we don't close outright, perhaps @zee-bit may want to take this on given previous work, or @Ezio-Sarthak ?

zee-bit · 2021-07-15T07:09:47Z

@neiljp The middle commit here looks unnecessary after #953. If I understand correctly, it handles 'j/k' keys for scrolling because keys_for_command includes those along with the directional keys (up/down etc), so adding that middle commit before migrating to keys_for_command makes sense.
But, in #953, we don't use keys_for_command, but rather primary_keys_for_command, which returns only the directional keys, so there's no need to handle other navigational keys such as j/k.

Perhaps @preetmishra can confirm/validate my above hypothesis before closing this?

preetmishra · 2021-07-19T14:52:20Z

@neiljp As @zee-bit mentions, this was needed earlier as I was relying on keys_for_command to add the support. However, using primary_keys_for_command makes more sense and helps avoid unnecessary changes.

Thanks once again, @zee-bit, for taking the PR further. I am closing this now.

zulipbot added the size: XL [Automatic label added by zulipbot] label Mar 11, 2020

preetmishra mentioned this pull request Mar 11, 2020

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

Closed

2 tasks

neiljp added the PR needs review PR requires feedback to proceed label Mar 11, 2020

neiljp reviewed Mar 20, 2020

View reviewed changes

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

preetmishra changed the title ~~Migrate mouse_event(s) to use keys_for_command.~~ [WIP] Migrate mouse_event(s) to use keys_for_command. Mar 20, 2020

preetmishra force-pushed the migrate-mouse-tests branch from ed4df89 to 9f8d739 Compare March 21, 2020 17:39

preetmishra changed the title ~~[WIP] Migrate mouse_event(s) to use keys_for_command.~~ Migrate mouse_event(s) to use keys_for_command. Mar 21, 2020

preetmishra force-pushed the migrate-mouse-tests branch from 9f8d739 to 5b218a9 Compare March 21, 2020 17:50

preetmishra closed this Mar 21, 2020

preetmishra reopened this Mar 21, 2020

neiljp added the PR needs review PR requires feedback to proceed label Mar 22, 2020

preetmishra force-pushed the migrate-mouse-tests branch 2 times, most recently from 0060d6e to 86a260a Compare March 22, 2020 19:25

preetmishra mentioned this pull request Mar 23, 2020

views: Support 'j/k', 'J/K' and 'G/end' for navigation in popups. #524

Closed

preetmishra force-pushed the migrate-mouse-tests branch from 86a260a to 547490b Compare March 24, 2020 15:55

neiljp reviewed Apr 9, 2020

View reviewed changes

neiljp removed the PR needs review PR requires feedback to proceed label Apr 9, 2020

preetmishra changed the title ~~Migrate mouse_event(s) to use keys_for_command.~~ [WIP] Migrate mouse_event(s) to use keys_for_command. Apr 9, 2020

preetmishra force-pushed the migrate-mouse-tests branch 2 times, most recently from bb2339e to 7d39d46 Compare April 11, 2020 14:23

preetmishra changed the title ~~[WIP] Migrate mouse_event(s) to use keys_for_command.~~ Migrate mouse_event(s) to use keys_for_command. Apr 11, 2020

zulipbot added the has conflicts label Apr 19, 2020

preetmishra force-pushed the migrate-mouse-tests branch from 7d39d46 to 5e5cb4f Compare April 19, 2020 15:27

zulipbot removed the has conflicts label Apr 19, 2020

neiljp added the PR needs review PR requires feedback to proceed label May 8, 2020

zulipbot added the has conflicts label May 13, 2020

preetmishra added 3 commits June 15, 2020 19:52

refactor: tests: ui_tools: Reduce duplication in mouse_event tests.

7ab82c5

This introduces parametrize calls in the mouse_event tests for StreamsView and UsersView to reduce code duplication.

preetmishra force-pushed the migrate-mouse-tests branch from 5e5cb4f to 1860452 Compare June 15, 2020 14:29

zulipbot removed the has conflicts label Jun 15, 2020

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

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

zulipbot added the has conflicts label Jan 30, 2021

zee-bit mentioned this pull request Apr 1, 2021

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

Merged

neiljp removed this from the Next Release milestone Jul 15, 2021

preetmishra closed this Jul 19, 2021

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate mouse_event(s) to use keys_for_command. #539

Migrate mouse_event(s) to use keys_for_command. #539

preetmishra commented Mar 11, 2020

zulipbot commented Mar 11, 2020

preetmishra commented Mar 11, 2020

neiljp left a comment

neiljp Mar 20, 2020

preetmishra Mar 21, 2020

neiljp Mar 21, 2020

neiljp Mar 20, 2020

preetmishra Mar 21, 2020

neiljp Mar 20, 2020

preetmishra Mar 21, 2020

preetmishra commented Mar 21, 2020

preetmishra commented Mar 22, 2020

neiljp left a comment

neiljp Apr 9, 2020

preetmishra Apr 11, 2020 •

edited

Loading

preetmishra commented Apr 11, 2020

neiljp commented Apr 19, 2020

preetmishra commented Apr 19, 2020

preetmishra commented Jun 15, 2020

zulipbot commented Jan 30, 2021

Ezio-Sarthak commented Mar 18, 2021

preetmishra commented Mar 30, 2021

neiljp commented Jul 15, 2021

zee-bit commented Jul 15, 2021

preetmishra commented Jul 19, 2021

Migrate mouse_event(s) to use keys_for_command. #539

Migrate mouse_event(s) to use keys_for_command. #539

Conversation

preetmishra commented Mar 11, 2020

zulipbot commented Mar 11, 2020

preetmishra commented Mar 11, 2020

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

preetmishra commented Mar 21, 2020

preetmishra commented Mar 22, 2020

neiljp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

preetmishra Apr 11, 2020 • edited Loading

Choose a reason for hiding this comment

preetmishra commented Apr 11, 2020

neiljp commented Apr 19, 2020

preetmishra commented Apr 19, 2020

preetmishra commented Jun 15, 2020

zulipbot commented Jan 30, 2021

Ezio-Sarthak commented Mar 18, 2021

preetmishra commented Mar 30, 2021

neiljp commented Jul 15, 2021

zee-bit commented Jul 15, 2021

preetmishra commented Jul 19, 2021

preetmishra Apr 11, 2020 •

edited

Loading