Fix Unicode #135

kirawi · 2021-06-06T03:26:12Z

Fix #123, should probably still include #129 as a fallback. Remember how I said there shouldn't be any regressions in #121? Well, I figured out my "fix" for #88 broke b on unicode especially, but now I guess that's fixed here since it properly detects unicode?

kirawi · 2021-06-06T03:26:50Z

Oops, accidentally included old commits.

helix-core/src/transaction.rs

Co-authored-by: Benoît Cortier <benoit.cortier@fried-world.eu>

helix-core/src/movement.rs

kirawi · 2021-06-06T04:12:19Z

Updated. Is the order of the match a problem?

kirawi · 2021-06-06T04:15:25Z

Also, I just found out that it doesn't fix the unicode problem regex problem exactly, I'll look into it tomorrow.

archseer · 2021-06-06T04:15:29Z

I think it looks good, but what is '_' categorized as now?

kirawi · 2021-06-06T04:34:49Z

Added a test, and it's still recognized as a word. The categories can be seen here, https://www.compart.com/en/unicode/category. I'll look into the aforementioned regression tomorrow, or possibly later depending on my schedule.

helix-core/src/transaction.rs

pickfire · 2021-06-06T05:02:17Z

helix-core/src/movement.rs

+        | GeneralCategory::OpenPunctuation
+        | GeneralCategory::InitialPunctuation
+        | GeneralCategory::FinalPunctuation => Category::Punctuation,
+        _ => unreachable!("unknown '{}' character category", ch),


This will fail for other cases, like + it will get a panic. We have to cover all the cases for this, or even just put it under unknown category.

helix-core/src/movement.rs

kirawi · 2021-06-06T05:20:13Z

Might be worth having a separate test containing the equivalent of, "the fox jumped over the... Etc.", though I have no idea what that would be for Unicode. Maybe foreign equivalents? The goal of it would be to check for any panics. I guess we should just get one character from each category.

pickfire · 2021-06-06T05:27:07Z

Might be worth having a separate test containing the equivalent of, "the fox jumped over the... Etc.", though I have no idea what that would be for Unicode. Maybe foreign equivalents? The goal of it would be to check for any panics. I guess we should just get one character from each category.

If you want that, fuzzing will be easier since it spams random characters, then we run our code through the fuzzer.

archseer · 2021-06-06T09:02:20Z

Property testing: https://github.com/BurntSushi/quickcheck

`is_ascii_punctuation` will only work for ASCII punctuations, and when we have unicode punctuation (or other) we jump into the `unreachable`. This patch fallback into categorizing everything in this branch as `Unknown`. Fixes helix-editor#123 helix-editor#135: add better support for unicode categories.

CBenoit · 2021-06-06T23:36:59Z

I changed a bit #129 to return a Unknown variant instead. Your changes should be enough to handle most useful unicodes.

`is_ascii_punctuation` will only work for ASCII punctuations, and when we have unicode punctuation (or other) we jump into the `unreachable`. This patch fallback into categorizing everything in this branch as `Unknown`. Fixes #123 #135: add better support for unicode categories.

kirawi · 2021-06-07T00:38:40Z

I changed a bit #129 to return a Unknown variant instead. Your changes should be enough to handle most useful unicodes.

That's perfect, I was a little worried about how I could create tests for this PR.

pickfire · 2021-06-07T04:33:51Z

@kirawi Can you please rebase and add it for ctrl-w in words.rs as well? And the tests should not have weird behavior like not deleting the unicode punctuations.

kirawi · 2021-06-07T20:13:25Z

#88 was not fixed, that issue should be reopened. I still have no idea what is causing the bug.

hovsater · 2021-06-07T20:25:31Z

@kirawi actually the test case in #88 is fixed. I guess what I'm seeing now could count as a new issue. I'll create one!

kirawi · 2021-06-07T20:27:06Z

@kirawi actually the test case in #88 is fixed. I guess what I'm seeing now could count as a new issue. I'll create one!

Are you sure? I noticed that it chokes if there is more than one unicode, i.e.

key
ささ
key
key

hovsater · 2021-06-07T21:07:49Z

Ah, interesting. Never tried that. Perhaps that's why it chokes on the new input. Well, I'll add a comment to the original issue tomorrow. Getting late here.

helix-core/src/words.rs

pickfire · 2021-06-08T04:15:34Z

helix-core/src/movement.rs

+#[inline]
+pub(crate) fn is_punctuation(ch: char) -> bool {
+    use unicode_general_category::{get_general_category, GeneralCategory};
+
+    matches!(
+        get_general_category(ch),
+        GeneralCategory::OtherPunctuation
+            | GeneralCategory::OpenPunctuation
+            | GeneralCategory::ClosePunctuation
+            | GeneralCategory::InitialPunctuation
+            | GeneralCategory::FinalPunctuation
+            | GeneralCategory::ConnectorPunctuation
+            | GeneralCategory::DashPunctuation
+            | GeneralCategory::MathSymbol
+            | GeneralCategory::CurrencySymbol
+            | GeneralCategory::ModifierSymbol
+    )
 }


I think we should move these pub(crate) and categories stuff to category.rs or something. Maybe in another PR.

pickfire · 2021-06-08T04:16:01Z

helix-core/src/movement.rs

@@ -231,6 +249,7 @@ where
    // need to +1 so that prev() includes current char
    let mut chars = slice.chars_at(*pos + 1);

+    #[allow(clippy::while_let_on_iterator)]


Is this a false positive, maybe report it to clippy?

Not exactly, but the next line operates on next(), and I didn't know what to do with that so I decided to suppress the warning for now.

pickfire

Looks good to me. Merging now as it has gotten quite long but we need some follow up on the category thing. Will test it tonight.

Oh wait, can't merge due to conflicts. Can you please fix the conflicts and address the issues?

archseer · 2021-06-08T04:20:08Z

Squash and merge works

kirawi added 5 commits June 4, 2021 23:13

init

8db48d6

wip

40fe795

wip

1c3dafe

fix unicode break

2b9cd74

fix unicode break

c5edc6c

Merge branch 'master' of github.com:helix-editor/helix into patch-03

c837334

CBenoit reviewed Jun 6, 2021

View reviewed changes

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

kirawi and others added 5 commits June 5, 2021 23:29

Update helix-core/src/transaction.rs

d461702

Co-authored-by: Benoît Cortier <benoit.cortier@fried-world.eu>

Merge branch 'master' into patch-03

4e791f7

clippy

5bbc263

Merge branch 'patch-03' of github.com:kirawi/helix into patch-03

a5a7b78

fix

9d4b0fd

archseer reviewed Jun 6, 2021

View reviewed changes

helix-core/src/movement.rs Show resolved Hide resolved

add changes

e3167f5

added test

4847ef9

wip

5eaa79e

pickfire reviewed Jun 6, 2021

View reviewed changes

helix-core/src/transaction.rs Show resolved Hide resolved

pickfire reviewed Jun 6, 2021

View reviewed changes

helix-core/src/movement.rs Show resolved Hide resolved

kirawi marked this pull request as draft June 6, 2021 18:57

CBenoit mentioned this pull request Jun 6, 2021

Fix panic when moving over unicode punctuation #129

Merged

kirawi added 4 commits June 7, 2021 13:40

wip

deb7b91

wip

3e0cce0

wip

32817e2

wip

b667163

kirawi marked this pull request as ready for review June 7, 2021 20:12

fix

816b206

fix view

98b9e32

kirawi commented Jun 7, 2021

View reviewed changes

helix-core/src/words.rs Show resolved Hide resolved

fix helix-editor#88

732d65a

archseer approved these changes Jun 8, 2021

View reviewed changes

archseer requested a review from pickfire June 8, 2021 03:19

pickfire reviewed Jun 8, 2021

View reviewed changes

pickfire approved these changes Jun 8, 2021

View reviewed changes

archseer merged commit b873fb9 into helix-editor:master Jun 8, 2021

kirawi mentioned this pull request Jun 9, 2021

Movement fixes, refactor and unit test suite #193

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Unicode #135

Fix Unicode #135

kirawi commented Jun 6, 2021

kirawi commented Jun 6, 2021

kirawi commented Jun 6, 2021 •

edited

Loading

kirawi commented Jun 6, 2021

archseer commented Jun 6, 2021

kirawi commented Jun 6, 2021

pickfire Jun 6, 2021 •

edited

Loading

kirawi commented Jun 6, 2021 •

edited

Loading

pickfire commented Jun 6, 2021 •

edited

Loading

archseer commented Jun 6, 2021

CBenoit commented Jun 6, 2021

kirawi commented Jun 7, 2021

pickfire commented Jun 7, 2021 •

edited

Loading

kirawi commented Jun 7, 2021

hovsater commented Jun 7, 2021 •

edited

Loading

kirawi commented Jun 7, 2021

hovsater commented Jun 7, 2021 •

edited

Loading

pickfire Jun 8, 2021

pickfire Jun 8, 2021

kirawi Jun 8, 2021 •

edited

Loading

pickfire left a comment •

edited

Loading

archseer commented Jun 8, 2021

Fix Unicode #135

Fix Unicode #135

Conversation

kirawi commented Jun 6, 2021

kirawi commented Jun 6, 2021

kirawi commented Jun 6, 2021 • edited Loading

kirawi commented Jun 6, 2021

archseer commented Jun 6, 2021

kirawi commented Jun 6, 2021

pickfire Jun 6, 2021 • edited Loading

Choose a reason for hiding this comment

kirawi commented Jun 6, 2021 • edited Loading

pickfire commented Jun 6, 2021 • edited Loading

archseer commented Jun 6, 2021

CBenoit commented Jun 6, 2021

kirawi commented Jun 7, 2021

pickfire commented Jun 7, 2021 • edited Loading

kirawi commented Jun 7, 2021

hovsater commented Jun 7, 2021 • edited Loading

kirawi commented Jun 7, 2021

hovsater commented Jun 7, 2021 • edited Loading

pickfire Jun 8, 2021

Choose a reason for hiding this comment

pickfire Jun 8, 2021

Choose a reason for hiding this comment

kirawi Jun 8, 2021 • edited Loading

Choose a reason for hiding this comment

pickfire left a comment • edited Loading

Choose a reason for hiding this comment

archseer commented Jun 8, 2021

kirawi commented Jun 6, 2021 •

edited

Loading

pickfire Jun 6, 2021 •

edited

Loading

kirawi commented Jun 6, 2021 •

edited

Loading

pickfire commented Jun 6, 2021 •

edited

Loading

pickfire commented Jun 7, 2021 •

edited

Loading

hovsater commented Jun 7, 2021 •

edited

Loading

hovsater commented Jun 7, 2021 •

edited

Loading

kirawi Jun 8, 2021 •

edited

Loading

pickfire left a comment •

edited

Loading