Yank destroys undo/redo stack. #167

Closed
jordwalke opened this Issue Mar 25, 2013 · 18 comments

Comments

Projects
None yet
3 participants
@jordwalke

This is a horrible bug that appeared in Vintage - that Vintageous actually fixed. But recently something went wrong and now yank destroys the redo stack. Here's the repro.

Line One (then exit insert mode)
new line
Line Two (then exit insert mode)

Then undo the second line entry.
Then visually select the first line and yank it.
Then redo is broken.

This might have to do with the fixes that were added for repeat recently.

@guillermooo

This comment has been minimized.

Show comment Hide comment
@guillermooo

guillermooo Mar 25, 2013

Owner

This is going to be the bane of my life... I'm growing more and more skeptical about the feasibility of making it work as we need, but I have an idea why this particular bug might be happening, though in general this is a global issue with Vintageous. I'm aiming at having it sorted out by 1.0. A Vim mode without (perfectly) working undo/redo/repeat commands would make for a quite poor experience.

Owner

guillermooo commented Mar 25, 2013

This is going to be the bane of my life... I'm growing more and more skeptical about the feasibility of making it work as we need, but I have an idea why this particular bug might be happening, though in general this is a global issue with Vintageous. I'm aiming at having it sorted out by 1.0. A Vim mode without (perfectly) working undo/redo/repeat commands would make for a quite poor experience.

@quarnster

This comment has been minimized.

Show comment Hide comment
@quarnster

quarnster Mar 25, 2013

Do you have a regression test suite?

Do you have a regression test suite?

@guillermooo

This comment has been minimized.

Show comment Hide comment
@guillermooo

guillermooo Mar 25, 2013

Owner

I plan to add tests for state.py and run.py, but they aren't available yet. It should be possible to keep the undo stack monitored. The problem is that undo/redo/repeat have never worked as they should so far... I'll have to take the time to try and understand the problem fully.

Owner

guillermooo commented Mar 25, 2013

I plan to add tests for state.py and run.py, but they aren't available yet. It should be possible to keep the undo stack monitored. The problem is that undo/redo/repeat have never worked as they should so far... I'll have to take the time to try and understand the problem fully.

@jordwalke

This comment has been minimized.

Show comment Hide comment
@jordwalke

jordwalke Mar 25, 2013

I agree, this is as important as basic movement, because you can potentially ruin important editing and you have no hope of recovering it :)

I agree, this is as important as basic movement, because you can potentially ruin important editing and you have no hope of recovering it :)

@guillermooo

This comment has been minimized.

Show comment Hide comment
@guillermooo

guillermooo Mar 26, 2013

Owner

The redo stack started to get clobbered by non-modifying commands as of rev 244.

(I think GH lets you diff on arbitrary rev pairs, but I don't know how to off the top of my head.)

Basically, the new locations for maybe_mark_undo_groups_for_gluing is causing this. Removing the calls seemed to restore order for me.

I've also made promising progress with repeat, but redo is still giving me a hard time.

Owner

guillermooo commented Mar 26, 2013

The redo stack started to get clobbered by non-modifying commands as of rev 244.

(I think GH lets you diff on arbitrary rev pairs, but I don't know how to off the top of my head.)

Basically, the new locations for maybe_mark_undo_groups_for_gluing is causing this. Removing the calls seemed to restore order for me.

I've also made promising progress with repeat, but redo is still giving me a hard time.

@guillermooo

This comment has been minimized.

Show comment Hide comment
@guillermooo

guillermooo Mar 26, 2013

Owner

I'm a genous... 244 in my local mercurial repo. :)

I don't even thing the hashes are shared between Hg and Git...

Owner

guillermooo commented Mar 26, 2013

I'm a genous... 244 in my local mercurial repo. :)

I don't even thing the hashes are shared between Hg and Git...

@guillermooo

This comment has been minimized.

Show comment Hide comment
@guillermooo

guillermooo Mar 26, 2013

Owner

This is the one:

7a14708

Owner

guillermooo commented Mar 26, 2013

This is the one:

7a14708

@guillermooo

This comment has been minimized.

Show comment Hide comment
@guillermooo

guillermooo Mar 26, 2013

Owner

Related to #170.

Owner

guillermooo commented Mar 26, 2013

Related to #170.

@jordwalke

This comment has been minimized.

Show comment Hide comment
@jordwalke

jordwalke Mar 30, 2013

Oh interesting, I just determined that it's not the yank that's destroying the undo/redo stack, it's the visual selection! Selecting with the mouse in non-vim mode does not destroy the undo stack in the same way.

Oh interesting, I just determined that it's not the yank that's destroying the undo/redo stack, it's the visual selection! Selecting with the mouse in non-vim mode does not destroy the undo stack in the same way.

@jordwalke

This comment has been minimized.

Show comment Hide comment
@jordwalke

jordwalke Mar 30, 2013

And this doesn't really make a lot of sense to me, since in Vim, visual highlights never show up in the undo/redo stack. Maybe the solution is to completely get rid of highlighting in the undo stack? I don't really see the value in having selections show up in the undo/redo history - I'd even be satisfied if the only way to get it to work correctly is to get rid of soft-undo support. I'm not sure someone who's in love with Vim would miss it anyways.

And this doesn't really make a lot of sense to me, since in Vim, visual highlights never show up in the undo/redo stack. Maybe the solution is to completely get rid of highlighting in the undo stack? I don't really see the value in having selections show up in the undo/redo history - I'd even be satisfied if the only way to get it to work correctly is to get rid of soft-undo support. I'm not sure someone who's in love with Vim would miss it anyways.

@guillermooo

This comment has been minimized.

Show comment Hide comment
@guillermooo

guillermooo Apr 1, 2013

Owner

What's the repro for that? I might be mistaken, but non-modifying commands (like a pure visual selection), should not contribute to the undo/redo stack (in a user-facing way, in any case).

Owner

guillermooo commented Apr 1, 2013

What's the repro for that? I might be mistaken, but non-modifying commands (like a pure visual selection), should not contribute to the undo/redo stack (in a user-facing way, in any case).

@jordwalke

This comment has been minimized.

Show comment Hide comment
@jordwalke

jordwalke Apr 1, 2013

Line One (then exit insert mode)
new line
Line Two (then exit insert mode)

Then undo the second line entry. (you will be left with only Line One).
Then visually select the first using v.
Then redo is broken/ruined.

It repros reliably.

Line One (then exit insert mode)
new line
Line Two (then exit insert mode)

Then undo the second line entry. (you will be left with only Line One).
Then visually select the first using v.
Then redo is broken/ruined.

It repros reliably.

@jordwalke

This comment has been minimized.

Show comment Hide comment
@jordwalke

jordwalke Apr 1, 2013

The reason why this should probably be a V1 blocker:

If you edit an entire day's worth of work, and undo to something very far back just to see what you changed, if you visually select anything (to copy it for example) - your entire day's worth of work is lost. I know not to do this, but I wouldn't want someone else to get bitten by it.

The reason why this should probably be a V1 blocker:

If you edit an entire day's worth of work, and undo to something very far back just to see what you changed, if you visually select anything (to copy it for example) - your entire day's worth of work is lost. I know not to do this, but I wouldn't want someone else to get bitten by it.

@guillermooo

This comment has been minimized.

Show comment Hide comment
@guillermooo

guillermooo Apr 1, 2013

Owner

I agree it's a nasty bug and a potential pitfall for data loss, but if you were doing anything remotely important, you should have been using version control to begin with. It's clearly a bug and a strong mental disruption, since it's a completely unexpected behavior, but could hardly be blamed for any (huge) data loss like in your example.

Anyhow, I'm ok with considering this a V1 blocker, since a fully working undo/redo is one of my goals a good selling point at any rate.

The problem is I don't really know how to fix it. After taking a cursory glance, this seems to be S3 going about its business. It's probably related to the soft undo feature, as you've pointed out, but to my knowledge that isn't exposed in any way to the user/plugin author, so it can't be disabled.

Owner

guillermooo commented Apr 1, 2013

I agree it's a nasty bug and a potential pitfall for data loss, but if you were doing anything remotely important, you should have been using version control to begin with. It's clearly a bug and a strong mental disruption, since it's a completely unexpected behavior, but could hardly be blamed for any (huge) data loss like in your example.

Anyhow, I'm ok with considering this a V1 blocker, since a fully working undo/redo is one of my goals a good selling point at any rate.

The problem is I don't really know how to fix it. After taking a cursory glance, this seems to be S3 going about its business. It's probably related to the soft undo feature, as you've pointed out, but to my knowledge that isn't exposed in any way to the user/plugin author, so it can't be disabled.

@jordwalke

This comment has been minimized.

Show comment Hide comment
@jordwalke

jordwalke Apr 1, 2013

It can happen with small edits too, that aren't worthy of checkpointing in git - (think, 6/7 edit steps) which can still be disruptive if they are destroyed.

Either way, I would hardly blame you for my data loss, just putting myself in the mind of the user so that this plugin gets the recognition it deserves :)

Can you see my comment/findings here (sublimehq/Vintage#153). A while ago, I had convinced myself that I had a (possibly hacky) fix but I don't know the plugin architecture that well.

It can happen with small edits too, that aren't worthy of checkpointing in git - (think, 6/7 edit steps) which can still be disruptive if they are destroyed.

Either way, I would hardly blame you for my data loss, just putting myself in the mind of the user so that this plugin gets the recognition it deserves :)

Can you see my comment/findings here (sublimehq/Vintage#153). A while ago, I had convinced myself that I had a (possibly hacky) fix but I don't know the plugin architecture that well.

@guillermooo

This comment has been minimized.

Show comment Hide comment
@guillermooo

guillermooo Apr 1, 2013

Owner

Yeah, I must say I don't know why I was trying to downplay the data loss issue :) Whether it's two steps or one hundred, it would be a PR catastrophe ;)

Hm... The referenced issue definitely looks like it's pointing in the right direction. But that's easy for a copy op. In Vintageous, when you enter visual mode, a mark is set to group the following commands. When you exit visual mode, the groups, if any, are finally glued together. If, however, there is actually no command between v and v (or however you exited visual mode), we don't want that to create a new head in the undo stack and wipe the redo stack (as it is surprisingly happening now). That's the tricky part, although I'm starting to see a solution or two. Things would be far simpler if we could get the current index/serial into the stack. I would then know how far I should look back for potential modifying commands and umark or not based on that.

Owner

guillermooo commented Apr 1, 2013

Yeah, I must say I don't know why I was trying to downplay the data loss issue :) Whether it's two steps or one hundred, it would be a PR catastrophe ;)

Hm... The referenced issue definitely looks like it's pointing in the right direction. But that's easy for a copy op. In Vintageous, when you enter visual mode, a mark is set to group the following commands. When you exit visual mode, the groups, if any, are finally glued together. If, however, there is actually no command between v and v (or however you exited visual mode), we don't want that to create a new head in the undo stack and wipe the redo stack (as it is surprisingly happening now). That's the tricky part, although I'm starting to see a solution or two. Things would be far simpler if we could get the current index/serial into the stack. I would then know how far I should look back for potential modifying commands and umark or not based on that.

@guillermooo

This comment has been minimized.

Show comment Hide comment
@guillermooo

guillermooo Apr 1, 2013

Owner

I think it'd suffice to walk back in the stack until we either hit a command with a non-empty 'action' attribute or 'vi_enter_visual_mode' in it, whichever happened first. This could potentially turn out to be slow, but I suppose that that'd only occur if you'd performed a huge amount of motions in visual mode, which is highly unlikely.

Owner

guillermooo commented Apr 1, 2013

I think it'd suffice to walk back in the stack until we either hit a command with a non-empty 'action' attribute or 'vi_enter_visual_mode' in it, whichever happened first. This could potentially turn out to be slow, but I suppose that that'd only occur if you'd performed a huge amount of motions in visual mode, which is highly unlikely.

guillermooo added a commit that referenced this issue Apr 2, 2013

#167 - visual mode should not add entries to the undo stack unless m…
…odifying commands have been issued

'mark_undo_groups_for_gluing' and 'maybe_mark_undo_groups_for_gluing' in
conjunction with 'glue_marked_undo_groups' seem to add entries to the undo
stack regardless of whether intervening commands were modifying or non-
modifying. To avoid undesired entries in the undo stack (that will break
the redo feature too), we need to perform some checks.
@jordwalke

This comment has been minimized.

Show comment Hide comment
@jordwalke

jordwalke Apr 5, 2013

Alright, now this plugin is getting pretty solid 👍

I'll keep testing it for another day or so and if I don't find anything, I'll close this out.

Alright, now this plugin is getting pretty solid 👍

I'll keep testing it for another day or so and if I don't find anything, I'll close this out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment