Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactoring Notebook.command_mode #5268

Merged
merged 3 commits into from Mar 6, 2014
Merged

Refactoring Notebook.command_mode #5268

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c691663
Refactoring Notebook.command_mode.
ellisonbg Mar 3, 2014
9988030
Removing conditionals that are not needed.
ellisonbg Mar 4, 2014
30c5160
Make sure we are in command mode before we select a new cell.
ellisonbg Mar 4, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 0 additions & 2 deletions IPython/html/static/notebook/js/keyboardmanager.js
View file
Open in desktop
Expand Up @@ -84,7 +84,6 @@ var IPython = (function (IPython) {
help_index : 'aa',
handler : function (event) {
IPython.notebook.command_mode();
IPython.notebook.focus_cell();
return false;
}
},
Expand All @@ -93,7 +92,6 @@ var IPython = (function (IPython) {
help_index : 'ab',
handler : function (event) {
IPython.notebook.command_mode();
IPython.notebook.focus_cell();
return false;
}
},
Expand Down
52 changes: 27 additions & 25 deletions IPython/html/static/notebook/js/notebook.js
View file
Open in desktop
Expand Up @@ -116,11 +116,11 @@ var IPython = (function (IPython) {
});

$([IPython.events]).on('edit_mode.Cell', function (event, data) {
that.handle_edit_mode(that.find_cell_index(data.cell));
that.handle_edit_mode(data.cell);
});

$([IPython.events]).on('command_mode.Cell', function (event, data) {
that.command_mode();
that.handle_command_mode(data.cell);
});

$([IPython.events]).on('status_autorestarting.Kernel', function () {
Expand Down Expand Up @@ -519,40 +519,44 @@ var IPython = (function (IPython) {
};

/**
* Make the notebook enter command mode.
* Handle when a a cell blurs and the notebook should enter command mode.
*
* @method command_mode
* @method handle_command_mode
* @param [cell] {Cell} Cell to enter command mode on.
**/
Notebook.prototype.command_mode = function () {
// Make sure there isn't an edit mode cell lingering around.
var cell = this.get_cell(this.get_edit_index());
if (cell) {
cell.command_mode();
}

// Notify the keyboard manager if this is a change of mode for the
// notebook as a whole.
Notebook.prototype.handle_command_mode = function (cell) {
if (cell === null) { return; } // TODO: do I really need this?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need this. It only existed for command_mode the callable method, and doesn't need to exist in the cell event handler.

if (this.mode !== 'command') {
cell.command_mode(); // TODO: is this OK here?
this.mode = 'command';
$([IPython.events]).trigger('command_mode.Notebook');
IPython.keyboard_manager.command_mode();
}
};

/**
* Make the notebook enter command mode.
*
* @method command_mode
**/
Notebook.prototype.command_mode = function () {
var cell = this.get_cell(this.get_edit_index());
if (cell && this.mode !== 'command') {
// We don't call cell.command_mode, but rather call cell.focus_cell()
// which will blur and CM editor and trigger the call to
// handle_command_mode.
cell.focus_cell();
}
};

/**
* Handle when a cell fires it's edit_mode event.
*
* @method handle_edit_mode
* @param [index] {int} Cell index to select. If no index is provided,
* the current selected cell is used.
* @param [cell] {Cell} Cell to enter edit mode on.
**/
Notebook.prototype.handle_edit_mode = function (index) {
// Make sure the cell exists.
var cell = this.get_cell(index);
if (cell === null) { return; }

// Set the cell to edit mode and notify the keyboard manager if this
// is a change of mode for the notebook as a whole.
Notebook.prototype.handle_edit_mode = function (cell) {
if (cell === null) { return; } // TODO: do I really need this?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto here

if (this.mode !== 'edit') {
cell.edit_mode();
this.mode = 'edit';
Expand Down Expand Up @@ -1449,7 +1453,6 @@ var IPython = (function (IPython) {
var cell_index = this.find_cell_index(cell);

cell.execute();
cell.focus_cell();
this.command_mode();
this.set_dirty(true);
};
Expand Down Expand Up @@ -1501,7 +1504,6 @@ var IPython = (function (IPython) {
}

this.select(cell_index+1);
this.get_cell(cell_index+1).focus_cell();
this.command_mode();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just thinking out loud, should command_mode optionally accept an index like edit_mode does? So you don't have to call this.select above it? Or maybe the same logic in reverse, should we remove the optional index from edit_mode so the user has to call select before calling it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me do an audit about the different ways command_mode and edit_mode are called to see which makes more sense. I agree that consistency makes sense here.

this.set_dirty(true);
};
Expand Down Expand Up @@ -1972,7 +1974,7 @@ var IPython = (function (IPython) {
this.edit_mode(0);
} else {
this.select(0);
this.command_mode();
this.handle_command_mode(this.get_cell(0));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little confused, why can't we use the callable command_mode() here? Invoking the event seems a little funky to me...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I do this is that the cells CM editor isn't focused so calling command_mode won't trigger the event to calls handle_command_mode. This is basically a manual call to set the notebooks initial state to command mode.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this fixes the problem @ivanov is having in his tutorial branch.

}
this.set_dirty(false);
this.scroll_to_top();
Expand Down