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

Add a NotebookActions.executed signal. #4744

Merged
merged 9 commits into from Jun 20, 2018
Merged

Conversation

@afshin
Copy link
Member

@afshin afshin commented Jun 18, 2018

Fixes #4740

Adds an executed signal on the NotebookActions collection of functionality that emits whenever a Markdown or code cell is run. The args that the signal receiver gets is an object that has the notebook: Notebook and the cell: Cell.

If multiple cells are run, each individual cell will be emitted.

Here is an example extension that uses this signal:

import {
  JupyterLabPlugin
} from '@jupyterlab/application';

import {
  NotebookActions
} from '@jupyterlab/notebook';

/**
 * A cell execution notifier extension.
 */
const plugin: JupyterLabPlugin<void> = {
  id: '@jupyterlab/pull-4744:plugin',
  autoStart: true,
  activate: () => {
    NotebookActions.executed.connect((sender, args) => {
      const { notebook, cell } = args;

      console.log('Executed cell:', cell);
      console.log('Parent notebook:', notebook);
    });
  }
};

export default plugin;

Additionally, this PR cleans up the actions.tsx file:

  • Use native array forEach(), filter(), and map() where appropriate.
  • Only use let when a variable is mutable, otherwise use const.
  • Name the notebook parameter of functions notebook instead of widget for clarity.
@afshin afshin changed the title [wip] Add a NotebookActions.executed signal. Add a NotebookActions.executed signal. Jun 19, 2018
Copy link
Member

@ian-r-rose ian-r-rose left a comment

Looks good to me, just a couple of questions/comments.

/**
* A signal that emits whenever a cell is run.
*/
static get executed(): ISignal<any, { parent: Notebook, child: Cell }> {
Copy link
Member

@ian-r-rose ian-r-rose Jun 19, 2018

Out of curiosity, do we have any other examples of static signals that are not attached to a class instance?

Copy link
Member Author

@afshin afshin Jun 19, 2018

I don't believe we have any other static signals, but since we use the NotebookActions namespace basically as though it is a static class, from an API consumer point of view, I thought of it as basically the same thing as the signals on the CommandRegistry or any other class that we only instantiate once.

Do you find the pattern problematic or just a curious case?

Copy link
Member

@ian-r-rose ian-r-rose Jun 19, 2018

I don't find it problematic, just wondering if we had any other examples of the pattern.

* A signal that emits whenever a cell is run.
*/
export
const executed = new Signal<any, { parent: Notebook, child: Cell }>({ });
Copy link
Member

@ian-r-rose ian-r-rose Jun 19, 2018

What winds up being bound to the first argument of the signal template? Also, a couple of thoughts about how this would be used:

  1. Would notebook and cell be better names for the object attributes?
  2. What if somebody wanted to know the index of the cell that was executed? They could iterate over notebook.widgets to find a match for cell, but perhaps we should bundle that information in the args?

Copy link
Member Author

@afshin afshin Jun 19, 2018

We don't have that index either at the point where the signal is emitted. I can see it being useful but I am not sure I want to incur the lookup every time a cell is executed, especially because many cells might run in a row and each of them would require a linear search.

I am not wedded to parent and child. I'm happy to change those to notebook and cell.

Copy link
Member

@ian-r-rose ian-r-rose Jun 19, 2018

True enough, it just puts the onus of the linear search on the consumer of the signal (which they may or may not need to do).

How about the any for the first template argument? What gets bound to that if the NotebookActions class is never instantiated?

Copy link
Member Author

@afshin afshin Jun 19, 2018

The Signal instance has an object that it is instantiated with:

const executed = new Signal<any, { notebook: Notebook, cell: Cell }>({ });

I would have made it sender: void or sender: null but the underlying Signal class implementation uses the senders and receivers in a weak map, so you can't use undefined or null as keys in that map. A consumer of this signal could reference the sender but it'll be harmless and effectively meaningless. We have several signals where we effectively ignore the sender even though it has a type because we often connect() in a spot that already has a reference to that sender.

Copy link
Member Author

@afshin afshin Jun 19, 2018

index = notebook.widgets.indexOf(cell) is simple enough that since it's possible many connectors to this signal won't need it, I don't think it's too bad to impose it on those who do ... although, I suppose the risk of that is if there are ten signal connections and each one actually does care about that index, then we've imposed a 10x heavier CPU load than if the index were calculated upfront.

I think that since adding the index would be backward-compatible, maybe we should wait to see how it is used. @mkery might have an opinion about this.

Copy link
Member

@ian-r-rose ian-r-rose Jun 19, 2018

That's reasonable. Thanks @afshin!

@afshin afshin merged commit 75b830b into jupyterlab:master Jun 20, 2018
2 checks passed
@afshin afshin deleted the notebook-executed branch Jul 5, 2018
@afshin afshin mentioned this pull request Jul 5, 2018
@jasongrout jasongrout added this to the 0.33 milestone Jul 19, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants