Line ending detection #224

janhrastnik · 2021-06-11T21:42:20Z

…\n', as this fixes CRLF files not rendering properly. Code that inserts line endings still needs to be changed and a platform default line ending could be added.
@archseer may I ask why slice here isn't a borrow like in the original code?

Fixes #118
Fixes #68

helix-view/src/document.rs

helix-core/src/line_ending.rs

helix-view/src/document.rs

helix-view/src/editor.rs

helix-view/src/document.rs

helix-core/src/line_ending.rs

helix-term/src/ui/editor.rs

helix-core/src/line_ending.rs

janhrastnik · 2021-06-14T13:13:18Z

@archseer I started replacing some occurances of '\n' in commands.rs. I noticed that a lot of places take in '\n' as a char, not a &str. But doc.line_ending() returns a &str, because (as much as I understand) "\r\n" is 2 characters long. Any ideas what to do here? Also I can't call doc.line_ending() at some points because doc can't be instantiated there yet because of lifetime errors.

cessen · 2021-06-14T16:22:51Z

@archseer I started replacing some occurances of '\n' in commands.rs. I noticed that a lot of places take in '\n' as a char, not a &str. But doc.line_ending() returns a &str, because (as much as I understand) "\r\n" is 2 characters long.

Any place that needs a line ending as a literal should take a &str for sure, due to the CRLF case as you noted. I think the lifetime issues can be dealt with by ensuring that all cases are &'static str, which shouldn't be hard for line endings: we have a limited set of known possible strings.

Something else that occurred to me is we could have LineEnding implement a to_str() method as well, that just returns a &'static str representation. I'm not sure if that would actually be useful enough in Helix's code base to justify the impl, so maybe no a good idea. But just a thought.

helix-view/src/document.rs

helix-term/src/commands.rs

helix-core/src/line_ending.rs

helix-term/src/commands.rs

cessen · 2021-06-20T02:57:00Z

To anyone watching: I got permission to take this PR over, so I'll be finishing this up.

Also some general cleanup and some minor fixes along the way.

…ng_detection Rebasing was making me manually fix conflicts on every commit, so merging instead.

It's plain-old-data. If we want to do fancier things later, it's easy to switch back.

This is needed in some places.

janhrastnik requested a review from archseer June 11, 2021 21:42

janhrastnik mentioned this pull request Jun 11, 2021

Support CRLF #118

Closed

cessen reviewed Jun 11, 2021

View reviewed changes

helix-view/src/document.rs Outdated Show resolved Hide resolved

archseer requested changes Jun 12, 2021

View reviewed changes

helix-view/src/document.rs Outdated Show resolved Hide resolved

helix-view/src/document.rs Outdated Show resolved Hide resolved

helix-view/src/document.rs Outdated Show resolved Hide resolved

archseer requested changes Jun 12, 2021

View reviewed changes

archseer changed the title ~~added line ending detection and '\r\n' is now being matched same as '…~~ Line ending detection Jun 12, 2021

cessen reviewed Jun 12, 2021

View reviewed changes

helix-core/src/line_ending.rs Outdated Show resolved Hide resolved

janhrastnik force-pushed the line_ending_detection branch from c2693db to ec4b7c6 Compare June 13, 2021 10:19

cessen reviewed Jun 13, 2021

View reviewed changes

helix-core/src/line_ending.rs Outdated Show resolved Hide resolved

cessen reviewed Jun 13, 2021

View reviewed changes

helix-core/src/line_ending.rs Outdated Show resolved Hide resolved

cessen reviewed Jun 13, 2021

View reviewed changes

helix-term/src/ui/editor.rs Outdated Show resolved Hide resolved

cessen reviewed Jun 13, 2021

View reviewed changes

helix-core/src/line_ending.rs Outdated Show resolved Hide resolved

archseer reviewed Jun 14, 2021

View reviewed changes

helix-core/src/line_ending.rs Outdated Show resolved Hide resolved

archseer reviewed Jun 15, 2021

View reviewed changes

helix-view/src/document.rs Outdated Show resolved Hide resolved

janhrastnik added 8 commits June 16, 2021 17:00

rebase on branch line_ending_detection

3756c21

ran cargo clippy and cargo fmt

17f69a0

resolved conflict in rebase

5eb6918

added more changes from pr review for line_ending_detection

9c419fe

fix typo

e4849f4

added some tests and a line_ending helper function in document.rs

a9a718c

trying out line ending helper functions in commands.rs

a4f5a01

doc.line_ending() now returns &'static str

7cf0fa0

janhrastnik force-pushed the line_ending_detection branch from 856fd95 to 9c3eadb Compare June 16, 2021 15:22

fixed some problems from rebasing

9c3eadb

archseer requested changes Jun 17, 2021

View reviewed changes

helix-term/src/commands.rs Outdated Show resolved Hide resolved

helix-term/src/commands.rs Outdated Show resolved Hide resolved

helix-term/src/commands.rs Outdated Show resolved Hide resolved

applied changes from pr review

8bccd6d

archseer self-requested a review June 17, 2021 16:27

cessen reviewed Jun 18, 2021

View reviewed changes

helix-core/src/line_ending.rs Outdated Show resolved Hide resolved

cessen reviewed Jun 18, 2021

View reviewed changes

helix-core/src/line_ending.rs Outdated Show resolved Hide resolved

cessen reviewed Jun 18, 2021

View reviewed changes

helix-term/src/commands.rs Outdated Show resolved Hide resolved

archseer mentioned this pull request Jun 19, 2021

Add system clipboard yank and paste commands #310

Merged

janhrastnik added 6 commits June 19, 2021 14:03

added get_line_ending from pr comment

ecb884d

ran cargo fmt

97323dc

Merge remote-tracking branch 'origin/master' into line_ending_detection

cdd9347

fix merge issue

1e80fbb

changed some hardcoded newlines, removed a else if in line_ending.rs

701eb0d

added the line_end helper function

8634e04

cessen added 12 commits June 20, 2021 00:40

Misc fixes and clean up of line ending detect code.

5d22e3c

Work on moving code over to LineEnding instead of assuming '\n'.

4efd671

Also some general cleanup and some minor fixes along the way.

Merge branch 'master' of github.com:helix-editor/helix into line_endi…

e686c3e

…ng_detection Rebasing was making me manually fix conflicts on every commit, so merging instead.

Silence clippy warning.

3d3149e

Don't need getters/setters for line_ending property.

7140020

It's plain-old-data. If we want to do fancier things later, it's easy to switch back.

Add function to get the line ending of a str slice.

07e2880

This is needed in some places.

Update replace command to use document line ending setting.

23d6188

Make split_selection_on_newline command handle all line endings.

e436c30

Convert remaining commands to use the document's line ending setting.

d333556

Fix clippy warnings.

7c4fa18

Add command to set the document's default line ending.

a18d50b

Flesh out the line ending utility unit tests.

f2954fa

archseer approved these changes Jun 22, 2021

View reviewed changes

archseer merged commit a70de6e into master Jun 22, 2021

archseer deleted the line_ending_detection branch June 22, 2021 02:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Line ending detection #224

Line ending detection #224

janhrastnik commented Jun 11, 2021 •

edited by archseer

Loading

janhrastnik commented Jun 14, 2021

cessen commented Jun 14, 2021 •

edited

Loading

cessen commented Jun 20, 2021

Line ending detection #224

Line ending detection #224

Conversation

janhrastnik commented Jun 11, 2021 • edited by archseer Loading

janhrastnik commented Jun 14, 2021

cessen commented Jun 14, 2021 • edited Loading

cessen commented Jun 20, 2021

janhrastnik commented Jun 11, 2021 •

edited by archseer

Loading

cessen commented Jun 14, 2021 •

edited

Loading