Add support for navigation using keys 'j' and 'k' in help popup menu. #518
Conversation
There was a problem hiding this comment.
@preetmishra This was a good catch, and both commits expose some changes that are needed 👍
I've left a few comments inline.
With only a little extra work this should improve the generality of the navigation and apply nicely to the help menu, but note that we might want to apply this to other popups, such as message-info. That could a good follow-up.
preetmishra
force-pushed
the
fix-help
branch
from
18a5aea to
469978a
Compare
zulipbot
added
size: M
|
@neiljp, Thanks for the review. I agree we should make the navigation consistent throughout the user interface which includes adding support for navigation using 'j/k' and 'J/K' in other popups as well. I will add a follow-up PR regarding this. I have made the requested changes which include updating labels and description for the suggested keys and added support for other movement keys ('J/K'). It is ready for review. |
There was a problem hiding this comment.
@preetmishra This looks good, but I think we should have the response to navigation keys work the same in the Help menu as elsewhere, so the scrolling keys should work differently, and also add G? This actually is a regression on the previous behavior, since the page-up/down keys don't scroll, but only go-up/down.
You could add some tests here for the help view, to ensure this continues to work for all the keys.
I don't think this is far from merging :)
preetmishra
force-pushed
the
fix-help
branch
from
469978a to
20ad772
Compare
zulipbot
added
size: L
|
@neiljp, I have updated the navigation keys and added the tests. However, I have not written much test code so I am not sure if the proposed implementation is what we are looking for. It'd be helpful to have your insight on this to improve it further. |
There was a problem hiding this comment.
@preetmishra This looks ready to merge, except for the tests; I have a solution which I can substitute, but you are of course welcome to work on this by yourself if you wish. Let me know :)
I left some feedback on the test in any case.
tests/ui/test_ui_tools.py
Outdated
| def test_keypress_navigation(self, key): | ||
| size = (200, 20) | ||
| self.controller.keypress(size, key) | ||
| assert self.controller.keypress.called |
There was a problem hiding this comment.
As written, this test is always going to pass. self.controller is set to be a Mock in the first method, as the way these are currently set up (due to autouse=True) that method is a little like an __init__ for each test call. So the assert is testing that an explicit function call you made on a mock was actually made...which it was, in the test itself :)
You likely want self.help_view instead of self.controller on line 1110. This is the object/function you're actually testing, and when reading tests you'll maybe see some kind of setup/mocking, always a function call of the thing that's being tested, and then the assert type checks which specify what the end result should be, given the setup and call.
You'd also want to re-think the form of the parametrize here and use keys_for_command to generate lists of tuples, rather than hard-coding in all the keys in the parametrize - in particular since the assert code really needs to consider the 'input' key and the resulting key passed to urwid, which are not always the same.
|
@neiljp, Thanks for the feedback that you have provided. I have updated the tests based on that. I have added |
There was a problem hiding this comment.
@preetmishra This test is much more meaningful and very close to ready 🎉
My inline test feedback is mostly minor - you might find it useful to experiment with how to generate the test list/iterator in a different way, though I'm more concerned with what is being tested.
Other than the test, consider checking your git text:
- are the files all relevant?
- reduce any duplication in the summaries?
- consider using gitlint (see the README)
Once the tests are finalized, I would squash them with the related commit, as I mentioned previously.
tests/ui/test_ui_tools.py
Outdated
| size = (200, 20) | ||
| for key in keys: | ||
| return_value = self.help_view.keypress(size, key) | ||
| assert return_value == expected_key |
There was a problem hiding this comment.
I can see you follow more what is happening now :) To see the details, you might try exploring the --lf, -k, -s and -v options of pytest, among others, if you haven't already. We could put a link to this kind of information in the README.
For example, pytest -k press_navig -v might be informative here. If you have debug print() calls in the code then -s can also help with debugging.
The two potential issues I would point out here are:
- While having a loop in a particular test is reasonable (and maybe faster), the indication of which specific case fails is not indicated by the parameters, only the assert. The alternative is to generate combinations which form the list/iterator of parameters. This minimizes the test code itself, albeit against potential complexity in the parameters.
- Is the return value from
superguaranteed to be the same as the key passed in? It seems to be, but while this ties to the specific implementation a little, my inclination was to test against the call to the keypress on the super class instead, unless we can find specific documentation which implies this.
There was a problem hiding this comment.
Thanks for the tip. Adding a link explaining pytest's options in the README would be helpful.
I have used list comprehension to generate parameters and removed the loop from the test code. Regarding the latter point, I have pushed an implementation without the call to the keypress method on the super class for now as urwid's implementation suggests that the value returned by the super's keypress function is the key which was passed to it.
Urwid's link: https://github.com/urwid/urwid/blob/0b3bd90b2482e2d528aa37608304783ccadb43e2/urwid/listbox.py#L934
However, if this doesn't seem convincing, I have posted another implementation in #zulip-terminal.
Conversation link: https://chat.zulip.org/#narrow/stream/206-zulip-terminal/topic/.5Bhelp.20popup.5D.20Navigation.20using.20'j'.20and.20'k'.2E/near/825457
The current labels and descriptions for the following keys are not completely accurate: * Keys 'PREVIOUS/NEXT_MESSAGE' not only navigate between messages but also work for navigation in general. * Keys 'SCROLL_TO_TOP/BOTTOM' don't go to the top/bottom but scroll. * Key 'END_MESSAGE' not only goes to the last message but also to the bottom. Updated the labels and descriptions (help_text) for the mentioned keys to better capture their functionality.
This makes navigation more consistent throughout the user interface. Fixes zulip#517.
|
@neiljp, Thanks. I have rewritten the commit messages and removed irrelevant files from the commit message. Gitlint is pretty handy. I have also amended the tests based on your input, more on that is here: #518 (comment). |
|
@neiljp, I have amended the test to mock the super class and check how its |
|
@preetmishra I just merged this - thanks for seeing this through to the end! 🎉 I made a few very minor adjustments to the commit text and squashed the last two commits together as I indicated previously. The commits in master are be801c7 and the preceding one. It was good to see an alternative solution for the test in the end - I had used |
|
Thanks, I got to learn more about the pytest library while working on this one. I enjoyed it. |
@neiljp, I have added two separate commits.
Fixes #517.