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.

Sign up for GitHub

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

Jump to bottom

Feature: save additional mime bundles in notebook #3107

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Feature: save additional mime bundles in notebook #3107

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a7b69ef
feat: save additional mime bundles in notebook
maartenbreddels Feb 9, 2021
883bbd3
use async, and do not return widget mime type
maartenbreddels Feb 9, 2021
4f3f57b
example for HTML and Image
maartenbreddels Feb 9, 2021
e1c5ab1
fix: skip non-existing models
maartenbreddels Feb 9, 2021
ffd91d2
Do not fail if there is no output_area
martinRenou Sep 13, 2021
84a1574
Allow for overwriting the mimebundle
martinRenou Sep 14, 2021
f4c89fa
Lint
martinRenou Sep 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions packages/base/src/widget.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,21 @@ export class WidgetModel extends Backbone.Model {
};
}

/**
* Generate extra mime bundle for this widget
*/
async generateMimeBundle() {
return {};
}

/**
* Whether the `generateMimeBundle` output should
* overwrite the mimeBundle or extend it
*/
shouldOverwriteMimeBundle(): boolean {
return false;
}

Comment on lines +94 to +105
Copy link
Member Author

Choose a reason for hiding this comment

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

What about generateMimeBundle making the default mimebundle (with the widget in it).
subclases can then do:

return {'text/html': view.el.outerHTM, ... await super.generateMimeBundle()}
// or
return {'text/html': view.el.outerHTM}

depending on if they want to overwrite it. Does that make sense?

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense!

/**
* Test to see if the model has been synced with the server.
*
Expand Down
9 changes: 9 additions & 0 deletions packages/controls/src/widget_image.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,15 @@ export class ImageModel extends CoreDOMWidgetModel {
},
},
};

async generateMimeBundle() {
const view: ImageView = await this.widget_manager.create_view<ImageView>(
this
);
return Promise.resolve({
'text/html': view.el.outerHTML,
});
}
}

export class ImageView extends DOMWidgetView {
Expand Down
5 changes: 5 additions & 0 deletions packages/controls/src/widget_string.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,11 @@ export class HTMLModel extends StringModel {
_model_name: 'HTMLModel',
};
}
generateMimeBundle() {
return Promise.resolve({
'text/html': this.get('value'),
});
}
}

export class HTMLView extends StringView {
Expand Down
43 changes: 43 additions & 0 deletions widgetsnbextension/src/manager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,49 @@ export class WidgetManager extends ManagerBase {
*/
_init_actions() {
var notifier = Jupyter.notification_area.widget('widgets');
this.notebook.events.on('before_save.Notebook', async () => {
var cells = Jupyter.notebook.get_cells();
// notebook.js save_notebook doesn't want for this promise, we are simply lucky when this
// finishes before saving.
Copy link
Member

Choose a reason for hiding this comment

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

This seems very weak :S

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I think this means it will be a lab feature only

Copy link
Member

Choose a reason for hiding this comment

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

What about not making it a Promise?

Copy link
Member Author

Choose a reason for hiding this comment

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

we to await the model, and making `generateMimeBundle async also makes sense, since it will involve async cases in bqplot at least.

await Promise.all(
cells.map(async (cell) => {
if (!cell.output_area) {
return;
}

var widget_output = cell.output_area.outputs.find((output) => {
return (
(output.data && output.data[MIME_TYPE]) ||
(output.metadata && output.metadata[MIME_TYPE])
);
});
if (widget_output) {
var model_id = widget_output.data[MIME_TYPE]
? widget_output.data[MIME_TYPE].model_id
: widget_output.metadata[MIME_TYPE].model_id;
var model = await this.get_model(model_id);
if (model) {
var bundle = await model.generateMimeBundle();

if (
widget_output.data[MIME_TYPE] &&
model.shouldOverwriteMimeBundle()
) {
widget_output.metadata[MIME_TYPE] =
widget_output.data[MIME_TYPE];
widget_output.metadata['text/plain'] =
widget_output.data['text/plain'];

delete widget_output.data[MIME_TYPE];
delete widget_output.data['text/plain'];
}

_.extend(widget_output.data, bundle);
}
}
})
);
});
this.saveWidgetsAction = {
handler: function () {
this.get_state({
Expand Down