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.

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

Use a scheduling list for running marked cells #946

Merged
merged 14 commits into from
Sep 24, 2017

Conversation

juhasch
Copy link
Member

@juhasch juhasch commented Apr 5, 2017

Run each cell individually after the previous one has finished. This allows stopping execution of a following cell:
run_list

Note: Only works with Notebook >= 5.0

@juhasch
Copy link
Member Author

juhasch commented Apr 9, 2017

Allow interrupting scheduled execution. Second click on interrupt button stops execution immediately.
interrupt

@gabyx
Copy link
Contributor

gabyx commented Aug 23, 2017

Awesome feature! Really helpful

Copy link
Member

@jcb91 jcb91 left a comment

Choose a reason for hiding this comment

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

Nice idea, although it does lead to potential problems when multiple frontends have different execution queues 😨

if (cell.metadata.run_control != undefined) {
if (cell.metadata.run_control.marked == true ) {
var g=cell.code_mirror.getGutterElement();
$(g).css({"background-color": "#0f0"});
Copy link
Member

Choose a reason for hiding this comment

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

To reset, this should just set an empty string, I think

Copy link
Member

Choose a reason for hiding this comment

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

Wait, I think maybe it's not a reset, and the comment needs altering?

};

// updates default params with any specified in the provided config data
var update_params = function (config_data) {
Copy link
Member

Choose a reason for hiding this comment

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

this function never actually gets called. Also, given parameters are all runtools.x, it would make more sense to just do

$.extend(true, params, config_data.runtools);

"use strict";

var base_url = utils.get_body_data("baseUrl");
var config = new configmod.ConfigSection('notebook', {base_url: base_url});
Copy link
Member

Choose a reason for hiding this comment

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

it's better I think to use Jupyter.notebook.config, which is loaded by the page already, and means we don't have to make a second request to the server (or load utils/config modules)

}
};

config.loaded.then(function() {
Copy link
Member

Choose a reason for hiding this comment

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

if using Jupyter.notebook.config, as suggested above, loaded has already resolved, so this would get executed immediately. Instead, this should be changed into a function (say initialize), then in load_ipython_extension you can call Jupyter.notebook.config.loaded.then(initialize);

function set_cell_state(cell, state) {
var icon = "";
if (state === 'locked')
{ icon = '<i class="fa fa-lock" /i>' }
Copy link
Member

Choose a reason for hiding this comment

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

should this also have aria-hidden="true"?

events.one('notebook_loaded.Notebook', initGutter);
}
});
config.load()
Copy link
Member

Choose a reason for hiding this comment

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

this can be replaced with Jupyter.notebook.config.loaded.then(initialize); as discussed above

@@ -4,4 +4,61 @@ Description: Runtools provide a number of additional functions for working with
Link: readme.md
Icon: icon.png
Main: main.js
Compatibility: 4.x
Compatibility: 5.x
Copy link
Member

Choose a reason for hiding this comment

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

does this no longer work on 4.x?

- name: runtools.marked_color
description: |
Color for marking a codecell
default: '#0f0'
Copy link
Member

Choose a reason for hiding this comment

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

I have a feeling that the configurator fails to load correctly 3-character colour codes, so it's probably best to use the full 6-char code here

- name: runtools.scheduled_color
description: |
Color when a codecell is scheduled to be rund
default: '#00f'
Copy link
Member

Choose a reason for hiding this comment

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

as above

if (val) {
cm.state.cellState = new State(parseOptions(val));
updateInViewport(cm);
cm.on("gutterClick", onGutterClick);
Copy link
Member

Choose a reason for hiding this comment

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

are these defined in this scope? How does this file get loaded?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is loaded at load_extension time now.

@juhasch
Copy link
Member Author

juhasch commented Aug 23, 2017

@jcb91: Thanks for the review

The reason I did not continue to work on this (apart from restricted time) is, that I am not sure what the right behavior should be. Or if there is the a 'right' behavior at all.

What happens if you reshuffle the cells during execution? Should they be executed in the original sequence or should the cells that are now further up be executed first?
Both approaches have their merits. A UI that makes this transparent is necessary for this and I have no idea how it should look right now. Suggestions are welcome 😄

@juhasch
Copy link
Member Author

juhasch commented Sep 20, 2017

OK, I have updated the extension, incorporated @jcb91's comments and updated the documentation.

@juhasch juhasch merged commit 862c6b3 into ipython-contrib:master Sep 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants