fix: detach custom camera controls correctly

Only detach once. Unit test. Return camera controls in promise when detaching.
mapillary · Nov 8, 2021 · 208b5ae · 208b5ae
1 parent e07c314
commit 208b5ae
Show file tree

Hide file tree

Showing 3 changed files with 76 additions and 16 deletions.
diff --git a/src/viewer/CustomCameraControls.ts b/src/viewer/CustomCameraControls.ts
@@ -11,6 +11,7 @@ import {
     skip,
     startWith,
     switchMap,
+    take,
     withLatestFrom,
 } from "rxjs/operators";
 
@@ -157,22 +158,31 @@ export class CustomCameraControls {
         this.detach(viewer);
     }
 
-    public detach(viewer: IViewer): void {
-        if (!this._controls) {
-            return;
-        }
+    public detach(viewer: IViewer): Promise<ICustomCameraControls> {
+        const controls = this._controls;
+        this._controls = null;
 
         this._subscriptions.unsubscribe();
 
-        this._navigator.stateService.state$
-            .subscribe(state => {
-                if (state === State.Custom) {
-                    this._controls.onDeactivate(viewer);
-                }
-
-                this._controls.onDetach(viewer);
-                this._controls = null;
-            });
+        return new Promise(resolve => {
+            this._navigator.stateService.state$
+                .pipe(take(1))
+                .subscribe(state => {
+                    if (!controls) {
+                        resolve(null);
+                        return;
+                    }
+
+                    if (state === State.Custom) {
+                        controls.onDeactivate(viewer);
+                    }
+
+                    controls.onDetach(viewer);
+                    resolve(controls);
+                });
+        });
+
+
     }
 
     private _updateProjectionMatrix(projectionMatrix: number[]): void {

diff --git a/src/viewer/Viewer.ts b/src/viewer/Viewer.ts
@@ -319,8 +319,8 @@ export class Viewer extends EventEmitter implements IViewer {
      * be detached before attaching another custom camera
      * control instance.
      */
-    public detachCustomCameraControls(): void {
-        this._customCameraControls.detach(this);
+    public detachCustomCameraControls(): Promise<ICustomCameraControls> {
+        return this._customCameraControls.detach(this);
     }
 
     public fire(

diff --git a/test/viewer/CustomCameraControls.test.ts b/test/viewer/CustomCameraControls.test.ts
@@ -24,7 +24,7 @@ global.WebGL2RenderingContext = <any>jest.fn();
 type WebGLMocks = {
     context: WebGL2RenderingContext;
     renderer: WebGLRenderer;
-}
+};
 
 function createWebGLMocks(): WebGLMocks {
     const renderer = <WebGLRenderer><unknown>new RendererMock();
@@ -684,6 +684,56 @@ describe("CustomRenderer.detach", () => {
             .next(State.Earth);
     });
 
+    test("should only invoke onDetach once", () => {
+        const navigator = new NavigatorMockCreator().create();
+        const container = new ContainerMockCreator().create();
+        spyOn(Navigator, "Navigator").and.returnValue(navigator);
+        spyOn(Container, "Container").and.returnValue(container);
+
+        const controls = new CustomCameraControls(
+            container,
+            navigator);
+
+        const viewer = <any>{};
+
+        let detachCount = 0;
+        controls.attach(
+            {
+                onActivate: () => {
+                    fail();
+                },
+                onAnimationFrame: () => {
+                    fail();
+                },
+                onAttach: () => {
+                    fail();
+                },
+                onDeactivate: (v) => {
+                    fail();
+                },
+                onDetach: (v) => {
+                    expect(v).toBe(viewer);
+                    detachCount++;
+                },
+                onReference: () => {
+                    fail();
+                },
+                onResize: () => {
+                    fail();
+                },
+            },
+            viewer);
+
+        controls.detach(viewer);
+
+        (<Subject<State>>navigator.stateService.state$)
+            .next(State.Earth);
+        (<Subject<State>>navigator.stateService.state$)
+            .next(State.Traversing);
+
+        expect(detachCount).toBe(1);
+    });
+
     test("should invoke onDeactive when detatching if in custom state", done => {
         const navigator = new NavigatorMockCreator().create();
         const container = new ContainerMockCreator().create();