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

[preetmishra]

This PR is a follow-up to #518. This will generalize navigation and make the user interface consistent.
@neiljp

[neiljp]

@preetmishra I can see the approach you have here working, but there's quite a bit of duplication; have you considered an inheritance approach to this?

[preetmishra]

@neiljp Thanks for the review. I agree that there's duplication but there are two seperate views (Stream and Message). One possibility I can think of is making a base test class for popups, would that be fine?

[neiljp]

@preetmishra Right, it would seem straightforward to do that, maybe an InfoView? (based on the current class names)

I would keep this code around, but this other approach may be simpler.

[preetmishra]

Yes, that seems plausible. I'll start working on it.

[preetmishra]

@neiljp I have added an InfoView which encapsulates the basic tests for any popup and leveraged it to simplify TestStream/MsgInfoView tests.

I have also added another commit to refactor TestHelpMenu to leverage InfoView.

[neiljp]

@preetmishra Sorry if I wasn't clear; I was meaning refactoring the code to use another class with the common features, not the tests :)

[preetmishra]

@neiljp Thanks. I have added InfoView with the common method and inherited it in the other Stream/MsgInfoView.

The second commit is a suggested refactor of HelpView to leverage InfoView class.

[neiljp]

@preetmishra Note that the refactor commit is not quite what I would expect - it makes the help menu still work, with new code, but not within just that one commit, since the new code is in another commit.

A structure that might be clearer, if in two commits, would be to extract the functionality into the base class from the existing class (the refactor), and then use the newly-available base class in the other cases (the feature). Try comparing the two different sequences of commits and let me know what you think.

[preetmishra]

@neiljp Thanks for the review. The sequence you suggested is indeed much cleaner than what I had proposed previously. I have pushed the new changes.

Also, I renamed InfoView to PopUpView as it seemed more consistent with HelpView and Stream/MsgInfoView (InfoView seemed only limited to infoviews).

[neiljp]

@preetmishra This is really close, but since we're introducing the PopUpView (better name 👍), let's get it as good as we can first time.

My other comment re fixtures is something we could address in this PR, but also is fine elsewhere.

[preetmishra]

@neiljp I have added another refactor commit to introduce a fixture which will help in reducing code duplication. I think it is apt to introduce it here before the 'new' feature since they are going to use it in keypress_navigation tests. However, the commit heading already got too long with 'tests', 'areas' and 'refactor' that I had to settle with a short heading that is not as descriptive.

For the rest of the two commits, I have made the changes that were suggested. I have also renamed help_menu_content to log in HelpView to make it consistent with Msg/StreamInfoView.

[neiljp]

@preetmishra This is looking great, the fixture makes the (test) code smaller 👍

Except for the name issues and perhaps a test for the new class, I would say this is ready!

[preetmishra]

@neiljp Thanks. I have added a test for PopUpView's init method as its keypress is being tested in the derived classes anyway. I have also renamed the variables as per your suggestions.

[neiljp]

I understand the keypress is being tested in the derived classes, but the derived classes may change behavior later; I'm not sure this is much overhead or a challenge? :)

[preetmishra]

@neiljp My bad, I didn't consider the fact that the derived classes may change the behavior later. Thanks. I have added the tests for different keypress options as well.

Other than that, I have added another refactor commit to change super_view to super_keypress
in HelpView's test as the latter better defines what the variable holds.

Also, the commit order that GitHub is showing isn't what I had locally. The order that I pushed was 4 -> 3 -> 2 -> 1.

[neiljp]

@preetmishra I know you've been working on this quite a lot for something small, but we're uncovering lots of things and new test code 👍

Inline feedback is all test-related, except for a note about including a change in behavior for the enhanced navigation commit (the original feature!).

I appreciate the initiative and it does clarify the tests in my view, but I think we should keep the size parameters in the test function body for now. We can perhaps do a sweep of this after some discussion, as this is a format used in many places in the tests, so would be good to change uniformly.

[preetmishra]

@neiljp Thanks. I'll revert the size variable back in the tests. I also did this in #539 as per your suggestion to use default parameter as a compromise for size variable, I'll make these changes there as well.

[preetmishra]

@neiljp Thanks. I have made the amendments in the test as per your suggestions. I also rewrote the commit message to include the Walker usage change. It is ready for review.

[neiljp]

@preetmishra Thanks for this! I just merged it after some very minor tweaks 🎉

There were no code changes, I just moved one import from one commit to another, as you'd introduced an import in an earlier commit than it was required.

[preetmishra]

Thanks so much! 🎉