From 25c40f7a3fa0a2ab3dcda38c584631072cea970d Mon Sep 17 00:00:00 2001 From: Sumedha Pramod Date: Thu, 1 Nov 2018 18:35:25 -0700 Subject: [PATCH 1/2] Chore: Fix publish script (#275) --- build/publish.sh | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/build/publish.sh b/build/publish.sh index 9d52d7bc6..be80aa4ee 100755 --- a/build/publish.sh +++ b/build/publish.sh @@ -114,6 +114,8 @@ clean_assets() { } build_assets() { + yarn run pre-build; + echo "----------------------------------------------------" echo "Starting babel build for version" $VERSION echo "----------------------------------------------------" @@ -169,13 +171,6 @@ publish_to_npm() { exit 1 fi - if [[ $(git status --porcelain 2>/dev/null| egrep "^(M| M)") != "" ]] ; then - echo "----------------------------------------------------" - echo "Your branch has uncommited files!" - echo "----------------------------------------------------" - exit 1 - fi - echo "----------------------------------------------------" echo "Checking out version" $VERSION echo "----------------------------------------------------" From feedc876ab8cdfa4178bca6f6e396f77ba3136a9 Mon Sep 17 00:00:00 2001 From: Sumedha Pramod Date: Fri, 2 Nov 2018 12:55:28 -0700 Subject: [PATCH 2/2] Fix: Ensure drawings are only registered once with the controller (#278) --- src/AnnotationThread.js | 47 ++++++++++--------- src/Annotator.js | 2 +- src/__tests__/AnnotationThread-test.js | 9 ---- src/__tests__/Annotator-test.js | 2 +- src/constants.js | 2 - src/controllers/AnnotationModeController.js | 34 +++++++++----- src/controllers/DrawingModeController.js | 7 ++- .../AnnotationModeController-test.js | 15 ++---- .../__tests__/DrawingModeController-test.js | 1 - src/doc/DocAnnotator.js | 22 ++++----- src/doc/DocHighlightThread.js | 35 +++++++------- src/doc/__tests__/DocHighlightThread-test.js | 10 +--- src/drawing/DrawingThread.js | 14 ++---- src/drawing/__tests__/DrawingThread-test.js | 1 - 14 files changed, 92 insertions(+), 109 deletions(-) diff --git a/src/AnnotationThread.js b/src/AnnotationThread.js index b9b22a374..decf9144b 100644 --- a/src/AnnotationThread.js +++ b/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 */ diff --git a/src/Annotator.js b/src/Annotator.js index fa57628e0..e739ab7fe 100644 --- a/src/Annotator.js +++ b/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; } diff --git a/src/__tests__/AnnotationThread-test.js b/src/__tests__/AnnotationThread-test.js index d8ffa27e7..b1a85c0da 100644 --- a/src/__tests__/AnnotationThread-test.js +++ b/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(); }); }); diff --git a/src/__tests__/Annotator-test.js b/src/__tests__/Annotator-test.js index 5a6053366..ebab43e23 100644 --- a/src/__tests__/Annotator-test.js +++ b/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(); }); diff --git a/src/constants.js b/src/constants.js index 2184130cc..00e299890 100644 --- a/src/constants.js +++ b/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', diff --git a/src/controllers/AnnotationModeController.js b/src/controllers/AnnotationModeController.js index e62da2fb6..de692882a 100644 --- a/src/controllers/AnnotationModeController.js +++ b/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: diff --git a/src/controllers/DrawingModeController.js b/src/controllers/DrawingModeController.js index 07de46c63..ecff6d792 100644 --- a/src/controllers/DrawingModeController.js +++ b/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 diff --git a/src/controllers/__tests__/AnnotationModeController-test.js b/src/controllers/__tests__/AnnotationModeController-test.js index 2847e730d..9fca01504 100644 --- a/src/controllers/__tests__/AnnotationModeController-test.js +++ b/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); diff --git a/src/controllers/__tests__/DrawingModeController-test.js b/src/controllers/__tests__/DrawingModeController-test.js index 8233b26d1..07d986a29 100644 --- a/src/controllers/__tests__/DrawingModeController-test.js +++ b/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)); diff --git a/src/doc/DocAnnotator.js b/src/doc/DocAnnotator.js index 9010bc3f2..51c46a514 100644 --- a/src/doc/DocAnnotator.js +++ b/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; diff --git a/src/doc/DocHighlightThread.js b/src/doc/DocHighlightThread.js index 2a923cd21..b405cc3a8 100644 --- a/src/doc/DocHighlightThread.js +++ b/src/doc/DocHighlightThread.js @@ -36,6 +36,24 @@ class DocHighlightThread extends AnnotationThread { this.canComment = canComment; } + /** + * [destructor] + * + * @return {void} + */ + destroy() { + this.threadID = null; + + // $FlowFixMe + const { page } = this.location; + this.emit(THREAD_EVENT.render, { page }); + super.destroy(); + + if (this.state === STATES.pending) { + window.getSelection().removeAllRanges(); + } + } + /** * Cancels the first comment on the thread * @@ -64,23 +82,6 @@ class DocHighlightThread extends AnnotationThread { this.cancelFirstComment(); }; - /** - * [destructor] - * - * @return {void} - */ - destroy() { - this.threadID = null; - - // $FlowFixMe - this.emit(THREAD_EVENT.render, this.location.page); - super.destroy(); - - if (this.state === STATES.pending) { - window.getSelection().removeAllRanges(); - } - } - /** * Hides the highlight by cutting out the annotation from context. Note * that if there are any overlapping highlights, this will cut out diff --git a/src/doc/__tests__/DocHighlightThread-test.js b/src/doc/__tests__/DocHighlightThread-test.js index 46336026d..5e1cf2bae 100644 --- a/src/doc/__tests__/DocHighlightThread-test.js +++ b/src/doc/__tests__/DocHighlightThread-test.js @@ -61,18 +61,10 @@ describe('doc/DocHighlightThread', () => { describe('destroy()', () => { it('should destroy the thread', () => { thread.emit = jest.fn(); - thread.state = STATES.pending; - - // This stubs out a parent method by forcing the method we care about - // in the prototype of the prototype of DocHighlightThread (ie - // AnnotationThread's prototype) to be a stub - Object.defineProperty(Object.getPrototypeOf(DocHighlightThread.prototype), 'destroy', { - value: jest.fn() - }); thread.destroy(); expect(thread.element).toBeUndefined(); - expect(thread.emit).toBeCalledWith(THREAD_EVENT.render, 1); + expect(thread.emit).toBeCalledWith(THREAD_EVENT.render, { page: thread.location.page }); }); }); diff --git a/src/drawing/DrawingThread.js b/src/drawing/DrawingThread.js index 805131fe1..8b04f5be8 100644 --- a/src/drawing/DrawingThread.js +++ b/src/drawing/DrawingThread.js @@ -96,13 +96,13 @@ class DrawingThread extends AnnotationThread { window.cancelAnimationFrame(this.lastAnimationRequestId); } - if (this.state !== STATES.pending) { - this.emit(THREAD_EVENT.render, this.location.page); - } - this.unmountPopover(); this.reset(); super.destroy(); + + // $FlowFixMe + const { page } = this.location; + this.emit(THREAD_EVENT.render, { page }); } /** @@ -196,8 +196,6 @@ class DrawingThread extends AnnotationThread { * @return {void} */ deleteThread() { - this.comments.forEach((annotation) => this.delete(annotation)); - // Calculate the bounding rectangle const [x, y, width, height] = this.getBrowserRectangularBoundary(); @@ -285,7 +283,6 @@ class DrawingThread extends AnnotationThread { } } - /** * Sets up the thread state. * @@ -394,7 +391,7 @@ class DrawingThread extends AnnotationThread { Object.assign(this.location, boundaryData); } - + /** @inheritdoc */ regenerateBoundary() { if (!this.location || this.location.boundaryData) { @@ -455,7 +452,6 @@ class DrawingThread extends AnnotationThread { /** @inheritdoc */ cleanupAnnotationOnDelete() { - this.destroy(); this.threadID = null; } } diff --git a/src/drawing/__tests__/DrawingThread-test.js b/src/drawing/__tests__/DrawingThread-test.js index 26043c0d7..9aca08cec 100644 --- a/src/drawing/__tests__/DrawingThread-test.js +++ b/src/drawing/__tests__/DrawingThread-test.js @@ -82,7 +82,6 @@ describe('drawing/DrawingThread', () => { expect(thread.getBrowserRectangularBoundary).toBeCalled(); expect(thread.concreteContext.clearRect).toBeCalled(); expect(thread.clearBoundary).toBeCalled(); - expect(thread.delete).toBeCalledWith({ id: '123abc' }); expect(thread.pathContainer).toEqual(null); }); });