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

Convert clipboard line ending to document line ending when pasting #716

Merged
merged 2 commits into from
Sep 10, 2021

Conversation

kirawi
Copy link
Member

@kirawi kirawi commented Sep 5, 2021

Resolves #715

WIP because it doesn't play nice with the LSP.

@kirawi kirawi marked this pull request as draft September 5, 2021 22:32
helix-core/src/line_ending.rs Outdated Show resolved Hide resolved
helix-term/src/commands.rs Outdated Show resolved Hide resolved
@kirawi kirawi changed the title Fix line ending paste Convert clipboard line ending to document line ending when pasting Sep 6, 2021
@cessen
Copy link
Contributor

cessen commented Sep 6, 2021

Yeah, I think what we actually want to do here is have a stand-alone function that (probably?) takes a &str and a target line ending, and returns a String with all non-specialized line endings replaced with the target line ending. Then we can just call that from the paste command to do the conversion before actually pasting.

The non-specialized line endings are CRLF, LF, CR, LS, and Nel, and should be replaced as needed.

(FF, VT, and PS have special uses, and shouldn't be replaced. Incidentally, I just noticed I screwed that up here:

cx.editor.set_status(match line_ending {
Crlf => "crlf".into(),
LF => "line feed".into(),
FF => "form feed".into(),
CR => "carriage return".into(),
Nel => "next line".into(),
// These should never be a document's default line ending.
VT | LS | PS => "error".into(),
Ha ha. So we should probably put what counts as "specialized" or not somewhere centralized, so we only have to get it right once. Probably just a const &[] of the relevant line endings will do, so we can call contains())

@cessen
Copy link
Contributor

cessen commented Sep 6, 2021

This prompted me to investigate the purpose of the various line endings further, which led me to this:
https://www.unicode.org/standard/reports/tr13/tr13-5.html

It's actually not entirely clear to me yet which line endings should be treated as "specialized" or not. So let's definitely centralize that, and then we can adjust as needed later. (I'm suspecting that everything except CRLF, LF, CR, and Nel should probably be treated as specialized, but I'm not sure.)

@kirawi
Copy link
Member Author

kirawi commented Sep 8, 2021

Perhaps we could use memchr to check for the line endings? It's already a dependency. Or perhaps regex.

@archseer
Copy link
Member

archseer commented Sep 8, 2021

for the clipboard one:

    // only compile the regex once
    #[allow(clippy::trivial_regex)]
    static REGEX: Lazy<Regex> =
        Lazy::new(|| Regex::new(r"\r\n|\r|\n").unwrap());
    REGEX.replace_all(paste_string, new_line_ending)

@archseer
Copy link
Member

archseer commented Sep 8, 2021

I'd completely drop the line ending conversion from this PR and only land the clipboard change. Fixing document-wide line endings is simple to do from the terminal, we can implement a better solution later.

@kirawi kirawi marked this pull request as ready for review September 8, 2021 18:17
helix-term/src/commands.rs Outdated Show resolved Hide resolved
@cessen
Copy link
Contributor

cessen commented Sep 9, 2021

Lazy::new(|| Regex::new(r"\r\n|\r|\n").unwrap());

Yeah, for now I think that's good. We know at least those should be replaced, and if/when I get around to more fully investigating the Unicode line ending situation I'll make a PR to include any others that should be covered as well.

@kirawi kirawi marked this pull request as draft September 10, 2021 01:43
@kirawi kirawi marked this pull request as ready for review September 10, 2021 01:47
helix-term/src/commands.rs Outdated Show resolved Hide resolved
@archseer archseer merged commit 987d8e6 into helix-editor:master Sep 10, 2021
@kirawi kirawi deleted the fix branch September 10, 2021 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pasting changes document's line-ending from LF to CRLF
3 participants