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

Add potential fix for increasing map size bug #3699

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

Add potential fix for increasing map size bug #3699

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7143969
Add potential fix for increasing map size bug
filipre Feb 9, 2023
b8a89ab
Make arguments explicit and add documentation
filipre Feb 9, 2023
10aae64
Add mode for manager state
filipre Feb 14, 2023
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
60 changes: 54 additions & 6 deletions python/ipywidgets/ipywidgets/embed.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,12 @@

import json
import re
from .widgets import Widget, DOMWidget, widget as widget_module
from .widgets.widget_link import Link
from .widgets.docutils import doc_subst

from ._version import __html_manager_version__
from .widgets import DOMWidget, Widget
from .widgets import widget as widget_module
from .widgets.docutils import doc_subst
from .widgets.widget_link import Link

snippet_template = """
{load}
Expand Down Expand Up @@ -284,7 +286,18 @@ def embed_snippet(views,


@doc_subst(_doc_snippets)
def embed_minimal_html(fp, views, title='IPyWidget export', template=None, **kwargs):
def embed_minimal_html(
fp,
views,
title='IPyWidget export',
template=None,
drop_defaults=True,
state=None,
indent=2,
embed_url=None,
requirejs=True,
cors=True,
):
"""Write a minimal HTML file with widget views embedded.

Parameters
Expand All @@ -297,9 +310,44 @@ def embed_minimal_html(fp, views, title='IPyWidget export', template=None, **kwa
This should be a Python string with placeholders
`{{title}}` and `{{snippet}}`. The `{{snippet}}` placeholder
will be replaced by all the widgets.
{embed_kwargs}
drop_defaults: boolean
Whether to drop default values from the widget states.
state: dict, string or None (default)
The state to include. When set to None or "dependent", the state of
all passed views is included. When set to "complete", the complete
state of the widget is included. Otherwise it uses the passed state
directly. This allows for end users to include a smaller state, under
the responsibility that this state is sufficient to reconstruct the
embedded views.
indent: integer, string or None
The indent to use for the JSON state dump. See `json.dumps` for
full description.
embed_url: string or None
Allows for overriding the URL used to fetch the widget manager
for the embedded code. This defaults (None) to a `jsDelivr` CDN url.
requirejs: boolean (True)
Enables the requirejs-based embedding, which allows for custom widgets.
If True, the embed_url should point to an AMD module.
cors: boolean (True)
If True avoids sending user credentials while requesting the scripts.
When opening an HTML file from disk, some browsers may refuse to load
the scripts.
"""
snippet = embed_snippet(views, **kwargs)

if state is None or state == "dependent":
Copy link
Member

Choose a reason for hiding this comment

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

Changing the default (None case) is the most controversial here, as it is strictly speaking backwards incompatible. There might be a case for calling it a workaround for the poor GC of ipywidgets, but making this change should at least have some more eyes on it before merging.

Copy link
Author

Choose a reason for hiding this comment

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

I understand your argument and it's difficult for me to estimate how much of an impact this change would have to other projects. Coming from ipyleaflet, it just looks like a bug to me and then I'd argue, that backwards compatibility should not rely on bugs. However, if other people really need the whole state and really have that implementation assumption as well, then I agree, we should stay backwards compatible as well. What do you think?

Your solution definelty will work in the end but as a user, it's not so clear to me when I'd use the dependent state and when the complete.

Copy link
Member

Choose a reason for hiding this comment

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

I kind of agree it is a bug, we didn't implement this feature for nothing. I think this was the intended behavior, but yeah, fixing a bug can be considered a breaking change :)

Copy link
Member

Choose a reason for hiding this comment

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

See original discussion here: #1387 (comment)

state = dependency_state(views, drop_defaults=drop_defaults)
elif state == "complete":
state = Widget.get_manager_state(drop_defaults=drop_defaults, widgets=None)['state']
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to move this logic into embed_data instead? Then we get support for these enums in more functions (and we also get to reuse the docstrings 😄).


snippet = embed_snippet(
views,
drop_defaults=drop_defaults,
state=state,
indent=indent,
embed_url=embed_url,
requirejs=requirejs,
cors=cors
)

values = {
'title': title,
Expand Down