Fix: Do not select drawings when creating a point annotation on top (#28

)

* Fix: Do not select drawings when creating a point annotation on top
* Update: Renaming hasPendingThreads to hadPendingThreads

src/AnnotationThread.js

@@ -59,6 +59,7 @@ class AnnotationThread extends EventEmitter {
         this.hasTouch = data.hasTouch;
         this.permissions = data.permissions;
         this.localized = data.localized;
+        this.state = STATES.inactive;
 
         this.setup();
     }

src/controllers/AnnotationModeController.js

@@ -129,6 +129,7 @@ class AnnotationModeController extends EventEmitter {
 
         this.unbindListeners(); // Disable mode
         this.emit(CONTROLLER_EVENT.exit);
+        this.hadPendingThreads = false;
     }
 
     /**
@@ -267,6 +268,11 @@ class AnnotationModeController extends EventEmitter {
      */
     handleThreadEvents(thread, data) {
         switch (data.event) {
+            case THREAD_EVENT.save:
+            case THREAD_EVENT.cancel:
+                this.hadPendingThreads = false;
+                this.emit(data.event, data.data);
+                break;
             case THREAD_EVENT.threadCleanup:
                 // Thread should be cleaned up, unbind listeners - we
                 // don't do this in annotationdelete listener since thread
@@ -334,20 +340,20 @@ class AnnotationModeController extends EventEmitter {
      * current file
      */
     destroyPendingThreads() {
-        let hasPendingThreads = false;
+        let hadPendingThreads = false;
 
         Object.keys(this.threads).forEach((page) => {
             const pageThreads = this.threads[page] || {};
 
             Object.keys(pageThreads).forEach((threadID) => {
                 const thread = pageThreads[threadID];
                 if (isPending(thread.state)) {
-                    hasPendingThreads = true;
+                    hadPendingThreads = true;
                     thread.destroy();
                 }
             });
         });
-        return hasPendingThreads;
+        return hadPendingThreads;
     }
 
     /**

src/controllers/PointModeController.js

@@ -47,13 +47,14 @@ class PointModeController extends AnnotationModeController {
 
         // Determine if a point annotation dialog is already open and close the
         // current open dialog
-        const hasPendingThreads = this.destroyPendingThreads();
-        if (hasPendingThreads) {
+        this.hadPendingThreads = this.destroyPendingThreads();
+        if (this.hadPendingThreads) {
             return;
         }
 
         // Exits point annotation mode on first click
         this.emit(CONTROLLER_EVENT.toggleMode);
+        this.hadPendingThreads = true;
 
         // Get annotation location from click event, ignore click if location is invalid
         const location = this.annotator.getLocationFromEvent(event, TYPES.point);

src/controllers/__tests__/AnnotationModeController-test.js

@@ -160,6 +160,7 @@ describe('controllers/AnnotationModeController', () => {
             expect(controller.destroyPendingThreads).to.be.called;
             expect(controller.emit).to.be.calledWith(CONTROLLER_EVENT.exit);
             expect(controller.unbindListeners).to.be.called;
+            expect(controller.hadPendingThreads).to.be.falsy;
         });
 
         it('should deactive mode button if available', () => {
@@ -305,6 +306,17 @@ describe('controllers/AnnotationModeController', () => {
             }
         });
 
+        it('should mark hadPendingThreads as false and emit event on thread save or cancel', () => {
+            controller.handleThreadEvents(stubs.thread, { event: THREAD_EVENT.save, data: {} });
+            expect(controller.emit).to.be.calledWith(THREAD_EVENT.save, sinon.match.object);
+            expect(controller.hadPendingThreads).to.be.falsy;
+
+            controller.hadPendingThreads = true;
+            controller.handleThreadEvents(stubs.thread, { event: THREAD_EVENT.cancel, data: {} });
+            expect(controller.emit).to.be.calledWith(THREAD_EVENT.cancel, sinon.match.object);
+            expect(controller.hadPendingThreads).to.be.falsy;
+        });
+
         it('should unregister thread on threadCleanup', () => {
             controller.handleThreadEvents(stubs.thread, { event: THREAD_EVENT.threadCleanup, data: {} });
             expect(controller.unregisterThread).to.be.called;

src/controllers/__tests__/PointModeController-test.js

@@ -112,6 +112,7 @@ describe('controllers/PointModeController', () => {
             expect(controller.registerThread).to.be.called;
             expect(controller.emit).to.be.calledWith(CONTROLLER_EVENT.toggleMode);
             expect(controller.emit).to.be.calledWith(THREAD_EVENT.pending, 'data');
+            expect(controller.hadPendingThreads).to.be.truthy;
         });
     });
 });

src/doc/DocAnnotator.js

@@ -788,11 +788,31 @@ class DocAnnotator extends Annotator {
      * @return {void}
      */
     drawingSelectionHandler(event) {
-        if (this.modeControllers[TYPES.draw]) {
-            this.modeControllers[TYPES.draw].handleSelection(event);
+        const controller = this.modeControllers[TYPES.draw];
+        if (controller && !this.isCreatingAnnotation() && !this.isCreatingHighlight) {
+            controller.handleSelection(event);
         }
     }
 
+    /**
+     * Returns whether any mode controller is currently creating an
+     * annotation thread
+     *
+     * @private
+     * @return {boolean} Whether any controller has a pending thread
+     */
+    isCreatingAnnotation() {
+        let isPending = false;
+        Object.keys(this.modeControllers).some((mode) => {
+            const controller = this.modeControllers[mode];
+            if (controller.hadPendingThreads) {
+                isPending = true;
+            }
+            return isPending;
+        });
+        return isPending;
+    }
+
     /**
      * Mouseup handler. Switches between creating a highlight and delegating
      * to highlight click handlers depending on whether mouse moved since

src/doc/__tests__/DocAnnotator-test.js

@@ -1450,18 +1450,34 @@ describe('doc/DocAnnotator', () => {
     });
 
     describe('drawingSelectionHandler()', () => {
-        it('should use the controller to select with the event', () => {
-            const drawController = {
+        beforeEach(() => {
+            stubs.drawController = {
                 handleSelection: sandbox.stub(),
                 removeSelection: sandbox.stub()
             };
             annotator.modeControllers = {
-                [TYPES.draw]: drawController
+                [TYPES.draw]: stubs.drawController
             };
 
+            stubs.isCreatingAnnotation = sandbox.stub(annotator, 'isCreatingAnnotation').returns(false);
+        });
+
+        it('should use the controller to select with the event', () => {
             const evt = 'event';
             annotator.drawingSelectionHandler(evt);
-            expect(drawController.handleSelection).to.be.calledWith(evt);
+            expect(stubs.drawController.handleSelection).to.be.calledWith(evt);
+        });
+
+        it('should do nothing if a mode is creating an annotation', () => {
+            stubs.isCreatingAnnotation.returns(true);
+            annotator.drawingSelectionHandler('irrelevant');
+            expect(stubs.drawController.handleSelection).to.not.be.called;
+        });
+
+        it('should do nothing if a highlight is being created', () => {
+            annotator.isCreatingHighlight = true;
+            annotator.drawingSelectionHandler('irrelevant');
+            expect(stubs.drawController.handleSelection).to.not.be.called;
         });
 
         it('should not error when no modeButtons exist for draw', () => {
@@ -1470,6 +1486,24 @@ describe('doc/DocAnnotator', () => {
         });
     });
 
+    describe('isCreatingAnnotation()', () => {
+        it('should return true if a mode is creating an annotation', () => {
+            annotator.modeControllers = {
+                [TYPES.draw]: { hasPendingThread: false },
+                [TYPES.point]: { hasPendingThread: true }
+            };
+            expect(annotator.isCreatingAnnotation()).to.be.truthy;
+        });
+
+        it('should return false if all modes are NOT creating annotations', () => {
+            annotator.modeControllers = {
+                [TYPES.draw]: { hasPendingThread: false },
+                [TYPES.point]: { hasPendingThread: false }
+            };
+            expect(annotator.isCreatingAnnotation()).to.be.falsy;
+        });
+    });
+
     describe('handleControllerEvents()', () => {
         const mode = 'something';