Add command to inc/dec number under cursor

[jasonrhansen]

Closes #967

With the cursor over a number in normal mode, Ctrl + A will increment the
number and Ctrl + X will decrement the number. It works with binary, octal,
decimal, and hexidecimal numbers. Here are some examples.

0b01110100
0o1734
-24234
0x1F245

[EpocSquadron]

Would it be possible to simplify some of the number locating logic by querying the current tree-sitter scope?

[jasonrhansen]

Using tree-sitter where possible sounds like a good idea and it's something I could look into. But wouldn't it still make sense to have a fallback for times when tree-sitter isn't available for a file type?

[archseer]

Generally looks good! I wonder if we could simplify detection:

• use textobject_word from helix_core (using the long word) to find the current word under cursor
• Slice the word, let word = Cow::from(range)
• check for 0b/0o/0x prefix, if so, try to parse the remainder. If it fails then it clearly either wasn't a number or was too big to fit
• fallback to base 10 parsing

I'm not sure if we want to change the selection, but if we do then you can return the new range. The algorithm should only take a text and a single range, that way we can selection.transform(|range| increment(range).or(range)) the entire selection to either increment the number if detected, or retain the old range across all ranges

[archseer]

I assume this is trying to safely downcast to i64? You can parse directly into i64::from_str_radix. Using try_from to convert from 128 -> 64 would also work, but no reason not parsing into i64 directly.

[jasonrhansen]

In Vim's implementation decrementing 0x0 results in 0xffffffffffffffff, which is -1 for i64. However, if we try to parse that with i64::from_str_radix we get an error because it tries to parse it as a positive number and overflows. If I parse it as an i128, I can cast it to i64 to get the negative value.

try_from wouldn't work here either because the i128 contains a positive number, which only becomes negative as an i64 because we're left with a 1 in the most significant bit.

Maybe there's a more elegant way we could do this?

[jasonrhansen]

@archseer Some of the complexity of this implementation is due to the fact that I was trying to match the behavior of Vim as much as possible. Of course there's no reason it needs to match Vim's behavior -- it's just what I'm used to. Based on your comments, I think you're expecting something a little simpler so I guess we really need to decide what helix should support.

Here are couple of things this implementation handles that maybe we don't want to:

1. Jumping to the first number if the cursor isn't already on a number. So if the cursor is here

this is a line of text 0x01FD with a number
          ^

pressing Ctrl + a will result in this

this is a line of text 0x01FE with a number
                            ^

2. Numbers that are part of a word with other characters are detected. So

foo456bar

gets incremented to

foo457bar

Do we want to change it so the cursor has to be on a number and the number must not be part of a word with other characters?

[sudormrfbin]

Jumping to the first number if the cursor isn't already on a number

Most of the time when I use ctrl-a/x in vim I rely on it jumping to the nearest number, so personally I do like this feature.

[archseer]

Hmm really? I was thinking we could scope it to the current word. It might get weird with multiple selections if you're trying to increment but some of them don't contain numbers and end up incrementing the next number in the text

[sudormrfbin]

Ah I didn't think about multiple selections; in that case it would be better to remove the feature.

[jasonrhansen]

@archseer This is why I explicitly disabled it for multiple cursors.

So I guess the problem is I implemented this to work the way it does in Vim, but it's sounding like this wouldn't feel right for helix, which I completely understand. I wouldn't mind simplifying it to work with the current word, but I'd like to hear other opinions.

[pickfire]

Most of the time when I use ctrl-a/x in vim I rely on it jumping to the nearest number, so personally I do like this feature.

I do want to use this as well, I wish it works for date too.

[pickfire]

So I guess the problem is I implemented this to work the way it does in Vim, but it's sounding like this wouldn't feel right for helix, which I completely understand. I wouldn't mind simplifying it to work with the current word, but I'd like to hear other opinions.

I do think it might be better to keep it for multiple selections, it would be surprising if users increment but it only work for primary.

[jasonrhansen]

I've simplified the implementation quite a bit by removing some of the complexity that was there only to make it behave like Vim. It also now works with multiple selections.

[archseer]

I quite like this version of the code! Will take a second look tomorrow

[archseer]

Hmm okay, after some testing I think you will still need to set .with_selection() on the transaction. Might be good enough to simply use a copy of the current selection before the edit, that way it doesn't move.

If a selection isn't set then the change automatically shifts my cursor to right after the selection, or it ends up selecting the whole number. I'm also able to increment the number if I'm positioned right after it:

https://asciinema.org/a/nvzSKoSMYz9dYup2g6jpfgyZv

I'm also not able to increment the number if it's followed by other text (i.e. 1; as in let a = 1;)

[archseer]

I think the second issue can be fixed by setting the long word parameter of the text object function to false, that way it stops the range before ;

[pickfire]

I don't understand what does this do. Can you please put a comment here? Is it because the integer may be too large? Or is it to handle the cast?

[jasonrhansen]

In Vim's implementation, decrementing 0x0 results in 0xffffffffffffffff, which is -1 for i64. However, if we try to parse that with i64::from_str_radix we get an error because it tries to parse it as a positive number and overflows. If I parse it as an i128, I can cast it to i64 to get the negative value.

This check just makes sure the number fits in an i64. You may be wondering why I didn't use try_from to convert to i64, and the reason is it wouldn't work for parsing negative numbers that aren't base 10.

let value = i128::from_str_radix("ffffffffffffffff", 16).unwrap();
let value = value as i64;
// `value` now is -1

but

let value = i128::from_str_radix("ffffffffffffffff", 16).unwrap();
let value = i64::try_from(value).unwrap();

This version will get a TryFromIntError error because the original i128 is actually a positive number that's too large to fit in an i64.

Can you think of a better way to handle this, or do you think adding a comment is the best option?

[jasonrhansen]

Maybe the best thing to do here is change the behavior from what Vim does.

Vim treats 0xFFFFFFFFFFFFFFFF as -1, because that's the bit pattern of -1 for a 64 bit two-complement integer. Vim only handles the minus sign for base 10 numbers.

So in Vim -0x01 gets incremented to -0x02 instead of 0x00 because it ignores the minus sign. We could make helix only interpret the number as negative if it has a minus sign.

Also in Vim 0x00 gets decremented to 0xffffffffffffffff, but we could have helix decrement it to -0x01.

But this would also mean that 0xffffffffffffffff would be too large and wouldn't be parsed as an i64.

What do you think?

[pickfire]

I think the easiest way here might be just store it as i128 but either way, this is already merged so it should be good.

[pickfire]

Other than the comments left it looks good to me. Well done!

[archseer]

Thanks for following through with this PR! I think the current iteration looks great 🎉