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 3 commits into
base: master
Choose a base branch
from

Conversation

hadronized
Copy link
Contributor

Solves #11766.

@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_*.

@nik-rev
Copy link
Contributor

nik-rev commented Dec 8, 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_*.

I think extend_file_end sounds more strange than extend_to_file_end

@hadronized hadronized force-pushed the 11766-extend-file-start-end branch 5 times, most recently from e12ad16 to 5606469 Compare March 1, 2025 23:31
@hadronized hadronized force-pushed the 11766-extend-file-start-end branch from 5606469 to 8506125 Compare March 1, 2025 23:37
Comment on lines +1825 to +1829
if cx.editor.mode == Mode::Select {
Movement::Extend
} else {
Movement::Move
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what to do there since I made goto_line_without_jumplist take the Movement.

@hadronized
Copy link
Contributor Author

@pascalkuthe @the-mikedavis whom should I ping for another review? :)

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.

4 participants