Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

fix for #1678, undo no longer clears cells #1965

Merged
merged 1 commit into from

2 participants

@ivanov
Owner

With these changes, Ctrl-Z inside of codemirror cells will only undo up to the text that was in the cell when it was loaded from JSON. This fixes #1678.

I found another bug where switching the cell type causes the loss of all
undo history for that cell. With this commit, switching the cell type
simply resets the history to whatever it was at the time of the cell type switch.

For example, if a cell had contents changed from X -> Y -> Z, and then you change the cell type, before this commit, its history becomes Ø -> Z (where Ø is just empty), so pressing Ctrl-Z once in a changed cell just erases all of it's contents. After this commit, the new cell's history just gets reset to Z

@ivanov ivanov fix for #1678, undo no longer clears cells
I found another bug where switching the cell type causes the loss of all
undo history for that cell. With this commit, switching the cell type
simply resets the history
221ff6a
@minrk
Owner

What's the difference between losing and resetting the history?

@ivanov
Owner

In the notebook, when a CodeMirror cell is initiated, it's text value is set to a blank. Later, the cell value gets set with set_text(text) - which causes that to be an entry in the cell's history, and so the undo history's initial entry is that of a blank value. So resetting the history after a call to set_text causes the initial entry to become whatever it was in the JSON.

@minrk
Owner

ok, thanks.

@ivanov
Owner

sorry about the confusing wording, if a cell had contents changed from X -> Y -> Z, and then you change the cell type, before this commit, its history becomes Ø -> Z (where Ø is just empty), so pressing Ctrl-Z once in a changed cell just erases all of it's contents. After this commit, the new cell's history just gets reset to Z

@minrk
Owner

Is there any case where calling 'set_text' shouldn't do this? I'm wondering if that's where this call belongs.

@minrk
Owner

If that's not true, then I'm fine merging as-is, and we can worry about consolidation later.

@ivanov
Owner

Is there any case where calling 'set_text' shouldn't do this?

yes. Merging or splitting a cell also uses set_text. So if you do a split, and then change your mind, you can actually undo back within both cells and one of them will return to the unified pre-split stage, and the other will return to empty state.

%load and %loadpy magics also ends up using set_text - and since those create new cells, and all new cells created within a session have Ø at the root of their history, not resetting the history keeps things consistent.

I can also imagine downstream widgets using this function, the effects of which would then be undo-able

@minrk
Owner

gotcha, merging as-is then.

@minrk minrk merged commit 172b680 into ipython:master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 15, 2012
  1. @ivanov

    fix for #1678, undo no longer clears cells

    ivanov authored
    I found another bug where switching the cell type causes the loss of all
    undo history for that cell. With this commit, switching the cell type
    simply resets the history
This page is out of date. Refresh to see the latest.
View
3  IPython/frontend/html/notebook/static/js/codecell.js
@@ -267,6 +267,9 @@ var IPython = (function (IPython) {
if (data.cell_type === 'code') {
if (data.input !== undefined) {
this.set_text(data.input);
+ // make this value the starting point, so that we can only undo
+ // to this state, instead of a blank cell
+ this.code_mirror.clearHistory();
}
if (data.prompt_number !== undefined) {
this.set_input_prompt(data.prompt_number);
View
15 IPython/frontend/html/notebook/static/js/notebook.js
@@ -602,6 +602,9 @@ var IPython = (function (IPython) {
text = '';
}
target_cell.set_text(text);
+ // make this value the starting point, so that we can only undo
+ // to this state, instead of a blank cell
+ target_cell.code_mirror.clearHistory();
source_element.remove();
this.dirty = true;
};
@@ -623,6 +626,9 @@ var IPython = (function (IPython) {
// The edit must come before the set_text.
target_cell.edit();
target_cell.set_text(text);
+ // make this value the starting point, so that we can only undo
+ // to this state, instead of a blank cell
+ target_cell.code_mirror.clearHistory();
source_element.remove();
this.dirty = true;
};
@@ -645,6 +651,9 @@ var IPython = (function (IPython) {
// The edit must come before the set_text.
target_cell.edit();
target_cell.set_text(text);
+ // make this value the starting point, so that we can only undo
+ // to this state, instead of a blank cell
+ target_cell.code_mirror.clearHistory();
source_element.remove();
this.dirty = true;
};
@@ -667,6 +676,9 @@ var IPython = (function (IPython) {
// The edit must come before the set_text.
target_cell.edit();
target_cell.set_text(text);
+ // make this value the starting point, so that we can only undo
+ // to this state, instead of a blank cell
+ target_cell.code_mirror.clearHistory();
source_element.remove();
this.dirty = true;
};
@@ -693,6 +705,9 @@ var IPython = (function (IPython) {
target_cell.set_level(level);
target_cell.edit();
target_cell.set_text(text);
+ // make this value the starting point, so that we can only undo
+ // to this state, instead of a blank cell
+ target_cell.code_mirror.clearHistory();
source_element.remove();
this.dirty = true;
};
View
3  IPython/frontend/html/notebook/static/js/textcell.js
@@ -158,6 +158,9 @@ var IPython = (function (IPython) {
if (data.cell_type === this.cell_type) {
if (data.source !== undefined) {
this.set_text(data.source);
+ // make this value the starting point, so that we can only undo
+ // to this state, instead of a blank cell
+ this.code_mirror.clearHistory();
this.set_rendered(data.rendered || '');
this.rendered = false;
this.render();
Something went wrong with that request. Please try again.