Fix: Don't save empty drawing 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
  *