Fix: Don't save empty drawing threads (#138)

- Fixes issue where users couldn't select drawings after pressing the "Done" button
- Drawing handlers require a blank temporary thread in order to actually start/stop drawing. This thread is not yet saved into the API and will not have valid location information until the user actually begins drawing. This prevents the controller from saving that empty thread until it contains valid location information.
- Validates min/max boundary coordinates for annotation threads

src/__tests__/util-test.js

@@ -39,7 +39,8 @@ import {
     clearCanvas,
     replaceHeader,
     isInDialog,
-    focusTextArea
+    focusTextArea,
+    hasValidBoundaryCoordinates
 } from '../util';
 import {
     STATES,
@@ -807,4 +808,26 @@ describe('util', () => {
             expect(focusTextArea(el)).to.have.class(CLASS_ACTIVE);
         });
     });
+
+    describe('hasValidBoundaryCoordinates()', () => {
+        it('return true only if boundary coordinates are valid', () => {
+            expect(hasValidBoundaryCoordinates({})).to.be.false;
+            expect(hasValidBoundaryCoordinates({
+                minX: NaN,
+                minY: 1,
+                maxX: 1,
+                maxY: 1
+            })).to.be.false;
+            expect(hasValidBoundaryCoordinates({
+                minX: 1,
+                minY: 1,
+                maxX: 1,
+                maxY: 1
+            })).to.be.true;
+        });
+
+        it('should return true for highlight annotations', () => {
+            expect(hasValidBoundaryCoordinates({ type: TYPES.highlight })).to.be.true;
+        })
+    });
 });

src/controllers/AnnotationModeController.js

@@ -1,6 +1,6 @@
 import rbush from 'rbush';
 import EventEmitter from 'events';
-import { insertTemplate, isPending, replaceHeader } from '../util';
+import { insertTemplate, isPending, replaceHeader, hasValidBoundaryCoordinates } from '../util';
 import {
     CLASS_HIDDEN,
     CLASS_ACTIVE,
@@ -207,7 +207,7 @@ class AnnotationModeController extends EventEmitter {
      * @return {void}
      */
     registerThread(thread) {
-        if (!thread || !thread.location) {
+        if (!thread || !thread.location || !hasValidBoundaryCoordinates(thread)) {
             return;
         }
 

src/controllers/DrawingModeController.js

@@ -1,7 +1,14 @@
 import AnnotationModeController from './AnnotationModeController';
 import shell from './drawingShell.html';
 import DocDrawingThread from '../doc/DocDrawingThread';
-import { replaceHeader, enableElement, disableElement, eventToLocationHandler, clearCanvas } from '../util';
+import {
+    replaceHeader,
+    enableElement,
+    disableElement,
+    eventToLocationHandler,
+    clearCanvas,
+    hasValidBoundaryCoordinates
+} from '../util';
 import {
     TYPES,
     STATES,
@@ -134,7 +141,10 @@ class DrawingModeController extends AnnotationModeController {
         });
 
         this.pushElementHandler(this.postButtonEl, 'click', () => {
-            this.saveThread(this.currentThread);
+            if (this.currentThread && this.currentThread.state === STATES.pending) {
+                this.saveThread(this.currentThread);
+            }
+
             this.toggleMode();
         });
 
@@ -306,7 +316,7 @@ class DrawingModeController extends AnnotationModeController {
      * @return {void}
      */
     saveThread(thread) {
-        if (!thread) {
+        if (!thread || !hasValidBoundaryCoordinates(thread)) {
             return;
         }
 

src/controllers/__tests__/AnnotationModeController-test.js

@@ -272,6 +272,12 @@ describe('controllers/AnnotationModeController', () => {
             expect(controller.emit).to.not.be.called;
         });
 
+        it('should do nothing if thread has invalid boundary', () => {
+            controller.registerThread({ minX: NaN, minY: 1, maxX: 1, maxY: 1 });
+            controller.registerThread({ type: 'someType' });
+            expect(controller.emit).to.not.be.called;
+        });
+
         it('should create a new rbush for the thread\'s page location', () => {
             stubs.threadMock.expects('addListener').withArgs('threadevent', sinon.match.func);
 
@@ -550,7 +556,7 @@ describe('controllers/AnnotationModeController', () => {
                 removeAllListeners: () => {}
             };
             stubs.pendingMock = sandbox.mock(pendingThread);
-            controller.registerThread(pendingThread);
+            controller.threads[1].insert(pendingThread);
 
             stubs.threadMock.expects('destroy').never();
             stubs.pendingMock.expects('destroy');

src/controllers/__tests__/DrawingModeController-test.js

@@ -3,6 +3,7 @@ import AnnotationModeController from '../AnnotationModeController';
 import DrawingModeController from '../DrawingModeController';
 import * as util from '../../util';
 import {
+    TYPES,
     THREAD_EVENT,
     SELECTOR_ANNOTATION_BUTTON_DRAW_CANCEL,
     SELECTOR_ANNOTATION_BUTTON_DRAW_POST,
@@ -30,14 +31,17 @@ describe('controllers/DrawingModeController', () => {
                 page: 1
             },
             info: 'I am a thread',
-            addListener: sandbox.stub(),
-            removeListener: sandbox.stub(),
-            saveAnnotation: sandbox.stub(),
-            handleStart: sandbox.stub(),
-            destroy: sandbox.stub(),
-            deleteThread: sandbox.stub(),
-            show: sandbox.stub()
+            addListener: () => {},
+            removeListener: () => {},
+            saveAnnotation: () => {},
+            handleStart: () => {},
+            destroy: () => {},
+            deleteThread: () => {},
+            clearBoundary: () => {},
+            drawBoundary: () => {},
+            show: () => {}
         };
+        stubs.threadMock = sandbox.mock(stubs.thread);
 
         sandbox.stub(controller, 'emit');
     });
@@ -209,12 +213,12 @@ describe('controllers/DrawingModeController', () => {
         });
 
         it('should save thread on softcommit', () => {
+            stubs.threadMock.expects('handleStart').never();
             controller.handleThreadEvents(stubs.thread, {
                 event: 'softcommit'
             });
             expect(controller.unbindListeners).to.be.called;
             expect(controller.bindListeners).to.be.called;
-            expect(stubs.thread.handleStart).to.not.be.called;
             expect(controller.saveThread).to.be.called;
         });
 
@@ -271,10 +275,10 @@ describe('controllers/DrawingModeController', () => {
 
         it('should soft delete a pending thread and restart mode listeners', () => {
             stubs.thread.state = 'pending';
+            stubs.threadMock.expects('destroy');
             controller.handleThreadEvents(stubs.thread, {
                 event: 'dialogdelete'
             });
-            expect(stubs.thread.destroy).to.be.called;
             expect(controller.unbindListeners).to.be.called;
             expect(controller.bindListeners).to.be.called;
         });
@@ -285,10 +289,10 @@ describe('controllers/DrawingModeController', () => {
             controller.registerThread(stubs.thread);
             const unregisterThreadStub = sandbox.stub(controller, 'unregisterThread');
 
+            stubs.threadMock.expects('deleteThread');
             controller.handleThreadEvents(stubs.thread, {
                 event: 'dialogdelete'
             });
-            expect(stubs.thread.deleteThread).to.be.called;
             expect(unregisterThreadStub).to.be.called;
         });
 
@@ -340,18 +344,9 @@ describe('controllers/DrawingModeController', () => {
     });
 
     describe('renderPage()', () => {
-        const thread = {
-            threadID: '123abc',
-            location: { page: 1 },
-            show: () => {},
-            addListener: () => {}
-        };
-
         beforeEach(() => {
             controller.annotatedElement = document.createElement('div');
             controller.annotatedElement.setAttribute('data-page-number', 1);
-
-            stubs.threadMock = sandbox.mock(thread);
             sandbox.stub(util, 'clearCanvas');
         });
 
@@ -360,14 +355,14 @@ describe('controllers/DrawingModeController', () => {
             controller.renderPage(1);
 
             controller.threads = {};
-            controller.registerThread(thread);
+            controller.registerThread(stubs.thread);
             controller.renderPage(2);
             expect(util.clearCanvas).to.be.calledTwice;
         });
 
         it('should render the annotations on every page', () => {
             controller.threads = {};
-            controller.registerThread(thread);
+            controller.registerThread(stubs.thread);
             stubs.threadMock.expects('show').once();
             controller.renderPage(1);
             expect(util.clearCanvas).to.be.called;
@@ -376,27 +371,19 @@ describe('controllers/DrawingModeController', () => {
 
     describe('removeSelection()', () => {
         it('should clean a selected thread boundary', () => {
-            const thread = {
-                clearBoundary: sandbox.stub()
-            };
-            controller.selectedThread = thread;
-
+            controller.selectedThread = stubs.thread;
+            stubs.threadMock.expects('clearBoundary');
             controller.removeSelection();
-            expect(thread.clearBoundary).to.be.called;
             expect(controller.selectedThread).to.be.undefined;
         });
     });
 
     describe('select()', () => {
         it('should draw the boundary', () => {
-            const thread = {
-                drawBoundary: sandbox.stub()
-            }
-
-            expect(controller.selectedThread).to.not.deep.equal(thread);
-            controller.select(thread);
-            expect(thread.drawBoundary).to.be.called;
-            expect(controller.selectedThread).to.deep.equal(thread);
+            stubs.threadMock.expects('drawBoundary');
+            expect(controller.selectedThread).to.not.deep.equal(stubs.thread);
+            controller.select(stubs.thread);
+            expect(controller.selectedThread).to.deep.equal(stubs.thread);
         });
     });
 
@@ -428,4 +415,23 @@ describe('controllers/DrawingModeController', () => {
             expect(stubs.disable).to.not.be.called;
         });
     });
+
+    describe('saveThread()', () => {
+        beforeEach(() => {
+            sandbox.stub(controller, 'registerThread');
+        });
+
+        it('should do nothing if thread has invalid boundary', () => {
+            stubs.threadMock.expects('saveAnnotation').withArgs(TYPES.draw).never();
+            controller.saveThread({ minX: NaN, minY: 1, maxX: 1, maxY: 1 });
+            controller.saveThread({ type: TYPES.draw });
+            expect(controller.registerThread).to.not.be.called;
+        });
+
+        it('should save and register the annotation thread', () => {
+            stubs.threadMock.expects('saveAnnotation').withArgs(TYPES.draw);
+            controller.saveThread(stubs.thread);
+            expect(controller.registerThread).to.be.called;
+        });
+    });
 });

src/controllers/__tests__/HighlightModeController-test.js

@@ -7,6 +7,7 @@ import {
     CLASS_ANNOTATION_MODE,
     THREAD_EVENT,
     STATES,
+    TYPES,
     CONTROLLER_EVENT,
     CLASS_ANNOTATION_LAYER_HIGHLIGHT
 } from '../../constants';
@@ -22,6 +23,7 @@ describe('controllers/HighlightModeController', () => {
         stubs.thread = {
             annotations: {},
             location: { page: 1 },
+            type: TYPES.highlight,
             show: () => {},
             addListener: () => {}
         };
@@ -115,7 +117,6 @@ describe('controllers/HighlightModeController', () => {
         beforeEach(() => {
             controller.annotatedElement = document.createElement('div');
             controller.annotatedElement.setAttribute('data-page-number', 1);
-
             sandbox.stub(util, 'clearCanvas');
         });
 
@@ -127,7 +128,7 @@ describe('controllers/HighlightModeController', () => {
 
         it('should render the annotations on the specified page', () => {
             controller.registerThread(stubs.thread);
-            stubs.threadMock.expects('show').once();
+            stubs.threadMock.expects('show');
             controller.renderPage(1);
             expect(util.clearCanvas).to.be.called;
         });

src/doc/DocDrawingThread.js

@@ -92,6 +92,7 @@ class DocDrawingThread extends DrawingThread {
 
         // Start drawing rendering
         this.lastAnimationRequestId = window.requestAnimationFrame(this.render);
+        this.state = STATES.pending;
     }
 
     /**

src/doc/__tests__/DocDrawingThread-test.js

@@ -6,6 +6,7 @@ import AnnotationThread from '../../AnnotationThread';
 import DrawingPath from '../../drawing/DrawingPath';
 import {
     DRAW_STATES,
+    THREAD_EVENT,
     STATES
 } from '../../constants';
 
@@ -101,18 +102,21 @@ describe('doc/DocDrawingThread', () => {
         it('should do nothing if no location is provided', () => {
             thread.handleStart();
             expect(stubs.pageChange).to.not.be.called;
+            expect(thread.state).to.equal(STATES.inactive);
         });
 
         it('should set the drawingFlag, pendingPath, and context if they do not exist', () => {
             stubs.pageChange.returns(false);
             thread.drawingFlag = DRAW_STATES.idle;
             thread.pendingPath = undefined;
+            expect(thread.state).to.equal(STATES.inactive);
             thread.handleStart(thread.location);
 
             expect(window.requestAnimationFrame).to.be.called;
             expect(thread.drawingFlag).to.equal(DRAW_STATES.drawing);
             expect(thread.hasPageChanged).to.be.called;
             expect(thread.pendingPath).to.be.an.instanceof(DrawingPath);
+            expect(thread.state).to.equal(STATES.pending);
         });
 
         it('should commit the thread when the page changes', () => {
@@ -125,6 +129,7 @@ describe('doc/DocDrawingThread', () => {
             expect(thread.hasPageChanged).to.be.called;
             expect(thread.onPageChange).to.be.called;
             expect(thread.checkAndHandleScaleUpdate).to.not.be.called;
+            expect(thread.state).to.equal(STATES.inactive);
         });
 
     });

src/drawing/DrawingThread.js

@@ -59,6 +59,9 @@ class DrawingThread extends AnnotationThread {
     constructor(data) {
         super(data);
 
+        // Default drawing thread state to inactive until user begins drawing
+        this.state = STATES.inactive;
+
         this.render = this.render.bind(this);
         this.handleStart = this.handleStart.bind(this);
         this.handleMove = this.handleMove.bind(this);

src/drawing/__tests__/DrawingThread-test.js

@@ -31,6 +31,7 @@ describe('drawing/DrawingThread', () => {
             threadID: 2,
             type: 'draw'
         });
+        expect(thread.state).to.equal(STATES.inactive);
     });
 
     afterEach(() => {

src/util.js

@@ -504,6 +504,16 @@ export function isPending(threadState) {
     return PENDING_STATES.indexOf(threadState) > -1;
 }
 
+/**
+ * Checks whether an annotation thread has valid min/max boundary coordinates
+ *
+ * @param {AnnotationThread} thread Annotation thread location object
+ * @return {boolean} Whether or not the annotation has valid boundary coordinates
+ */
+export function hasValidBoundaryCoordinates(thread) {
+    return !!(isHighlightAnnotation(thread.type) || (thread.minX && thread.minY && thread.maxX && thread.maxY));
+}
+
 /**
  * Checks whether a point annotation thread has the correct location params
  *