From 529b9a4631a0f1d8dbec470918f16555793755e9 Mon Sep 17 00:00:00 2001
From: Jeremy Press <jpress@box.com>
Date: Thu, 28 Sep 2017 14:37:10 -0700
Subject: [PATCH] Fix: Properly delete draw annotations threads (#412)

* Fix: Properly delete draw annotations threads
---
 src/lib/annotations/drawing/DrawingContainer.js      | 10 ++++++++++
 src/lib/annotations/drawing/DrawingModeController.js |  4 +++-
 src/lib/annotations/drawing/DrawingThread.js         |  8 ++++++--
 .../drawing/__tests__/DrawingContainer-test.js       | 12 ++++++++++++
 .../drawing/__tests__/DrawingModeController-test.js  |  3 +++
 .../drawing/__tests__/DrawingThread-test.js          |  6 ++++++
 6 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/src/lib/annotations/drawing/DrawingContainer.js b/src/lib/annotations/drawing/DrawingContainer.js
index 88bafe2e7..bd688fe0f 100644
--- a/src/lib/annotations/drawing/DrawingContainer.js
+++ b/src/lib/annotations/drawing/DrawingContainer.js
@@ -19,6 +19,16 @@ class DrawingContainer {
     /** @property {Array} - The item history stack for redo operations */
     redoStack = [];
 
+    /**
+     * Wipes the undo/redo stack after a deletion.
+     *
+     * @return {void}
+     */
+    destroy() {
+        this.undoStack = [];
+        this.redoStack = [];
+    }
+
     /**
      * Insert an item into the drawing container. Clears any redoable items.
      *
diff --git a/src/lib/annotations/drawing/DrawingModeController.js b/src/lib/annotations/drawing/DrawingModeController.js
index 289b00e9a..3f65a5b22 100644
--- a/src/lib/annotations/drawing/DrawingModeController.js
+++ b/src/lib/annotations/drawing/DrawingModeController.js
@@ -224,8 +224,10 @@ class DrawingModeController extends AnnotationModeController {
                     this.unbindModeListeners();
                     this.bindModeListeners();
                 } else {
-                    // Redraw any threads that the deleted thread could have been overlapping
                     thread.deleteThread();
+                    this.unregisterThread(thread);
+
+                    // Redraw any threads that the deleted thread could have been overlapping
                     this.threads.search(thread).forEach((drawingThread) => drawingThread.show());
                 }
 
diff --git a/src/lib/annotations/drawing/DrawingThread.js b/src/lib/annotations/drawing/DrawingThread.js
index 8b8d2b83a..3d2da276d 100644
--- a/src/lib/annotations/drawing/DrawingThread.js
+++ b/src/lib/annotations/drawing/DrawingThread.js
@@ -78,7 +78,7 @@ class DrawingThread extends AnnotationThread {
     }
 
     /**
-     * Soft destructor for a drawingthread object
+     * Destructor for a drawing thread object.
      *
      * [destructor]
      * @inheritdoc
@@ -140,7 +140,8 @@ class DrawingThread extends AnnotationThread {
     /* eslint-disable no-unused-vars */
 
     /**
-     * Delete a saved drawing thread
+     * Delete a saved drawing thread by deleting each annotation
+     * and then clearing the concrete context, boundary, and destroying its path.
      *
      * @public
      * @return {void}
@@ -159,6 +160,9 @@ class DrawingThread extends AnnotationThread {
         );
 
         this.clearBoundary();
+
+        this.pathContainer.destroy();
+        this.pathContainer = null;
     }
 
     /**
diff --git a/src/lib/annotations/drawing/__tests__/DrawingContainer-test.js b/src/lib/annotations/drawing/__tests__/DrawingContainer-test.js
index dfaa845f9..50b160471 100644
--- a/src/lib/annotations/drawing/__tests__/DrawingContainer-test.js
+++ b/src/lib/annotations/drawing/__tests__/DrawingContainer-test.js
@@ -17,6 +17,18 @@ describe('lib/annotations/drawing/DrawingContainer', () => {
         drawingContainer = null;
     });
 
+    describe('destroy()', () => {
+        it('should clear the undo and redo stack', () => {
+            drawingContainer.redoStack = [1,2,3];
+            drawingContainer.undoStack = [1,2,3];
+
+            drawingContainer.destroy();
+
+            expect(drawingContainer.undoStack.length).to.equal(0)
+            expect(drawingContainer.redoStack.length).to.equal(0)
+        });
+    });
+
     describe('insert()', () => {
         it('should insert an item into the undoStack and clear the redo stack', () => {
             drawingContainer.redoStack = [1,2,3];
diff --git a/src/lib/annotations/drawing/__tests__/DrawingModeController-test.js b/src/lib/annotations/drawing/__tests__/DrawingModeController-test.js
index 4960cbf32..d33a295db 100644
--- a/src/lib/annotations/drawing/__tests__/DrawingModeController-test.js
+++ b/src/lib/annotations/drawing/__tests__/DrawingModeController-test.js
@@ -272,10 +272,13 @@ describe('lib/annotations/drawing/DrawingModeController', () => {
                 search: sandbox.stub().returns([])
             };
 
+            const unregisterThreadStub = sandbox.stub(drawingModeController, 'unregisterThread');
+
             drawingModeController.handleAnnotationEvent(thread, {
                 event: 'dialogdelete'
             });
             expect(thread.deleteThread).to.be.called;
+            expect(unregisterThreadStub).to.be.called;
             expect(drawingModeController.threads.search).to.be.called;
         });
     });
diff --git a/src/lib/annotations/drawing/__tests__/DrawingThread-test.js b/src/lib/annotations/drawing/__tests__/DrawingThread-test.js
index 91a9e6cab..0102f2869 100644
--- a/src/lib/annotations/drawing/__tests__/DrawingThread-test.js
+++ b/src/lib/annotations/drawing/__tests__/DrawingThread-test.js
@@ -80,6 +80,11 @@ describe('lib/annotations/drawing/DrawingThread', () => {
             thread.concreteContext = {
                 clearRect: sandbox.stub()
             };
+
+            thread.pathContainer = {
+                destroy: sandbox.stub()
+            };
+
             thread.annotations = ['annotation'];
 
 
@@ -88,6 +93,7 @@ describe('lib/annotations/drawing/DrawingThread', () => {
             expect(thread.concreteContext.clearRect).to.be.called;
             expect(thread.clearBoundary).to.be.called;
             expect(thread.deleteAnnotationWithID).to.be.calledWith('annotation');
+            expect(thread.pathContainer).to.equal(null);
         });
     });