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,
@@ -10,7 +17,8 @@ import {
     SELECTOR_ANNOTATION_BUTTON_DRAW_UNDO,
     SELECTOR_ANNOTATION_BUTTON_DRAW_REDO,
     SELECTOR_DRAW_MODE_HEADER,
-    CLASS_ANNOTATION_LAYER_DRAW
+    CLASS_ANNOTATION_LAYER_DRAW,
+    THREAD_EVENT
 } from '../constants';
 
 class DrawingModeController extends AnnotationModeController {
@@ -100,6 +108,7 @@ class DrawingModeController extends AnnotationModeController {
         /* eslint-enable require-jsdoc */
 
         // Setup
+        this.hasPendingDrawing = false;
         const threadParams = this.annotator.getThreadParams([], {}, TYPES.draw);
         this.currentThread = new DocDrawingThread(threadParams);
         this.currentThread.addListener('threadevent', (data) => {
@@ -134,7 +143,10 @@ class DrawingModeController extends AnnotationModeController {
         });
 
         this.pushElementHandler(this.postButtonEl, 'click', () => {
-            this.saveThread(this.currentThread);
+            if (this.hasPendingDrawing) {
+                this.saveThread(this.currentThread);
+            }
+
             this.toggleMode();
         });
 
@@ -156,8 +168,12 @@ class DrawingModeController extends AnnotationModeController {
     handleThreadEvents(thread, data = {}) {
         const { eventData } = data;
         switch (data.event) {
+            case THREAD_EVENT.pending:
+                this.hasPendingDrawing = true;
+                break;
             case 'softcommit':
                 this.currentThread = undefined;
+                this.hasPendingDrawing = false;
                 this.saveThread(thread);
                 this.unbindListeners();
                 this.bindListeners();
@@ -176,6 +192,7 @@ class DrawingModeController extends AnnotationModeController {
                 if (thread.state === STATES.pending) {
                     // Soft delete, in-progress thread doesn't require a redraw or a delete on the server
                     // Clear in-progress thread and restart drawing
+                    this.hasPendingDrawing = false;
                     thread.destroy();
                     this.unbindListeners();
                     this.bindListeners();
@@ -306,7 +323,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');
     });
@@ -208,14 +212,22 @@ describe('controllers/DrawingModeController', () => {
             sandbox.stub(controller, 'updateUndoRedoButtonEls');
         });
 
+        it('should set hasPendingDrawing to true when the user begins drawing', () => {
+            controller.handleThreadEvents(stubs.thread, {
+                event: THREAD_EVENT.pending
+            });
+            expect(controller.hasPendingDrawing).to.be.true;
+        });
+
         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;
+            expect(controller.hasPendingDrawing).to.be.false;
         });
 
         it('should start a new thread on pagechanged', () => {
@@ -255,6 +267,7 @@ describe('controllers/DrawingModeController', () => {
             expect(controller.saveThread).to.be.called;
             expect(controller.unbindListeners).to.be.called;
             expect(controller.bindListeners).to.be.called;
+            expect(controller.hasPendingDrawing).to.be.false;
             expect(thread2.handleStart).to.be.calledWith(data.eventData.location);
         });
 
@@ -271,10 +284,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 +298,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 +353,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 +364,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 +380,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 +424,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

@@ -5,7 +5,8 @@ import {
     STATES,
     DRAW_STATES,
     CLASS_ANNOTATION_LAYER_DRAW,
-    CLASS_ANNOTATION_LAYER_DRAW_IN_PROGRESS
+    CLASS_ANNOTATION_LAYER_DRAW_IN_PROGRESS,
+    THREAD_EVENT
 } from '../constants';
 import { getBrowserCoordinatesFromLocation, getContext, getPageEl } from './docUtil';
 import { createLocation, getScale } from '../util';
@@ -92,6 +93,7 @@ class DocDrawingThread extends DrawingThread {
 
         // Start drawing rendering
         this.lastAnimationRequestId = window.requestAnimationFrame(this.render);
+        this.emit(THREAD_EVENT.pending);
     }
 
     /**