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

replace_selections_with_clipboard doesn't remove cr on windows #12329

Closed
sibouras opened this issue Dec 24, 2024 · 2 comments
Closed

replace_selections_with_clipboard doesn't remove cr on windows #12329

sibouras opened this issue Dec 24, 2024 · 2 comments
Labels
C-bug Category: This is a bug

Comments

@sibouras
Copy link

sibouras commented Dec 24, 2024

Summary

when copying multiline text on windows and then replacing the selection with space+R the carriage return cr doesn't get removed which causes bugs in biome formatting, pasting with space+p works fine though

Reproduction Steps

sample code to copy

const people = [
	{ name: "one", year: 2000 },
	{ name: "two", year: 2001 },
	{ name: "three", year: 2002 },
	{ name: "four", year: 2003 },
];
hx-biome-bug.mp4

I tried this:

  1. open helix, paste the sample text and save
  2. biome format , no cr is showing(expected)
  3. replace the selection with space+R
  4. biome format shows cr
  5. run fmt multiple times and biome gets confused

this is my config

[[language]]
name = "javascript"
language-servers = [
  { name = 'biome' },
  { name = 'typescript-language-server', except-features = ['format'] },
]

[language-server.biome]
command = "biome"
args = ["lsp-proxy"]
required-root-patterns = ["biome.json", "biome.jsonc"]

Helix log

~/.cache/helix/helix.log
please provide a copy of `~/.cache/helix/helix.log` here if possible, you may need to redact some of the lines

Platform

windows

Terminal Emulator

windows-terminal

Installation Method

source

Helix Version

helix 24.7 (91a5d40)

@sibouras sibouras added the C-bug Category: This is a bug label Dec 24, 2024
@RoloEdits
Copy link
Contributor

Looks like paste has a regex to remove the endings

// Only compiled once.
static REGEX: Lazy<Regex> = Lazy::new(|| Regex::new(r"\r\n|\r|\n").unwrap());
let mut values = values
.iter()
.map(|value| REGEX.replace_all(value, doc.line_ending.as_str()))
.map(|value| Tendril::from(value.as_ref().repeat(count)))
.chain(repeat);

Replace doesnt

fn replace_with_yanked_impl(editor: &mut Editor, register: char, count: usize) {
let Some(values) = editor
.registers
.read(register, editor)
.filter(|values| values.len() > 0)
else {
return;
};
let values: Vec<_> = values.map(|value| value.to_string()).collect();
let scrolloff = editor.config().scrolloff;
let (view, doc) = current!(editor);
let repeat = std::iter::repeat(
values
.last()
.map(|value| Tendril::from(&value.repeat(count)))
.unwrap(),
);
let mut values = values
.iter()
.map(|value| Tendril::from(&value.repeat(count)))
.chain(repeat);
let selection = doc.selection(view.id);
let transaction = Transaction::change_by_selection(doc.text(), selection, |range| {
if !range.is_empty() {
(range.from(), range.to(), Some(values.next().unwrap()))
} else {
(range.from(), range.to(), None)
}
});

Though I feel biome behaving like this should also be considered a bug. Might be worth to open an issue there as well to see if this is the failing behavior expected.

@sibouras
Copy link
Author

sibouras commented Dec 24, 2024

thanks for looking into this, and ye i was wondering where i should open this issue. but for consistency sake shouldn't helix replace remove the endings like in paste?

anyways i opened the issue in biome as well biomejs/biome#4789

@the-mikedavis the-mikedavis changed the title replace_selections_with_clipboard doesn't remoce cr on windows replace_selections_with_clipboard doesn't remove cr on windows Dec 25, 2024
CedricMeu pushed a commit to CedricMeu/helix that referenced this issue Jan 2, 2025
Previously we replaced line-endings in pasted text to the document
line-ending for some values in paste commands. We missed the `repeat`
values in paste though and didn't do any replacement in the replace
command.

Along with this change I've refactored the replace command to avoid
intermediary collections. We previously eagerly collected the values
from the input register as a `Vec<String>` but we can avoid both of
those conversions and only allocate for the conversion to a `Tendril`.
We can also switch from `str::repeat` to a manual implementation to
avoid the intermediary conversion to a String - this avoids an extra
allocation in the common case (i.e. no count).

Fixes helix-editor#12329
akhenakh pushed a commit to akhenakh/helix that referenced this issue Jan 4, 2025
Previously we replaced line-endings in pasted text to the document
line-ending for some values in paste commands. We missed the `repeat`
values in paste though and didn't do any replacement in the replace
command.

Along with this change I've refactored the replace command to avoid
intermediary collections. We previously eagerly collected the values
from the input register as a `Vec<String>` but we can avoid both of
those conversions and only allocate for the conversion to a `Tendril`.
We can also switch from `str::repeat` to a manual implementation to
avoid the intermediary conversion to a String - this avoids an extra
allocation in the common case (i.e. no count).

Fixes helix-editor#12329
GladkihEgor pushed a commit to GladkihEgor/helix that referenced this issue Jan 4, 2025
Previously we replaced line-endings in pasted text to the document
line-ending for some values in paste commands. We missed the `repeat`
values in paste though and didn't do any replacement in the replace
command.

Along with this change I've refactored the replace command to avoid
intermediary collections. We previously eagerly collected the values
from the input register as a `Vec<String>` but we can avoid both of
those conversions and only allocate for the conversion to a `Tendril`.
We can also switch from `str::repeat` to a manual implementation to
avoid the intermediary conversion to a String - this avoids an extra
allocation in the common case (i.e. no count).

Fixes helix-editor#12329
diucicd pushed a commit to diucicd/helix that referenced this issue Jan 8, 2025
Previously we replaced line-endings in pasted text to the document
line-ending for some values in paste commands. We missed the `repeat`
values in paste though and didn't do any replacement in the replace
command.

Along with this change I've refactored the replace command to avoid
intermediary collections. We previously eagerly collected the values
from the input register as a `Vec<String>` but we can avoid both of
those conversions and only allocate for the conversion to a `Tendril`.
We can also switch from `str::repeat` to a manual implementation to
avoid the intermediary conversion to a String - this avoids an extra
allocation in the common case (i.e. no count).

Fixes helix-editor#12329
rmburg pushed a commit to rmburg/helix that referenced this issue Jan 20, 2025
Previously we replaced line-endings in pasted text to the document
line-ending for some values in paste commands. We missed the `repeat`
values in paste though and didn't do any replacement in the replace
command.

Along with this change I've refactored the replace command to avoid
intermediary collections. We previously eagerly collected the values
from the input register as a `Vec<String>` but we can avoid both of
those conversions and only allocate for the conversion to a `Tendril`.
We can also switch from `str::repeat` to a manual implementation to
avoid the intermediary conversion to a String - this avoids an extra
allocation in the common case (i.e. no count).

Fixes helix-editor#12329
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug
Projects
None yet
Development

No branches or pull requests

2 participants