Undo register proposal #86
Conversation
also returns the function that the usage was found in. I'm trying to collate all the use-cases of undo/push/redo so that I have a complete picture
- using 2 deques, one for undo and one for redo
- hooked up the push, undo and redo - directly in the ui for now - added debug logs to the undo class
- undo register item is added for insertChar - redo and undo both work when running (kinda)
need to get my head around the basics
run with:
```shell
LINE_PROFILE=1 ./poc.py && \
python3 -m line_profiler -rtmz profile_output.lprof \
| less
```
- implement for horizontal flipping - and editing movie frames while playing
|
This looks intense. I'll need a bit of time to read, re-read and get my head around what this is actually proposing and changing.
shrug It's just the way the data structure evolved. Originally there was no color, and it accessed the characters in array of lines, or the "content" you see today. When I added color, it just made sense to me that I could take an X/Y coordinate and ask for the character from one array (content), or take that same x/y and ask for the color from another array (colormap). Simple. It also makes the art somewhat human viewable (and thus editable) in the plain text json representation, with separate content and colormap. If you think it can be improved, I'm open to suggestions.
It deals with multiple frames gracefully, right? As some edits hit multiple frames at once, so the undos also must hit multiple frames at once. One thing I like about the existing undo system is, when you undo, it jumps to the frame you were at when you made the edit. So you don't accidentally undo or redo in a frame that you are not currently viewing. The undo/redo should always be visible. This is a result of the movie (self.ui.mov in UndoManager() push()) containing a currentFrame variable (dumb place for it), which the UI utilizes. So maybe the mov's currentFrame should be contained in the undo stack (or queue as it were), so that when we undo, the UI can jump back to the relevant frame. It might even do the same with the appState's topLine, in case the edit being undone is scrolled off of the screen. If that makes sense. Another way of saying this is, hitting undo should jump your view to the spot that it is changing. Currently this happens via the UI reading mov.currentFrame when it updates, so it jumps to the right frame, but doesn't scroll to the right line.
Hmmm. Really big? :) There are some really long ANSIs out there. here is a 2000+ liner. And another one. Since your previous changes to the Undo system, Durdraw now handles these monsters with ease on my system. I also load 200+ column wide ANSIs into Durdraw, and it's cool with them. Right now there are no restrictions on these things, and the whole point of overhauling the undo system is to be able to scale up more. :) It seems to be doing well on "large" ANSIs, but tons of frames bogs it down. My thinking is that if a movie has too many frames for Durdraw to comfortably edit, then multiple movies could be queued together to form a larger movie. Think "ascii star wars." Right now the only way to do this is to script it together with the durdraw command-line (durdraw -p *.dur), and bash if you need custom paramaters (like times to repeat) per-file.
I will let you know if I think of anything else. |
I think the longest ANSI on 16colo.rs is 4641 lines. Unfortunately the ANSI parser in Durdraw kind of sucks, and it only seems to load a bit over 1000 lines from it. https://16colo.rs/pack/blocktronics_wtf4/Blocktronics-WTF4_Megajoint.ans Another 4k liner: https://16colo.rs/pack/impure77/goto80-goto20.ans |
Absolutely, take your time, I'm not in a rush
Nice, I like the separation of colour and character. I'm implementing an ANSI string reverse function in Go and one of the headaches was dealing with the colours and characters all at once. It's definitely easier and more flexible to store them separately I think.
My very rough prototype durdraw code does do this, I didn't explicitly call this out in the design/proposal
Good call out, I'll make sure that the cursor location/move is always considered in whatever undo is being performed. Adding tests as I go along will definitely help to check that nothing is missed.
Awesome! I definitely like the idea of raising the ceiling on what durdraw can handle.
I like this idea, I think it's a good way to support very large documents that don't need to be edited all-at-once. I think that it's also possible to improve durdraw to handle multiple frames of the largest image (Blocktronics-WTF4_Megajoint.ans) with ease |
|
Sorry I haven't replied sooner. Been a busy December. Quick question: Does this change how undo.push() is accessed? Like right now, push() pushes the next clipboard state changes into the buffer. Would this eliminate the need altogether, as it now appears to be moved into Movie and Frame state changes, instead of done ad-hoc by the UI and associated functions? Or would we still push() before any canvas changes? I'm not quite clear. Also, does this save canvas size changes? I want to make a crop feature, for example, and that would be nice to undo. UndoRegister sounds like as good of a name as any. I think I was calling it a "Handler" instead of a "Register" before. It's the same difference to me. Edit: UndoRegister probably makes more sense, as it contains a registry of undo states. Thanks, Tom! |
There is an arbitrary limit imposed in the ANSI file (ecape code) parser. See Commit 5e2f1a1. It is at 750 columns and 8500 lines. The reason is because the ANSI parser is imperfect at determining the correct dimensions of some ANSI files, and they occasionally are interpreted as having 1000 columns or extra lines or something weird. The limit is just a catch to keep Durdraw from accidentally creating an insanely large canvas. If that makes sense. If you want to play around with some extremely large ANSIs, these were the largest I could find on 16c. Even the 16c web interface has trouble with some of these! https://16colo.rs/pack/blocktronics_there_will_be_blocks/Luciano-The_Dream_Shall_Never_Die.ans |
No worries, Merry Christmas and Happy New Year! 🎉 🎄
Yeah I apologise for the lack of detail, I largely focused on the deque mechanism and didn't really flesh out the implementation in UI/frame/movie. I've had a nice break this December and I've had some ideas percolate through. I'm going to make an update to this PR with a proposal/idea for how the
It totally could! I'll make sure to include this in my POC.
Yeaah this is one of those things that I could bikeshed for ages, I think that For my update to the PR (implementing some specific actions properly with undo/redo, using my proposed approach), I'm thinking of implementing for |
Greetings! I've been slowly plodding along with some initial discovery and analysis of the undo/redo situation, and I now have some findings and rough plan for proposal.
This branch is not to be merged! This is just the proposal, I'll make a separate branch & PR for the actual implementation
diagram
The proposal
The proposal doc is on this branch, see "undo.md". This contains all my findings, some questions, implementation details and a proposed plan of attack
https://github.com/tmck-code/durdraw/blob/undo-register-proposal/undo.md
POC
In order to get my head around the problem in a sandpit, I chalked up a mini/simplified durdraw-like program that uses the new system, this can help to illustrate what's happening.
https://github.com/tmck-code/durdraw/blob/undo-register-proposal/poc.py
Durdraw
I've gone ahead and field-tested my idea somewhat in actual durdraw code. You should be able to kinda run durdraw, and see the undo/redo basically functioning.
I implemented it for
I also created some basic unit tests for the new undo system to further illustrate and check the behaviour:
https://github.com/tmck-code/durdraw/blob/undo-register-proposal/test/durdraw/test_undo.py
Review
I'd love to know what you think of this approach! I basically want to get your blessing before I start rolling up my sleeves as there's a lot of code to change.
And also, if this ends up moving forward, if I've missed anything or made any oversights 🙂