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.

Sign up for GitHub

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

Jump to bottom

Keybind for Extend/shrink selection up and down #9080

Merged
merged 7 commits into from Mar 17, 2024
Merged

Keybind for Extend/shrink selection up and down #9080

merged 7 commits into from Mar 17, 2024

Conversation

EmiOnGit
Copy link
Contributor

Implements #9079

@the-mikedavis the-mikedavis added S-waiting-on-review Status: Awaiting review from a maintainer. A-command Area: Commands labels Dec 14, 2023
chtenb
chtenb reviewed Dec 26, 2023
View reviewed changes
helix-term/src/commands.rs Outdated Show resolved Hide resolved
helix-term/src/commands.rs Outdated Show resolved Hide resolved
helix-term/src/commands.rs Outdated Show resolved Hide resolved
@EmiOnGit
Copy link
Contributor Author

EmiOnGit commented Jan 4, 2024

The implementation should work better now.
Especially when flipping the selection and starting on a empty line :)
I am unsure about the behaviour of flipping the selection as soon as the selection is one line.
e.g. JJK currently leaves us with the cursor on the line start instead of the end.
How do you feel about that? I'm open to change that.

chtenb
chtenb approved these changes Jan 9, 2024
View reviewed changes
Copy link
Contributor

@chtenb chtenb left a comment

Choose a reason for hiding this comment

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

I've played around with this for a bit, and it feels quite intuitive to me! I think the selection flipping is consistent, and makes sense. I'm going to keep these commands instead of my old binds.

@EmiOnGit EmiOnGit mentioned this pull request Jan 10, 2024
Closed
Vulpesx
Vulpesx approved these changes Jan 17, 2024
View reviewed changes
Copy link

@Vulpesx Vulpesx left a comment

Choose a reason for hiding this comment

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

this appears fine as is, there are a lot of pull requests though so it will be a while before 2 people with write access approve it.

@EmiOnGit
Copy link
Contributor Author

I don't mind as I can just use my personal branch for now.
Hope it will get merged eventually as I see people asking about this behaviour frequently.

@EmiOnGit
Copy link
Contributor Author

Wouldn't say that the lint improves readability of the code but it's fine either way I guess. Sorry for not checking beforehand

the-mikedavis
the-mikedavis requested changes Feb 11, 2024
View reviewed changes
helix-term/src/commands.rs Show resolved Hide resolved
helix-term/src/commands.rs Outdated Show resolved Hide resolved
helix-term/src/commands.rs Outdated Show resolved Hide resolved
@the-mikedavis the-mikedavis added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Feb 11, 2024
@the-mikedavis the-mikedavis added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 11, 2024
@pascalkuthe pascalkuthe merged commit 761df60 into helix-editor:master Mar 17, 2024
6 checks passed
@david-crespo david-crespo mentioned this pull request Mar 20, 2024
Closed
@chtenb
Copy link
Contributor

chtenb commented Mar 20, 2024 •
edited

Nice that this made it in. In case people are looking for ways to bind this, the following is a clean way to integrate this in the default keymapping.

[keys.normal]
J = ["select_line_below"]
K = ["select_line_above"]

"A-j" = "join_selections"
"A-J" = "join_selections_space"

X = "remove_primary_selection"
x = "keep_selections"
"A-x" = "remove_selections"

[keys.select]
J = ["select_line_below"]
K = ["select_line_above"]

"A-j" = "join_selections"
"A-J" = "join_selections_space"

X = "remove_primary_selection"
x = "keep_selections"
"A-x" = "remove_selections"

The bonus is that X pairs ergonomically with C and ( and ).

As an addendum, I find it useful to map H/L to select everything to the left/right:

[keys.normal]
"H" = ["select_mode", "goto_line_start", "exit_select_mode"]
"L" = ["select_mode", "goto_line_end", "exit_select_mode"]

[keys.select]
"H" = ["goto_line_start"]
"L" = ["goto_line_end"]

The rationale for choosing H/J/K/L is that they represent directions, while holding shift is often associated with extending a selection (in mainstream text editing UI).

Desdaemon pushed a commit to Desdaemon/helix that referenced this pull request Mar 26, 2024
@EmiOnGit @Desdaemon
Keybind for Extend/shrink selection up and down (helix-editor#9080)
5ef6190 
* implement another selection modifying command

* Selection feels more ergonomic in case of swapping the direction. This also fixes a problem when starting at an empty line.

* rename select_line_up/down to select_line_above/below

* apply clippy suggestion of using cmp instead of if-chain

* revert `Extent` implementing `Clone/Copy`

* move select_line functions below extend_line implementations

* implement help add function, which saturates at the number of text lines

---------

Co-authored-by: Emi <emanuel.boehm@gmail.com>
shortc pushed a commit to shortc/helix that referenced this pull request Mar 31, 2024
@EmiOnGit @chrisshort-udig
Keybind for Extend/shrink selection up and down (helix-editor#9080)
d63c5a7 
* implement another selection modifying command

* Selection feels more ergonomic in case of swapping the direction. This also fixes a problem when starting at an empty line.

* rename select_line_up/down to select_line_above/below

* apply clippy suggestion of using cmp instead of if-chain

* revert `Extent` implementing `Clone/Copy`

* move select_line functions below extend_line implementations

* implement help add function, which saturates at the number of text lines

---------

Co-authored-by: Emi <emanuel.boehm@gmail.com>
postsolar pushed a commit to postsolar/helix that referenced this pull request Apr 4, 2024
@EmiOnGit @postsolar
Keybind for Extend/shrink selection up and down (helix-editor#9080)
8db611d 
* implement another selection modifying command

* Selection feels more ergonomic in case of swapping the direction. This also fixes a problem when starting at an empty line.

* rename select_line_up/down to select_line_above/below

* apply clippy suggestion of using cmp instead of if-chain

* revert `Extent` implementing `Clone/Copy`

* move select_line functions below extend_line implementations

* implement help add function, which saturates at the number of text lines

---------

Co-authored-by: Emi <emanuel.boehm@gmail.com>
postsolar pushed a commit to postsolar/helix that referenced this pull request Apr 4, 2024
@EmiOnGit @postsolar
Keybind for Extend/shrink selection up and down (helix-editor#9080)
1ce53e7 
* implement another selection modifying command

* Selection feels more ergonomic in case of swapping the direction. This also fixes a problem when starting at an empty line.

* rename select_line_up/down to select_line_above/below

* apply clippy suggestion of using cmp instead of if-chain

* revert `Extent` implementing `Clone/Copy`

* move select_line functions below extend_line implementations

* implement help add function, which saturates at the number of text lines

---------

Co-authored-by: Emi <emanuel.boehm@gmail.com>
postsolar pushed a commit to postsolar/helix that referenced this pull request Apr 4, 2024
@EmiOnGit @postsolar
Keybind for Extend/shrink selection up and down (helix-editor#9080)
359f49b 
* implement another selection modifying command

* Selection feels more ergonomic in case of swapping the direction. This also fixes a problem when starting at an empty line.

* rename select_line_up/down to select_line_above/below

* apply clippy suggestion of using cmp instead of if-chain

* revert `Extent` implementing `Clone/Copy`

* move select_line functions below extend_line implementations

* implement help add function, which saturates at the number of text lines

---------

Co-authored-by: Emi <emanuel.boehm@gmail.com>
postsolar pushed a commit to postsolar/helix that referenced this pull request Apr 20, 2024
@EmiOnGit @postsolar
Keybind for Extend/shrink selection up and down (helix-editor#9080)
229aa9b 
* implement another selection modifying command

* Selection feels more ergonomic in case of swapping the direction. This also fixes a problem when starting at an empty line.

* rename select_line_up/down to select_line_above/below

* apply clippy suggestion of using cmp instead of if-chain

* revert `Extent` implementing `Clone/Copy`

* move select_line functions below extend_line implementations

* implement help add function, which saturates at the number of text lines

---------

Co-authored-by: Emi <emanuel.boehm@gmail.com>
@chtenb chtenb mentioned this pull request May 1, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chtenb chtenb chtenb approved these changes

@the-mikedavis the-mikedavis the-mikedavis approved these changes

@pascalkuthe pascalkuthe pascalkuthe approved these changes

@Vulpesx Vulpesx Vulpesx approved these changes

Assignees
No one assigned
Labels
A-command Area: Commands S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@EmiOnGit @chtenb @the-mikedavis @pascalkuthe @Vulpesx