-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Adding selection for ParagraphPrevious and ParagraphNext. #3353
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please document the new actions in runtime/help/keybindings.md
@Neko-Box-Coder done, thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested the changes, seems good to me.
internal/action/actions.go
Outdated
} | ||
} | ||
// Find the first empty line | ||
for ; line > 0; line-- { | ||
if len(h.Buf.LineBytes(line)) == 0 && line != h.Cursor.Y { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line != h.Cursor.Y
seems not needed anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, thanks.
internal/action/actions.go
Outdated
if line == h.Buf.LinesNum() { | ||
h.Cursor.Loc = h.Buf.End() | ||
} | ||
} | ||
|
||
// ParagraphPrevious moves the cursor to the first empty line that comes before the paragraph closest to the cursor, or beginning of the buffer if there isn't a paragraph |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noticed: these comment lines are way too long, could you split them?
Besides that, the PR looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Would you folks prefer I squash into one commit, or the 4 independent are fine? |
Ideally, squash. It's generally better to keep separate commits for separate logical changes, and squash them otherwise. In other words, organize commits in such a way that the commit history is easier to review, not harder. |
Also tweaked the behavior of Paragraph/{Previous/Next} so that it skips all empty lines immediately next to cursor position, until it finds the start/end of the paragraph closest to it. Once it finds the paragraph closest to it, the same behavior as before applies. With the previous behavior if the cursor was surrounded by empty lines, then Paragraph/{Previous/Next} would only jump to the next empty line, instead of jumping to the start/end of a paragraph.
@dmaluka done |
Also tweaked the behavior of Paragraph/{Previous/Next} so that it skips all empty lines immediately next to cursor position, until it finds the start/end of the paragraph closest to it. Once it finds the paragraph closest to it, the same behavior as before applies. With the previous behavior if the cursor was surrounded by empty lines, then Paragraph/{Previous/Next} would only jump to the next empty line, instead of jumping to the start/end of a paragraph.