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

Mimerender tracker #4762

Merged
merged 8 commits into from Jun 23, 2018
Merged

Mimerender tracker #4762

merged 8 commits into from Jun 23, 2018

Conversation

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Jun 21, 2018

Supersedes #4554.

We currently have no way for extension authors to interact with any documents created by the rendermime extension interface. So if anybody wants to iterate over open PDFs, JSON files, Vega files, rendered Markdown files, etc, they are out of luck.

This adds an IMimeDocumentTracker token for other extensions to import, which gives them access to an instance tracker that holds all the widgets rendered by a MIME renderer.

@afshin
Copy link
Member

@afshin afshin commented Jun 22, 2018

I was going through your PR and thinking about this ... if we add the factory name that generates a MIME document to that document as a readonly attribute, we can get rid of all the individual trackers and the separate MIME document tracker and we can have a single tracker (the one you call mimeDocumentTracker), which would call restorer.restore(...) after all the MIME renderers have been created and loaded:

const restore: JupyterLabPlugin<void> =  {
  id: '@jupyterlab/application-extension:restore',
  requires: [ILayoutRestorer],
  activate: (app: JupyterLab, restorer: ILayoutRestorer) => {
    restorer.restore(app.mimeDocumentTracker as InstanceTracker<MimeDocument>, {
      command: 'docmanager:open',
      args: ({ context, factory }) => ({ path: context.path, factory }),
      name: ({ context, factory }) => `${factory}:${context.path}`
    });
  }
}

@afshin
Copy link
Member

@afshin afshin commented Jun 22, 2018

Or perhaps change the mimeDocumentTracker to have type InstanceTracker<MimeDocument> instead of IInstanceTracker<MimeDocument>.

@afshin
Copy link
Member

@afshin afshin commented Jun 22, 2018

Actually, now that I'm thinking about it more, the name function probably also needs to be prefixed with the factory name so that there won't be name collisions when there are multiple views to the same path:

name: ({ context, factory }) => `${factory}:${context.path}`

I updated the snippet above as well, but it's worth its own comment in case you only read the email version.

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Jun 22, 2018

That's a nice idea, I'll give it a shot.

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Jun 22, 2018

Hmm, after looking at your suggestion @afshin, it does not look so straightforward. The factory name is not available to the MimeDocument, and there is some extra threading through of the name. I'll push a commit that does this, and let's decide whether it is an improvement. If not, I can revert it.

Edit: I am now thinking this is more trouble than it is worth. The factory name is buried pretty deeply, and I think it will require significant changes to rendermime-interfaces.

An alternative would be to have the localStorage key be ${widget.context.path}:${widget.content.mimeType}`, since we do have access to those.

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Jun 22, 2018

Okay, I got the factory name in there using an AttachedProperty. Let me know if you think that
7bcee0b is an improvement, otherwise I'll revert it.

@ian-r-rose ian-r-rose force-pushed the mimerender_tracker branch from 02f3481 to 37c28f6 Jun 22, 2018
@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Jun 22, 2018

Okay, this should be ready. I think we've landed on a good solution here.

@afshin
Copy link
Member

@afshin afshin commented Jun 22, 2018

How do you feel about just calling the variable tracker instead of mimeDocumentTracker since it's the only tracker in mimerenderers.ts?

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Jun 22, 2018

Sure.

afshin
afshin approved these changes Jun 23, 2018
Copy link
Member

@afshin afshin left a comment

Nice! This turned out really great. Thanks, @ian-r-rose!

@afshin afshin merged commit d8d72a2 into jupyterlab:master Jun 23, 2018
2 checks passed
@jasongrout jasongrout added this to the 0.33 milestone Jul 19, 2018
@ian-r-rose ian-r-rose mentioned this pull request Jul 20, 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.

None yet

3 participants