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

adds movement for start of word, regardless of stop characters #6445

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rwese
Copy link

@rwese rwese commented Nov 29, 2024

regarding #195 #1954 (comment)

Sample test/with-characters that cause stopps
                    -(cursor)
       -(after hitting MoveBackwardWordStart)

otherwise would need 2 MoveBackwardWord actions.

Sample test/with-characters that cause stopps
                    -(cursor)
       -(after hitting MoveBackwardWordStart)
@rwese
Copy link
Author

rwese commented Nov 29, 2024

it's missing documentation, I just wanted to get some feedback if this is the proper way to implement the new movement.

@bew
Copy link
Member

bew commented Nov 29, 2024

About naming, I think MoveBackwardWordStart & MoveBackwardWord are not different enough (I thought they were the same at the beginning).
Maybe rename to MoveBackwardWORD (but that breaks camelcase-ing..), or MoveBackwardBigWord ?

Also, instead of basically re-implementing move_backward_one_word with slightly different behavior, I think it would be better to refactor the function if possible, for example to make it accept a predicate function for the separator to stop at 🤔

@rwese
Copy link
Author

rwese commented Nov 29, 2024

Naming is always hard, I thought it would fit well with "moveForwardEnd" > "BackwardStart".

In terms of code, I took the easy way, not wanting to have to deal with breaking changes and refactoring. But I can give it a refactor, need to look into the test-cases.

Copy link
Member

@wez wez left a comment

Choose a reason for hiding this comment

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

Thanks for this!

Please also add a page to the docs for the new action; look at docs/config/lua/keyassignment/CopyMode/MoveForwardWordEnd.md for inspiration!

@@ -672,6 +672,7 @@ pub enum CopyModeAssignment {
MoveToStartOfNextLine,
MoveToSelectionOtherEnd,
MoveToSelectionOtherEndHoriz,
MoveBackwardWordStart,
Copy link
Member

Choose a reason for hiding this comment

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

TBH, I think this name is probably about as good as it's going to get; it is consistent with MoveForwardWordEnd and it does describe what it does reasonably well.

@@ -822,6 +822,54 @@ impl CopyRenderable {
}
}

fn move_backward_one_word_start(&mut self) {
Copy link
Member

Choose a reason for hiding this comment

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

It's hard to tell at a glance just how similar this is to the move_backward_one_word function. If adding a parameter to control whether it should be in "start mode" or not makes for a smaller diff and a more readable function, that would get my vote!

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