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 support for extend_file_{start,end}. #11767

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

hadronized
Copy link
Contributor

Solves #11766.

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().
@hadronized hadronized marked this pull request as ready for review September 24, 2024 16:02
@hadronized
Copy link
Contributor Author

I allowed myself for a bit of refactoring — the From<Mode> for Movement, as the pattern is present in many places — but it’s optional, even though I think it still makes sense to do it now, as I needed to do it again too.

@pascalkuthe
Copy link
Member

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 goto version of the command neber extend and changing the default select mode bindings to the extend version.

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

@hadronized
Copy link
Contributor Author

and changing the default select mode bindings to the extend version.

I’m not sure what that means, may I ask you to explain?

@the-mikedavis
Copy link
Member

A good example of what @pascalkuthe is describing is some of the existing bindings in select mode in the default keymap:

"h" | "left" => extend_char_left,
"j" | "down" => extend_visual_line_down,
"k" | "up" => extend_visual_line_up,
"l" | "right" => extend_char_right,

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 extend_file_{start,end} to the select mode keymap and update the equivalent entries in the normal mode keymap to use ones that only move and do not look at mode

@hadronized
Copy link
Contributor Author

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?

@the-mikedavis
Copy link
Member

Yeah exactly, eventually we shouldn't be converting Mode to Movement anywhere

@hadronized
Copy link
Contributor Author

hadronized commented Sep 27, 2024

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 extend_* and sometimes extend_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.

3 participants