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 2 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
17 changes: 14 additions & 3 deletions js/src/widget_ngl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,7 @@ class NGLView extends widgets.DOMWidgetView{
this.set_camera_orientation(that.model.get("_camera_orientation"));
}
}
that.model.set("_ngl_is_initialized", true)
Yoshanuikabundi marked this conversation as resolved.
Show resolved Hide resolved
}

isEmbeded(){
Expand Down Expand Up @@ -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")) {
Copy link
Collaborator

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

image

Copy link
Collaborator

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?

Copy link
Contributor Author

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.

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);
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 ngl_stage_params = that.model.get('_ngl_full_stage_parameters');
var loadfile_list = [];

Expand Down Expand Up @@ -1068,7 +1079,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 +1118,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