From 89b75c21e247d75714073489995c1fbc13da7dec Mon Sep 17 00:00:00 2001 From: Min RK Date: Mon, 18 Jan 2016 15:49:48 +0100 Subject: [PATCH 1/3] multiple-undo for cell deletion use a stack for undelete_backup instead of a singleton avoids data loss on multiple undelete --- notebook/static/notebook/js/notebook.js | 59 ++++++++++++++----------- notebook/tests/notebook/undelete.js | 20 ++++++++- 2 files changed, 51 insertions(+), 28 deletions(-) diff --git a/notebook/static/notebook/js/notebook.js b/notebook/static/notebook/js/notebook.js index a367d95580..77a15a4750 100644 --- a/notebook/static/notebook/js/notebook.js +++ b/notebook/static/notebook/js/notebook.js @@ -123,9 +123,7 @@ define(function (require) { this.kernel = null; this.kernel_busy = false; this.clipboard = null; - this.undelete_backup = null; - this.undelete_index = null; - this.undelete_below = false; + this.undelete_backup_stack = []; this.paste_enabled = false; this.writable = false; // It is important to start out in command mode to match the intial mode @@ -993,7 +991,11 @@ define(function (require) { indices = this.get_selected_cells_indices(); } - this.undelete_backup = []; + var undelete_backup = { + cells: [], + below: false, + index: 0, + }; var cursor_ix_before = this.get_selected_index(); var deleting_before_cursor = 0; @@ -1013,13 +1015,13 @@ define(function (require) { indices.sort(function(a, b) {return b-a;}); for (i=0; i < indices.length; i++) { var cell = this.get_cell(indices[i]); - this.undelete_backup.push(cell.toJSON()); + undelete_backup.cells.push(cell.toJSON()); this.get_cell_element(indices[i]).remove(); this.events.trigger('delete.Cell', {'cell': cell, 'index': indices[i]}); } // Flip the backup copy of cells back to first-to-last order - this.undelete_backup.reverse(); + undelete_backup.cells.reverse(); var new_ncells = this.ncells(); // Always make sure we have at least one cell. @@ -1028,14 +1030,13 @@ define(function (require) { new_ncells = 1; } - this.undelete_below = false; var cursor_ix_after = this.get_selected_index(); if (cursor_ix_after === null) { // Selected cell was deleted cursor_ix_after = cursor_ix_before - deleting_before_cursor; if (cursor_ix_after >= new_ncells) { cursor_ix_after = new_ncells - 1; - this.undelete_below = true; + undelete_backup.below = true; } this.select(cursor_ix_after); } @@ -1043,15 +1044,16 @@ define(function (require) { // Check if the cells were after the cursor for (i=0; i < indices.length; i++) { if (indices[i] > cursor_ix_before) { - this.undelete_below = true; + undelete_backup.below = true; } } // This will put all the deleted cells back in one location, rather than // where they came from. It will do until we have proper undo support. - this.undelete_index = cursor_ix_after; + undelete_backup.index = cursor_ix_after; $('#undelete_cell').removeClass('disabled'); + this.undelete_backup_stack.push(undelete_backup); this.set_dirty(true); return this; @@ -1075,29 +1077,30 @@ define(function (require) { * Restore the most recently deleted cells. */ Notebook.prototype.undelete_cell = function() { - if (this.undelete_backup !== null && this.undelete_index !== null) { + if (this.undelete_backup_stack.length > 0) { + var undelete_backup = this.undelete_backup_stack.pop(); var i, cell_data, new_cell; - if (this.undelete_below) { - for (i = this.undelete_backup.length-1; i >= 0; i--) { - cell_data = this.undelete_backup[i]; + if (undelete_backup.below) { + for (i = undelete_backup.cells.length-1; i >= 0; i--) { + cell_data = undelete_backup.cells[i]; new_cell = this.insert_cell_below(cell_data.cell_type, - this.undelete_index); + undelete_backup.index); new_cell.fromJSON(cell_data); } } else { - for (i=0; i < this.undelete_backup.length; i++) { - cell_data = this.undelete_backup[i]; + for (i=0; i < undelete_backup.cells.length; i++) { + cell_data = undelete_backup.cells[i]; new_cell = this.insert_cell_above(cell_data.cell_type, - this.undelete_index); + undelete_backup.index); new_cell.fromJSON(cell_data); } } this.set_dirty(true); - this.undelete_backup = null; - this.undelete_index = null; } - $('#undelete_cell').addClass('disabled'); + if (this.undelete_backup_stack.length === 0) { + $('#undelete_cell').addClass('disabled'); + } }; /** @@ -1202,11 +1205,13 @@ define(function (require) { } else { return false; } - - if (this.undelete_index !== null && index <= this.undelete_index) { - this.undelete_index = this.undelete_index + 1; - this.set_dirty(true); - } + + this.undelete_backup_stack.map(function (undelete_backup) { + if (index < undelete_backup.index) { + undelete_backup.index += 1; + } + }); + this.set_dirty(true); return true; }; @@ -1219,7 +1224,7 @@ define(function (require) { * @return {Cell|null} handle to created cell or null */ Notebook.prototype.insert_cell_above = function (type, index) { - if (index === null || index === undefined) { + if (index === null || index === undefined) { index = Math.min(this.get_selected_index(index), this.get_anchor_index()); } return this.insert_cell_at_index(type, index); diff --git a/notebook/tests/notebook/undelete.js b/notebook/tests/notebook/undelete.js index 371c1838c8..0449d02b96 100644 --- a/notebook/tests/notebook/undelete.js +++ b/notebook/tests/notebook/undelete.js @@ -47,12 +47,30 @@ casper.notebook_test(function () { this.trigger_keydown('esc'); this.trigger_keydown('d', 'd'); assert_cells("delete cell 1", [a, c, d], 1); + + // Delete cell 1 (2) + this.select_cell(1); + this.trigger_keydown('esc'); + this.trigger_keydown('d', 'd'); + assert_cells("delete cell 1 (2)", [a, d], 1); + + // Undelete cell 2 + this.evaluate(function () { + IPython.notebook.undelete_cell(); + }); + assert_cells("undelete cell 1 (2)", [a, c, d], 2); // Undelete cell 1 this.evaluate(function () { IPython.notebook.undelete_cell(); }); - assert_cells("undelete cell 1", [a, b, c, d], 2); + assert_cells("undelete cell 1", [a, b, c, d], 3); + + // Undelete cell (none) + this.evaluate(function () { + IPython.notebook.undelete_cell(); + }); + assert_cells("undelete cell (none)", [a, b, c, d], 3); // Merge cells 1-2 var bc = b + "\n\n" + c; From f0cb481cb27a5d096a7ed8275f781a1814d7a3a9 Mon Sep 17 00:00:00 2001 From: Min RK Date: Fri, 22 Jan 2016 11:42:45 +0100 Subject: [PATCH 2/3] add select_cells test utility selects [included,excluded) semi-open range, like Python conventions. --- notebook/tests/util.js | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/notebook/tests/util.js b/notebook/tests/util.js index 8a82c9a378..595f4ec538 100644 --- a/notebook/tests/util.js +++ b/notebook/tests/util.js @@ -446,6 +446,15 @@ casper.select_cell = function(index, moveanchor) { }, {i: index, moveanchor: moveanchor}); }; +casper.select_cells = function(index, bound, moveanchor) { + // Select a block of cells in the notebook. + // like Python range, selects [index,bound) + this.evaluate(function (i, n, moveanchor) { + Jupyter.notebook.select(i, moveanchor); + Jupyter.notebook.extend_selection_by(n); + }, {i: index, n: (bound - index - 1), moveanchor: moveanchor}); +}; + casper.click_cell_editor = function(index) { // Emulate a click on a cell's editor. From 5eec6a59510f9abdebeba9d623c7e77a048e57a9 Mon Sep 17 00:00:00 2001 From: Min RK Date: Fri, 22 Jan 2016 11:44:10 +0100 Subject: [PATCH 3/3] fix multi-cell undelete ordering and test multi-cell undelete at both the top and bottom of the notebook --- notebook/static/notebook/js/notebook.js | 24 ++++++----------- notebook/tests/notebook/undelete.js | 36 +++++++++++++++++++------ 2 files changed, 36 insertions(+), 24 deletions(-) diff --git a/notebook/static/notebook/js/notebook.js b/notebook/static/notebook/js/notebook.js index 77a15a4750..e78f9566a8 100644 --- a/notebook/static/notebook/js/notebook.js +++ b/notebook/static/notebook/js/notebook.js @@ -1020,9 +1020,6 @@ define(function (require) { this.events.trigger('delete.Cell', {'cell': cell, 'index': indices[i]}); } - // Flip the backup copy of cells back to first-to-last order - undelete_backup.cells.reverse(); - var new_ncells = this.ncells(); // Always make sure we have at least one cell. if (new_ncells === 0) { @@ -1079,21 +1076,16 @@ define(function (require) { Notebook.prototype.undelete_cell = function() { if (this.undelete_backup_stack.length > 0) { var undelete_backup = this.undelete_backup_stack.pop(); - var i, cell_data, new_cell; + var i, cell_data, new_cell, insert; if (undelete_backup.below) { - for (i = undelete_backup.cells.length-1; i >= 0; i--) { - cell_data = undelete_backup.cells[i]; - new_cell = this.insert_cell_below(cell_data.cell_type, - undelete_backup.index); - new_cell.fromJSON(cell_data); - } + insert = $.proxy(this.insert_cell_below, this); } else { - for (i=0; i < undelete_backup.cells.length; i++) { - cell_data = undelete_backup.cells[i]; - new_cell = this.insert_cell_above(cell_data.cell_type, - undelete_backup.index); - new_cell.fromJSON(cell_data); - } + insert = $.proxy(this.insert_cell_above, this); + } + for (i=0; i < undelete_backup.cells.length; i++) { + cell_data = undelete_backup.cells[i]; + new_cell = insert(cell_data.cell_type, undelete_backup.index); + new_cell.fromJSON(cell_data); } this.set_dirty(true); diff --git a/notebook/tests/notebook/undelete.js b/notebook/tests/notebook/undelete.js index 0449d02b96..1798dc2bfe 100644 --- a/notebook/tests/notebook/undelete.js +++ b/notebook/tests/notebook/undelete.js @@ -42,35 +42,55 @@ casper.notebook_test(function () { this.trigger_keydown('esc'); assert_cells("initial state", [a, b, c, d], 0); - // Delete cell 1 - this.select_cell(1); + // Delete cells 1,2 + this.select_cells(1,3); this.trigger_keydown('esc'); this.trigger_keydown('d', 'd'); - assert_cells("delete cell 1", [a, c, d], 1); + assert_cells("delete cells 1,2", [a, d], 1); - // Delete cell 1 (2) + // Delete cell 1 (3) this.select_cell(1); this.trigger_keydown('esc'); this.trigger_keydown('d', 'd'); - assert_cells("delete cell 1 (2)", [a, d], 1); + assert_cells("delete cell 1 (3)", [a], 0); - // Undelete cell 2 + // Undelete cell 1 (3) this.evaluate(function () { IPython.notebook.undelete_cell(); }); - assert_cells("undelete cell 1 (2)", [a, c, d], 2); + assert_cells("undelete cell 1 (3)", [a, d], 0); + + this.select_cell(1); // select after undelete, to test cursor movement // Undelete cell 1 this.evaluate(function () { IPython.notebook.undelete_cell(); }); - assert_cells("undelete cell 1", [a, b, c, d], 3); + assert_cells("undelete cell 1,2", [a, b, c, d], 3); // Undelete cell (none) this.evaluate(function () { IPython.notebook.undelete_cell(); }); assert_cells("undelete cell (none)", [a, b, c, d], 3); + + this.select_cells(0,2); + this.trigger_keydown('esc'); + this.trigger_keydown('d', 'd'); + assert_cells("delete first two cells", [c, d], 0); + this.evaluate(function () { + IPython.notebook.undelete_cell(); + }); + assert_cells("undelete first two cells", [a, b, c, d], 2); + + this.select_cells(2, 4); + this.trigger_keydown('esc'); + this.trigger_keydown('d', 'd'); + assert_cells("delete last two cells", [a, b], 1); + this.evaluate(function () { + IPython.notebook.undelete_cell(); + }); + assert_cells("undelete last two cells", [a, b, c, d], 1); // Merge cells 1-2 var bc = b + "\n\n" + c;