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

Run all archived messages when embedded widget is uninitialized #1064

Merged
merged 5 commits into from
Jun 20, 2023
Merged

Run all archived messages when embedded widget is uninitialized #1064

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
42b70bf
Run all archived messages when embedded widget is unitialized
Yoshanuikabundi Jun 14, 2023
2434ffa
Include generated static files
Yoshanuikabundi Jun 14, 2023
7fa3335
Add await elsewhere in handleEmbed
Yoshanuikabundi Jun 19, 2023
349a783
Remove _ngl_is_initialized trait and rely on existing traits
Yoshanuikabundi Jun 19, 2023
a7a52d9
Remove set(_ngl_is_initialized , true)
Yoshanuikabundi Jun 19, 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
2 changes: 1 addition & 1 deletion environment.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ dependencies:
- python>=3.6
- pip
- pytest
- jupyterlab
- jupyterlab~=3.0
- nodejs
- yarn
- ipywidgets
Expand Down
32 changes: 24 additions & 8 deletions js/src/widget_ngl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,10 @@ class NGLView extends widgets.DOMWidgetView{

createStage(){
// init NGL stage
var stage_params = this.model.get("_ngl_full_stage_parameters");
var stage_params = {
// Shallow copy so that _ngl_full_stage_parameters is not updated yet
...this.model.get("_ngl_full_stage_parameters")
hainm marked this conversation as resolved.
Show resolved Hide resolved
};
if (!("backgroundColor" in stage_params)){
stage_params["backgroundColor"] = "white"
}
Expand Down Expand Up @@ -399,6 +402,19 @@ class NGLView extends widgets.DOMWidgetView{
var that = this;
var ngl_msg_archive = that.model.get("_ngl_msg_archive");
var ngl_stage_params = that.model.get('_ngl_full_stage_parameters');
const camera_orientation = that.model.get("_camera_orientation");

if (
Object.keys(ngl_stage_params).length === 0
&& camera_orientation.length === 0
) {
console.log("No state stored; initializing embedded widget for the first time.");
for (const msg of ngl_msg_archive) {
await that.on_msg(msg);
Copy link
Collaborator

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?

Copy link
Contributor Author

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 awaited - 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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

got it. thanks.

}
return
}

var loadfile_list = [];

_.each(ngl_msg_archive, function(msg: any){
Expand All @@ -415,7 +431,7 @@ class NGLView extends widgets.DOMWidgetView{

var compList = await Promise.all(loadfile_list)
that.stage.setParameters(ngl_stage_params);
that.set_camera_orientation(that.model.get("_camera_orientation"));
that.set_camera_orientation(camera_orientation);
that.touch();

// Outside notebook
Expand All @@ -434,11 +450,11 @@ class NGLView extends widgets.DOMWidgetView{


// fire any msg with "fire_embed"
that.model.get("_ngl_msg_archive").forEach(function(msg){
for (const msg of that.model.get("_ngl_msg_archive")) {
if (msg.fire_embed){
that.on_msg(msg);
await that.on_msg(msg);
}
})
}

// Must call _set_representation_from_repr_dict after "fire_embed"
// User might add Shape (buffer component) to the view and the buffer component
Expand Down Expand Up @@ -1068,7 +1084,7 @@ class NGLView extends widgets.DOMWidgetView{
return label
}

on_msg(msg) {
async on_msg(msg) {
// TODO: re-organize
if (('ngl_view_id' in msg) && (msg.ngl_view_id !== this.ngl_view_id)){
return
Expand Down Expand Up @@ -1107,9 +1123,9 @@ class NGLView extends widgets.DOMWidgetView{
// are serialized separately, also it unwantedly sets the orientation
msg.kwargs.defaultRepresentation = false
}
this._handleStageLoadFile(msg);
await this._handleStageLoadFile(msg);
} else {
stage_func.apply(stage, new_args);
stage_func.apply(stage, new_args);
}
break;
case 'Viewer':
Expand Down
Loading