fix: restore cursor position correctly when using dot repeat

[ribru17]

Fixes #112

[ribru17]

Hey! Hope you're having a great summer. Just thought I would reopen the dot repeat discussion if that's alright. I know solutions that involve an extra mapping are never ideal but I honestly don't think there is any other way of solving this issue. Unless there's something I'm unaware of, I believe the pre-motion cursor position is totally lost when doing dot repeat due to the way Vim works and I think this PR may be the most idiomatic (or only) way to fix the issue. But if you or anyone else thinks this is not the way to go I totally get it and would love to learn from someone wiser than myself. If not, hopefully this can get merged. Thanks!

[kylechui]

Hey Riley, I'm having a good summer and hope you are too! Huge thanks for the PR as well. I do agree that this is probably the only way to do things, since as you mentioned dot-repeating should discard cursor information (when working with motions). To be honest with you I didn't consider (or even think of) remapping . directly. Seeing as we're just caching some data (i.e. read-only), there shouldn't be any compatibility issues/data races with other plugins. I'm quite tempted to merge this soon, since it would finally close #112.

In the meantime, do you mind using updated syntax for cmd, i.e. something like vim.cmd.normal(".", { bang = true, remap = true })? I would prefer less hard coded strings in my code, regardless of how minor. Also probably move it into the config.setup, since the setup function you put it in is meant to be a compatibility layer (so you can call setup directly). Many thanks for the contribution; I'll try to remember to ask some others if it's possible without this "hack", or if normal does anything unexpected (I know feedkeys is a bit weird).

[ribru17]

Oh cool thanks Kyle! I didn't know about this syntax that's pretty cool. Just migrated everything, and I also removed the remap = true from the keymap. I sort of left it in the first time by accident, I think the bang being in the normal command will prevent remapping from that end and other than that the function doesn't run or return any expression which could be remapped. Hopefully that sounds right to you? (Side note: the bang must be there in the normal command or else the mapping will recursively call itself forever.) I've tested this minimally to confirm all is working. Thanks for the help!

[kylechui]

Alright so I asked around on the Matrix chat and there are two things to note:

• If another plugin decides it needs to overwrite . somehow (or if the user does so themselves), then there will be a conflict (very bad)
• Folke's flash.nvim apparently uses some kind of handler to detect when a dot-repeat command is executed; probably something to look into

[ribru17]

I see... take a look at this most recent commit, taken straight from folke's plugin. Pretty smart solution. I've tested it and it works correctly even when the user has plugins that map to and from '.' (even without remap = true ) which also means, I believe, that any plugins that redefine this behavior will be completely unimpacted. ~The only downside I can think of would be that it clobbers any other functions for the on_key event but I highly doubt this would be the case.~ This is not the case! :+1:

[kylechui]

Oh wow that does seem interesting. The one thing that immediately jumps out to me is why it's disabled inside macros, e.g. if I use dot-repeat inside a macro, I would think that the cursor position should still be cached. For example, would something like qaysiWbj.q properly keep the cursor position fixed? Another question that popped into my head is what if someone mapped , to .. Would ,-repeating work properly? It'll probably take me a bit more time before I merge this in because I want to add unit tests to this and properly ensure that the on_key call is transparent to the end-user.

[ribru17]

Yeah I'm not sure why that was put in there, I'm sure there's a good reason but definitely would need more looking-into. As for the remapping example, this works just fine. Even without remap = true such a mapping works and the cursor position is still tracked.

SIDE NOTE: The current commit does not work when surrounding text with a ., e.g. ysiw. will not keep the cursor position. This will need to be fixed as well.

[kylechui]

if I had to take a guess, it would be that flash doesn't need to show any animations when a register is being executed (since that would just be a waste of resources), so the check is in place for that.

[ribru17]

Ah smart. In that case check out the latest commit. I've done some more testing, and from what I can see initially this doesn't break anything and works properly with mappings, macros (both recording and executing), etc. It also solves the issue where something like ysiw. would not store the cursor position properly.

[kylechui]

I think the main issue with this approach is that we are essentially trying to mimick a predicate is_dot_repeat_executing(), which is impossible to simulate with on_key. For example, what if someone types . in a text box? What if someone types . in some Vim-motion? I'm no longer sure if those drawbacks are worth the benefit of closing this issue :/

[ribru17]

I'm having trouble understanding this, I am pretty sure this solution works. Essentially the current solution makes it so that any . key presses that are not part of a surround dot repeat are completely isolated events and won't break anything, since whenever a . key is pressed and it is part of a surround dot repeat, all previous data stored by past dot repeats are overwritten. Essentially the surround dot repeat exists in a vacuum, as each dot will overwrite the previous curpos with the current one (meaning something like :my.command.<CR> and then a dot repeat will work just fine). The only problem is that with something like ysiW.. This is unique because the dot comes before a dot repeat and before the actual surround, meaning the cursor will be (incorrectly) overwritten right before the surround executes, making it so that the cursor always goes back to the beginning of the word. That is why I added that hacky boolean which keeps track of whether we are currently in the middle of inputting some surround. Then we will know that in that specific case only, we should ignore the . and keep the cursor position as it was before. This also means that any . typed as a motion while surrounding will also work just fine, since the lock on that data only releases after the surround has been applied.

[kylechui]

Ah, yes that makes sense. I had originally gotten some stuff mixed up in my head. One last thing I would like to see before I merge: testing. More or less one or two basic ones that validate the cursor doesn't move during normal operation, then maybe a few of the edge cases with . that we mentioned before. I'd be down to write them myself as well, although I'm not sure how the commits would look for that (do I clone your fork and PR there?) I would like to get the changes + testing in one commit. Also as a side note I don't remember if I mentioned this "quirky" behavior anywhere in the help docs, so if that's the case I'll want to rove that as well (moreso a note for myself). Thanks for taking the time to explain it!

[ribru17]

No problem! And sure, I can also try and add some tests later today but if you get tired of waiting for me or if you want to touch up ones I have already done I think you should have full access to commit to this PR, you might just need to clone my fork or add my fork as a remote to your local repo (whichever is easiest). I believe I gave you full access to push to master (if you can't I did something wrong lol). Thanks!

[kylechui]

Hey @ribru17, sorry for the delay! I took some time to move some things around, but there's one last "bug" that's occurring that I would like to fix. After that, I'll merge it in!

[kylechui]

Update: If the normal_surround command is interrupted, i.e. ysiw<Esc>, then the cursor will still move.