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

Allow model id to be set externally on creation of the widget. #6194

Merged
merged 5 commits into from
Aug 19, 2014

Conversation

SylvainCorlay
Copy link
Member

With this change, the model_id of a widget to be set externally when creating the widget. (It is especially useful in cases where you want the model_id to correspond to some other object's uuid that you don't control, and you don't want to rely on a lookup table. )

Another feature is that if a widget is closed and reopened, the new model_id/comm_id will be the same as the previous one.

@SylvainCorlay SylvainCorlay changed the title Immediate widget comm Allow model id to be set externally on creation of the widget. Jul 23, 2014
# Create a comm, use self._model_id as comm_id
# if the comm is closed and re-opened, the same id will be used.
if self._model_id is not None:
self.comm = Comm(target_name=self._model_name, comm_id=self._model_id)
Copy link
Member Author

Choose a reason for hiding this comment

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

Should I check if there is already a comm with that id and raise an exception if it is the case? Or should I consider that people are responsible when externally setting model ids?

@jdfreder
Copy link
Member

@SylvainCorlay what can you do in this PR that you can't do in @jasongrout 's PR #6151 ? He mentions you can create and pass in comms in the widget constructor. Doesn't that allow you to set the comm/model id too?

@jasongrout 's comment:

Since comm is now a trait of the widget, and the open call only does anything if comm is None, then I imagine that you can just pass the existing comm into the widget (IntSlider(comm=EXISTING_COMM)), and things will just work. It won't try to create a new comm, but will use the existing one.

@jasongrout
Copy link
Member

@SylvainCorlay what can you do in this PR that you can't do in @jasongrout 's PR #6151 ? He mentions you can create and pass in comms in the widget constructor. Doesn't that allow you to set the comm/model id too?

That's a good point. However, it's sort of breaking the widget abstraction by requiring the user to know about and initialize the comm and pass it in. I imagine it's much nicer to just set a model id, if that's what you really want.

@SylvainCorlay
Copy link
Member Author

It is convenient to pass a model_id explicitly without having to create the comm externally with the right target name and pass it to the constructor.

@SylvainCorlay
Copy link
Member Author

I cleaned up the branch a bit.

@jdfreder
Copy link
Member

jdfreder commented Aug 3, 2014

@minrk could you take a quick look at the comm.py changes here?

@jdfreder
Copy link
Member

jdfreder commented Aug 5, 2014

@SylvainCorlay I think @minrk 's on vacation, but this also needs a rebase. You may want to wait till he gets a chance to review the comm stuff...

jasongrout and others added 5 commits August 11, 2014 18:30
This will immediately create a model on the javascript side when a widget is created.  This means that, for example, a widget that only interacts with its model can work without "displaying" it.
Instead of automagically instantiating a comm when it is accessed, require a call to open().  This makes the comm attribute much less magical, and hopefully more understandable.
@SylvainCorlay
Copy link
Member Author

Rebased

jdfreder added a commit that referenced this pull request Aug 19, 2014
Allow model id to be set externally on creation of the widget.
@jdfreder jdfreder merged commit 7acd657 into ipython:master Aug 19, 2014
@jdfreder
Copy link
Member

Thanks @SylvainCorlay and @jasongrout

@SylvainCorlay
Copy link
Member Author

Thanks @jdfreder .

@SylvainCorlay SylvainCorlay deleted the immediate-widget-comm branch August 20, 2014 03:55
@minrk minrk modified the milestone: 3.0 Oct 3, 2014
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
Allow model id to be set externally on creation of the widget.
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.

None yet

4 participants