-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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 support for extend_file_{start,end}. #11767
base: master
Are you sure you want to change the base?
Add support for extend_file_{start,end}. #11767
Conversation
That allows to refactor a bit of code; we have the following pattern in many different places: if cx.editor.mode == Mode::Select { Movement::Extend } else { Movement::Move } The From impl captures that pattern, so that we can just do cx.editor.mode.into().
I allowed myself for a bit of refactoring — the |
I think we generally want to move away from mode dependent keybindings. So in this case I would actually be ok with doing a breaking change and making the I am not sure about the refactor for that reason. Codewise looks fine but it's really a pattern that shouldn't be prevelant and will hopefully be gone at some point. So maybe we just kepe the code as is (less merge conflicts) since it's only temporary. @the-mikedavis what do you think think |
I’m not sure what that means, may I ask you to explain? |
A good example of what @pascalkuthe is describing is some of the existing bindings in select mode in the default keymap: helix/helix-term/src/keymap/default.rs Lines 338 to 341 in b18a471
Rather than "smart" commands that look at the mode and decide whether to move or extend we have the default keymap for normal mode use the move versions of the commands and the select mode default keybindings use the extend version. So in this case we would add these |
Okay, that makes sense. So the refactoring I’m doing is probably not needed, since it’s what looks at the mode to deduce the movement? |
Yeah exactly, eventually we shouldn't be converting Mode to Movement anywhere |
Okay, I’ll change the PR when I have some time, but am I correct assuming I should only do that for the commands I want to introduce and let the big refactoring to someone else / another PR? Alternative question: what name should I use for the commands? I realized we sometimes use |
Solves #11766.