Add vim bindings to TerminalMenus

[jonas-schulze]

In addition to the current way to interact with a menu, this PR adds

• k to move up,
• j to move down,
• <space> as an alternative to <enter>.

[jonas-schulze]

While the menus behave as expected when using them directly (on macOS, in case some relevant character codes are different), the tests are broken. I didn't manage to write the proper data to stdin such that it would be recognized as <space> during the tests.

[jonas-schulze]

@ararslan, I need some help with this.

I'm pinging you because I think you might be a community manager of sorts. Sorry, if I'm wrong about that. I don't know who would be the right person to ask.

[ararslan]

Sorry, I'm not familiar with the TerminalMenus code so I'm afraid I can't be of much help here. A good place to start would be to rebase your branch on master to resolve the conflicts, then go from there.

[jonas-schulze]

I'm a fool. I didn't notice that request modifies the state stored in a menu, so I looked for the problem in the wrong place. A simple deepcopy(menu) did the trick.

However, not having seen this code for quite some time makes me want to refactor the tests. What do you think? Originally, I didn't want to touch every usage of simulate_input. Now, I think that would result in simpler code. The new usages would look like this:

multi_menu = MultiSelectMenu(string.(1:10), charset=:ascii)
@test simulate_input(deepcopy(multi_menu), keydict, :enter, :down, :enter, 'd') == Set([1,2])
@test simulate_input(deepcopy(multi_menu), vimdict, :enter, :down, :enter, 'd') == Set([1,2])

Instead of this:

# ...
@test simulate_input(Set([1,2]), multi_menu, :enter, :down, :enter, 'd')

Moving the expectation to the outside of the call makes checking errors easier. That's why I opted to have simulate_input return the actual outputs on failure as a non-boolean so that I can see them in the error messages at least.

[jonas-schulze]

Bump 🙂

[jonas-schulze]

@KristofferC bump 🙂

[fredrikekre]

LGTM and will be nice together with #38956

[timholy]

This is a pretty breaking change, for example for FoldingTrees which was already using the space bar to mean something different and now would have to change.

Since we haven't released 1.7 yet, should we revert this? It could be arranged as a configuration option given to the AbstractMenu option itself, but just grabbing extra keys to mean redundant things seems more intrusive than merited for a non-breaking release.