[undo-] prevent undo of command that created sheet #2244
Conversation
visidata/undo.py
Outdated
| @@ -37,14 +37,23 @@ def undo(vd, sheet): | |||
| if not vd.options.undo: | |||
| vd.fail("options.undo not enabled") | |||
|
|
|||
| # don't allow undo of first command on a sheet, which is always the command that created the sheet. | |||
| for i, cmdlogrow in enumerate(sheet.cmdlog_sheet.rows[:0:-1]): | |||
| cmdlogrows = sheet.cmdlog_sheet.rows | |||
There was a problem hiding this comment.
How about:
cmdlogrows = itertools.filterfalse(lambda r: r.longname == 'set-option')
for i, cmdlogrow in enumerate(reversed(cmdlogrows[1:])):
?
There was a problem hiding this comment.
Okay I've incorporated this change.
Right now cmdlog.py only adds 'set-option' commands before the sheet creation command, but never after it. So filterfalse() is fine as long as that is true.
Is there any situation where user-created code might cause the insertion of 'set-option' after sheet creation? If that ever happens, row_idx will not be correct and the undo log would be corrupted.
Line 49 in e4f603b
There was a problem hiding this comment.
If an option gets set by the user, it should be added as a set-option command on the source sheet cmdlog. (I think this doesn't happen currently, which would mean that in-session option changes aren't recorded properly for replay.) So your original code might have been better, though I can't see it after the force push. Sorry to make an incorrect suggestion if that's the case!
Also what's the difference between this islice and [1:]?
There was a problem hiding this comment.
Oops, I shouldn't have force pushed to overwrite the original code, then! That code skipped just the first sequential set-option commands. But it did it by calculating indices. It is shorter and cleaner to translate it to iterators as you suggested. So I've done that now with 43ad4ae.
As for the islice, it was needed because the generator from filterfalse didn't allow subscripts. But next() is all we actually need, so I got rid of the islice.
|
@midichef If you get the modification in by tomorrow, this can be part of v3.0.2 =) |
midichef
force-pushed
the
undo_sheet_creation
branch
from
0328928
to
e4f603b
Compare
|
@anjakefala okay this is ready to go, if it gets approved. Can you squash it into 1 commit before doing the final merge? Or I can do this myself. I don't quite know the usual protocol, but I do like to keep the commit history clean. |
For sure! I can make a habit of cleaning your commits before merging. |
The undo command currently incorrectly attempts to undo the command that created the sheet.
To reproduce:
vd sample_data/sample.tsvtheng"andundo-last. This will empty the sheet out.The cause is an assumption in the undo code, that the first line of the command log is for the command that created the sheet:
visidata/visidata/undo.py
Line 40 in 5772c0f
Because
set-optioncommands are carried into new sheets, that assumption is no longer true. (since 1925808)This PR skips over the first sequence of command log rows for the
globalsheet, wherecmdlogrow.sheet == 'global'. I think these commands are all forset-option, so we could instead match longname:cmdlogrow.longname == "set-option". But I think excluding the first commands from the global sheet, is more robust to future changes.