Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add vim bindings to TerminalMenus #37940

Merged
merged 1 commit into from
Apr 13, 2021

Conversation

jonas-schulze
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

@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
Copy link
Member

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.

@ararslan ararslan added the REPL Julia's REPL (Read Eval Print Loop) label Jan 29, 2021
* `k` to move up
* `j` to move down
* `<space>` as an alternative to `<enter>`
@jonas-schulze
Copy link
Contributor Author

jonas-schulze commented Jan 30, 2021

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
Copy link
Contributor Author

Bump 🙂

@jonas-schulze
Copy link
Contributor Author

@KristofferC bump 🙂

@fredrikekre
Copy link
Member

LGTM and will be nice together with #38956

@fredrikekre fredrikekre merged commit 4a19b75 into JuliaLang:master Apr 13, 2021
@jonas-schulze jonas-schulze deleted the js/vim-bindings branch April 13, 2021 11:56
ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021
* `k` to move up
* `j` to move down
* `<space>` as an alternative to `<enter>`
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this pull request May 9, 2021
* `k` to move up
* `j` to move down
* `<space>` as an alternative to `<enter>`
johanmon pushed a commit to johanmon/julia that referenced this pull request Jul 5, 2021
* `k` to move up
* `j` to move down
* `<space>` as an alternative to `<enter>`
@timholy
Copy link
Member

timholy commented Aug 6, 2021

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.

@timholy timholy modified the milestone: 1.7 Aug 6, 2021
timholy added a commit that referenced this pull request Aug 9, 2021
vchuravy added a commit that referenced this pull request Aug 11, 2021
Revert "Add vim bindings to TerminalMenus (#37940)"
KristofferC pushed a commit that referenced this pull request Aug 25, 2021
This reverts commit 4a19b75.
Closes #41799.

(cherry picked from commit 702cf55)
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
REPL Julia's REPL (Read Eval Print Loop)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants