Fix: Unregister drawing on mode cancel (#288)

- Unregister drawing on mode cancel - Save drawing on done w/ undoStack isn't empty
box · Nov 15, 2018 · 12d5f6d · 12d5f6d
1 parent e972a72
commit 12d5f6d
Show file tree

Hide file tree

Showing 5 changed files with 71 additions and 13 deletions.
diff --git a/src/controllers/AnnotationModeController.js b/src/controllers/AnnotationModeController.js
@@ -599,6 +599,17 @@ class AnnotationModeController extends EventEmitter {
         Object.keys(this.annotations).forEach((pageNum) => this.renderPage(pageNum));
     }
 
+    /**
+     * Unregisters and destroys the specified thread
+     *
+     * @param {AnnotationThread} thread thread to destroy
+     * @return {void}
+     */
+    destroyThread(thread: AnnotationThread) {
+        this.unregisterThread(thread);
+        thread.destroy();
+    }
+
     /**
      * Renders annotations from memory for a specified page.
      *
@@ -614,8 +625,7 @@ class AnnotationModeController extends EventEmitter {
         pageThreads.forEach((thread, index) => {
             // Destroy any pending threads that may exist on re-render
             if (thread.state === STATES.pending || thread.id === this.pendingThreadID) {
-                this.unregisterThread(thread);
-                thread.destroy();
+                this.destroyThread(thread);
                 return;
             }
 
@@ -645,10 +655,9 @@ class AnnotationModeController extends EventEmitter {
             const pageThreads = this.annotations[pageNum].all() || [];
             pageThreads.forEach((thread) => {
                 if (thread.state === STATES.pending || thread.id === this.pendingThreadID) {
-                    this.unregisterThread(thread);
+                    this.destroyThread(thread);
                     hadPendingThreads = true;
                     this.pendingThreadID = null;
-                    thread.destroy();
                 }
             });
         });

diff --git a/src/controllers/DrawingModeController.js b/src/controllers/DrawingModeController.js
@@ -93,7 +93,7 @@ class DrawingModeController extends AnnotationModeController {
      */
     cancelDrawing(): void {
         if (this.currentThread) {
-            this.currentThread.destroy();
+            this.destroyThread(this.currentThread);
         }
 
         this.exit();
@@ -108,7 +108,8 @@ class DrawingModeController extends AnnotationModeController {
         if (
             this.currentThread &&
             this.currentThread.state === STATES.pending &&
-            this.currentThread.pathContainer.length > 0
+            this.currentThread.pathContainer &&
+            this.currentThread.pathContainer.undoStack.length > 0
         ) {
             this.currentThread.save(TYPES.draw);
         }
@@ -305,8 +306,7 @@ class DrawingModeController extends AnnotationModeController {
                     this.unbindListeners();
                     this.bindListeners();
                 } else {
-                    this.unregisterThread(thread);
-                    thread.destroy();
+                    this.destroyThread(thread);
                     this.renderPage(thread.location.page);
                 }
 

diff --git a/src/controllers/PointModeController.js b/src/controllers/PointModeController.js
@@ -65,8 +65,7 @@ class PointModeController extends AnnotationModeController {
 
         const thread = this.getThreadByID(this.pendingThreadID);
         if (thread) {
-            this.unregisterThread(thread);
-            thread.destroy();
+            this.destroyThread(thread);
         }
 
         this.pendingThreadID = null;

diff --git a/src/controllers/__tests__/DrawingModeController-test.js b/src/controllers/__tests__/DrawingModeController-test.js
@@ -87,6 +87,55 @@ describe('controllers/DrawingModeController', () => {
         });
     });
 
+    describe('cancelDrawing()', () => {
+        it('should exit drawing mode', () => {
+            controller.exit = jest.fn();
+            controller.destroyThread = jest.fn();
+            controller.currentThread = thread;
+
+            controller.cancelDrawing();
+            expect(controller.exit).toBeCalled();
+            expect(controller.destroyThread).toBeCalled();
+        });
+    });
+
+    describe('postDrawing()', () => {
+        beforeEach(() => {
+            controller.exit = jest.fn();
+            thread.state = STATES.pending;
+        });
+
+        it('should exit drawing mode', () => {
+            controller.postDrawing();
+            expect(controller.exit).toBeCalled();
+            expect(thread.save).not.toBeCalled();
+        });
+
+        it('should not save non-pending threads', () => {
+            thread.state = STATES.inactive;
+            controller.currentThread = thread;
+            controller.postDrawing();
+            expect(controller.exit).toBeCalled();
+            expect(thread.save).not.toBeCalled();
+        });
+
+        it('should not save pending threads without paths', () => {
+            thread.pathContainer = { undoStack: [] };
+            controller.currentThread = thread;
+            controller.postDrawing();
+            expect(controller.exit).toBeCalled();
+            expect(thread.save).not.toBeCalled();
+        });
+
+        it('should save the current pending thread', () => {
+            thread.pathContainer = { undoStack: [{}] };
+            controller.currentThread = thread;
+            controller.postDrawing();
+            expect(controller.exit).toBeCalled();
+            expect(thread.save).toBeCalled();
+        });
+    });
+
     describe('setupHandlers()', () => {
         it('should successfully contain draw mode handlers if undo and redo buttons exist', () => {
             controller.annotatedElement = {};

diff --git a/src/controllers/__tests__/PointModeController-test.js b/src/controllers/__tests__/PointModeController-test.js
@@ -91,22 +91,23 @@ describe('controllers/PointModeController', () => {
     });
 
     describe('resetMode()', () => {
-        beforeEach(() => {
+        it('should reset annotation mode', () => {
             // Set up annotation mode
             controller.annotatedElement = document.createElement('div');
             controller.annotatedElement.classList.add(CLASS_ANNOTATION_MODE);
             controller.headerElement = document.createElement('div');
 
             controller.buttonEl = document.createElement('button');
             controller.buttonEl.classList.add(CLASS_ACTIVE);
-        });
+            controller.getThreadByID = jest.fn().mockReturnValue(thread);
+            controller.destroyThread = jest.fn();
 
-        it('should reset annotation mode', () => {
             controller.buttonEl = document.createElement('button');
             controller.buttonEl.classList.add(CLASS_ACTIVE);
             controller.resetMode();
             expect(controller.buttonEl.classList).not.toContain(CLASS_ACTIVE);
             expect(controller.hadPendingThreads).toBeFalsy();
+            expect(controller.destroyThread).toBeCalledWith(thread);
         });
     });