Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix backwards character deletion on other whitespaces #2855

Merged
merged 4 commits into from
Jul 1, 2022
Merged

Fix backwards character deletion on other whitespaces #2855

merged 4 commits into from
Jul 1, 2022

Conversation

SoraTenshi
Copy link
Contributor

This issue closes #2842

In summary:
delete_char_backward ignored any other whitespace other than ' ', which resulted in you not being able to backspace or C-h on insert mode when there's only whitespace with the character next to the cursor being a lexiographic whitespace.

@kirawi
Copy link
Member

kirawi commented Jun 21, 2022

We actually probably just want to change this check here:

if !fragment.is_empty() && fragment.chars().all(|ch| ch.is_whitespace()) {

Most indentation will be using spaces or tabs, rather than full-width characters. I don't have a strong opinion on it though.

@SoraTenshi
Copy link
Contributor Author

We actually probably just want to change this check here:

if !fragment.is_empty() && fragment.chars().all(|ch| ch.is_whitespace()) {

Most indentation will be using spaces or tabs, rather than full-width characters. I don't have a strong opinion on it though.

actually, i don't have any strong opinion either, however the change happened like that because this already looked rather wrong.

@@ -2981,7 +2981,7 @@ pub mod insert {
for _ in 0..drop {
// delete up to `drop` spaces
match chars.next() {
Some(' ') => start -= 1,
Some(c) if c.is_whitespace() => start -= 1,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this won't work correctly with whitespace that has a wider width than 1 (eg. space used in Japanese text).

The body of this function should be extracted into helix-core and properly unit tested.

Copy link
Contributor Author

@SoraTenshi SoraTenshi Jun 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this won't work correctly with whitespace that has a wider width than 1 (eg. space used in Japanese text).

Well the Japanese text spaces are Ideographic Spaces.
I also filed this bug because i am using a japanese ime quite frequently.

This change has been tested locally, and works.

However i see what you mean, would it maybe make sense to something similar as the width variable for the substraction of the pos?

Some(c) if c.is_whitespace() => start -= c.width().unwrap_or(1),

@@ -2981,7 +2981,7 @@ pub mod insert {
for _ in 0..drop {
// delete up to `drop` spaces
match chars.next() {
Some(' ') => start -= 1,
Some(c) if c.is_ascii_whitespace() => start -= 1,
Copy link
Member

@kirawi kirawi Jun 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is an unnecessary change, as aside from line endings, tab and space are the only two recognized as ascii whitespace.

@SoraTenshi SoraTenshi requested review from archseer and kirawi June 28, 2022 00:23
@archseer archseer merged commit edee2f4 into helix-editor:master Jul 1, 2022
@SoraTenshi SoraTenshi deleted the bw-deletion-accept-any-ws branch July 1, 2022 18:07
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot backspace on full-width space character
3 participants