Skip to content

Commit

Permalink
Support undo/redo for "move" actions
Browse files Browse the repository at this point in the history 
Includes minor improvements to logging/error handling.
See #279 (does not fix that issue, but this was part of the problem)
  • Loading branch information
@bago
bago committed Apr 9, 2019
1 parent 3c51cb3 commit 227a0a6
Show file tree
Hide file tree
Showing 4 changed files with 92 additions and 37 deletions.
66 changes: 56 additions & 10 deletions spec/wrapper-spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,14 @@

var mockery = require('mockery');
mockery.enable();
mockery.registerAllowables(['../src/js/converter/declarations.js', './wrapper.js', 'console', './utils.js', './domutils.js', 'console', '../node_modules/mensch', 'fs', 'path', 'mkdirp']);
mockery.registerAllowables(['../src/js/converter/declarations.js', './wrapper.js', 'console', './utils.js', './domutils.js', 'console', 'mensch', 'fs', 'path', 'mkdirp']);

var main = require('../src/js/converter/main.js');
var fs = require('fs');

mockery.registerMock('knockout', require('../node_modules/knockout'));
mockery.registerMock('knockout', require('knockout'));
// mockery.registerMock('knockout.wrap', require('../node_modules/knockout.wrap/knockout.wrap.js'));
mockery.registerMock('knockoutjs-reactor', require('../node_modules/ko-reactor/dist/ko-reactor.js'));
mockery.registerMock('knockoutjs-reactor', require('ko-reactor/dist/ko-reactor.js'));
// mockery.registerMock('knockout-undomanager', require('../node_modules/knockout-undomanager/knockout-undomanager.js'));

var undoserializer = require("../src/js/undomanager/undoserializer.js");
Expand Down Expand Up @@ -75,9 +75,8 @@ describe('model wrapper and undomanager', function() {

content().mainBlocks().blocks.push(titleBlock);

/* This is not supported in the current code
var unwrapped = ko.utils.parseJson(jsonContent);
content(unwrapped);
content._wrap(unwrapped);

expect(content().titleText()).toEqual("TITLE");

Expand All @@ -87,7 +86,23 @@ describe('model wrapper and undomanager', function() {

undoRedoStack.undoCommand.execute();

// This requires the "sync" option (async: false) of ko-reactor
expect(content().titleText()).toEqual("TITLE");

undoRedoStack.redoCommand.execute();

expect(content().titleText()).toEqual("New Title 2");

var unwrapped = ko.utils.parseJson(jsonContent);
content._wrap(unwrapped);

expect(content().titleText()).toEqual("TITLE");

// Fails on current code because the "undo" of a full content _wrap doesn't wrap the previous model
/*
undoRedoStack.undoCommand.execute();
expect(content().titleText()).toEqual("New Title 2");
*/

});
Expand Down Expand Up @@ -128,20 +143,20 @@ describe('model wrapper and undomanager', function() {

var debug = function(prefix, blocks) {
for (var i = 0; i < blocks().length; i++) {
console.log(prefix, i, blocks()[i]().text());
// console.log(prefix, i, blocks()[i]().text());
}
};

undoserializer.setListener(function(path, child, oldVal, item) {
console.log("UL:", path, oldVal, item.status, item.index, item.moved, item.value.text);
// console.log("UL:", path, oldVal, item.status, item.index, item.moved, item.value.text);
});

debug("A", blocks);

blocks.subscribe(function (changes) {
var ch = ko.toJS(changes);
for (var i = 0; i < ch.length; i++) {
console.log("AC", i, ch[i].status, ch[i].index, ch[i].moved, ch[i].value.text);
// console.log("AC", i, ch[i].status, ch[i].index, ch[i].moved, ch[i].value.text);
}
}, undefined, 'arrayChange');

Expand All @@ -156,16 +171,17 @@ describe('model wrapper and undomanager', function() {
debug("B", blocks);

expect(blocks()[0]().text()).toEqual("Title 3");
expect(blocks()[1]().text()).toEqual("Title 1");
expect(blocks()[2]().text()).toEqual("Title 2");

undoRedoStack.undoCommand.execute();

debug("C", blocks);

expect(blocks()[0]().text()).toEqual("Title 1");
expect(blocks()[1]().text()).toEqual("Title 2");
expect(blocks()[2]().text()).toEqual("Title 3");

/* This is not supported in the current code

// using "move action" (sortable with move strategy will use valueWillMute/hasMutated
blocks.valueWillMutate();
Expand All @@ -177,15 +193,45 @@ describe('model wrapper and undomanager', function() {
debug("D", blocks);

expect(blocks()[0]().text()).toEqual("Title 3");
expect(blocks()[1]().text()).toEqual("Title 1");
expect(blocks()[2]().text()).toEqual("Title 2");

undoRedoStack.undoCommand.execute();

debug("E", blocks);

expect(blocks()[0]().text()).toEqual("Title 1");
expect(blocks()[1]().text()).toEqual("Title 2");
expect(blocks()[2]().text()).toEqual("Title 3");
*/

// using "move action" (sortable with move strategy will use valueWillMute/hasMutated
blocks.valueWillMutate();
var underlyingBlocks = ko.utils.unwrapObservable(blocks);
var removed2 = underlyingBlocks.splice(0, 1);
underlyingBlocks.splice(2, 0, removed2[0]);
blocks.valueHasMutated();

debug("F", blocks);

expect(blocks()[0]().text()).toEqual("Title 2");
expect(blocks()[1]().text()).toEqual("Title 3");
expect(blocks()[2]().text()).toEqual("Title 1");

undoRedoStack.undoCommand.execute();

debug("G", blocks);

expect(blocks()[0]().text()).toEqual("Title 1");
expect(blocks()[1]().text()).toEqual("Title 2");
expect(blocks()[2]().text()).toEqual("Title 3");

undoRedoStack.redoCommand.execute();

debug("H", blocks);

expect(blocks()[0]().text()).toEqual("Title 2");
expect(blocks()[1]().text()).toEqual("Title 3");
expect(blocks()[2]().text()).toEqual("Title 1");

});

Expand Down
14 changes: 9 additions & 5 deletions src/js/converter/wrapper.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -136,12 +136,15 @@ var _getVariants = function(def) {
var _makeComputedFunction = function(def, defs, thms, ko, contentModel, isContent, t) {
if (typeof def == 'undefined') {
if (typeof ko.utils.unwrapObservable(t).type === 'undefined') {
console.log("TODO ERROR Found a non-typed def ", def, t);
console.error("Found a non-typed def ", def, t);
throw "Found a non-typed def " + def;
}
var type = ko.utils.unwrapObservable(ko.utils.unwrapObservable(t).type);
def = defs[type];
if (typeof def !== 'object') console.log("TODO ERROR Found a non-object def ", def, "for", type);
if (typeof def !== 'object') {
console.error("Found a non-object def ", def, "for", type);
throw "Found a non-object def " + def;
}
}

if (typeof contentModel == 'undefined' && typeof isContent != 'undefined' && isContent) {
Expand All @@ -166,7 +169,8 @@ var _makeComputedFunction = function(def, defs, thms, ko, contentModel, isConten
if (schemePathOrig.substr(0, selfPath.length) == selfPath) {
schemePath = schemePathOrig.substr(selfPath.length);
} else {
console.log("IS THIS CORRECT?", schemePathOrig, selfPath);
// Debug this scenario if it happens
console.log("Scheme path doesn't match selfPath", schemePathOrig, selfPath);
schemePath = schemePathOrig;
}

Expand Down Expand Up @@ -216,8 +220,8 @@ var _makeComputedFunction = function(def, defs, thms, ko, contentModel, isConten
pTarget = pTarget._defaultComputed;
}
if (typeof pTarget == 'undefined') {
console.log("ERROR looking for variant target", def._variant, t);
throw "ERROR looking for variant target " + def._variant;
console.error("Error looking for variant target", def._variant, t);
throw "Error looking for variant target " + def._variant;
}
pParent._nextVariant = _nextVariantFunction.bind(pTarget, ko, pTarget, _getVariants(def));
}
Expand Down
39 changes: 20 additions & 19 deletions src/js/undomanager/undomanager.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,9 +90,7 @@ var undoManager = function (model, options) {
state = workState;
var oldMode = mode;
mode = MODE_MERGE;
// console.log("XDO", "before", label);
action();
// console.log("XDO", "after", label);
_removeMergedAction(lastPushedStack);
mode = oldMode;
state = prevState;
Expand All @@ -106,7 +104,6 @@ var undoManager = function (model, options) {
if (typeof myStack == 'undefined') throw "Unexpected operation: stack cleaner called with undefined stack";

if (myStack().length > 0 && typeof myStack()[myStack().length - 1].mergedAction !== 'undefined') {
// console.log("Removing mergedAction from stack");
delete myStack()[myStack().length - 1].mergedAction;
}
};
Expand All @@ -123,16 +120,25 @@ var undoManager = function (model, options) {
};

var executeUndoAction = function(child, value, item) {
// console.log("executeUndoAction", child, value, item);
if (typeof value !== 'undefined') {
child(value);
} else if (item) {
if (item.status == 'deleted') {
child.splice(item.index, 0, item.value);
} else if (item.status == 'added') {
child.splice(item.index, 1);
if (item.status !== 'added' && item.status !== 'deleted') throw "Unsupported item.status: "+item.status;

// When the items are "moved" we do both delete and add on the "add" action and ignore the delete action
if (typeof item.moved !== 'undefined') {
if (item.status == 'added') {
child.splice(item.index, 1);
child.splice(item.moved, 0, item.value);
} else {
// console.log("Ignoring undo move", item);
}
} else {
throw "Unsupported item.status: "+item.status;
if (item.status == 'added') {
child.splice(item.index, 1);
} else {
child.splice(item.index, 0, item.value);
}
}
} else {
throw "Unexpected condition: no item and no child.oldValues!";
Expand All @@ -146,14 +152,12 @@ var undoManager = function (model, options) {
var makeUndoAction = makeUndoActionDefault;

var changePusher = function(parents, child, item) {
// console.log("CP", item, child.oldValues);
var oldVal = typeof child.oldValues != 'undefined' ? child.oldValues[0] : undefined;
var act = makeUndoAction(executeUndoAction, parents, child, oldVal, item);

if (mode == MODE_IGNORE) return;

if (mode == MODE_MERGE) {
// console.log("UR", "mergemode");
if (typeof act !== 'undefined') {
act.mergedAction = function(newAction) {
if (typeof newAction.mergeMe !== 'undefined' && newAction.mergeMe) {
Expand All @@ -167,48 +171,45 @@ var undoManager = function (model, options) {
if (child.oldValues && mode == MODE_ONCE) {
act.mergedAction = function(oldChild, oldItem, newAction) {
if (typeof newAction.mergeableAction == 'object' && oldChild == newAction.mergeableAction.child) {
// console.log("UR", "ignore update for property in MODE_ONCE");
return this;
} else return null;
}.bind(act, child, item);
act.mergeableAction = { child: child, item: item };
}
// console.log("UR", "item.status", item.status);

// "item" is valued when an item is added/removed/reteined in an array
// sometimes KO detect "moves" and add a "moved" property with the index but
// this doesn't happen for example using knockout-sortable or when moving objects
// between arrays.
// So this ends up handling this with "mergeableMove" and "mergedAction":
if (item && item.status == 'deleted') {
if (item && (item.status == 'deleted' || item.status == 'added')) {
// TODO se sono in MODE = MERGE devo metteer una funzione di merge che accetta tutto.
// altrimenti lascio questa.
act.mergedAction = function(oldChild, oldItem, newAction) {
// console.log("UR", "act.mergedAction", typeof newAction.mergeableMove);
// a deleted action is able to merge with a added action if they apply to the same
// object.
if (typeof newAction.mergeableMove == 'object' && oldItem.value == newAction.mergeableMove.item.value) {
// in this case I simply return a single action running both actions in sequence,
// this way the "undo" will need to undo only once for a "move" operation.
return _combinedFunction(newAction, this);
} else {
// console.log("UR", "not mergeable", typeof newAction.mergeableMove);
}

return null;
}.bind(act, child, item);
} else if (item && item.status == 'added') {

// add a mergeableMove property that will be used by the next action "mergedAction" to see if this action
// can be merged.
act.mergeableMove = { child: child, item: item };
} else if (item) {
console.warn("Unsupported item.status", item.status);
console.warn("Unsupported item.status", item.status, typeof item, item);
}
}
}
if (typeof act !== 'undefined') _push(act);
};

var reactorOptions = { depth: -1, oldValues: 1, mutable: true, /* tagParentsWithName: true */ tagFields: true };
var reactorOptions = { depth: -1, oldValues: 1, mutable: true, /* tagParentsWithName: true */ tagFields: true, async: false /*, splitArrayChanges: false */ };

var context = {};
var react = typeof reactor == 'function' ? reactor : ko.watch;
Expand Down
10 changes: 7 additions & 3 deletions src/js/undomanager/undoserializer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,24 +41,28 @@ var _reference = function(model, path) {

var _getPath = function(parents, child) {
var path = "";
var len = parents.length;
var p;
for (var k = 0; k <= parents.length; k++) {
for (var k = 0; k <= len; k++) {
p = k < parents.length ? parents[k] : child;
if (ko.isObservable(p)) path += '()';
if (typeof p._fieldName !== 'undefined') {
if (ko.isObservable(p)) path += '()';
path += "." + p._fieldName;
} else if (k > 0 && typeof parents[k - 1].pop == 'function') {
var parentArray = ko.isObservable(parents[k - 1]) ? ko.utils.peekObservable(parents[k - 1]) : parents[k - 1];
var pos = ko.utils.arrayIndexOf(parentArray, p);
if (pos != -1) {
if (ko.isObservable(p)) path += '()';
path += "[" + pos + "]";
} else {
// NOTE this happen, sometimes when TinyMCE sends updates for objects already removed.
console.error("Unexpected object not found in parent array", parentArray, p, k, parents.length, ko.toJS(parentArray), ko.utils.unwrapObservable(p));
throw "Unexpected object not found in parent array";
}
} else if (len === 0 && typeof p._fieldName === 'undefined') {
// root path
} else {
console.error("Unexpected parent with no _fieldName and no parent array", k, parents);
console.error("Unexpected parent with no _fieldName and no parent array", k, parents, typeof p);
throw "Unexpected parent with no _fieldName and no parent array";
}
}
Expand Down

0 comments on commit 227a0a6

Please sign in to comment.