helix
helix copied to clipboard
helix-editor
Fix reflow for CRLF line ending (wip)
This is a fix for the #2645.
It assumes a fix for the textwrap library, which is already in master but not yet released.
See details here https://github.com/mgeisler/textwrap/issues/453
Here's a quick demonstration of how it works https://asciinema.org/a/KZNzsnuHYXppwzw6tvVHyfiSe
@kirawi , could you please take a look and let me know if overall approach looks ok? Thanks for your time.
This looks good besides the clippy lint. Looks like your PR was merged but isn't released in a version of textwrap yet? https://github.com/mgeisler/textwrap/pull/454
@the-mikedavis , I fixed the clippy error.
Yes, the change in textwrap is not yet released. Are you are fine with this PR overall? If you intend to merge some time in the future I'll ask textwrap maintainer for an ETA on their side. If you want to merge asap, I'll ask to cut a release.
Yeah this is looking good to me 👍. No need to rush I don't think, it'd be good to have at least one other reviewer check this out before merge anyways
Yes, the change in textwrap is not yet released.
There has been a Textwrap release now, you should be able to depend on 0.16.0 and get the line-ending control you need.
I am not sure is this is the right approach. It fixes the common case of CRLF linenedings but actually just exposes a more deeply rooted issue: text-wrap handles line breaks differently then helix. In fact this PR will cause new bugs for crlf files which also contain LF linenedings which are not detected as line breaks at all anymore with this PR. It also fails for other supported (unicode) line-ending and CR line endings.
AFAIK the line_ending field on Document is just the line break that is used for inserting new line but the document may contain other linebreaks. I wish text-wrap would just let us supply our own lines iterator. We could use ropeys lines iterator here which correctly accounts for the linebreak feature flags helix uses. We could easily strip the EOL character ourselfs.
I am closing this pr as it has gone stale (over a year without development). Like I said in a different issue we are planning to move away from textwrap and would instead reuese our own softwrap infrastructure in the long run to gain the flexibility to solve issues like this.
Thank you for contributing!