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

[Feature request] restore "Output view" #5976

Closed
defeo opened this issue Feb 11, 2019 · 8 comments · Fixed by #5981
Closed

[Feature request] restore "Output view" #5976

defeo opened this issue Feb 11, 2019 · 8 comments · Fixed by #5981

Comments

@defeo
Copy link

@defeo defeo commented Feb 11, 2019

Hello,

This issue popped up while discussing with @fperez. I have an output view on a notebook cell, like this:

screenshot_2019-02-11 jupyterlab

The split view is saved in the current workspace, however without enough information to restore the output view. If I reload the page, the output view disappears:

screenshot_2019-02-11 jupyterlab 1

It would be nice if the output view could be restored. I'm especially interested in having this for Binder.

Tested in 1.0.0a1

@ian-r-rose
Copy link
Member

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

Thanks @defeo, this is a nagging issue that I've been wanting to fix for a while. Unfortunately, the output area clone doesn't work that well with the layout restoration system. It should be possible to figure out, but it's not very natural.

It would definitely be nice to be able to rely on this for binder demos.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Feb 11, 2019

@ian-r-rose - a 1.0 issue, or Future?

@ian-r-rose
Copy link
Member

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

Let me take a look this afternoon. Last I checked it wasn't trivial, but maybe I can see a way through...

@ian-r-rose
Copy link
Member

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

I took a look at this: it's a really tricky problem. The cloned output area widgets are not normal widgets to restore: they are irrevocably tied to a particular notebook. This means, for instance, that they cannot be restored until after all the notebooks are restored. Unfortunately, this leads to a cycle of promise resolution that would have to be broken:

  1. Notebook widgets restore, but the documanager does not load the contents until the app is restored.
  2. Cloned output areas need the notebook contents to be loaded before restoring, which needs to happen after the app is restored.
  3. The app restore promise is not resolved until both restores above complete.

The solution is probably to put empty placeholder widgets for cloned output areas and populate them once their notebook is loaded (right now they are populated synchronously), but that requires some reworking of things. I have some initial work here, but it's not functional yet.

This is not even touching the problem of out-of-band or unsaved output clones: a cloned output could be of an unsaved or moved cell that is not possible to restore, since it does not exist on the server. That can of worms likely couldn't be solved until we have a server-side data model.

Marking as future, unless I feel inspired.

@ian-r-rose ian-r-rose added this to the Future milestone Feb 13, 2019
@ian-r-rose
Copy link
Member

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

Okay, I went after this in #5981. The problem of unsaved notebook changes is still there, but this should still work for a lot of use-cases.

@ian-r-rose ian-r-rose removed this from the Future milestone Feb 13, 2019
@ian-r-rose ian-r-rose added this to the 1.0 milestone Feb 13, 2019
@bollwyvl
Copy link
Contributor

@bollwyvl bollwyvl commented Feb 13, 2019

@ian-r-rose
Copy link
Member

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

There wasn't anything like that before (except in the hover text for the tab). In #5981, however, I added a path and index attribute to the cloned output content. Would that do the trick?

@afshin afshin closed this in #5981 Feb 14, 2019
@bollwyvl
Copy link
Contributor

@bollwyvl bollwyvl commented Feb 14, 2019

added a path and index attribute to the cloned output content

That's a good start! Here's what my current janky 🦆 typing code (poorly) does at present (obviously should be checking types):

    getSession(widget: Widget): Session.ISession {
      let unsafe = widget as any;
      let clientSession =
        unsafe.session || (unsafe.context && unsafe.context.session);
      let session = (clientSession && clientSession._session) || null;
      return session || clientSession || null;
    }

I'm pulling the private session because i then immediately call changeKernel, and doing so on a ClientSession kills the underlying ISession, which may not be what I want if I have, say, a console also attached to that kernel. Anyhow, if the above doesn't work, I can look through the running sessions and slap it under whatever one is open... there isn't really a way to know, probably, exactly which widget it came from, as I don't think my current setup would actually allow you to move a "new view" of a notebook onto another kernel, as I'm pretty sure they share the same underlying session, and there's probably no way to break that now (if that's even a good thing).

This is definitely model-breaking stuff, but then we'll never know if it can work if we don't look 👹... looking forward to messing with 1.0.0!
screenshot from 2019-02-14 08-07-06

@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 pull requests

Successfully merging a pull request may close this issue.

4 participants