Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for Undo #141

Closed
cclark opened this issue Apr 17, 2018 · 18 comments

Comments

@cclark
Copy link

commented Apr 17, 2018

As I'm learning VisiData I find that sometimes I hit the wrong key combo and want to undo. - vs _ for instance is a pain to go to the Columns sheet and set the width to be non-zero when I was trying to expand the column in the first place. Or maybe I'm playing with a regex to split out a column and I just bungle it. A simple undo of the last command would be an awesome enhancement.

@saulpw

This comment has been minimized.

Copy link
Owner

commented Apr 18, 2018

Thanks for the request, @cclark. I haven't been able to figure out how to implement undo effectively, but your examples gave me a new insight that might be workable. It would probably be bound to the "Backspace" by default, replaying from the previous sheet all commands but the last one. Does that work?

@deinspanjer

This comment has been minimized.

Copy link
Contributor

commented Jun 9, 2018

If you load a large file and your first action is to accidentally wipe out the wrong column with -, that wouldn't generate a new sheet, so the undo would potentially take much longer than resetting the size of the column in that particular case, right?

Here are a list of the oopsies I've run into so far:

  • Accidentally removing a column
    fix -- reset the column size from the Columns sheet
  • Accidentally deleting the wrong row
    fix -- Don't know of a good one yet besides closing that sheet and restarting
  • Sorting the wrong column or the wrong way
    fix -- If no previous sort, it is easy, just hit it again. If there was a previous sort, find that column and sort on it again.
  • Adding the wrong set of rows as tagged via ,
    fix -- If there weren't any previous tags, it is fine, but if there were, I haven't found a fix besides untagging all and restarting

In most of these cases, while it should be possible to restore state by starting from the previous sheet (or starting over if there isn't a previous derived sheet) and replaying, the large file issue I mentioned above would be a problem, as would trying to undo if you are sourcing from stdin.

Since we already have a mechanism for recording each action taken, might it be possible to also record what the reversing action would be? Then, if the user desires an undo, it is available. I think this would work well for cases such as renaming, resizing or hiding columns; sorting; tagging/selecting, and similar. I think it would require too much additional data storage for recovering from row deletions.

For the row deletion case, what about creating a snapshot sheet before the action (maybe optionally only if a certain size of data is impacted) and then that does give you the fall-back to replay for an undo.

@saulpw

This comment has been minimized.

Copy link
Owner

commented Jun 13, 2018

@deinspanjer, I really appreciated your comments. I've been wondering how to implement undo, and you have several good observations and suggestions here.

I think what we should do, is have an undo command which looks at the last cmdlog entry and gets the undo- from the list of commands. If there isn't one, undo will error() and go no further. If there is one, then undo moves the cursor back to the same sheet/col/row as in the cmdlog, and exec()s the undo-command. We'll need to completely transition the cmdlog to use the longname instead of the keybinding, so this depends on completing the Great Command Renaming, which was slated for 1.3 anyway.

Let's work through your examples (and thank you so much for providing these):

  1. Undo column hide. We'd make the width allowed to be negative, hide() would set it negative its regular width, and then 'undo-hide-column' be cursorCol.width = abs(cursorCol.width) (or factor into cursorCol.hide(False)). We'd have to make the undo column finder look in all columns instead of just visibleCols, in contrast to the existing replay column finder.

BTW as an easier workaround, gv (as of 1.2) unhides all columns.

  1. Undo delete. The original row numbers are actually stored with the deleted rows on the clipboard. If it's only one row, you can restore it with p (paste after) or P (paste before); these are like vim. They will also work if you have deleted multiple rows; but they will be pasted all together by design. 'undo-delete' would have to recreate the original list from the clipboard, which should be reasonably straightforward.

  2. Undo sort. This one is trickier. But this one can be solved by having the sheet 'know' its own ordering criteria, which will be necessary for efficiently viewing SQL databases (and pandas etc) anyway.
    Then 'undo-sort' would remove the most recent sort item and call doSort(). This would also open up the possibility of having an sortby meta-sheet, such that the rows could be ordered with a little more finesse (non-keycols, mixed orderings). z[ would naturally add a column to the sortby list (whereas [ replaces it with one column and g[ replaces it with all key columns).

  3. Undo select. Similar to sort, each sheet would maintain its current set of selection criteria. 'undo-select' would pop the latest off the stack, unselect everything and reselect. There would be a selectby metasheet also. We'd want to maintain selectedRows as a full cache (even though it might be more complicated), so we can get the best of both worlds performance-wise.

This method works for these cases, but probably would not work for everything, but notably would not work for cell edits unless we made the choice to move the previous edit value to the clipboard (which might not be a bad idea actually). Though 'undo-rename-col' could maybe already use the value in the cmdlog cursor col itself.

I think these are all pretty reasonable to do and I would like to have them in there, so we can hopefully get them into 1.4 if not 1.3. The biggest question, of course, is what key 'undo' should be bound to? u is lamentably taken, but Backspace is open, though I'm not sure how that would feel...any suggestions?

@deinspanjer

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2018

Okay.. so I decided something that would help me make sense of the used and available keys was to make a template that overlays the binds on an actual keyboard. I'm working on that and I'll put it up somewhere for your review soon.

Basically, I have a few suggestions for options:

  1. Undo: ^[ (Esc) / Redo: ^]
  • Pros:
    • No conflicts (technically because Esc aborts certain operations, but that isn't a bad conflict)
    • Complementary keys next to each other on keyboard (if using [ instead of Esc)
  • Con: I don't know of any other program that actually uses Esc for Undo. :/
  1. Undo: ^u / Redo: ^↑U
  • Pros:
    • U is a very convenient mnemonic for undo
    • Complementary keys (shift to toggle)
  • Cons:
    • ^u conflicts with replay pause/resume
    • U key is already loaded with unrelated functions (untag)
  1. Undo: ^u / Redo: ^r
  • Pros:
    • U is a very convenient mnemonic for undo
    • R is a very convenient mnemonic for redo
    • Complementary key modifiers (both ctrl)
    • ^r is the same Redo command as in vim
  • Cons:
    • ^u conflicts with replay pause/resume
    • ^r conflicts with reload/refresh
    • U key is already loaded with unrelated functions (untag)
  1. Undo: u / Redo: ^r
  • Pros:
    • U is a very convenient mnemonic for undo
    • R is a very convenient mnemonic for redo
    • u is the same Undo command as in vim
    • ^r is the same Redo command as in vim
  • Cons:
    • u conflicts with untag
    • ^r conflicts with reload/refresh

I think that the ^[/^] approach is probably the least invasive, but I do really like the idea of trying to get past the pain of finding new binds for untag and reload and getting into alignment with vim by using u/^r.

@saulpw

This comment has been minimized.

Copy link
Owner

commented Jun 14, 2018

Good breakdown, @deinspanjer, thank you. As long as we're exploring options like ^[, for completeness we should mention the classic ^Z, though that is bound to suspend-process, as in the shell.

I understand the desire to align with vim, and I can see using u for undo, but I've never liked its asymmetric ^R. ^U seems to be the least offensive of the options so far. Would z^U make sense for 'redo'? What could g^Umean?

Also, I love the idea of having a graphic of the actual keyboard command layout; could you upload a copy of the image when you have something? We've recently added a dev/commands.tsv, which lists the commands bound for each of the keystrokes, including columns for the prefix combinations, which might be helpful for such a project.

@anjakefala

This comment has been minimized.

Copy link
Collaborator

commented Jun 14, 2018

Hello :)

I empathise deeply with the allure of using u for undo, however, as a longtime user, I have a strong preference against remapping u and ^R. They are pretty much solidly in my bones at this point. Did we bring up any downsides for Backspace? I guess top of my mind is that it may be too snappy for undo but I still think it is a reasonable contender.

Though, as a side note, I think regardless of where this conversation goes I would love to provide a .visidatarc snippet for vim users which aligns the command syntax more strongly with vim. ^^ I think that could be cute.

@deinspanjer

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2018

I didn't like Backspace because it doesn't have a good complementary key, and because Backspace just doesn't strike me as an "undo" so much as a "destruction". For instance, I wouldn't intuitively assume that hitting backspace might cause 5 rows (that were previously deleted) to appear.

I wouldn't throw a fit if it was decided on though. :)

@cclark

This comment has been minimized.

Copy link
Author

commented Jun 14, 2018

I'd like to vote -1 on backspace for the exact reasons @deinspanjer brings up:

  • it does feel like it is a key bound to destructive behavior
  • it doesn't have a complimentary partner for redo

I'm trying to be objective above but if I let opinion enter the conversation, it feels awkward when it comes down to it. I don't think I have come across Backspace as Undo in any other programs. I suspect the counterintuitive behavior might throw off new users because it could be jarring. I ❤️ visidata and want to see the userbase grow so I think some of the softer aspects like this are worth considering.

This isn't going to be an easy decision and it sounds like a number of the potentially preferred bindings could conflict with existing muscle memory. However, I do think that aligning with vim where possible makes sense. There is a natural feel for vim in most of the keybindings currently and being able to maintain that consistency makes visidata easier to grok and feel like you're becoming a power user (at least if you're familiar with vim already).

@anjakefala anjakefala closed this Jun 14, 2018

@anjakefala anjakefala reopened this Jun 14, 2018

@saulpw

This comment has been minimized.

Copy link
Owner

commented Jun 15, 2018

I agree, Backspace-as-undo is kind of odd.

What if we had only one-level undo, so if you press it again, it undoes the undo. Like a toggle between the last two states. And maybe we can map it to Ctrl+H. :D Or I'm willing to give up Ctrl+U for this noble cause. Seems like it should be on a control key, anyway.

@cclark and @deinspanjer, would you like to be part of a larger discussion on keybindings?

@deinspanjer

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2018

I think that starting with a single undo is a great way to push past the initial difficulty and make an incremental improvement. If we did that, I think that maybe Ctrl-[ (Esc) makes even more sense because it will quickly be learned by the user (I found out where it worked and didn't because of the "Ctrl-[ is unbound" message when hitting it randomly), and it would be non-destructive because if they didn't want to undo, they can just hit it again.

Yes, I'm happy to contribute where I can. #startup-life can easily get in the way however, so please forgive me in advance if I sometimes go silent.

@saulpw

This comment has been minimized.

Copy link
Owner

commented Jan 13, 2019

Implemented in d4549f9, and bound to Ctrl+[ (undo) and Ctrl+] (redo). Must be enabled with options.undo=True (or --undo from CLI). Please try this out on the develop branch if you are able. Should be in next minor release, though not sure about the keybindings.

@anjakefala

This comment has been minimized.

Copy link
Collaborator

commented Mar 31, 2019

Since a752762, undo has been bound to Shift-U and redo to Shift-R. =)

@anjakefala

This comment has been minimized.

Copy link
Collaborator

commented Apr 16, 2019

The work on undo / redo that is currently on develop is considered 'shippable'. It will be enabled by default on v1.6.

Upon pressing Shift-U, the most recent modification on that sheet will be undone. In the future, every single new command which modifies data, will require the implementation of an undo function before it can be part of VisiData core.

Please check it out on develop and give it a whirl. And please let us know if a command lacks an undo or if its undo func does not work intuitively.

@anjakefala anjakefala closed this Apr 16, 2019

@saulpw

This comment has been minimized.

Copy link
Owner

commented Apr 16, 2019

The rule of thumb: a command is undoable if it would change how the sheet is saved. Therefore width changes are not undoable, but hide-col is. Changes in selected rows are currently undoable, but I'm not sure about that (esp because it violates the above rule). Comments welcome.

@saulpw

This comment has been minimized.

Copy link
Owner

commented Apr 18, 2019

@deinspanjer I forgot it was you that had suggested Ctrl+[ and Ctrl+] in the first place! We had those bindings for awhile as you can see, not sure if you played with it during that time, but I liked it for all the reasons you suggested. I found ESC felt a bit weird as undo, but the deal-breaker was actually a few times where an ANSI sequence got misinterpreted somewhere in my terminal stack and the leading ESC made it into VisiData, undoing the previous action. So we had to use another option.

@khughitt

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2019

Thanks for implementing this! Just a quick note - I searched the docs, and it looks like this may not be mentioned there yet..

@anjakefala

This comment has been minimized.

Copy link
Collaborator

commented Sep 4, 2019

@khughitt It is not! VisiData 2.x has not been released yet. It is currently in beta. However, yeah it would be useful to have a way to share documentation for it, regardless. If you install 2.-1, the manpage that comes with it mentions undo.

@khughitt

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

@anjakefala Ah, thanks for the heads up! I didn't even know there was a 2.x branch. I will have to give that a spin 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.