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

Make vim bindings opt-in #41834

Closed
wants to merge 2 commits into from

Conversation

jonas-schulze
Copy link
Contributor

@jonas-schulze jonas-schulze commented Aug 9, 2021

as an alternative to #41799.

If accepted, the second commit should be backported to v1.7.

@jonas-schulze

This comment has been minimized.

@vchuravy vchuravy added this to the 1.7 milestone Aug 9, 2021
and make `simulate_input` more robust against menus not supporting vim
bindings.
@jonas-schulze jonas-schulze marked this pull request as ready for review August 9, 2021 11:33
@jonas-schulze
Copy link
Contributor Author

This should be good now. #41834 (comment) remains relevant, though.

Copy link
Member

@timholy timholy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to fix it this way, we'll have to backport #41576 as well. The safest option is to revert from 1.7 and do all this in 1.8. Thoughts?

stdlib/REPL/src/TerminalMenus/RadioMenu.jl Outdated Show resolved Hide resolved
@vchuravy
Copy link
Member

vchuravy commented Aug 9, 2021

If you want to fix it this way, we'll have to backport #41576 as well. The safest option is to revert from 1.7 and do all this in 1.8. Thoughts?

I am in favor of that.

@jonas-schulze
Copy link
Contributor Author

If you want to fix it this way, we'll have to backport #41576 as well. The safest option is to revert from 1.7 and do all this in 1.8. Thoughts?

There is no urge, I was just hoping to get that feature in half a year earlier (or what else the release schedule is). 🙃

Regarding the backport: I would just disallow vim bindings for RadioMenus for 1.7 (i.e. not definining any of accepts_*(::RadioMenu)) such that the keybindings won't "become breaking" in 1.8.

@timholy
Copy link
Member

timholy commented Aug 11, 2021

Personally I think we should wait until Julia 1.8. The issue is this: this is an extension API. Whatever we pick has to be something we're happy supporting for the long term. This design seems reasonable, but it adds three additional methods that need to be defined to support a very simple feature, and it would be package-specific rather than user-specific. There might not be a better way to do this, but are we sure?

I would argue that the stabilization process of 1.7 is not the right time to make that decision; better to "do no harm" than to rush something.

@KristofferC
Copy link
Member

+1 for revert and get some extra time to think about it. The three extra methods also stuck out to me and I think it deserves a bit more thought.

@jonas-schulze
Copy link
Contributor Author

Ok. So should #41835 be merged into master (such that vim bindings have to be redone from scratch) or just into release-1.7 (such that this PR can remain as it is)?

@vchuravy
Copy link
Member

Ok. So should #41835 be merged into master (such that vim bindings have to be redone from scratch) or just into release-1.7 (such that this PR can remain as it is)?

You can rebase this PR on a revert of #41835

@JeffBezanson
Copy link
Member

From triage: seems like this should not be on the 1.7 milestone? Let's plan for it in 1.8.

@JeffBezanson JeffBezanson modified the milestones: 1.7, 1.8 Aug 12, 2021
@timholy
Copy link
Member

timholy commented Aug 31, 2021

@jonas-schulze, if you do rewrite this for 1.8, just couple of thoughts:

  • One could check for additional configuration via hasfield in the Config object. You might be able to declare a navigation_keys field.
  • The hardest part to deal with well is the custom keypress function. In a world where we can configure the key bindings for the user, hard-coded choices seem non-ideal.

Perhaps you could mimic my own strategy in rewriting the old API, and declare a further subtype of _ConfiguredMenu{C} that becomes the model of what we'd change the interface to when we can break it.

@ViralBShah
Copy link
Member

Should plan for 1.9 now if we want to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants