When timing metadata changes, ensure metadata.changed fires

[mlucool]

This bug was caused by updating the object in place instead of copying before each change. With the former, metadata.changed does not signal.

[jupyterlab-dev-mode]

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link:

[mlucool]

Added a second commit on top which fixes another bug related to this. If we clear execution count, this implies the metadata should be removed. This fixes a bug where "Clear All Outputs" was not clearing timing data.

I considered adding a second function to ICodeModel (e.g. clearOutputs), but it seems cleaner to assume that setting an execution count to 0/null means there is no execution metadata.

[saulshanabrook]

Looks good, these both work locally for me.

If we clear execution count, this implies the metadata should be removed. This fixes a bug where "Clear All Outputs" was not clearing timing data.

I changed it to just reset it in that command, instead of whenever you are setting execution count, just to be a bit more safe about when we are resetting it. What do you think?

[mlucool]

I changed it to just reset it in that command, instead of whenever you are setting execution count, just to be a bit more safe about when we are resetting it. What do you think?

I coupled them as it seemed logical to clear this metadata when the execution count was cleared. My hope was this would prevent more bugs than it causes. I'll also note, I believe the timing stays until we hear back from the kernel.

For example, in the cell widget, I think we'd want to clear the metadata here as we set executionCount to null. I found a few such places in the code. Your commit fixes clearOutputs, but not clearAllOutputs is an example of how the bug creeps up too.

[saulshanabrook]

For example, in the cell widget, I think we'd want to clear the metadata here as we set executionCount to null. I found a few such places in the code. Your commit fixes clearOutputs, but not clearAllOutputs is an example of how the bug creeps up too.

Ah, that makes sense. I was reluctant to include clearing output timings on settings this attribute. Instead it might be better to have a method that you call that resets both of them, which we use in those places. I can try this out to see how it looks.

[mlucool]

I can make that change. I'll add clearExecution() to ICodeCellModel?

[saulshanabrook]

@mlucool OK cool. Yeah that sounds good, maybe does something like:

    this.outputs.clear();
    this.executionCount = null;
    this.metadata.delete('execution');

?

[mlucool]

If you want to include outputs.clear(), it may not be exactly the same. Here for example, this is not coupled. I am not sure if they should have been and this is a bug or just a noop.

[mlucool]

I made the change but I want to call out these two lines to be sure you are ok with them:

1. https://github.com/jupyterlab/jupyterlab/pull/7576/files#diff-901b6622e3beb3db23ab3289d24f5583R1050
2. https://github.com/jupyterlab/jupyterlab/pull/7576/files#diff-fec8375f9426c263bcd547883abe072eR1606

[saulshanabrook]

Yeah this looks good to me. It changes some behavior, but it still seems to work locally for me and I am not sure if the old behavior was important. All tests also pass, so I am good with this.

[mlucool]

Can this be pulled into to 1.x as well?

[jasongrout]

We aren't planning on releasing another 1.x minor release.