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 12 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
85 changes: 55 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 @@ -76,8 +76,8 @@ class Annotator extends EventEmitter {
this.unbindModeListeners();

if (this.threads) {
Object.keys(this.threads).forEach((page) => {
this.threads[page].forEach((thread) => {
Object.values(this.threads).forEach((pageThreads) => {
Object.values(pageThreads).forEach((thread) => {
this.unbindCustomListenersOnThread(thread);
});
});
Expand Down Expand Up @@ -244,8 +244,8 @@ class Annotator extends EventEmitter {
* @return {void}
*/
hideAnnotations() {
Object.keys(this.threads).forEach((page) => {
this.threads[page].forEach((thread) => {
Object.values(this.threads).forEach((pageThreads) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you follow the same idea for what I mention below, about showAnnotationsOnPage()

Object.values(pageThreads).forEach((thread) => {
thread.hide();
});
});
Expand All @@ -258,11 +258,14 @@ 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.getThreadsOnPage(pageNum);
Object.values(pageThreads).forEach((thread) => {
thread.hide();
});
}

/**
Expand All @@ -272,8 +275,8 @@ class Annotator extends EventEmitter {
* @return {void}
*/
renderAnnotations() {
Object.keys(this.threads).forEach((page) => {
this.threads[page].forEach((thread) => {
Object.values(this.threads).forEach((pageThreads) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use this.renderAnnotationsOnPage()

Object.values(pageThreads).forEach((thread) => {
thread.show();
});
});
Expand All @@ -287,11 +290,14 @@ 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.getThreadsOnPage(pageNum);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be setup so that if it is given a certain value, it renders ALL annotations?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you mean? We are currently rendering all annotations on that page

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad, nvm

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

/**
Expand Down Expand Up @@ -517,7 +523,7 @@ class Annotator extends EventEmitter {
* @return {void}
*/
setupAnnotations() {
// Map of page => [threads on page]
// Map of page => {threads on page}
this.threads = {};
this.bindDOMListeners();
this.bindCustomListenersOnService(this.annotationService);
Expand Down Expand Up @@ -660,14 +666,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 @@ -794,9 +793,21 @@ 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);
const page = thread.location.page || 1; // Defaults to page 1 if thread has no page'
const pageThreads = this.getThreadsOnPage(page);
pageThreads[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;
delete this.threads[page][thread.threadID];
}

/**
Expand Down Expand Up @@ -825,6 +836,19 @@ class Annotator extends EventEmitter {
this.setScale(data.scale);
this.rotateAnnotations(data.rotationAngle, data.pageNum);
}
/**
* Gets threads on page
*
* @private
* @param {number} page - Current page number
* @return {Map|[]} Threads on page
*/
getThreadsOnPage(page) {
if (!(page in this.threads)) {
this.threads[page] = {};
}
return this.threads[page];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Newline after the if!

}

/**
* Destroys pending threads.
Expand All @@ -835,11 +859,12 @@ class Annotator extends EventEmitter {
*/
destroyPendingThreads() {
let hasPendingThreads = false;
Object.keys(this.threads).forEach((page) => {
this.threads[page].forEach((pendingThread) => {
if (annotatorUtil.isPending(pendingThread.state)) {

Object.values(this.threads).forEach((pageThreads) => {
Object.values(pageThreads).forEach((thread) => {
if (annotatorUtil.isPending(thread.state)) {
hasPendingThreads = true;
pendingThread.destroy();
thread.destroy();
}
});
});
Expand Down
100 changes: 64 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 = {};

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).to.not.be.undefined;
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).to.not.be.undefined;
expect(annotator.createAnnotationThread).to.be.calledTwice;
expect(annotator.bindCustomListenersOnThread).to.be.calledTwice;
expect(result).to.be.an.object;
Expand Down Expand Up @@ -713,26 +715,31 @@ 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 };

annotator.threads = {};
const threadMap = { '456def': stubs.thread2 };
annotator.threads = { 1: threadMap };
annotator.addThreadToMap(stubs.thread);

expect(annotator.threads).to.deep.equal({
2: [stubs.thread]
});
const pageThreads = annotator.getThreadsOnPage(1);
expect(pageThreads).to.have.any.keys(stubs.thread.threadID);
});
});

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.getThreadsOnPage(1);
expect(pageThreads).to.not.have.any.keys(stubs.thread.threadID);
expect(pageThreads).to.have.any.keys(stubs.thread2.threadID);
});
});

Expand Down Expand Up @@ -762,9 +769,30 @@ describe('lib/annotations/Annotator', () => {
});
});

describe('getThreadsOnPage()', () => {
it('should add page to threadMap if it does not already exist', () => {
annotator.threads = {
1: 'not empty'
};
const threads = annotator.getThreadsOnPage(2);
expect(threads).to.not.be.undefined;
annotator.threads = {};
});

it('should return an existing page in the threadMap', () => {
annotator.threads = {
1: 'not empty'
};
const threads = annotator.getThreadsOnPage(1);
expect(threads).equals('not empty');
annotator.threads = {};
});
});

describe('destroyPendingThreads()', () => {
beforeEach(() => {
stubs.thread = {
threadID: '123abc',
location: { page: 2 },
type: 'type',
state: STATES.pending,
Expand All @@ -773,27 +801,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 = {};
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 @@ -802,17 +832,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