Fix: Ensure drawings are only registered once with the controller (#278)

src/AnnotationThread.js

@@ -22,25 +22,25 @@ class AnnotationThread extends EventEmitter {
 
     /** @param {Object} */
     annotations: Object;
-    
+
     /** @param {AnnotationAPI} */
     api: AnnotationAPI;
-    
+
     /** @param {string} */
     fileVersionId: string;
-    
+
     /** @param {Location} */
     location: ?Location;
-    
+
     /** @param {string} */
     threadID: ?string;
-    
+
     /** @param {string} */
     threadNumber: string;
-    
+
     /** @param {AnnotationType} */
     type: AnnotationType;
-    
+
     /** @param {boolean} */
     canComment: boolean;
 
@@ -120,10 +120,6 @@ class AnnotationThread extends EventEmitter {
 
             this.element = null;
         }
-
-        if (this.state !== STATES.pending) {
-            this.emit(THREAD_EVENT.threadDelete);
-        }
     }
 
     /**
@@ -147,7 +143,7 @@ class AnnotationThread extends EventEmitter {
 
     /**
      * Positions the annotation popover
-     * 
+     *
      * @return {void}
      */
     position = () => {
@@ -157,14 +153,14 @@ class AnnotationThread extends EventEmitter {
 
     /**
      * Returns the parent element for the annotation popover
-     * 
+     *
      * @return {HTMLElement} Parent element for the annotation popover
      */
     getPopoverParent() {
         return util.shouldDisplayMobileUI(this.container)
             ? this.container
-            // $FlowFixMe
-            : util.getPageEl(this.annotatedElement, this.location.page);
+            : // $FlowFixMe
+            util.getPageEl(this.annotatedElement, this.location.page);
     }
 
     /**
@@ -251,16 +247,18 @@ class AnnotationThread extends EventEmitter {
         this.renderAnnotationPopover();
 
         // Save annotation on server
-        return this.api
-            // $FlowFixMe
-            .create(annotationData)
-            .then((savedAnnotation) => this.updateTemporaryAnnotation(id, savedAnnotation))
-            .catch((error) => this.handleThreadSaveError(error, id));
+        return (
+            this.api
+                // $FlowFixMe
+                .create(annotationData)
+                .then((savedAnnotation) => this.updateTemporaryAnnotation(id, savedAnnotation))
+                .catch((error) => this.handleThreadSaveError(error, id))
+        );
     }
 
     /**
      * Update the annotation thread instance with annotation data/comments
-     * 
+     *
      * @param {Object} data - Annotation data
      * @return {void}
      */
@@ -353,7 +351,6 @@ class AnnotationThread extends EventEmitter {
 
         if (this.canDelete && this.comments.length <= 0) {
             // If this annotation was the last one in the thread, destroy the thread
-            this.destroy();
             this.threadID = null;
         } else {
             // Otherwise, display annotation with deleted comment
@@ -378,6 +375,12 @@ class AnnotationThread extends EventEmitter {
      * @return {void}
      */
     deleteErrorHandler(error: Error) {
+        // $FlowFixMe
+        const { page } = this.location;
+
+        // Re-render page
+        this.emit(THREAD_EVENT.render, { page });
+
         // Broadcast error
         this.emit(THREAD_EVENT.deleteError);
         /* eslint-disable no-console */

src/Annotator.js

@@ -393,7 +393,7 @@ class Annotator extends EventEmitter {
         thread.renderAnnotationPopover();
         thread.save(TYPES.point, commentText);
 
-        this.emit(THREAD_EVENT.threadSave, thread.getThreadEventData());
+        this.emit(THREAD_EVENT.save, thread.getThreadEventData());
         return thread;
     }
 

src/__tests__/AnnotationThread-test.js

@@ -57,15 +57,6 @@ describe('AnnotationThread', () => {
         it('should unbind listeners and remove thread element and broadcast that the thread was deleted', () => {
             thread.destroy();
             expect(thread.unbindDOMListeners).toBeCalled();
-            expect(thread.emit).not.toBeCalledWith(THREAD_EVENT.threadDelete);
-            expect(thread.unmountPopover).toBeCalled();
-        });
-
-        it('should emit annotationthreaddeleted only if thread is not in a pending state', () => {
-            thread.state = STATES.inactive;
-            thread.destroy();
-            expect(thread.unbindDOMListeners).toBeCalled();
-            expect(thread.emit).toBeCalledWith(THREAD_EVENT.threadDelete);
             expect(thread.unmountPopover).toBeCalled();
         });
     });

src/__tests__/Annotator-test.js

@@ -486,7 +486,7 @@ describe('Annotator', () => {
 
                 expect(thread.state).toEqual(STATES.active);
                 expect(thread.save).toBeCalledWith(TYPES.point, 'text');
-                expect(annotator.emit).toBeCalledWith(THREAD_EVENT.threadSave, expect.any(Object));
+                expect(annotator.emit).toBeCalledWith(THREAD_EVENT.save, expect.any(Object));
                 expect(result).not.toBeNull();
                 expect(thread.renderAnnotationPopover).toBeCalled();
             });

src/constants.js

@@ -195,8 +195,6 @@ export const ANNOTATOR_EVENT = {
 
 export const THREAD_EVENT = {
     pending: 'annotationpending',
-    threadSave: 'annotationthreadsaved',
-    threadDelete: 'annotationthreaddeleted',
     render: 'annotationrender',
     save: 'annotationsaved',
     delete: 'annotationdeleted',

src/controllers/AnnotationModeController.js

@@ -90,9 +90,6 @@ class AnnotationModeController extends EventEmitter {
     constructor(annotatorType: string): void {
         super();
         this.annotatorType = annotatorType;
-
-        // $FlowFixMe
-        this.unregisterThread = this.unregisterThread.bind(this);
     }
 
     /**
@@ -136,7 +133,8 @@ class AnnotationModeController extends EventEmitter {
     destroy(): void {
         Object.keys(this.annotations).forEach((pageNum) => {
             const pageThreads = this.annotations[pageNum].all() || [];
-            pageThreads.forEach(this.unregisterThread);
+            pageThreads.forEach((thread) => thread.removeListener('threadevent', this.handleThreadEvents));
+            this.annotations[pageNum].clear();
         });
 
         if (this.buttonEl) {
@@ -365,10 +363,21 @@ class AnnotationModeController extends EventEmitter {
         return thread;
     }
 
+    /**
+     * Custom comparator function to remove annotations from the rbush
+     *
+     * @param {DrawingThread} currentAnnotation current annotation in rbush
+     * @param {DrawingThread} annotationToRemove annotation which is being unregistered
+     * @return {boolean} Whether or not the
+     */
+    deleteAnnotation(currentAnnotation: DrawingThread, annotationToRemove: DrawingThread): boolean {
+        return currentAnnotation.id === annotationToRemove.id;
+    }
+
     /**
      * Unregister a previously registered thread
      *
-     * 
+     *
      * @param {AnnotationThread} thread - The thread to unregister with the controller
      * @param {Object} annotation - The annotation with comments to register with the controller
      * @return {void}
@@ -379,6 +388,9 @@ class AnnotationModeController extends EventEmitter {
         }
 
         this.annotations[thread.location.page].remove(thread);
+        if (this.annotations[thread.location.page].data.children.length === 0) {
+            delete this.annotations[thread.location.page];
+        }
         this.emit(CONTROLLER_EVENT.unregister, thread);
         thread.removeListener('threadevent', this.handleThreadEvents);
     }
@@ -461,7 +473,7 @@ class AnnotationModeController extends EventEmitter {
 
     /**
      * Clean up any selected annotations
-     * 
+     *
      * @return {void}
      */
     removeSelection(): void {}
@@ -500,22 +512,18 @@ class AnnotationModeController extends EventEmitter {
                 this.visibleThreadID = null;
                 break;
             case THREAD_EVENT.render:
-                if (eventData) {
-                    this.renderPage(eventData);
+                if (eventData && eventData.page) {
+                    this.renderPage(eventData.page);
                 } else {
                     this.render();
                 }
-                
+
                 break;
             case THREAD_EVENT.delete:
                 // Thread should be cleaned up, unbind listeners - we
                 // don't do this in annotationdelete listener since thread
                 // may still need to respond to error messages
                 this.unregisterThread(thread);
-                break;
-            case THREAD_EVENT.threadDelete:
-                // Thread was deleted, remove from thread map
-                this.unregisterThread(thread);
                 this.emit(event, threadData);
                 break;
             case THREAD_EVENT.deleteError:

src/controllers/DrawingModeController.js

@@ -132,7 +132,11 @@ class DrawingModeController extends AnnotationModeController {
      * @return {void}
      */
     postDrawing(): void {
-        if (this.currentThread && this.currentThread.state === STATES.pending) {
+        if (
+            this.currentThread &&
+            this.currentThread.state === STATES.pending &&
+            this.currentThread.pathContainer.length > 0
+        ) {
             this.currentThread.save(TYPES.draw);
         }
 
@@ -292,7 +296,6 @@ class DrawingModeController extends AnnotationModeController {
 
                 this.currentThread = undefined;
                 this.selectedThread = thread;
-                this.registerThread(thread);
                 this.unbindListeners();
 
                 // Clear existing canvases

src/controllers/__tests__/AnnotationModeController-test.js

@@ -379,10 +379,9 @@ describe('controllers/AnnotationModeController', () => {
 
             it('should internally keep track of the registered thread', () => {
                 // eslint-disable-next-line new-cap
-                const pageThreads = {
-                    all: jest.fn().mockReturnValue([thread]),
-                    remove: jest.fn()
-                };
+                const pageThreads = new rbush();
+                pageThreads.all = jest.fn().mockReturnValue([thread]);
+                pageThreads.remove = jest.fn();
 
                 controller.annotations = { 1: pageThreads };
 
@@ -468,19 +467,13 @@ describe('controllers/AnnotationModeController', () => {
             });
 
             it('should re-render the annotations on render', () => {
-                controller.handleThreadEvents(thread, { event: THREAD_EVENT.render, eventData: 1 });
+                controller.handleThreadEvents(thread, { event: THREAD_EVENT.render, eventData: { page: 1 } });
                 expect(controller.renderPage).toBeCalled();
 
                 controller.handleThreadEvents(thread, { event: THREAD_EVENT.render });
                 expect(controller.render).toBeCalled();
             });
 
-            it('should unregister thread on threadDelete', () => {
-                controller.handleThreadEvents(thread, { event: THREAD_EVENT.threadDelete, data: {} });
-                expect(controller.unregisterThread).toBeCalled();
-                expect(controller.emit).toBeCalledWith(THREAD_EVENT.threadDelete, expect.any(Object));
-            });
-
             it('should unregister thread on deleteError', () => {
                 controller.handleThreadEvents(thread, { event: THREAD_EVENT.deleteError, data: {} });
                 expect(controller.emit).toBeCalledWith(CONTROLLER_EVENT.error, controller.localized.deleteError);

src/controllers/__tests__/DrawingModeController-test.js

@@ -287,7 +287,6 @@ describe('controllers/DrawingModeController', () => {
             });
             expect(controller.unbindListeners).toBeCalled();
             expect(controller.bindListeners).toBeCalled();
-            expect(controller.registerThread).toBeCalled();
             expect(controller.currentThread).toBeUndefined();
             expect(thread.handleStart).not.toBeCalled();
             expect(thread.removeListener).toBeCalledWith('threadevent', expect.any(Function));

src/doc/DocAnnotator.js

@@ -133,7 +133,7 @@ class DocAnnotator extends Annotator {
 
         if (annotationType === TYPES.point) {
             let clientEvent = event;
-            
+
             // $FlowFixMe
             if (this.hasTouch && event.targetTouches) {
                 if (event.targetTouches.length <= 0) {
@@ -383,7 +383,7 @@ class DocAnnotator extends Annotator {
 
     /**
      * Handles click events when not in an annotation mode
-     * 
+     *
      * @param {Event} event - Mouse event
      * @return {void}
      */
@@ -428,9 +428,9 @@ class DocAnnotator extends Annotator {
     };
 
     /**
-     * Hides the create highlight dialog 
-     * 
-     * @param {Event} event - Mouse event 
+     * Hides the create highlight dialog
+     *
+     * @param {Event} event - Mouse event
      * @return {void}
      */
     hideCreateDialog(event: ?Event) {
@@ -513,7 +513,7 @@ class DocAnnotator extends Annotator {
 
         thread.state = STATES.active;
         thread.save(highlightType, commentText);
-        this.emit(THREAD_EVENT.threadSave, thread.getThreadEventData());
+        this.emit(THREAD_EVENT.save, thread.getThreadEventData());
         return thread;
     }
 
@@ -576,7 +576,7 @@ class DocAnnotator extends Annotator {
         }
 
         let mouseEvent = event;
-            
+
         // $FlowFixMe
         if (this.hasTouch && event.targetTouches) {
             mouseEvent = event.targetTouches[0];
@@ -631,7 +631,7 @@ class DocAnnotator extends Annotator {
      */
     highlightMousedownHandler = (event: Event) => {
         this.mouseDownEvent = event;
-            
+
         // $FlowFixMe
         if (this.hasTouch && event.targetTouches) {
             this.mouseDownEvent = event.targetTouches[0];
@@ -668,7 +668,7 @@ class DocAnnotator extends Annotator {
             }
             return isPending;
         });
-            
+
         // $FlowFixMe
         return isPending || this.createHighlightDialog.isVisible;
     }
@@ -693,7 +693,7 @@ class DocAnnotator extends Annotator {
         }
 
         let mouseUpEvent = event;
-            
+
         // $FlowFixMe
         if (this.hasTouch && event.targetTouches) {
             mouseUpEvent = event.targetTouches[0];
@@ -753,7 +753,7 @@ class DocAnnotator extends Annotator {
         if (!this.plainHighlightEnabled && !this.commentHighlightEnabled) {
             return false;
         }
-            
+
         // $FlowFixMe
         if (this.createHighlightDialog.isVisible) {
             return true;