Fix: Cleanup page thread access methods (#103)

- Threads should be accessed from rbush trees rather than arrays.
box · Jan 29, 2018 · 3922c96 · 3922c96
1 parent c9275ab
commit 3922c96
Show file tree

Hide file tree

Showing 4 changed files with 7 additions and 60 deletions.
diff --git a/src/Annotator.js b/src/Annotator.js
@@ -487,12 +487,11 @@ class Annotator extends EventEmitter {
         }
 
         const controller = this.modeControllers[TYPES.point];
-        const pageThreads = controller.threads[location.page] || {};
-        if (!controller || !pageThreads[pendingThreadID]) {
+        const thread = controller.getThreadByID(pendingThreadID);
+        if (!controller || !thread) {
             return null;
         }
 
-        const thread = pageThreads[pendingThreadID];
         thread.dialog.hasComments = true;
         thread.state = STATES.hover;
         thread.showDialog();

diff --git a/src/__tests__/Annotator-test.js b/src/__tests__/Annotator-test.js
@@ -40,7 +40,8 @@ describe('Annotator', () => {
             getButton: () => {},
             enter: () => {},
             exit: () => {},
-            setupSharedDialog: () => {}
+            setupSharedDialog: () => {},
+            getThreadByID: () => {}
         };
         stubs.controllerMock = sandbox.mock(stubs.controller);
 
@@ -609,7 +610,7 @@ describe('Annotator', () => {
                 stubs.thread.dialog = { postAnnotation: sandbox.stub() };
 
                 annotator.modeControllers = {
-                    'point': { threads: {} }
+                    'point': stubs.controller
                 };
             });
 
@@ -650,6 +651,7 @@ describe('Annotator', () => {
             });
 
             it('should do nothing the thread does not exist in the page specified by lastPointEvent', () => {
+                stubs.controllerMock.expects('getThreadByID').returns(null);
                 const result = annotator.createPointThread({
                     lastPointEvent: {},
                     pendingThreadID: '123abc',
@@ -662,10 +664,7 @@ describe('Annotator', () => {
             it('should create a point annotation thread using lastPointEvent', () => {
                 stubs.threadMock.expects('showDialog');
                 stubs.threadMock.expects('getThreadEventData').returns({});
-
-                annotator.modeControllers['point'].threads = {
-                    1: { '123abc': stubs.thread }
-                };
+                stubs.controllerMock.expects('getThreadByID').returns(stubs.thread);
 
                 const result = annotator.createPointThread({
                     lastPointEvent: {},

diff --git a/src/__tests__/util-test.js b/src/__tests__/util-test.js
@@ -35,8 +35,6 @@ import {
     canLoadAnnotations,
     insertTemplate,
     generateBtn,
-    addThreadToMap,
-    removeThreadFromMap,
     createCommentTextNode,
     clearCanvas
 } from '../util';
@@ -745,24 +743,6 @@ describe('util', () => {
         });
     });
 
-    describe('addThreadToMap()', () => {
-        it('should add thread to in-memory map', () => {
-            const thread = { threadID: '123abc', location: { page: 2 } };
-            const result = addThreadToMap(thread, {});
-            expect(result.page).equals(2);
-            expect(result.pageThreads).to.deep.equal({ '123abc': thread });
-        });
-    });
-
-    describe('removeThreadFromMap()', () => {
-        it('should remove thread from in-memory map', () => {
-            const thread = { threadID: '123abc', location: { page: 2 } };
-            const result = removeThreadFromMap(thread, { '123abc': thread });
-            expect(result.page).equals(2);
-            expect(result.pageThreads).to.deep.equal({});
-        });
-    });
-
     describe('createCommentTextNode()', () => {
 
         it('should add a <br> for each newline', () => {

diff --git a/src/util.js b/src/util.js
@@ -773,37 +773,6 @@ export function canLoadAnnotations(permissions) {
     return !!canAnnotate || !!canViewAllAnnotations || !!canViewOwnAnnotations;
 }
 
-/**
- * Adds thread to in-memory map.
- *
- * @protected
- * @param {AnnotationThread} thread - Thread to add
- * @param {Object} threadMap - Thread map
- * @return {void}
- */
-export function addThreadToMap(thread, threadMap) {
-    // Add thread to in-memory map
-    const page = thread.location.page || 1; // Defaults to page 1 if thread has no page'
-    const pageThreads = threadMap[page] || {};
-    pageThreads[thread.threadID] = thread;
-    return { page, pageThreads };
-}
-
-/**
- * Removes thread to in-memory map.
- *
- * @protected
- * @param {AnnotationThread} thread - Thread to bind events to
- * @param {Object} threadMap - Thread map
- * @return {void}
- */
-export function removeThreadFromMap(thread, threadMap) {
-    const page = thread.location.page || 1; // Defaults to page 1 if thread has no page'
-    const pageThreads = threadMap[page] || {};
-    delete pageThreads[thread.threadID];
-    return { page, pageThreads };
-}
-
 /**
  * Creates a paragraph node that preserves newline characters.
  *