FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix Issue #6530 by SylvainCorlay · Pull Request #6532 · ipython/ipython · GitHub

Fix Issue #6530 - #6532

Merged
takluyver merged 2 commits into
ipython:masterfrom
SylvainCorlay:barewidgetmodels
Sep 25, 2014
Merged

Fix Issue #6530#6532
takluyver merged 2 commits into
ipython:masterfrom
SylvainCorlay:barewidgetmodels

Conversation

Copy link
Copy Markdown
Member

@jdfreder A quick fix to #6530. Widget and DOMWidget' s view name is set to None. Widgets without a view are not displayed anymore, like what Thomas suggested earlier

Copy link
Copy Markdown
Member

Do these views actually do anything? I wonder if this is better fixed on the Python side - i.e. don't attempt to display a widget unless there's something it can display.

Copy link
Copy Markdown
Member Author

I cannot think of a use case where you would really want to display a bare empty view. This is just to avoid an error on the JavaScript when display is called implicitly.
It might be simpler than an ad hoc solution on the python side to avoid display to do anything for widgets whose view names is one of those.

Copy link
Copy Markdown
Member

I guess my fix more specifically would be: set view_name to an empty string instead of something like 'WidgetView' (which isn't really a view, just a base class). And then in Widget._ipython_display_(), do nothing if view_name is empty.

Copy link
Copy Markdown
Member Author

Ok, it makes sense. ( Although then calling Widget() still raises an error on the JavaScript side :) )

Copy link
Copy Markdown
Member Author

Actually we could do both, I find it conceptually annoying that widgets with no views are "displayed".

Copy link
Copy Markdown
Member

What error would Widget() raise on the JS side? Can't it instantiate the model without a view name?

Copy link
Copy Markdown
Member Author

Oh you also want Widget to have no view? Not just custom stuff like Link. Then ok.

Copy link
Copy Markdown
Member

Yeah, I'm thinking that view_name should only be set if there is a real, displayable view for the widget.

SylvainCorlay changed the title Register bare views Fix #6530 Sep 25, 2014
SylvainCorlay changed the title Fix #6530 Fix Issue #6530 Sep 25, 2014

Copy link
Copy Markdown
Member Author

Updated as suggested by Thomas.

SylvainCorlay mentioned this pull request Sep 25, 2014

Copy link
Copy Markdown
Member

+1 to having a null view that actually is special and does nothing (rather than just being the default base class).

Copy link
Copy Markdown
Member

Thinking about this some more, I wonder if instantiating Widget() should actually throw an error on the Python side - Widget is effectively an Abstract Base Class, i.e. it should be used by subclassing it.

Copy link
Copy Markdown
Contributor

+1 to having a null view that actually is special and does nothing (rather than just being the default base class).

I'm +1 for this too, only if we decide not to persist invisible models.

takluyver added a commit that referenced this pull request Sep 25, 2014
takluyver merged commit 398a03e into ipython:master Sep 25, 2014
SylvainCorlay deleted the barewidgetmodels branch September 25, 2014 19:14

Copy link
Copy Markdown
Member Author

Thanks!

minrk modified the milestone: 3.0 Oct 3, 2014
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants


Back | FazBrowse Home | New Git URL