Skip to content
This repository

Repeating text changing doesn't work. #148

Closed
jordwalke opened this Issue · 51 comments

4 participants

Jordan W Guillermo López-Anglada Weslly H Fredrik Ehnbom
Jordan W

Type the following text:

abcdefg
abcdefg

Visually highlight abc and then type "c" for change.

Then replace abc with 123 and hit escape.

You will now have

123defg
abcdefg

Then position your cursor on the next "a".

Hit repeat (".") - it will not replace abc with 123.

Instead, you will have:

123defg
123abcdefg
Guillermo López-Anglada
Owner

I've been postponing this one for too long, but it looks like it's going to be rather tricky to get working. I seems surprisingly simple in Vintage, but I can't find a way to make it work in Vintageous.

Jordan W

I haven't thought about it in too much depth, but couldn't it be as simple as replaying the last set of keystrokes that resulted in a text edit? Or one segment of operations that were glued together in the undo stack?

Guillermo López-Anglada
Owner

It is indeed as simple as that, but, as far as I can tell, some intermediary steps are getting in the way and polluting the undo stack. Separating genuine edits to the buffer from these other bookkeeping commands is what's not so obvious here. Most likely, I will have to revert vi_enter_insert_mode and friends to stand-alone commands, and stop processing them through the usual set_command path that legit edit commands must use.

Confusingly, though, the marking and gluing of edits is working for insert mode, and I don't think entering insert mode is too much different from entering visual mode right now.

Guillermo López-Anglada
Owner

Also, a quick experiment to enable grouping of edits in replace mode failed as well. Must take a closer look.

Guillermo López-Anglada
Owner

I've found a fix, at least a partial one, but the downside is I don't fully understand why it's working :/

Jordan W

I'd be curious to see a gist of what ended up working - it might be a good way for me to get a good overview of the Vintageous codebase. Maybe you could point out your initial reasoning.

Guillermo López-Anglada
Owner

I'm trying to figure out when is it ok to call the mark_undo... and glue... S3 built-in commands. Vintage does a bit of extra work with them, and I'm not taking exactly the same approach in Vintageous. It remains to be seen whether Vintage's way is the only valid one.

In the meantime, my experiments show that you cannot call glue_marked_undo_groups from a command that is meant to be subsumed into the glued group.

That would explain why I can make v3lcXXX<ESC> work, but not v2ed (unpublished code). In the first case, the gluing occurs after you press ESC, which is a completely separate step, so everything is fine. In the second case, the run.py code transitions to the next mode while the full run of the command hasn't finished. In fact, if I override the code dealing with the follow_up_mode in run.py, v2ed can be repeated too just as good, but then, of course, the mode handling gets broken.

I think the solution requires to move .do_follow_up_mode() from run.py to state.py's .run() and put it after the full evaluation of the vi command is done (the call to vi_run). I seem to remeber I've tried this before quite unsuccessfully, but I'll have to revisit the idea.

There's also another piece in the puzzle. For me, v3lcXXX<ESC> started to be repeatable by simply replacing mark_undo_groups_for_gluing for maybe_mark_undo_groups_for_gluing in the current code. The former should be used in some places where a whole new 'group' is desired (like by pressing v), but if I do use it, things stop working.

Guillermo López-Anglada
Owner

Might be related, because I used to handle the next mode transition in state.py's .reset(). #149.

Jordan W

Here's an issue from Vintage, where I had some difficulties with gluing and redo (not sure if the same troubles would come with repeat as well).

sublimehq/Vintage#153

It may not be related at all, but at least I can share in your confusion. This one issue related to yank was the Vintage deal-breaker bug IMHO.

Guillermo López-Anglada
Owner

I'm almost there, at least on the surface it looks like it... It seems GH won't let me create a fork of my own repo, and because some problems with hg-git (I use Mercurial), I cannot create temp branches either without breaking my local repo.

Basically, the mode change cycle needs to be defined in more stricter terms. I'm taking a first step toward that goal, but it won't be finished with this patch when it's ready. Shoehorning the mode handling code into S3's macro and repeat restrictions feels too forcible at the moment. It isn't too bad now, but it isn't too clear either.

Guillermo López-Anglada
Owner

(Your referenced issue is definitely related, but will probably have to be dealt with separately after this one.)

Jordan W

As far as I can tell, my referenced issue works excellently in Vintageous so that's great news.

Guillermo López-Anglada guillermooo referenced this issue from a commit
Guillermo López-Anglada #148 #149: improve mode handling and enable repetition of commands pe…
…rformed in visual and visual line mode
f0cada2
Guillermo López-Anglada
Owner

@jordow Can we close this one?

Jordan W

It doesn't seem to work. In fact, ce used to work correctly, but now that doesn't even work (I downloaded master and tried just a moment ago).

Jordan W

Here's the repro:

test1

test2

test3

Then in normal mode, do cw on "test1" and type anything like "hello".

Then, go to the next line and hit the repeat key. Nothing happens.

Guillermo López-Anglada
Owner

Weird. I made sure to test on my end and it was working. I'll take a look at this tonight.

Guillermo López-Anglada
Owner

I get the same. It was definitely working before. I must have messed up something while refactoring.

Guillermo López-Anglada
Owner

ce in normal mode is working fine here, though.

Guillermo López-Anglada
Owner

Are you ussing S3 dev 3022? I've just upgraded and everything is back to normal.

Guillermo López-Anglada
Owner

Ok, so ce in normal mode works. ce, then "." works as well. What isn't working is ceXXX then ".".

Jordan W

I was using 3021. I can update and then try again, but in 3022 are both ce and cw working for you?

Guillermo López-Anglada
Owner

They are, but they don't repeat correctly.

Guillermo López-Anglada
Owner

BTW, updating is a requirement, because the repeat command "." depends on this change introduced in 3022:

API: view.command_history(0, True) now returns the last modifying command, as expected

Jordan W

I see. Then what version were you running when everything worked correctly for you (including repeat?)

Guillermo López-Anglada
Owner

It's getting a bit confusing, so let my clarify:

Repeating commands has never worked as expected in all cases. It used to work acceptably well in normal mode, but not so much so in any visual mode. The whole design of Vintageous revolves around satisfying macro replay, repeat and undo requirements. It's clearly not meeting its goal, but I still don't think it's completely flawed either.

The latest far-reaching change addressing mode handling/repetition was f0cada2.

Since then, I seem to be able to repeat commands in visual mode fine, but repeating in command mode has started to work worse.

Specifically, this is the current situation for the c command (if I'm not mistaken):

(I'm focusing on c because it changes modes midway from normal to insert mode, so AFAICT it's the most complex modifying command to get right.)

Normal mode

  1. ce (Can be repeated)
  2. ceXXX (Cannot be repeated; XXX gets tucked at the front of the word under the cursor, instead of overwriting 3 chars.)

(1) is expected, as the whole thing happens in a single vi_run evaluation.

(2) is kind of expected too, because ce and XXX happen in two separate steps (vi_run, insert_characters). After ce, Vintageous transitions to insert mode and puts a mark to glue undo groups. The mark, however, should be set during ce. My first experiments with that have failed, though.

It beats me that (2) used to work before the aforementioned change.

Visual mode

  1. ce (Can be repeated.)
  2. ceXXX (Can be repeated.)

(1) and (2) are both comprised in a single undo group (because we are in visual mode, and we've set the to be glued mark). This is quite a success, but it doesn't translate well to normal mode (technically, _MODE_INTERNAL_NORMAL in the implementation; MODE_NORMAL is used for pure motions only, not for modifying commands), although it should, because it's basically the same thing minus the visible selections.

I suspect the key is .enter_normal_mode(), where undo groups are finally glued together. Same as .enter_visual_mode(), it should set a to be glued mark too, but doing both things doesn't seem to work.

I think this is more or less an accurate picture of the current situation, but take it with a grain of salt...

Guillermo López-Anglada
Owner

Is this working better now after the recent changes?

Guillermo López-Anglada
Owner

@quarnster, @weslly, @jordow Sorry to barge into your inboxes, but this a fundamental feature. Any feedback? Keeping my fingers crossed (it seems to work for me now).

Weslly H

Works fine for me.

Guillermo López-Anglada
Owner

:joy::joy_cat:

Fredrik Ehnbom

Undo and then repeat doesn't work as expected.

Testdata:
Hello world

Input:
ce goodbye esc u . u .

Expected behaviour would be to toggle between Hello world and goodbye world

Guillermo López-Anglada
Owner

@quarnster Oh, so "." must actually redo_or_repeat? I didn't know that. If in the above you substitute "." for Ctrl+r, do you get the desired outcome?

Fredrik Ehnbom

In this particular instance, yes. However if I change the input sequence to:

ce goodbye esc u w .

The output should be:
Hello goodbye

Which it isn't if I simply replace the . with ctrl+r

Jordan W

Testing now - sorry for the delay. I agree that this is important.

Jordan W

It seems this works better than it did before (on latest sublime build). However it seems there is one remaining critical behavior that seems to be incorrect (possibly an easy fix). It's the issue that @quarnster pointed out. I believe the easiest way to describe it is:

"." has nothing to do with redo. It is it's own concept that is separate from the undo stack itself, and more tied to the concept of "the last set of keystrokes input by the user that resulted in a text edit to the document, apart from any undoing/redoing".

Does this sound correct @quarnster? On the bright side, the general motion feels correct. I'll keep using this and let you know if I see any other issues.

Jordan W

One other observation: Repeating text that was inserted via an autocompletion does not seem to work either, but I think that could be broken out into a separate fix. (Unless it's immediately obvious how to solve that).

Jordan W

Also, I think in this latest version, the undoing is a little bit aggressive.

Line One

Yank and paste to get.

Line One
Line One

Then move down to the second line and delete one character (by pressing x for character delete on the L)

Then undo, and it undoes the character delete and the yank/paste. I believe it should have only undone the character delete.

Maybe this is not related to these changes.

Fredrik Ehnbom

"." has nothing to do with redo. It is it's own concept that is separate from the undo stack itself, and more tied to the concept of "the last set of keystrokes input by the user that resulted in a text edit to the document, apart from any undoing/redoing".

Does this sound correct @quarnster?

I think so.

Jordan W

I also noticed that now, on redo, some selections seem to be redone - when in Vim, selection (visual or other) is not part of the undo stack (whereas Sublime's soft undo does reason about selection.) However, ctrl+r, doesn't do a "soft" undo/redo IIRC, so I can't explain that.

Guillermo López-Anglada
Owner

Thanks all for the feedback. Let's focus here on getting ., Ctrl+r and u to behave and cooperate at a basic level.

Feel free to enter separate issues for other problems.

Guillermo López-Anglada
Owner

Related to #170.

Jordan W

I know I mentioned this before, but do you think it makes sense to treat repeat completely separately? It seems like once you fix undo/redo/yank the repeat bugs surface, then when fixing repeat the undo/redo/yank bugs reappear. I know you had it working pretty solidly at one point, but it was clear that you were somehow confusing repeat and redo/undo when they are completely separate concepts and almost entirely independent.

Would it make your life easier if you were to solve undo/redo using completely separate code from the repeat code?

Guillermo López-Anglada
Owner

That's the idea :) I know it isn't very clear by looking at the separate issues, but I'm aware repeat is an entirely different concept to undo/redo here. It also seems to be the easiest to fix. Until know I thought Sublime Text was capable behaving exactly as Vim by default wrt repeating, but now I know it can't, because in ST, undoing affects repeating.

So I'm letting Vintageous take care of storing and managing the repeat command all by itself, and it seems to be working fine in my dev version.

It's undo/redo + yy what's not working well still, but we'll deal with that in separate issues.

To sum up: I hope we can close this issue once and for all in the next build.

Jordan W

That will be awesome!

Guillermo López-Anglada
Owner

The changes are live now. Both 'repeat' and 'redo' should be working.

Jordan W

I just synced with the latest master. Basic repeat does not work and visual yy (yank) still destros the redo stack. Here's the repro for basic repeat:

one two three

Place cursor at start of two.
ce and type asdf

You will now have:

`one asdf three`

Move to the start of one or three and hit . - nothing happens.

Jordan W

This is on build 3021

Guillermo López-Anglada
Owner

I'm on 3022 and all that seems to be working fine... Don't know what might be wrong.

Guillermo López-Anglada
Owner

Oh, yeah, you need to upgrade due to the api changes in 3022 wrt command history.

Jordan W

Agh - Sublime's update feature doesn't show 3022 - I upgraded but switched computes and this one has 3021 - I'll test on the latest (fingers crossed)

Jordan W

My initial testing feels pretty good so far. This fix, combined with the global "next" search makes it usable for daily use now - awesome! (that little bug with visual selection destroying redo could seriously bite me though - I commented on the other issue.)

Guillermo López-Anglada
Owner

Can we close this one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.