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

Compatibility with ipywidgets v8 #3930

Merged
merged 8 commits into from
Jan 10, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ This project adheres to [Semantic Versioning](http://semver.org/).
## UNRELEASED

### Updated
- Support for ipywidgets 8 [#3930](https://github.com/plotly/plotly.py/pull/3930)
- Updated Plotly.js to from version 2.16.1 to version 2.17.0. See the [plotly.js CHANGELOG](https://github.com/plotly/plotly.js/blob/master/CHANGELOG.md#2170----2022-12-22) for more information. Notable changes include:
- Add `shift` and `autoshift` to cartesian y axes to help avoid overlapping of multiple axes [[#6334](https://github.com/plotly/plotly.js/pull/6334)],
with thanks to [Gamma Technologies](https://www.gtisoft.com) for sponsoring the related development!
Expand Down
182 changes: 143 additions & 39 deletions packages/javascript/jupyterlab-plotly/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion packages/javascript/jupyterlab-plotly/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@
"webpack-cli": "^4.0.0"
},
"dependencies": {
"@jupyter-widgets/base": "^2.0.0 || ^3.0.0 || ^4.0.0",
"@jupyter-widgets/base": ">=2.0.0 <7.0.0",
"@jupyterlab/rendermime-interfaces": "^1.3.0 || ^2.0.0 || ^3.0.0",
"@lumino/messaging": "^1.2.3",
"@lumino/widgets": "^1.8.1",
Expand Down
28 changes: 23 additions & 5 deletions packages/javascript/jupyterlab-plotly/src/Figure.ts
Original file line number Diff line number Diff line change
Expand Up @@ -716,7 +716,10 @@ export class FigureModel extends DOMWidgetModel {

static serializers: ISerializers = {
...DOMWidgetModel.serializers,
_data: { deserialize: py2js_deserializer, serialize: js2py_serializer },
_data: {
deserialize: py2js_deserializer,
serialize: js2py_serializer,
},
_layout: {
deserialize: py2js_deserializer,
serialize: js2py_serializer,
Expand Down Expand Up @@ -807,7 +810,7 @@ export class FigureView extends DOMWidgetView {
resizeEventListener: () => void;

/**
* The perform_render method is called by processPhosphorMessage
* The perform_render method is called by processLuminoMessage
* after the widget's DOM element has been attached to the notebook
* output cell. This happens after the initialize of the
* FigureModel, and it won't happen at all if the Python FigureWidget
Expand Down Expand Up @@ -901,10 +904,10 @@ export class FigureView extends DOMWidgetView {
}

/**
* Respond to phosphorjs events
* Respond to Lumino events
*/
processPhosphorMessage(msg: any) {
super.processPhosphorMessage.apply(this, arguments);
_processLuminoMessage(msg: any, _super: any) {
_super.apply(this, arguments);
var that = this;
switch (msg.type) {
case "before-attach":
Expand Down Expand Up @@ -939,6 +942,21 @@ export class FigureView extends DOMWidgetView {
}
}

processPhosphorMessage(msg: any) {
this._processLuminoMessage(msg, super["processPhosphorMessage"]);

var that = this;
if (msg.type === "before-attach") {
window.addEventListener("resize", function () {
that.autosizeFigure();
});
}
Comment on lines +948 to +953
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
var that = this;
if (msg.type === "before-attach") {
window.addEventListener("resize", function () {
that.autosizeFigure();
});
}
var that = this;
if (msg.type === "before-attach") {
window.addEventListener("resize", function () {
that.autosizeFigure();
});
}

I guess we can remove this?

Copy link
Contributor Author

@abdelq abdelq Jan 9, 2023

Choose a reason for hiding this comment

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

It's been awhile so unsure, but if we want compatibility, probably not? This is not done in processLuminoMessage, just in processPhosphorMessage.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see the same code in _processLuminoMessage right? (Modulo the code changes due to #3805)

Copy link
Contributor Author

@abdelq abdelq Jan 9, 2023

Choose a reason for hiding this comment

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

Ah, this was from @nicolaskruchten's merge. Should perhaps be removed from _processLuminoMessage... I remember there were changes to resizing events and whatnot in newer ipywidgets.

Copy link

@Alexboiboi Alexboiboi Jan 10, 2023

Choose a reason for hiding this comment

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

Sorry to ask, but is this also related to

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was referring to jupyter-widgets/ipywidgets#3124, but I think with this current PR shrinking actually works fine (worth double checking).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, let's please remove those lines, as they are indeed already executed in the this._processLuminoMessage call immediately above

Copy link
Contributor

Choose a reason for hiding this comment

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

Otherwise we get two event listeners, right?

Choose a reason for hiding this comment

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

The shrinking works fine for me too ;)

But inside an ipywidgets container it stilll does not really resizes correctly. Maybe what @maartenbreddels suggested, applying the [bqplot/bqplot#1531] strategy would solve the issues.

}

processLuminoMessage(msg: any) {
this._processLuminoMessage(msg, super["processLuminoMessage"]);
}

autosizeFigure() {
var that = this;
var layout = that.model.get("_layout");
Expand Down
10 changes: 1 addition & 9 deletions packages/python/plotly/plotly/basedatatypes.py
Original file line number Diff line number Diff line change
Expand Up @@ -819,18 +819,10 @@ def _repr_mimebundle_(self, include=None, exclude=None, validate=True, **kwargs)

renderer_str = pio.renderers.default
renderers = pio._renderers.renderers
renderer_names = renderers._validate_coerce_renderers(renderer_str)
abdelq marked this conversation as resolved.
Show resolved Hide resolved
renderers_list = [renderers[name] for name in renderer_names]
from plotly.io._utils import validate_coerce_fig_to_dict
from plotly.io._renderers import MimetypeRenderer

fig_dict = validate_coerce_fig_to_dict(self, validate)
# Mimetype renderers
bundle = {}
for renderer in renderers_list:
if isinstance(renderer, MimetypeRenderer):
bundle.update(renderer.to_mimebundle(fig_dict))
return bundle
return renderers._build_mime_bundle(fig_dict, renderer_str, **kwargs)

def _ipython_display_(self):
"""
Expand Down
21 changes: 19 additions & 2 deletions packages/python/plotly/plotly/basewidget.py
Original file line number Diff line number Diff line change
Expand Up @@ -733,12 +733,29 @@ def _handler_js2py_pointsCallback(self, change):

# Display
# -------
def _repr_html_(self):
"""
Customize html representation
"""
raise NotImplementedError # Prefer _repr_mimebundle_

def _repr_mimebundle_(self, include=None, exclude=None, validate=True, **kwargs):
"""
Return mimebundle corresponding to default renderer.
"""
return {
"application/vnd.jupyter.widget-view+json": {
"version_major": 2,
"version_minor": 0,
"model_id": self._model_id,
},
}

def _ipython_display_(self):
"""
Handle rich display of figures in ipython contexts
"""
# Override BaseFigure's display to make sure we display the widget version
widgets.DOMWidget._ipython_display_(self)
raise NotImplementedError # Prefer _repr_mimebundle_
nicolaskruchten marked this conversation as resolved.
Show resolved Hide resolved

# Callbacks
# ---------
Expand Down
9 changes: 6 additions & 3 deletions packages/python/plotly/plotly/io/_base_renderers.py
Original file line number Diff line number Diff line change
Expand Up @@ -609,11 +609,14 @@ def to_mimebundle(self, fig_dict):

def build_filename(self):
ip = IPython.get_ipython() if IPython else None
cell_number = list(ip.history_manager.get_tail(1))[0][1] + 1 if ip else 0
filename = "{dirname}/figure_{cell_number}.html".format(
if ip:
cell_number = next(ip.history_manager.get_tail(1), (0, -1, ""))[1] + 1
else:
cell_number = 0

return "{dirname}/figure_{cell_number}.html".format(
dirname=self.html_directory, cell_number=cell_number
)
return filename

def build_url(self, filename):
return filename
Expand Down
Loading