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

Remove special handling of line ending characters in selection replacement #10786

Merged
merged 2 commits into from
Jun 10, 2024
Merged

Remove special handling of line ending characters in selection replacement #10786

merged 2 commits into from
Jun 10, 2024
+18 −12

Conversation

chrispyles
Copy link
Contributor

@chrispyles chrispyles commented May 18, 2024 •
edited

Remove special handling of line ending characters in selection replacement which prevented newline characters from being replaced

Fixes #10725

@pascalkuthe
Copy link
Member

This adds more weird special cases instead of removing them like is described in that issue by us.

The desired behavior for r<ret> is for every grapheme to be replaced by the documents line ending (like it currently already does). No changes are necessary there.

The only necessary change is to remove the special case for checking weather a grapheme (that is being replaced) is a line ending.

@chrispyles chrispyles force-pushed the master branch 2 times, most recently from dc5182d to 9795e12 Compare May 18, 2024 02:43
@chrispyles
Copy link
Contributor Author

@pascalkuthe ACK, I misunderstood what the code here was doing. I've rewritten the PR to do as the comment in the issue suggests and just remove the special handling of line endings.

@chrispyles chrispyles changed the title Fix selection replacement handling of newline characters Remove special handling of line ending characters in selection replacement May 18, 2024
@kirawi kirawi added S-waiting-on-review Status: Awaiting review from a maintainer. A-command Area: Commands labels May 18, 2024
godalming123
godalming123 approved these changes May 25, 2024
View reviewed changes
Copy link

@godalming123 godalming123 left a comment •
edited

Choose a reason for hiding this comment

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

This works as expected:
https://github.com/helix-editor/helix/assets/68993177/413be1fa-35a3-46b7-a7fc-b813781d6321
Why can't it be merged?

neduard
neduard approved these changes May 27, 2024
View reviewed changes
Copy link

@neduard neduard left a comment

Choose a reason for hiding this comment

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

simplifies existing code & adds unit test. LGTM

the-mikedavis
the-mikedavis requested changes Jun 3, 2024
View reviewed changes
helix-term/src/commands.rs Outdated Show resolved Hide resolved
helix-term/tests/test/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 Jun 3, 2024
@chrispyles
Copy link
Contributor Author

@the-mikedavis I've pushed a commit to address your comments. Please let me know if any other changes are needed.

@pascalkuthe pascalkuthe merged commit 03813bb into helix-editor:master Jun 10, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neduard neduard neduard approved these changes

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

@pascalkuthe pascalkuthe pascalkuthe approved these changes

@godalming123 godalming123 godalming123 approved these changes

Assignees
No one assigned
Labels
A-command Area: Commands S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cannot replace newline with r
6 participants
@chrispyles @pascalkuthe @neduard @the-mikedavis @godalming123 @kirawi