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

Restore cloned output #5981

Merged
merged 7 commits into from Feb 14, 2019
Merged

Restore cloned output #5981

merged 7 commits into from Feb 14, 2019

Conversation

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Feb 13, 2019

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.

@@ -448,6 +467,7 @@ export class InstanceTracker<T extends Widget>
}

private _restore: InstanceTracker.IRestoreOptions<T> | null = null;
private _restored = new PromiseDelegate<void>();
Copy link
Member

@afshin afshin Feb 13, 2019

Choose a reason for hiding this comment

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

I appreciate the attention to alphabetization ✏️📘

})
);
});
return Promise.all(promises)
Copy link
Member

@afshin afshin Feb 13, 2019

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@ian-r-rose ian-r-rose Feb 13, 2019

Choose a reason for hiding this comment

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

Agreed.

@afshin
Copy link
Member

@afshin afshin commented Feb 13, 2019

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
Copy link
Member

@afshin afshin commented Feb 13, 2019

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
Copy link
Member

@afshin afshin commented Feb 13, 2019

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
Copy link
Member Author

@ian-r-rose ian-r-rose commented Feb 13, 2019

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
Copy link
Member

@afshin afshin commented Feb 13, 2019

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.

return val;
});
);
this._restored.resolve(void 0);
Copy link
Member

@afshin afshin Feb 13, 2019

Choose a reason for hiding this comment

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

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
Copy link
Member

@afshin afshin commented Feb 13, 2019

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

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Feb 13, 2019

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.

@ian-r-rose ian-r-rose force-pushed the restore-cloned-output branch from ecd59e5 to bf70f2c Feb 13, 2019
afshin
afshin approved these changes Feb 13, 2019
Copy link
Member

@afshin afshin left a comment

Thanks!

👍

@afshin
Copy link
Member

@afshin afshin commented Feb 13, 2019

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

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Feb 13, 2019

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
Copy link
Member

@afshin afshin commented Feb 14, 2019

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

#5983

@afshin afshin merged commit 61fe9f2 into jupyterlab:master Feb 14, 2019
9 checks passed
@bollwyvl bollwyvl mentioned this pull request Feb 20, 2019
@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.

2 participants