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

Rendermime Refactor #2555

Merged
merged 47 commits into from Jul 4, 2017
Merged

Rendermime Refactor #2555

merged 47 commits into from Jul 4, 2017

Conversation

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jun 30, 2017

Refactors rendermime with the following top level ideas:

  • What we called IRenderers are now IRendererFactories, and the returned widgets have a renderModel(model): Promise method. This allows us to re-render by updating the model and calling that method.
  • Handle mime extensions in the application object by creating plugins for each of the mime renderers.
  • The application gains rendermime, docregistry, and commandLinker properties, which were already being considered singletons.
  • Removes the now unused rendermime-extension and docregistry-extension packages as well as the command linker plugin in apputils-extension.
  • Removes the markdownviewer extension in favor of using the MimeRenderer in DocumentRegistry.
  • Fixes a bug where renaming a file did not update the layout restorer, leaving it unrestored.
if (layout.widgets.length === 2) {
// The toolbar is layout.widgets[0]
layout.widgets[1].dispose();
this._mimeModel.data.set(this._mimeType, model.toJSON());
Copy link
Contributor

@sccolbert sccolbert Jun 30, 2017

Choose a reason for hiding this comment

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

Not of a fan of this mutation here, then passing the same mime model back into the renderer. If the renderer is expecting the model to be immutable, it might not re-render.

Copy link
Contributor

@sccolbert sccolbert left a comment

This looks pretty good to me, modulo a few likely oversights during the refactoring.

if (!this.trusted) {
source = this.sanitizer.sanitize(source);
}
Private.appendHtml(this.node, source);
Copy link
Contributor

@sccolbert sccolbert Jun 30, 2017

Choose a reason for hiding this comment

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

need to clear the node content first?

if (!this.trusted) {
content = this.sanitizer.sanitize(content);
}
Private.appendHtml(this.node, content);
Copy link
Contributor

@sccolbert sccolbert Jun 30, 2017

Choose a reason for hiding this comment

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

need to clear the node content first?

* The options used to initialize a document widget factory.
*/
export
interface IWidgetFactoryOptions {
interface IDocumentWidgetFactoryOptions {
Copy link
Contributor

@sccolbert sccolbert Jul 3, 2017

Choose a reason for hiding this comment

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

Why the name "document widget" here?

Copy link
Member Author

@blink1073 blink1073 Jul 3, 2017

Choose a reason for hiding this comment

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

Added this:

   * These are options used to open a rendered view of a given file type
   * using a rendermime factory.

* Add the default renderer factories to a rendermime instance.
*/
export
function addDefaultFactories(rendermime: RenderMime): void {
Copy link
Contributor

@sccolbert sccolbert Jul 3, 2017

Choose a reason for hiding this comment

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

I thought we were not going to do this, and have the app add the default factories?

Copy link
Member Author

@blink1073 blink1073 Jul 3, 2017

Choose a reason for hiding this comment

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

This was needed for outside uses of the rendermime (such as our standalone notebook example).

@sccolbert sccolbert mentioned this pull request Jul 4, 2017
Copy link
Contributor

@ellisonbg ellisonbg left a comment

Looks good, some minor comments/changes inline.

@@ -24,6 +28,11 @@ import {
export
interface IOutputModel extends IRenderMime.IMimeModel {
Copy link
Contributor

@ellisonbg ellisonbg Jul 4, 2017

Choose a reason for hiding this comment

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

@sccolbert will this IOutputModel class get moved to the outputarea in your work?

Copy link
Member Author

@blink1073 blink1073 Jul 4, 2017

Choose a reason for hiding this comment

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

Yes

* Negative indices count from the end, so -1 refers to the last index.
* Use the index of `.order.length` to add to the end of the render precedence list,
* which would make the new renderer the last choice.
* @param rank - The rank of the renderer. Defaults to 100.
Copy link
Contributor

@ellisonbg ellisonbg Jul 4, 2017

Choose a reason for hiding this comment

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

Better note indicating what low/high rank means.

Copy link
Member Author

@blink1073 blink1073 Jul 4, 2017

Choose a reason for hiding this comment

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

Done.

this._renderers[mimeType] = renderer;
ArrayExt.insert(this._order, index, mimeType);
addFactory(factory: IRenderMime.IRendererFactory, mimeType: string, rank?: number): void {
this._addFactory(factory, mimeType, rank);
Copy link
Contributor

@ellisonbg ellisonbg Jul 4, 2017

Choose a reason for hiding this comment

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

Is this indirection needed?

Copy link
Member Author

@blink1073 blink1073 Jul 4, 2017

Choose a reason for hiding this comment

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

Both are needed so we don't call a public method in the constructor.

delete this._renderers[mimeType];
ArrayExt.removeFirstOf(this._order, mimeType);
removeFactory(mimeType: string): void {
this._removeFactory(mimeType);
Copy link
Contributor

@ellisonbg ellisonbg Jul 4, 2017

Choose a reason for hiding this comment

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

Is this indirection needed?

@@ -166,7 +170,7 @@ const RENDER_TIMEOUT = 1000;
* A base cell widget.
*/
export
class Cell extends Widget implements IRenderMime.IReadyWidget {
class Cell extends Widget implements DocumentRegistry.IReadyWidget {
Copy link
Contributor

@ellisonbg ellisonbg Jul 4, 2017

Choose a reason for hiding this comment

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

I am a bit surprised that cell depends on the document registry. At least conceptually, cells shouldnn't know anything about documents. The IReadyWidget is really simply, can we put it somewhere else that cell can depend on?

Copy link
Member Author

@blink1073 blink1073 Jul 4, 2017

Choose a reason for hiding this comment

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

We don't need this to implement the interface explicitly, I'll just remove it.

@@ -451,6 +452,11 @@ class MimeRenderer extends Widget implements IRenderMime.IReadyWidget {
}

/**
* The renderime instance associated with the widget.
Copy link
Contributor

@ellisonbg ellisonbg Jul 4, 2017

Choose a reason for hiding this comment

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

rendermime

Copy link
Member Author

@blink1073 blink1073 Jul 4, 2017

Choose a reason for hiding this comment

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

Done.

}


.jp-MarkdownViewer .jp-Toolbar {
Copy link
Contributor

@ellisonbg ellisonbg Jul 4, 2017

Choose a reason for hiding this comment

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

Does the new widget we are using for rendering MIME based documents have the micro toolbar at the top?

Copy link
Member Author

@blink1073 blink1073 Jul 4, 2017

Choose a reason for hiding this comment

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

Yep.

z-index: 1;
}

.jp-MarkdownViewer .jp-RenderedMarkdown {
Copy link
Contributor

@ellisonbg ellisonbg Jul 4, 2017

Choose a reason for hiding this comment

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

All of this CSS is missing now. We probably have the CSS classes needed to keep it, but we will need to add it as something like jp-Document.jp-MimeRenderer jp.RenderedMarkdown.


.jp-MarkdownViewer {
border-top: var(--jp-border-width) solid var(--jp-border-color2);
outline: none;
Copy link
Contributor

@ellisonbg ellisonbg Jul 4, 2017

Choose a reason for hiding this comment

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

We will need to make sure the overflow is working properly so that it wraps/scrolls as expected.

Copy link
Contributor

@ellisonbg ellisonbg Jul 4, 2017

Choose a reason for hiding this comment

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

In this branch, the markdown won't scroll at all.

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Jul 4, 2017

I have tried this locally and things seem to be mostly working. I have added a few more comments based on those tests. Given the size of this, we should probably merge soon and continue to iterate.

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Jul 4, 2017

Looks like real test failures

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Jul 4, 2017

@blink1073 I can clean up the markdown preview CSS later so we can merge this once the tests are passing.

@sccolbert
Copy link
Contributor

@sccolbert sccolbert commented Jul 4, 2017

Merging and iterating.

@sccolbert sccolbert merged commit 43b8c77 into jupyterlab:master Jul 4, 2017
2 checks passed
@ellisonbg ellisonbg mentioned this pull request Jul 4, 2017
gnestor added a commit to gnestor/jupyterlab that referenced this issue Jul 5, 2017
gnestor added a commit to jupyterlab/jupyter-renderers that referenced this issue Jul 5, 2017
@blink1073 blink1073 mentioned this pull request Jul 7, 2017
@blink1073 blink1073 deleted the rendermime-refactor branch Jul 10, 2017
@lock lock bot locked as resolved and limited conversation to collaborators Aug 10, 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