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

Fix find commands for buffers with non-LF line-endings #8111

Merged
merged 2 commits into from
Sep 3, 2023

Conversation

woojiq
Copy link
Contributor

@woojiq woojiq commented Aug 29, 2023

Adds separate method for finding <ret> char.

Closes #8057

Resolves TODO:

// TODO: this isn't quite correct when CRLF is involved.
// This hack will work in most cases, since documents don't
// usually mix line endings. But we should fix it eventually
// anyway.

@woojiq woojiq marked this pull request as draft August 29, 2023 21:19
@woojiq woojiq force-pushed the find_char_ret branch 2 times, most recently from 4cc2c1d to 7ad3c3c Compare August 30, 2023 21:51
@woojiq woojiq marked this pull request as ready for review August 31, 2023 09:51
Copy link
Member

@pascalkuthe pascalkuthe left a comment

Choose a reason for hiding this comment

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

I think this is an ok fix (the implementation seems correct) but I think a better solution would be to special case this branch. And just use ropey to find the next line ending/start from the cursor (look at the implementation of gl/gh). That should be faster and correctly handle files with mixed line endings (resolving the TODO comment here)

@pascalkuthe pascalkuthe added C-bug Category: This is a bug E-easy Call for participation: Experience needed to fix: Easy / not much S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. A-command Area: Commands labels Sep 1, 2023
@woojiq woojiq force-pushed the find_char_ret branch 2 times, most recently from 17b55e0 to 557a34a Compare September 2, 2023 13:59
@woojiq
Copy link
Contributor Author

woojiq commented Sep 2, 2023

Done. Thank you for the suggestion. There are some edge cases thought. Comments explain them more clearly.

Copy link
Member

@pascalkuthe pascalkuthe left a comment

Choose a reason for hiding this comment

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

this looks very nice, some style nits but it seems like you covered all edgecases 👍

helix-term/src/commands.rs Outdated Show resolved Hide resolved
helix-term/src/commands.rs Outdated Show resolved Hide resolved
@woojiq woojiq changed the title fix: change line-ending char when finding ret fix: find char <ret> for files with different line-endings Sep 2, 2023
pascalkuthe
pascalkuthe previously approved these changes Sep 2, 2023
@pascalkuthe pascalkuthe 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 Sep 2, 2023
* Takes into account direction and an inclusive flag to select the first or last char of the line-ending string.
* Adds tests to ensure `T` and `F` work with `\n` and `\r\n` line-endings.
pascalkuthe
pascalkuthe previously approved these changes Sep 3, 2023
@the-mikedavis the-mikedavis changed the title fix: find char <ret> for files with different line-endings Fix find commands for buffers with non-LF line-endings Sep 3, 2023
@pascalkuthe pascalkuthe merged commit bb3e699 into helix-editor:master Sep 3, 2023
6 checks passed
@woojiq woojiq deleted the find_char_ret branch September 4, 2023 06:02
dgkf pushed a commit to dgkf/helix that referenced this pull request Jan 30, 2024
mtoohey31 pushed a commit to mtoohey31/helix that referenced this pull request Jun 2, 2024
smortime pushed a commit to smortime/helix that referenced this pull request Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-command Area: Commands C-bug Category: This is a bug E-easy Call for participation: Experience needed to fix: Easy / not much S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

T<ret> on windows selects the previous newline
3 participants