Catrope has uploaded a new change for review.
https://gerrit.wikimedia.org/r/88728
Change subject: Implement ve.dm.Surface.prototype.undo() and redo() in terms of
change()
......................................................................
Implement ve.dm.Surface.prototype.undo() and redo() in terms of change()
...or really changeInternal(), so we can avoid adding undo transactions
to the undo stack.
Also get rid of the pattern where undo() and redo() return a selection
which the caller then has to restore, and instead just restore the
selection.
Bug: 53224
Change-Id: If5a3b4d4162e9f0713ee9cd26e79a66efe52770f
---
M modules/ve/dm/ve.dm.Surface.js
M modules/ve/test/ce/ve.ce.Surface.test.js
M modules/ve/test/ui/actions/ve.ui.IndentationAction.test.js
M modules/ve/test/ui/actions/ve.ui.ListAction.test.js
M modules/ve/test/ve.test.utils.js
M modules/ve/ui/actions/ve.ui.HistoryAction.js
6 files changed, 47 insertions(+), 52 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/VisualEditor
refs/changes/28/88728/1
diff --git a/modules/ve/dm/ve.dm.Surface.js b/modules/ve/dm/ve.dm.Surface.js
index bf949ea..a5690c2 100644
--- a/modules/ve/dm/ve.dm.Surface.js
+++ b/modules/ve/dm/ve.dm.Surface.js
@@ -454,12 +454,28 @@
* @method
* @param {ve.dm.Transaction|ve.dm.Transaction[]|null} transactions One or
more transactions to
* process, or null to process none
- * @param {ve.Range|undefined} selection
+ * @param {ve.Range} [selection] Selection to apply
* @emits lock
* @emits contextChange
* @emits unlock
*/
ve.dm.Surface.prototype.change = function ( transactions, selection ) {
+ this.changeInternal( transactions, selection, false );
+};
+
+/**
+ * Internal implementation of change(). Do not use this, use change() instead.
+ *
+ * @private
+ * @method
+ * @param {ve.dm.Transaction|ve.dm.Transaction[]|null} transactions
+ * @param {ve.Range} [selection] [selection]
+ * @param {boolean} [skipUndoStack=false] If true, do not modify the undo stack
+ * @emits lock
+ * @emits contextChange
+ * @emits unlock
+ */
+ve.dm.Surface.prototype.changeInternal = function ( transactions, selection,
skipUndoStack ) {
var i, len, contextChange = false;
if ( !this.enabled ) {
@@ -477,8 +493,10 @@
this.emit( 'lock' );
for ( i = 0, len = transactions.length; i < len; i++ ) {
if ( !transactions[i].isNoOp() ) {
- this.truncateUndoStack();
- this.smallStack.push( transactions[i] );
+ if ( !skipUndoStack ) {
+ this.truncateUndoStack();
+ this.smallStack.push( transactions[i] );
+ }
// The .commit() call below indirectly invokes
setSelection()
this.documentModel.commit( transactions[i] );
if (
transactions[i].hasElementAttributeOperations() ) {
@@ -533,66 +551,52 @@
* Step backwards in history.
*
* @method
- * @emits lock
- * @emits unlock
* @emits history
- * @returns {ve.Range} Selection or null if no further state could be reached
*/
ve.dm.Surface.prototype.undo = function () {
+ var i, item, selection, transaction, transactions = [];
if ( !this.enabled || !this.hasPastState() ) {
return;
}
- var item, i, transaction, selection;
+
this.breakpoint();
this.undoIndex++;
- if ( this.bigStack[this.bigStack.length - this.undoIndex] ) {
- this.emit( 'lock' );
- item = this.bigStack[this.bigStack.length - this.undoIndex];
+ item = this.bigStack[this.bigStack.length - this.undoIndex];
+ if ( item ) {
+ // Apply reversed transactions in reversed order, and translate
the selection accordingly
selection = item.selection;
-
for ( i = item.stack.length - 1; i >= 0; i-- ) {
transaction = item.stack[i].reversed();
selection = transaction.translateRange( selection );
- this.documentModel.commit( transaction );
+ transactions.push( transaction );
}
- this.emit( 'unlock' );
+ this.changeInternal( transactions, selection, true );
this.emit( 'history' );
- return selection;
}
- return null;
};
/**
* Step forwards in history.
*
* @method
- * @emits lock
- * @emits unlock
* @emits history
- * @returns {ve.Range} Selection or null if no further state could be reached
*/
ve.dm.Surface.prototype.redo = function () {
+ var item;
if ( !this.enabled || !this.hasFutureState() ) {
return;
}
- var item, i, transaction, selection;
+
this.breakpoint();
- if ( this.bigStack[this.bigStack.length - this.undoIndex] ) {
- this.emit( 'lock' );
- item = this.bigStack[this.bigStack.length - this.undoIndex];
- selection = item.selection;
- for ( i = 0; i < item.stack.length; i++ ) {
- transaction = item.stack[i].clone();
- this.documentModel.commit( transaction );
- }
+ item = this.bigStack[this.bigStack.length - this.undoIndex];
+ if ( item ) {
+ // ve.copy( item.stack ) invokes .clone() on each transaction
in item.stack
+ this.changeInternal( ve.copy( item.stack ), item.selection,
true );
this.undoIndex--;
- this.emit( 'unlock' );
this.emit( 'history' );
- return selection;
}
- return null;
};
/**
diff --git a/modules/ve/test/ce/ve.ce.Surface.test.js
b/modules/ve/test/ce/ve.ce.Surface.test.js
index 13093eb..38c26e0 100644
--- a/modules/ve/test/ce/ve.ce.Surface.test.js
+++ b/modules/ve/test/ce/ve.ce.Surface.test.js
@@ -101,8 +101,8 @@
assert.deepEqual( model.getSelection(), expectedRange, msg + ':
range' );
// Roll back the test Surface
- while ( model.undo() ) {
- /*jshint noempty:false */
+ while ( model.hasPastState() ) {
+ model.undo();
}
}
diff --git a/modules/ve/test/ui/actions/ve.ui.IndentationAction.test.js
b/modules/ve/test/ui/actions/ve.ui.IndentationAction.test.js
index 1e112dc..df98ae9 100644
--- a/modules/ve/test/ui/actions/ve.ui.IndentationAction.test.js
+++ b/modules/ve/test/ui/actions/ve.ui.IndentationAction.test.js
@@ -10,8 +10,7 @@
/* Tests */
function runIndentationChangeTest( assert, range, method, expectedSelection,
expectedData, expectedOriginalData, msg ) {
- var selection,
- surface = ve.test.utils.createSurfaceFromHtml(
ve.dm.example.isolationHtml ),
+ var surface = ve.test.utils.createSurfaceFromHtml(
ve.dm.example.isolationHtml ),
indentationAction = new ve.ui.IndentationAction( surface ),
data = ve.copy( surface.getModel().getDocument().getFullData()
),
originalData = ve.copy( data );
@@ -27,10 +26,10 @@
assert.deepEqual( surface.getModel().getDocument().getFullData(), data,
msg + ': data models match' );
assert.deepEqual( surface.getModel().getSelection(), expectedSelection,
msg + ': selections match' );
- selection = surface.getModel().undo();
+ surface.getModel().undo();
assert.deepEqual( surface.getModel().getDocument().getFullData(),
originalData, msg + ' (undo): data models match' );
- assert.deepEqual( selection, range, msg + ' (undo): selections match' );
+ assert.deepEqual( surface.getModel().getSelection(), range, msg + '
(undo): selections match' );
surface.destroy();
}
diff --git a/modules/ve/test/ui/actions/ve.ui.ListAction.test.js
b/modules/ve/test/ui/actions/ve.ui.ListAction.test.js
index a41e6fa..bf4a1d5 100644
--- a/modules/ve/test/ui/actions/ve.ui.ListAction.test.js
+++ b/modules/ve/test/ui/actions/ve.ui.ListAction.test.js
@@ -10,8 +10,7 @@
/* Tests */
function runListConverterTest( assert, html, method, style, range,
expectedSelection, expectedData, expectedOriginalData, msg ) {
- var selection,
- surface = ve.test.utils.createSurfaceFromHtml( html ||
ve.dm.example.html ),
+ var surface = ve.test.utils.createSurfaceFromHtml( html ||
ve.dm.example.html ),
listAction = new ve.ui.ListAction( surface ),
data = ve.copy( surface.getModel().getDocument().getFullData()
),
originalData = ve.copy( data );
@@ -26,10 +25,10 @@
assert.deepEqual( surface.getModel().getDocument().getFullData(), data,
msg + ': data models match' );
assert.deepEqual( surface.getModel().getSelection(), expectedSelection,
msg + ': selections match' );
- selection = surface.getModel().undo();
+ surface.getModel().undo();
assert.deepEqual( surface.getModel().getDocument().getFullData(),
originalData, msg + ' (undo): data models match' );
- assert.deepEqual( selection, range, msg + ' (undo): selections match' );
+ assert.deepEqual( surface.getModel().getSelection(), range, msg + '
(undo): selections match' );
surface.destroy();
}
diff --git a/modules/ve/test/ve.test.utils.js b/modules/ve/test/ve.test.utils.js
index 26ffaad..30d84f0 100644
--- a/modules/ve/test/ve.test.utils.js
+++ b/modules/ve/test/ve.test.utils.js
@@ -26,8 +26,7 @@
};
ve.test.utils.runFormatConverterTest = function ( assert, range, type,
attributes, expectedSelection, expectedData, msg ) {
- var selection,
- surface = ve.test.utils.createSurfaceFromHtml(
ve.dm.example.isolationHtml ),
+ var surface = ve.test.utils.createSurfaceFromHtml(
ve.dm.example.isolationHtml ),
formatAction = new ve.ui.FormatAction( surface ),
data = ve.copy( surface.getModel().getDocument().getFullData()
),
originalData = ve.copy( data );
@@ -40,10 +39,10 @@
assert.deepEqual( surface.getModel().getDocument().getFullData(), data,
msg + ': data models match' );
assert.deepEqual( surface.getModel().getSelection(), expectedSelection,
msg + ': selections match' );
- selection = surface.getModel().undo();
+ surface.getModel().undo();
assert.deepEqual( surface.getModel().getDocument().getFullData(),
originalData, msg + ' (undo): data models match' );
- assert.deepEqual( selection, range, msg + ' (undo): selections match' );
+ assert.deepEqual( surface.getModel().getSelection(), range, msg + '
(undo): selections match' );
surface.destroy();
};
diff --git a/modules/ve/ui/actions/ve.ui.HistoryAction.js
b/modules/ve/ui/actions/ve.ui.HistoryAction.js
index 6580c8c..6c37c5d 100644
--- a/modules/ve/ui/actions/ve.ui.HistoryAction.js
+++ b/modules/ve/ui/actions/ve.ui.HistoryAction.js
@@ -42,10 +42,7 @@
* @method
*/
ve.ui.HistoryAction.prototype.undo = function () {
- var range = this.surface.getModel().undo();
- if ( range ) {
- this.surface.getModel().change( null, range );
- }
+ this.surface.getModel().undo();
};
/**
@@ -54,10 +51,7 @@
* @method
*/
ve.ui.HistoryAction.prototype.redo = function () {
- var range = this.surface.getModel().redo();
- if ( range ) {
- this.surface.getModel().change( null, range );
- }
+ this.surface.getModel().redo();
};
/* Registration */
--
To view, visit https://gerrit.wikimedia.org/r/88728
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: If5a3b4d4162e9f0713ee9cd26e79a66efe52770f
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/VisualEditor
Gerrit-Branch: master
Gerrit-Owner: Catrope <roan.katt...@gmail.com>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
