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 same_line and anchored movements #10576

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

Conversation

pantos9000
Copy link

@pantos9000 pantos9000 commented Apr 23, 2024

This PR picks up the stale PR #4204. Big thanks to @mitsuhiko for his PR. Seeing the old changes made it really easy to jump right into the code 😎 👍

This PR adds two kinds of new movement sets: same_line and anchored movement.

For more info, check the old PR #4204.

same_line movement

Commands added:

  • move_same_line_char_left
  • move_same_line_char_right
  • extend_same_line_char_left
  • extend_same_line_char_right
  • append_mode_same_line

These new commands move cursors, while making them stay in the same line. So if a cursor would wrap around into another line, instead it won't move and stay at its current position.

anchored movement

Commands added:

  • move_anchored_line_up
  • move_anchored_line_down
  • move_anchored_visual_line_up
  • move_anchored_visual_line_down
  • extend_anchored_line_up
  • extend_anchored_line_down
  • extend_anchored_visual_line_up
  • extend_anchored_visual_line_down

These new commands move cursors vertically. A cursor will move depending on its position:

  • If it is on a newline character of a non-empty line, the cursor will stay on newlines (i.e. on a line's last character).
  • If it is on a non-newline character of a non-empty line, the cursor will try to avoid newline characters. It will move normally, but if it would end up on a newline, instead it will be moved one position left of it (i.e. the line's second to last character).
  • If it is on the newline character of an empty line (that contains nothing except the newline character), the cursor will continue to move like before: If it stayed on newline before, it will continue to do so. Otherwise it will try to avoid them (except on empty lines).

Differences to the old PR

  • Based on recent master
  • Commands support soft-wrap / visual movement
  • Added some unit tests
  • Slightly changed command names
  • Additional command append_mode_same_line (for consistency, as append is more or less move right + insert mode)

Remarks

Command / function names

I was not sure about the general naming - e.g. anchored I think is used somewhere else in the code in a different context. I guess it would be bad if this somehow collided, so you might want to keep that in mind during review. If anyone spots any ambiguities - I'm always open for improvements :)

Helper functions context

I created some functions that are only used locally, e.g. is_in_visual_empty_line(). In case you think it makes sense to have some of those in a more common context (core?), I can gladly move them.

Unit tests

I struggled a bit for the soft-wrap functionality, the corresponding tests might be too implementation-specific and break in the future, if the soft-wrap functionality is adjusted. Couldn't find a better way, so I hope the tests are okay like this.

Also I skipped the unit tests for Movement::Extend, as the functionality is exactly the same as with Movement::Move.

Docs

I don't know if these commands should be documented somewhere, e.g. in the book. If it makes sense, I can write a few describing words.

Only a single setting

If these movements should have only a single setting (i.e. a bool that can be set) then this would probably affect a bit the architecture. Not sure how it would be best to implement it, but I would maybe go for additional values in either helix_core::Movement or helix_core::Direction.

But I think the way it was implemented previously (and still is) allows for quite versatile configuration and fits nice into the existing architecture, so I would prefer it the way it is.

@pantos9000 pantos9000 marked this pull request as draft April 24, 2024 16:18
@pantos9000
Copy link
Author

pantos9000 commented Apr 24, 2024

Found a bug in the anchored implementation, made the PR a draft.

@kirawi kirawi added the A-command Area: Commands label Apr 27, 2024
@pantos9000 pantos9000 marked this pull request as ready for review May 2, 2024 02:07
Commands added:
* `move_same_line_char_left`
* `move_same_line_char_right`
* `extend_same_line_char_left`
* `extend_same_line_char_right`
* `append_mode_same_line`

These new commands move cursors, while making them stay in the same
line. So if a cursor would wrap around into another line, instead it
won't move and stay at its current position.
Commands added:
* `move_anchored_line_up`
* `move_anchored_line_down`
* `move_anchored_visual_line_up`
* `move_anchored_visual_line_down`
* `extend_anchored_line_up`
* `extend_anchored_line_down`
* `extend_anchored_visual_line_up`
* `extend_anchored_visual_line_down`

These new commands move cursors vertically. A cursor will move depending
on its position:

* If it is on a newline character of a non-empty line, the cursor will
  stay on newlines (i.e. on a line's last character).

* If it is on a non-newline character of a non-empty line, the cursor
  will try to avoid newline characters. It will move normally, but if
  it would end up on a newline, instead it will be moved one position
  left of it (i.e. the line's second to last character).

* If it is on the newline character of an empty line (that contains
  nothing except the newline character), the cursor will continue to
  move like before: If it stayed on newline before, it will continue to
  do so. Otherwise it will try to avoid them (except on empty lines).
@mitsuhiko
Copy link
Contributor

Thank you so much for picking this back up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-command Area: Commands
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants