owickstrom / komposition

The video editor built for screencasters
https://owickstrom.github.io/komposition/
Mozilla Public License 2.0
429 stars 21 forks source link

Undo support #21

Closed owickstrom closed 6 years ago

owickstrom commented 6 years ago

This is a larger feature that I don't have any good design in mind for right now. In any case, some sort of undo stack would be very useful. Most commands are pure (not having any side effects) so those are simple to revert, or "play backwards", but things like importing and rendering are not. Maybe those could be ignored by the undo history recorder. Anyone with experience implementing undo functionality for applications like this one, please chime in!

snowleopard commented 6 years ago

Here is a simple idea I suggested on Twitter:

data State s = State { past :: [s], current :: s, future :: [s] }

modify :: (s -> s) -> State s -> State s
modify f (State ps c _) = State (c:ps) (f c) []

undo :: State s -> State s
undo (State (p:ps) c fs) = State ps p (c:fs)

redo :: State s -> State s
redo (State ps c (f:fs)) = State (c:ps) f fs

I guess s = Komposition.Project or something similar. Note that this is quite memory-efficient: although we seem to store N state copies, most of these copies differ very little and the common information can be shared.

owickstrom commented 6 years ago

One approach suggested by @ocharles was to explore a Group structure for timeline commands, such that the application could hold a stack of commands, and use the inverse operation to undo. I like the idea of it, but not sure how to deal with effects, and how they would be undone.

The components involved in timeline commands (non-movement) are the timeline data structure, the current focus, and the command itself (e.g. Delete or AppendClip). I'm thinking the data type to define a Group instance for will need to store those values to be able to calculate an inverse command. Not sure at all, would need to experiment more with the idea.

Any pointers to papers or other resources would be great!

owickstrom commented 6 years ago

@snowleopard thanks! Do you have any ideas on how to deal with effects? For instance, when importing a video file, the project data structure gets the new assets added as you'd expect (pure operation), but also there's proxy media files generated (impure). If you undo the import command, do you as a user expect the proxy media files to be deleted? Or should only pure commands be supported in the undo history? Interested in hearing any ideas around this. 👍

snowleopard commented 6 years ago

@owickstrom Is adding/deleting proxy media files the only side-effect we should consider?

Proxy media files seem to be relatively benign to me: one can think of them as always existing and lazily materialised in a shared cache directory when needed with some form of garbage collection (say, at the application exit, or as a special user command).

This shared cache can be content-addressable, i.e. the file names could be hashes of the original assets, so you'll never have any conflicts in this shared cache.

owickstrom commented 6 years ago

@snowleopard Looked more closely at it, and there is one other effect that we should consider, which is importing audio with "automatic classification of parts". It uses the sox tool to split up an audio file into many small audio files (one for each "sentence"). Those are then picked up by the importer and added as separate assets. This is a somewhat ugly consequence of how sox works. Ideally, it would work as the video classification does, i.e. that it wouldn't create files on disk for each classified segment, but that it would simply store the timespans of the segment in the original media file:

https://github.com/owickstrom/komposition/blob/064a2a8561200481ac10e16a8920be6eafd37e38/src/Komposition/Library.hs#L37-L42

I haven't found a way to extract only the timestamps using sox, I could only do the audio file splitting and import those files as separate assets. Had this been different we could've gone with your suggestion on regarding proxy media as a shared cache that can be garbage collected, as that would be the only effect involved in performing timeline commands. And as you mention, it should be content addressable, which I think I just left some TODOs for in the code. I'll create a separate issue for it. 👍

owickstrom commented 6 years ago

@snowleopard Then again, maybe those classified audio part files could be handles the same way as proxy media files. Doesn't matter if they live longer than their references, if there's some period garbage collection.

snowleopard commented 6 years ago

@owickstrom Yes, fundamentally this looks like another instance of caching an expensive computation.

Inside every large program there is a small build system struggling to get out! :-) Oh, I must tweet this.

owickstrom commented 6 years ago

Yeah.

😀 Agreed. And it's very nice to have you involved in discussion, given that you are an expert on fundamentals of build systems. 🙏

owickstrom commented 6 years ago

@snowleopard And by the way, I'd love a PR. I think the zipper structure with states is a good fit for Komposition. It would be interesting to try the list of commands with a Group instance approach, but I'm not sure it fits that well, at least not with how commands, focus, and timeline are currently encoded in data types.

I suppose you could just ignore the proxy media for now, and we can implement the garbage collection functionality later on.

snowleopard commented 6 years ago

@owickstrom Sure, I'll send you a (small) PR, without touching the proxy media issue.

As an "expert on fundamentals of build systems" I have troubles with building the project 😄

At first I had an issue with Git permissions, which I resolved by replacing git@github.com: with https://github.com/ in the stack.yaml file, like this:

[...]
 - git: https://github.com/owickstrom/gi-gtk-declarative.git
[...]
 - git: https://github.com/reinerp/indexed-extras.git
[...]

This seems to be a safer default? I can include it into the PR (or send as a separate patch) if you like.

Now stack build leads to:

Configuring haskell-gi-base-0.21.1...
Cabal-simple_Z6RU0evB_2.2.0.1_ghc-8.4.3.exe: The program 'pkg-config' version
>=0.9.0 is required but it could not be found.

Feel free to spawn this into a separate issue, not to clutter this thread.

Looks like you don't have CI yet? In principle I could just send a PR without building it (I'm a bit pressed with time -- will need to disappear soon).

P.S.: I'm on a Windows 10 machine.

owickstrom commented 6 years ago

@snowleopard thanks!

The permission thing is weird, but yeah, that looks fine. Thanks for including it in the PR.

Regarding the dependencies, it's an issue I've been expecting to hit me soon. Sorry about that! I've sort of iteratively added things to Komposition (that required OS package manager dependencies) and not properly documented all the things that I needed to install along the way. I'll probably do a clean VM install to find out what's needed, but haven't had time yet.

Anyway, I'll pick up where you left off in the PR. Thanks a bunch!

owickstrom commented 6 years ago

Thanks for the reminder about CI, by the way. It's been on my ever-growing TODO list. 😄