-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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.
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
base: master
Are you sure you want to change the base?
Conversation
Should we diverge from Neovim here and implement checking based on https://docs.rs/unicode-script/0.5.7/unicode_script so that different scripts are treated as different words? Or should that kind of behavior be limited to #11423? |
This comment was marked as outdated.
This comment was marked as outdated.
@@ -2,11 +2,23 @@ | |||
|
|||
use crate::LineEnding; | |||
|
|||
#[derive(Debug, Eq, PartialEq)] | |||
pub enum WordCategory { |
There was a problem hiding this comment.
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:
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)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
As defined per https://github.com/neovim/neovim/blob/ab9cfc4dc3422af5235759efef456d3e02745217/src/nvim/mbyte.c#L1220-L1344 This PR improves word-level segmentation for CJK in particular especially with respect to kanji-hiragana-katakana boundaries.
All changes outside of
chars.rs
were just to make it compile. I need to inspect how the affected functions are being used.