Smithsonian / dpo-voyager

DPO Voyager - 3D Explorer and Tool Suite
Apache License 2.0
164 stars 28 forks source link

Can't change voyager-story's mode after initialization #246

Closed sdumetz closed 5 months ago

sdumetz commented 6 months ago

If I try to call taskProvider.ins.mode.setValue(3) - or whatever number, after voyager story has initialized, everything breaks.

The funny thing is: it breaks even before CVTaskManager.update() gets called.

I looked around a bit and it might not be easy to make this reentrant. Maybe the easiest fix would be to prevent modification like this:

--- a/source/client/components/CVTaskProvider.ts
+++ b/source/client/components/CVTaskProvider.ts
@@ -72,10 +72,12 @@ export default class CVTaskProvider extends CComponentProvider<CVTask>
         const languageManager = this.languageManager;

         if (ins.mode.changed) {
-            this.scopedComponents.forEach(task => task.dispose());
-
-            const taskSet = taskSets[ins.mode.getValidatedValue()];
-            taskSet.forEach(taskType => this.createComponent(taskType));
+            if(this.scopedComponents.length) {
+                console.error("Can't change CVTaskProvider mode after initialization");
+            }else{
+                const taskSet = taskSets[ins.mode.getValidatedValue()];
+                taskSet.forEach(taskType => this.createComponent(taskType));
+            }
         }

Or there is an easy(ish) fix that I missed? I'm not yet familiar enough with this part of the code.

gjcope commented 6 months ago

I will take a look. It looks like it was intended to be reentrant (I did not write this code) but I also don't have a good use case for updating mode at runtime so your proposal may be fine.

gjcope commented 5 months ago

@sdumetz if you make the small bugfix to CaptionView here (https://github.com/Smithsonian/dpo-voyager/commit/2a57d75c1df2dee211460c14ac66a1c0df26ffcd)

and remove the 2 lines here: https://github.com/Smithsonian/dpo-voyager/blob/82cb081f243311ef5a7d2a4d7121dac8ef9c5e85/source/client/ui/explorer/MainView.ts#L167

it should work.

The issue is that I think this will break the app memory being released in the case where someone destroys the component element in the page. I.e., recreating to change scene rather than using the app reload functionality.

gjcope commented 5 months ago

Fixed in v0.37.0