-
Notifications
You must be signed in to change notification settings - Fork 134
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
Run all archived messages when embedded widget is uninitialized #1064
Conversation
Does this PR actually work as expected? |
Yep, seems to! I was surprised too 😂 I made a notebook that creates a widget, clears it's representations and adds new ones. It appears correctly when executed either with |
Sorry Hai, I'm not going to have time to put that more comprehensive demonstration together this week. Here's the test I was developing off of, along with the compiled JS and the converted HTML file - let me know what more you need to review this! |
console.log("No state stored; initializing embedded widget for the first time."); | ||
for (const msg of that.model.get("_ngl_msg_archive")) { | ||
console.log("Running msg " + JSON.stringify(msg)); | ||
await that.on_msg(msg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should update await
for other places using on_msg(msg)
too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uhmmm possibly. I didn't make this sort of change to avoid changing behaviour. The other places calling on_msg
are:
l450 (in handleEmbed()
):
// fire any msg with "fire_embed"
that.model.get("_ngl_msg_archive").forEach(function(msg){
if (msg.fire_embed){
that.on_msg(msg);
}
})
This should probably be await
ed - it's in an async function already and its possible that _set_representation_from_repr_dict
could try to set a representation on a component added here? The comment above _set_representation_from_repr_dict
seems to suggest that completing all these messages is a requirement, and completing them out of order could mean assigning the wrong representations. (I will make this change shortly)
l152 (in handleMessage
):
this.model.on("msg:custom", function(msg){
this.on_msg(msg);
}, this);
From what I can google, ipywidgets/backbone don't seem to treat async callbacks any different to regular callbacks, so making the callback function async and adding an await here wouldn't do anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it. thanks.
js/src/widget_ngl.ts
Outdated
@@ -398,6 +399,16 @@ class NGLView extends widgets.DOMWidgetView{ | |||
async handleEmbed(){ | |||
var that = this; | |||
var ngl_msg_archive = that.model.get("_ngl_msg_archive"); | |||
|
|||
if (!that.model.get("_ngl_is_initialized")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we check for ngl_stage_params
instead?
in another line:
var ngl_stage_params = that.model.get('_ngl_full_stage_parameters');
Basically if the ngl is not initialize, check for empty of something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or check for both ngl_full_stage_parameters
and _camera_orientation
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! It looks _ngl_full_stage_parameters
object is already modified in createStage
, so I've just made this a shallow copy so that changes aren't reflected in the shared state until we deliberately sync them back up. I think this is more robust to future changes than checking if _ngl_full_stage_parameters
is the value it is modified to.
yeah, those files are intentional to be included in |
OK I've addressed those points! One thing I've noticed is that |
I will do when doing release. |
The modified JS file introduced in this commit is from: nglviewer/nglview#1064 Once that PR makes it into a release, the logic and JavaScript added in this commit should be replaced with a pin to the appropriate version of NGLView.
Thanks @Yoshanuikabundi very much for your contribution. |
This PR implements the proposal discussed in #1063.
I made two changes that weren't discussed there:
_ngl_is_initialized
is set increateStage
, nothandleEmbed
, to ensure it is always set when the JS runs (but also afterhandleEmbed
)on_msg
async. This is because it is essential that each message completes before the next one is triggered to avoid things like trying to add representations to a missing component. I learnt this the hard way. My understanding is that this won't change behavior - on paths whereon_msg
doesn'tawait
, execution will never be suspended and thus the function will run synchronously; on the other hand, when the function doesawait
it's the last thing it does, so the only difference is that theon_msg
function sticks around on the queue instead of the already async inner function. What it does do is allowon_msg
to be awaited so that messages can be executed and completed in order. This may be of use elsewhere, I'm not sure - I feel like NGLView does sometimes "forget" something I told it.I also added the
jupyterlab~=3.0
pin to the Conda environment as discussed.I have a lot of files that have been generated by NPM (or something), including three that are already checked in - so I've added them in a separate commit in case this is a mistake of some sort.
Hope this is useful! I think from what I've learned I can hack something together for my cookbook even if this doesn't get merged, so thanks for your help!
Closes #1063