search

onivim / libvim

libvim: The core Vim editing engine as a minimal C library
https://v2.onivim.io
MIT License
691 stars 34 forks source link

N-Squared D fix. #177

Closed CrossR closed 4 years ago

CrossR commented 4 years ago

A quick test of a fix for the n-squared delete issue: https://github.com/onivim/oni2/issues/561

As far as I can tell, elsewhere in the code, when we bail out of a command as it has not completed, we set opcount = 0. However, at this location (that runs when a command is not complete) we do not.

This means on the second pass through for the final d (in an example command of 5dd) we have 5 in opcount and 5 in count0. This is actually a valid state in vim, since you could do 2d2d to delete 4 lines. So when we have 5 in both, they get multiplied together to give us the final n squared.

Setting it back to 0 here seems to be consistent with the other parts of the code, and fixes the issue, but I'm going to do some more checks I think.

@bryphe, I think this part is mainly written specifically for Oni, so can you see if this looks sensible?

CrossR commented 4 years ago

I'm going to see if I can fix the 5d3d case which should delete 15 lines as well, which currently fails by deleting 53 lines (by sticking 5 and 3 together as strings as far as I can tell).

(That said, without the current fix it used to do the exact same and then times that 53 by 5 to give 265 🤦‍♂ , so its "better" already).

EDIT: Looks like that is the desired behaviour, but we aren't fixing it back to 15 at a later point.

bryphe commented 4 years ago

Thanks for looking at this @CrossR , awesome - will be so nice to have this fixed!

bryphe commented 4 years ago

EDIT: Looks like that is the desired behaviour, but we aren't fixing it back to 15 at a later point.

Ya, it looks like for the 5d3d case, we're hitting the concatenation functionality here: https://github.com/onivim/libvim/blob/f01c98716d556375a96d8da38f1d9a3d520418c8/src/normal.c#L670

Instead of the multiplication here: https://github.com/onivim/libvim/blob/f01c98716d556375a96d8da38f1d9a3d520418c8/src/normal.c#L709

Might help to log the values of ca.count0 and ca.opcount across / along with the different 'states' - might help us highlight where things are getting out of sync.

CrossR commented 4 years ago

Might help to log the values of ca.count0 and ca.opcount across / along with the different 'states' - might help us highlight where things are getting out of sync.

I've done this, and I still can't see where we are going wrong, it looks like there is just a missing state as we never actually seem to set opcount such that we could multiply by it.

CrossR commented 4 years ago

Looks like I was missing that opcount is set to 0 in vim normally, but it is then restored. Since we don't restore it (from opcount directly I mean, we do restore ca as a whole), we don't want to set it to 0. Instead, we do need to set count0 to be empty, such that when we do the concat part we end up with just that number, not the previous step and the current one.

I could be missing something, but this seems correct and all the tests pass.

bryphe commented 4 years ago

Nice catch on the set_prevcount. Looks great @CrossR - can't wait to have this in!