Restore cloned output

[ian-r-rose]

Fixes #5976.

This restores cloned output areas. It is kind of a tricky proposition, since the output areas are not available to clone until the notebook is actually loaded. Furthermore, if there are unsaved changes in the notebook (especially cell reorderings), this will restore the wrong output area, or none at all. So really, this should be considered as fixing #5976 on a reasonably-good-effort basis. It's not particularly clean, but it should work for the most common use-cases.

[afshin]

I appreciate the attention to alphabetization ✏️📘

[afshin]

The InstanceTracker#restore() method seems like a good candidate to make async.

[ian-r-rose]

Agreed.

[afshin]

This seems like a reasonable approach. Here are two alternatives that I thought of and I'm wondering what you think about them. I don't propose you implement either, but they might be worth considering:

• What if the restored promise exists only on the NotebookTracker class, which is already sub-classing InstanceTracker?
• What if the we have protected onRestored(): void { /* no-op */ } on the InstanceTracker class, which can be used in the NotebookTracker sub-class to either emit a signal or to resolve a restored promise on that sub-class?

[afshin]

The first option would be trivial to add to NotebookTracker:

/**
  * A promise that resolves when the tracker has restored its notebook panels.
  */
get restored(): Promise<void> {
  return this._restored.promise;
}

/**
  * Restore the notebooks in this tracker.
  */
async restore(
  options: InstanceTracker.IRestoreOptions<NotebookPanel>
): Promise<void> {
  await super.restore(options);
  this._restored.resolve(undefined);
}

private _restored = new PromiseDelegate<void>();

I think I just talked myself out of the second option, in fact. I also seem to be talking myself into the first option.

[afshin]

Actually, we might even consider adding readonly restored: Promise<void> to the INotebookTracker interface, because I can imagine there may be other plugins that wish to do some restoration of their own after the notebooks they relied on have been restored. I feel less confident about this than the above, though. I'm curious what your thoughts are.

[ian-r-rose]

My thinking was roughly this: the restore method was not public on the IInstanceTracker interface, so it seemed odd to add a restored promise. I was also trying to keep a light API-change footprint while still adding what I needed for this functionality. That being said, it does seem like other plugins may want to do some restoration after some other plugin has restored, so maybe it is a good idea to make it more public.

I don't really see why we would add restored to the INotebookTracker instead of the IInstanceTracker base interface. This might not be the only case of such a pattern, and I think we might as well hit more use-cases by exposing it on IInstanceTracker.

[afshin]

If we add it to INotebookTracker and no other instance tracker ever needs it again, our bases are covered. If it turns out others do need it and we later add it to IInstanceTracker then the original notebook trackers will be backward-compatible.

But sure, if you think it'll be broadly useful, let's put it on the base class.

[afshin]

Since we're using undefined now, could we change this to undefined instead of void 0.

Also since Promise.all resolves with an array, could we call it values instead of a singular?

[afshin]

Do you think we need to guarantee that restore() is only called once?

[ian-r-rose]

Sure, I think a check to make sure it is only restored once makes sense. We are still partially protected by it not being on the public interface.

[afshin]

Thanks!

👍

[afshin]

Maybe an additional test is in order for the new API surface area, even though it's minor.

[ian-r-rose]

I actually sat down to do that this morning, and saw that restore wasn't tested either, and adding tests for that seemed significantly more daunting :)

[afshin]

Good point. Let's leave that for another PR.

#5983