Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Chore: Store annotation threads by threadID in the thread map #316

Merged
merged 16 commits into from
Sep 5, 2017
Merged

Chore: Store annotation threads by threadID in the thread map #316

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
0967d5b
Chore: Store annotation threads by threadID in the thread map
pramodsum Aug 17, 2017
43ebc0c
Chore: unit tests
pramodsum Aug 17, 2017
4252e64
Chore: Actually making the Annotator threadMap a map
pramodsum Aug 23, 2017
e8ad909
Chore: unit tests + removing unused docAnnotator.isInDialogOnPage()
pramodsum Aug 24, 2017
b6c7218
Chore: Moving getThreadsOnPage() to Annotator.js
pramodsum Aug 24, 2017
cba44e4
Chore: Maps aren't supported everywhere unfortunately
pramodsum Aug 31, 2017
810b673
Chore: Fixing unit tests
pramodsum Aug 31, 2017
45f931f
Merge branch 'master' into annotations
pramodsum Aug 31, 2017
89c8610
Chore: Fix merge conflicts
pramodsum Aug 31, 2017
7d62852
Merge branch 'master' into annotations
pramodsum Aug 31, 2017
ae12fa4
Merge branch 'master' into annotations
pramodsum Sep 1, 2017
b6f9751
Merge branch 'master' into annotations
pramodsum Sep 1, 2017
b1e8d12
Chore: PR fixes
pramodsum Sep 2, 2017
57c919d
Merge branch 'master' into annotations
pramodsum Sep 2, 2017
4a7ff2f
Merge branch 'master' into annotations
pramodsum Sep 3, 2017
100674a
Chore: fix
pramodsum Sep 3, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 58 additions & 30 deletions src/lib/annotations/Annotator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,8 @@ class Annotator extends EventEmitter {
this.unbindModeListeners();

if (this.threads) {
Object.keys(this.threads).forEach((page) => {
this.threads[page].forEach((thread) => {
this.threads.forEach((pageThreads) => {
pageThreads.forEach((thread) => {
this.unbindCustomListenersOnThread(thread);
});
});
Expand Down Expand Up @@ -234,8 +234,8 @@ class Annotator extends EventEmitter {
* @return {void}
*/
hideAnnotations() {
Object.keys(this.threads).forEach((page) => {
this.threads[page].forEach((thread) => {
this.threads.forEach((pageThreads) => {
pageThreads.forEach((thread) => {
thread.hide();
});
});
Expand All @@ -248,11 +248,18 @@ class Annotator extends EventEmitter {
* @return {void}
*/
hideAnnotationsOnPage(pageNum) {
if (this.threads[pageNum]) {
this.threads[pageNum].forEach((thread) => {
thread.hide();
});
if (!this.threads) {
return;
}

const pageThreads = this.threads.get(pageNum);
if (!pageThreads) {
return;
}

pageThreads.forEach((thread) => {
thread.hide();
});
}

/**
Expand All @@ -262,8 +269,8 @@ class Annotator extends EventEmitter {
* @return {void}
*/
renderAnnotations() {
Object.keys(this.threads).forEach((page) => {
this.threads[page].forEach((thread) => {
this.threads.forEach((pageThreads) => {
pageThreads.forEach((thread) => {
thread.show();
});
});
Expand All @@ -277,11 +284,18 @@ class Annotator extends EventEmitter {
* @return {void}
*/
renderAnnotationsOnPage(pageNum) {
if (this.threads && this.threads[pageNum]) {
this.threads[pageNum].forEach((thread) => {
thread.show();
});
if (!this.threads) {
return;
}

const pageThreads = this.threads.get(pageNum);
if (!pageThreads) {
return;
}

pageThreads.forEach((thread) => {
thread.show();
});
}

/**
Expand Down Expand Up @@ -508,7 +522,7 @@ class Annotator extends EventEmitter {
*/
setupAnnotations() {
// Map of page => [threads on page]
this.threads = {};
this.threads = new Map();
this.bindDOMListeners();
this.bindCustomListenersOnService(this.annotationService);
this.addListener('scaleAnnotations', this.scaleAnnotations);
Expand All @@ -522,7 +536,7 @@ class Annotator extends EventEmitter {
* @return {Promise} Promise for fetching saved annotations
*/
fetchAnnotations() {
this.threads = {};
this.threads = new Map();

return this.annotationService.getThreadMap(this.fileVersionId).then((threadMap) => {
// Generate map of page to threads
Expand Down Expand Up @@ -639,14 +653,7 @@ class Annotator extends EventEmitter {

// Thread was deleted, remove from thread map
thread.addListener('threaddeleted', () => {
const page = thread.location.page || 1;

// Remove from map
if (this.threads[page] instanceof Array) {
this.threads[page] = this.threads[page].filter(
(searchThread) => searchThread.threadID !== thread.threadID
);
}
this.removeThreadFromMap(thread);
});

// Thread should be cleaned up, unbind listeners - we don't do this
Expand Down Expand Up @@ -855,8 +862,29 @@ class Annotator extends EventEmitter {
addThreadToMap(thread) {
// Add thread to in-memory map
const page = thread.location.page || 1; // Defaults to page 1 if thread has no page
this.threads[page] = this.threads[page] || [];
this.threads[page].push(thread);
let pageThreads = this.threads.get(page);
if (!pageThreads) {
this.threads.set(page, new Map());
pageThreads = this.threads.get(page);
}
pageThreads.set(thread.threadID, thread);
}

/**
* Removes thread to in-memory map.
*
* @protected
* @param {AnnotationThread} thread - Thread to bind events to
* @return {void}
*/
removeThreadFromMap(thread) {
const page = thread.location.page || 1;

// Remove from map
const pageThreads = this.threads.get(page);
if (pageThreads) {
pageThreads.delete(thread.threadID);
}
}

/**
Expand Down Expand Up @@ -895,11 +923,11 @@ class Annotator extends EventEmitter {
*/
destroyPendingThreads() {
let hasPendingThreads = false;
Object.keys(this.threads).forEach((page) => {
this.threads[page].forEach((pendingThread) => {
if (annotatorUtil.isPending(pendingThread.state)) {
this.threads.forEach((pageThreads) => {
pageThreads.forEach((thread) => {
if (annotatorUtil.isPending(thread.state)) {
hasPendingThreads = true;
pendingThread.destroy();
thread.destroy();
}
});
});
Expand Down
81 changes: 45 additions & 36 deletions src/lib/annotations/__tests__/Annotator-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,10 @@ describe('lib/annotations/Annotator', () => {
options,
modeButtons: {}
});
annotator.threads = new Map();

stubs.thread = {
threadID: '123abc',
show: () => {},
hide: () => {},
addListener: () => {},
Expand All @@ -56,6 +58,7 @@ describe('lib/annotations/Annotator', () => {
stubs.threadMock = sandbox.mock(stubs.thread);

stubs.thread2 = {
threadID: '456def',
show: () => {},
hide: () => {},
addListener: () => {},
Expand All @@ -66,6 +69,7 @@ describe('lib/annotations/Annotator', () => {
stubs.threadMock2 = sandbox.mock(stubs.thread2);

stubs.thread3 = {
threadID: '789ghi',
show: () => {},
hide: () => {},
addListener: () => {},
Expand All @@ -88,9 +92,8 @@ describe('lib/annotations/Annotator', () => {

describe('destroy()', () => {
it('should unbind custom listeners on thread and unbind DOM listeners', () => {
annotator.threads = {
1: [stubs.thread]
};
stubs.thread.location = { page: 1 };
annotator.addThreadToMap(stubs.thread);

const unbindCustomStub = sandbox.stub(annotator, 'unbindCustomListenersOnThread');
const unbindDOMStub = sandbox.stub(annotator, 'unbindDOMListeners');
Expand Down Expand Up @@ -178,7 +181,7 @@ describe('lib/annotations/Annotator', () => {

annotator.setupAnnotations();

expect(Object.keys(annotator.threads).length === 0).to.be.true;
expect(annotator.threads.size).equals(0);
expect(annotator.bindDOMListeners).to.be.called;
expect(annotator.bindCustomListenersOnService).to.be.called;
expect(annotator.addListener).to.be.calledWith('scaleAnnotations', sinon.match.func);
Expand All @@ -193,10 +196,12 @@ describe('lib/annotations/Annotator', () => {
sandbox.stub(annotator, 'setupAnnotations');
sandbox.stub(annotator, 'showAnnotations');

annotator.threads = {
1: [stubs.thread],
2: [stubs.thread2, stubs.thread3]
};
stubs.thread.location = { page: 1 };
stubs.thread2.location = { page: 2 };
stubs.thread3.location = { page: 2 };
annotator.addThreadToMap(stubs.thread);
annotator.addThreadToMap(stubs.thread2);
annotator.addThreadToMap(stubs.thread3);

annotator.init();
});
Expand All @@ -215,7 +220,7 @@ describe('lib/annotations/Annotator', () => {
stubs.threadMock.expects('hide');
stubs.threadMock2.expects('hide').never();
stubs.threadMock3.expects('hide').never();
annotator.hideAnnotationsOnPage('1');
annotator.hideAnnotationsOnPage(1);
});
});

Expand All @@ -230,13 +235,10 @@ describe('lib/annotations/Annotator', () => {

describe('renderAnnotationsOnPage()', () => {
it('should call show on each thread', () => {
stubs.thread2.location = { page: 2 };
stubs.thread3.location = { page: 2 };

stubs.threadMock.expects('show');
stubs.threadMock2.expects('show').never();
stubs.threadMock3.expects('show').never();
annotator.renderAnnotationsOnPage('1');
annotator.renderAnnotationsOnPage(1);
});
});

Expand Down Expand Up @@ -432,7 +434,7 @@ describe('lib/annotations/Annotator', () => {

const result = annotator.fetchAnnotations();
return stubs.threadPromise.then(() => {
expect(Object.keys(annotator.threads).length === 0).to.be.true;
expect(annotator.threads.size).equals(0);
expect(annotator.createAnnotationThread).to.be.calledTwice;
expect(annotator.bindCustomListenersOnThread).to.be.calledTwice;
expect(result).to.be.an.object;
Expand Down Expand Up @@ -764,26 +766,32 @@ describe('lib/annotations/Annotator', () => {
});
});

describe('addToThreadMap()', () => {
describe('addThreadToMap()', () => {
it('should add valid threads to the thread map', () => {
stubs.thread.location = { page: 2 };
stubs.thread2.location = { page: 3 };
stubs.thread3.location = { page: 2 };
stubs.thread.location = { page: 1 };
stubs.thread2.location = { page: 1 };

const threadMap = new Map([['456def', stubs.thread2]]);
annotator.threads = new Map([[1, threadMap]]);

annotator.threads = {};
annotator.addThreadToMap(stubs.thread);

expect(annotator.threads).to.deep.equal({
2: [stubs.thread]
});
const pageThreads = annotator.threads.get(1);
expect(pageThreads.has(stubs.thread.threadID)).to.be.true;
});
});

describe('removeThreadFromMap()', () => {
it('should remove a valid thread from the thread map', () => {
stubs.thread.location = { page: 1 };
stubs.thread2.location = { page: 1 };
annotator.addThreadToMap(stubs.thread);
annotator.addThreadToMap(stubs.thread2);
annotator.addThreadToMap(stubs.thread3);

expect(annotator.threads).to.deep.equal({
2: [stubs.thread, stubs.thread3],
3: [stubs.thread2]
});
annotator.removeThreadFromMap(stubs.thread);
const pageThreads = annotator.threads.get(1);
expect(pageThreads.has(stubs.thread.threadID)).to.be.false;
expect(pageThreads.has(stubs.thread2.threadID)).to.be.true;
});
});

Expand Down Expand Up @@ -816,6 +824,7 @@ describe('lib/annotations/Annotator', () => {
describe('destroyPendingThreads()', () => {
beforeEach(() => {
stubs.thread = {
threadID: '123abc',
location: { page: 2 },
type: 'type',
state: STATES.pending,
Expand All @@ -824,27 +833,29 @@ describe('lib/annotations/Annotator', () => {
removeAllListeners: () => {}
};
stubs.threadMock = sandbox.mock(stubs.thread);
stubs.isPending = sandbox.stub(annotatorUtil, 'isPending').returns(false);
stubs.isPending.withArgs(STATES.pending).returns(true);

annotator.addThreadToMap(stubs.thread);
annotator.init();
});

it('should destroy and return true if there are any pending threads', () => {
annotator.init();
annotator.addThreadToMap(stubs.thread);
stubs.threadMock.expects('destroy');
const destroyed = annotator.destroyPendingThreads();
expect(destroyed).to.equal(true);
});

it('should not destroy and return false if there are no threads', () => {
annotator.init();
annotator.threads = new Map();
stubs.threadMock.expects('destroy').never();
stubs.isPending.returns(false);
const destroyed = annotator.destroyPendingThreads();
expect(destroyed).to.equal(false);
});

it('should not destroy and return false if the threads are not pending', () => {
stubs.thread.state = 'NOT_PENDING';
annotator.init();
annotator.addThreadToMap(stubs.thread);
stubs.threadMock.expects('destroy').never();
const destroyed = annotator.destroyPendingThreads();
expect(destroyed).to.equal(false);
Expand All @@ -853,17 +864,15 @@ describe('lib/annotations/Annotator', () => {
it('should destroy only pending threads, and return true', () => {
stubs.thread.state = 'NOT_PENDING';
const pendingThread = {
location: { page: 2 },
threadID: '456def',
location: { page: 1 },
type: 'type',
state: STATES.pending,
destroy: () => {},
unbindCustomListenersOnThread: () => {},
removeAllListeners: () => {}
};
stubs.pendingMock = sandbox.mock(pendingThread);

annotator.init();
annotator.addThreadToMap(stubs.thread);
annotator.addThreadToMap(pendingThread);

stubs.threadMock.expects('destroy').never();
Expand Down
Loading