Skip to content

More closely match Neovim's word-movement semantics #12160

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

More closely match Neovim's word-movement semantics #12160

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 43 additions & 8 deletions helix-core/src/chars.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,23 @@

use crate::LineEnding;

#[derive(Debug, Eq, PartialEq)]
pub enum WordCategory {
Copy link
Member

Choose a reason for hiding this comment

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

why do we need this differentation? I don't see us ever inspecting the category, we only seem to check wether something is a word character or not.

In general it's not clear to me what the goal of this PR is, is just to treat

      '\u{2070}'..='\u{207f}' => Superscript,
        '\u{2080}'..='\u{2094}' => Subscript,
        '\u{2800}'..='\u{28ff}' => Braille,
        '\u{3040}'..='\u{309f}' => Hiragana,
        '\u{30a0}'..='\u{30ff}' => Katakana,
        '\u{ac00}'..='\u{d7a3}' => HangulSyllable,

        '\u{3300}'..='\u{9fff}'
        | '\u{f900}'..='\u{faff}'
        | '\u{20000}'..='\u{2a6df}'
        | '\u{2a700}'..='\u{2b73f}'
        | '\u{2b740}'..='\u{2b81f}'
        | '\u{2f800}'..='\u{2fa1f}' => CJKIdeograph,

as word characters? I can't read ow write these languages but generally the rust unicode characterization is quite accurate and I don't think we should be blindly adding to it (also adds overhead to one of the most commonly called functions in the codebase).

If you take a look here:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=5e40c4176f08205842feb4f39b3a5399

some of the codepoint ranges covered here already fully fall under alpha_numeric and for those that don't I am skeptical they make sense to include. For example some output from the script:

no alphanumeric 207a ⁺
no alphanumeric 207b ⁻
no alphanumeric 207c ⁼
no alphanumeric 207d ⁽
no alphanumeric 207e ⁾
no alphanumeric 208a ₊
no alphanumeric 208b ₋
no alphanumeric 208c ₌
no alphanumeric 208d ₍
no alphanumeric 208e ₎

I wouldn't want to just blindly copy what nvim is doing. Historically unicode treatment is a mess. If we are going to go beyond unicode standard definition of alphanumeric then I would like to see the ranges selected so they don't overlap with our current definition and an explanation why these ranges are necessary in a comment.

My gut feeling on this is that the only thing where this more complex unicode word definition actually makes sense is for movements that care about word boundaries/segmentation (like miw and e/b/w). Proper Unicode segmentation is quite a bit more complex than adding a couple characters here (unicode-segmentation has that builtin, it's not a trivial algorithm: https://docs.rs/unicode-segmentation/latest/src/unicode_segmentation/word.rs.html#202)

Copy link
Member Author

@kirawi kirawi Dec 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

So what this change does is that when we check word boundaries for w/b, we check if the previous and next char have the same category. The subcategory adds differentiation so they are treated as separate words

Copy link
Member

@pascalkuthe pascalkuthe Dec 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

ah ok I didn't see it show up in the diff but the comparison is different I suppose. I still wonder if we shouldn't just do proper word segmentation in the places where this is used

Could we just leave char_is_word as is and only change the categorization? The function is very frequently used and I dont' really want to change it

Copy link

@TopSalad3530 TopSalad3530 Apr 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

"Proper word segmentation" as performed by unicode_segmentation simply treats each CJK character as its own "word". It is valid for soft-wrapping, but is extremely useless for editing. Example: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=520d54ca25851bd7b5e19c1b800f1c4e

This produces ["吾", "輩", "は", "猫", "で", "あ", "る"], which is, as you can probably guess, extremely useless. The categorization proposed by this PR would instead produce ["吾輩", "は", "猫", "である"], which is much more useful for actually writing in the language.

This does however only improve the situation for Japanese. For Chinese and Korean, useful segmentations can only be produced through the use of a dictionary, since those languages each use only a single script. This could be a use case for the proposed plugin system, so people who don't edit text in those language won't get any of the bloat.

Alphanumeric,
Superscript,
Subscript,
Braille,
Hiragana,
Katakana,
HangulSyllable,
CJKIdeograph,
}

#[derive(Debug, Eq, PartialEq)]
pub enum CharCategory {
Whitespace,
Eol,
Word,
Word(WordCategory),
Punctuation,
Unknown,
}
Expand All @@ -17,8 +29,8 @@ pub fn categorize_char(ch: char) -> CharCategory {
CharCategory::Eol
} else if ch.is_whitespace() {
CharCategory::Whitespace
} else if char_is_word(ch) {
CharCategory::Word
} else if let Some(cat) = char_word_category(ch) {
CharCategory::Word(cat)
} else if char_is_punctuation(ch) {
CharCategory::Punctuation
} else {
Expand Down Expand Up @@ -55,7 +67,7 @@ pub fn char_is_whitespace(ch: char) -> bool {
// En Quad, Em Quad, En Space, Em Space, Three-per-em Space,
// Four-per-em Space, Six-per-em Space, Figure Space,
// Punctuation Space, Thin Space, Hair Space, Zero Width Space.
ch if ('\u{2000}' ..= '\u{200B}').contains(&ch) => true,
'\u{2000}' ..= '\u{200B}' => true,

_ => false,
}
Expand All @@ -82,7 +94,31 @@ pub fn char_is_punctuation(ch: char) -> bool {

#[inline]
pub fn char_is_word(ch: char) -> bool {
ch.is_alphanumeric() || ch == '_'
char_word_category(ch).is_some()
}

pub fn char_word_category(ch: char) -> Option<WordCategory> {
use WordCategory::*;

// Different subcategories so e.g. おはよう世界 is not treated as one block
let level = match ch {
'\u{2070}'..='\u{207f}' => Superscript,
'\u{2080}'..='\u{2094}' => Subscript,
'\u{2800}'..='\u{28ff}' => Braille,
'\u{3040}'..='\u{309f}' => Hiragana,
'\u{30a0}'..='\u{30ff}' => Katakana,
'\u{ac00}'..='\u{d7a3}' => HangulSyllable,

'\u{3300}'..='\u{9fff}'
| '\u{f900}'..='\u{faff}'
| '\u{20000}'..='\u{2a6df}'
| '\u{2a700}'..='\u{2b73f}'
| '\u{2b740}'..='\u{2b81f}'
| '\u{2f800}'..='\u{2fa1f}' => CJKIdeograph,
ch if ch.is_alphanumeric() || ch == '_' => Alphanumeric,
_ => return None,
};
Some(level)
}

#[cfg(test)]
Expand Down Expand Up @@ -115,9 +151,8 @@ mod test {
}

for ch in WORD_TEST_CASE.chars() {
assert_eq!(
CharCategory::Word,
categorize_char(ch),
assert!(
matches!(categorize_char(ch), CharCategory::Word(_)),
"Testing '{}', but got `{:?}` instead of `Category::Word`",
ch,
categorize_char(ch)
Expand Down
6 changes: 3 additions & 3 deletions helix-core/src/movement.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -495,16 +495,16 @@ fn is_word_boundary(a: char, b: char) -> bool {

fn is_long_word_boundary(a: char, b: char) -> bool {
match (categorize_char(a), categorize_char(b)) {
(CharCategory::Word, CharCategory::Punctuation)
| (CharCategory::Punctuation, CharCategory::Word) => false,
(CharCategory::Word(_), CharCategory::Punctuation)
| (CharCategory::Punctuation, CharCategory::Word(_)) => false,
(a, b) if a != b => true,
_ => false,
}
}

fn is_sub_word_boundary(a: char, b: char, dir: Direction) -> bool {
match (categorize_char(a), categorize_char(b)) {
(CharCategory::Word, CharCategory::Word) => {
(CharCategory::Word(_), CharCategory::Word(_)) => {
if (a == '_') != (b == '_') {
return true;
}
Expand Down
Loading