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

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

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

wants to merge 1 commit into from
+16 −9

Conversation

chrispyles
Copy link

@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
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

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

@godalming123 godalming123 godalming123 approved these changes

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
A-command Area: Commands S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cannot replace newline with r
5 participants
@chrispyles @pascalkuthe @neduard @godalming123 @kirawi