Permalink
Browse files

Add UndoManager and make undo/redo shortcuts

Currently
  * Control-z is undo, and control-shift-z is redo.
  * Only track events that are added can be undone and redone.
  • Loading branch information...
mbuttu committed Mar 25, 2012
1 parent f515aa8 commit 3d6dd405f92e410470e4129fb1974e63c31dbc2f
Showing with 94 additions and 3 deletions.
  1. +36 −0 src/butter-main.js
  2. +54 −0 src/core/undomanager.js
  3. +4 −3 src/timeline/media.js
View
@@ -10,6 +10,7 @@
"./core/target",
"./core/media",
"./core/page",
+ "./core/undomanager",
"./modules"
],
function(
@@ -18,6 +19,7 @@
Target,
Media,
Page,
+ UndoManager,
Modules
){
@@ -40,6 +42,7 @@
_logger = new Logger( _id ),
_em = new EventManager( this ),
_page = new Page(),
+ _undoManager = UndoManager.getInstance(),
_config = {
ui: {},
icons: {}
@@ -53,6 +56,39 @@
} //if
} //checkMedia
+ function addTrackEventCommand ( options ){
+ var track = options.track,
+ trackEvent;
+
+ return {
+ execute: function(){
+ trackEvent = track.addTrackEvent({
+ type: options.type,
+ popcornOptions: options.popcornOptions
+ });
+ return trackEvent;
+ },
+ undo: function(){
+ track.removeTrackEvent( trackEvent );
+ }
+ };
+ };
+
+ document.addEventListener( "keydown", function( e ){
+ if ( e.ctrlKey && e.shiftKey && e.keyCode === 90 ){
+ _undoManager.canRedo() && _undoManager.redo();

This comment has been minimized.

Show comment
Hide comment
@ScottDowne

ScottDowne Mar 28, 2012

You check canRedo inside redo(), no need to check it twice.

@ScottDowne

ScottDowne Mar 28, 2012

You check canRedo inside redo(), no need to check it twice.

This comment has been minimized.

Show comment
Hide comment
@mbuttu

mbuttu Mar 28, 2012

Owner

Done.

+ }
+ else if ( e.ctrlKey && e.keyCode === 90 ){
+ _undoManager.canUndo() && _undoManager.undo();

This comment has been minimized.

Show comment
Hide comment
@ScottDowne

ScottDowne Mar 28, 2012

You check canUndo inside undo(), no need to check it twice.

@ScottDowne

ScottDowne Mar 28, 2012

You check canUndo inside undo(), no need to check it twice.

This comment has been minimized.

Show comment
Hide comment
@mbuttu

mbuttu Mar 28, 2012

Owner

Thank you, done!

@mbuttu

mbuttu Mar 28, 2012

Owner

Thank you, done!

+ }
+ });
+
+ this.addTrackEvent = function( options ){
+ var commandAddTrackEvent = addTrackEventCommand( options );
+ _undoManager.register( commandAddTrackEvent );
+ return commandAddTrackEvent.execute();
+ }
+
this.getManifest = function ( name ) {
checkMedia();
return _currentMedia.getManifest( name );
View
@@ -0,0 +1,54 @@
+/* This Source Code Form is subject to the terms of the MIT license
+ * If a copy of the MIT license was not distributed with this file, you can
+ * obtain one at http://www.mozillapopcorn.org/butter-license.txt */
+
+define(function(){
+ var _undoManager;

This comment has been minimized.

Show comment
Hide comment
@secretrobotron

secretrobotron Mar 27, 2012

Let's make this a __ instead of a _ since it's in a scope above the class.

@secretrobotron

secretrobotron Mar 27, 2012

Let's make this a __ instead of a _ since it's in a scope above the class.

This comment has been minimized.

Show comment
Hide comment
@mbuttu

mbuttu Mar 28, 2012

Owner

Done.

+
+ function UndoManager(){
+ var _undoStack = [],
+ _redoStack = [];
+
+ return {
+ register: function( command ){
+ if ( command.execute && command.undo ){
+ _undoStack.push( command );

This comment has been minimized.

Show comment
Hide comment
@ScottDowne

ScottDowne Mar 27, 2012

Shouldn't we call execute here?

@ScottDowne

ScottDowne Mar 27, 2012

Shouldn't we call execute here?

This comment has been minimized.

Show comment
Hide comment
@ScottDowne

ScottDowne Mar 27, 2012

Shouldn't we clear redo here?

@ScottDowne

ScottDowne Mar 27, 2012

Shouldn't we clear redo here?

This comment has been minimized.

Show comment
Hide comment
@dseif

dseif Mar 27, 2012

I think this is just registering the command ( creating it for the trackevent ) so we don't need to call it. I may be wrong but that's what I see

@dseif

dseif Mar 27, 2012

I think this is just registering the command ( creating it for the trackevent ) so we don't need to call it. I may be wrong but that's what I see

This comment has been minimized.

Show comment
Hide comment
@ScottDowne

ScottDowne Mar 27, 2012

Hm, the fact it is going on the stack makes me think this is the first push of a new command.

The pattern is not concerned with command types, it just needs to know it has the two methods it needs on it, so I still see this as a bug, and now I am also thinking the word register is kinda misleading. Should just be do.

Cannot undo something that has not been done, and cannot redo something that has not been undone. do -> ( undo -> redo ) continuously.

@ScottDowne

ScottDowne Mar 27, 2012

Hm, the fact it is going on the stack makes me think this is the first push of a new command.

The pattern is not concerned with command types, it just needs to know it has the two methods it needs on it, so I still see this as a bug, and now I am also thinking the word register is kinda misleading. Should just be do.

Cannot undo something that has not been done, and cannot redo something that has not been undone. do -> ( undo -> redo ) continuously.

This comment has been minimized.

Show comment
Hide comment
@mbuttu

mbuttu Mar 27, 2012

Owner

@ScottDowne, it is not up to the UndoManager to decide when to first execute a command. The purpose of the UndoManager is to simply handle the undoing and redoing of an action that has already been executed. To that end, a command is onlyregistered with the UndoManager at the time of execution; for an example, see the addTrackEvent function on lines 86-90 in src/butter-main.js.

@mbuttu

mbuttu Mar 27, 2012

Owner

@ScottDowne, it is not up to the UndoManager to decide when to first execute a command. The purpose of the UndoManager is to simply handle the undoing and redoing of an action that has already been executed. To that end, a command is onlyregistered with the UndoManager at the time of execution; for an example, see the addTrackEvent function on lines 86-90 in src/butter-main.js.

This comment has been minimized.

Show comment
Hide comment
@ScottDowne

ScottDowne Mar 28, 2012

Is there a use case for adding a command and not executing it? Also, a use case for not clearing the redo when added? Seems like it could be abstracted more, and be less fragmented.

@ScottDowne

ScottDowne Mar 28, 2012

Is there a use case for adding a command and not executing it? Also, a use case for not clearing the redo when added? Seems like it could be abstracted more, and be less fragmented.

This comment has been minimized.

Show comment
Hide comment
@mbuttu

mbuttu Mar 28, 2012

Owner

A few use cases for not executing a command right away are

  • logging commands
  • grouping commands
  • sending commands over a network

Yes, you're right, the _redoStack should be cleared here, thanks for pointing it out. I am not sure what you mean by more abstraction and less fragmentation, though.

@mbuttu

mbuttu Mar 28, 2012

Owner

A few use cases for not executing a command right away are

  • logging commands
  • grouping commands
  • sending commands over a network

Yes, you're right, the _redoStack should be cleared here, thanks for pointing it out. I am not sure what you mean by more abstraction and less fragmentation, though.

This comment has been minimized.

Show comment
Hide comment
@ScottDowne

ScottDowne Mar 28, 2012

@ScottDowne

ScottDowne via email Mar 28, 2012

This comment has been minimized.

Show comment
Hide comment
@mbuttu

mbuttu Mar 28, 2012

Owner

I cleared the _redoStack in the register function.

I will explain the benefits of leaving execute up to the caller during my presentation on Thursday.

@mbuttu

mbuttu Mar 28, 2012

Owner

I cleared the _redoStack in the register function.

I will explain the benefits of leaving execute up to the caller during my presentation on Thursday.

+ } //if
+ }, //register
+ canUndo: function(){
+ return _undoStack.length > 0;
+ }, //canUndo
+ canRedo: function(){
+ return _redoStack.length > 0;
+ }, //canRedo
+ undo: function(){
+ if ( this.canUndo() ){
+ var command = _undoStack.pop();
+ _redoStack.push( command );
+ command.undo();
+ } //if
+ }, //undo
+ redo: function(){
+ if ( this.canRedo() ){
+ var command =_redoStack.pop();
+ _undoStack.push( command );
+ command.execute();

This comment has been minimized.

Show comment
Hide comment
@ScottDowne

ScottDowne Mar 27, 2012

Still not convinced execute is the right word here. Should just be redo or just do.

@ScottDowne

ScottDowne Mar 27, 2012

Still not convinced execute is the right word here. Should just be redo or just do.

This comment has been minimized.

Show comment
Hide comment
@mbuttu

mbuttu Mar 27, 2012

Owner

Why do you believe that execute is not the correct word?

As soon as a command object is created, it means that we have planned a course of action to take, but have not yet taken it.

execute was chosen because the literal meaning of the word implies that we are about to carry out the course of action specified in the command object. do, on the other hand, is often used in a more general sense. For example, two questions that are answered during a scrum meeting are: "what did you do yesterday?", "what are you going to do today?"

@mbuttu

mbuttu Mar 27, 2012

Owner

Why do you believe that execute is not the correct word?

As soon as a command object is created, it means that we have planned a course of action to take, but have not yet taken it.

execute was chosen because the literal meaning of the word implies that we are about to carry out the course of action specified in the command object. do, on the other hand, is often used in a more general sense. For example, two questions that are answered during a scrum meeting are: "what did you do yesterday?", "what are you going to do today?"

+ } //if
+ }, //redo
+ clear: function(){
+ _undoStack.length = 0;
+ _redoStack.length = 0;
+ } //clear
+ };
+ } //UndoManager
+
+ return {
+ getInstance: function(){
+ if ( !_undoManager ){
+ _undoManager = new UndoManager();
+ } //if
+ return _undoManager;
+ } //getInstance
+ };
+});
+
View
@@ -212,13 +212,14 @@ define( [
defaultTarget = butter.targets[ 0 ];
} //if
- var trackEvent = track.addTrackEvent({
+ var trackEvent = butter.addTrackEvent({
+ track: track,
+ type: type,
popcornOptions: {
start: start,
end: start + 1,
target: defaultTarget.elementID
- },
- type: type
+ }
});
if( defaultTarget ){

0 comments on commit 3d6dd40

Please sign in to comment.