From b6ed2dadad03310d0f58bd9f3d80152ff3108137 Mon Sep 17 00:00:00 2001 From: Sumedha Pramod Date: Tue, 30 Oct 2018 17:15:46 -0700 Subject: [PATCH 1/4] Chore: Cleanup flow types --- src/AnnotationThread.js | 7 ++++--- src/doc/DocHighlightThread.js | 35 ++++++++++++++++++++++------------- src/doc/DocPointThread.js | 1 + 3 files changed, 27 insertions(+), 16 deletions(-) diff --git a/src/AnnotationThread.js b/src/AnnotationThread.js index 90d4ef50d..b9b22a374 100644 --- a/src/AnnotationThread.js +++ b/src/AnnotationThread.js @@ -30,7 +30,7 @@ class AnnotationThread extends EventEmitter { fileVersionId: string; /** @param {Location} */ - location: Location; + location: ?Location; /** @param {string} */ threadID: ?string; @@ -163,6 +163,7 @@ class AnnotationThread extends EventEmitter { getPopoverParent() { return util.shouldDisplayMobileUI(this.container) ? this.container + // $FlowFixMe : util.getPageEl(this.annotatedElement, this.location.page); } @@ -172,7 +173,7 @@ class AnnotationThread extends EventEmitter { * @param {Event} event - Mouse event * @return {void} */ - renderAnnotationPopover(event: Event = null) { + renderAnnotationPopover(event: ?Event = null) { if (event) { event.stopPropagation(); event.preventDefault(); @@ -662,7 +663,7 @@ class AnnotationThread extends EventEmitter { * @param {Object} eventData - Event data * @return {void} */ - emit(event: Event, eventData: Object) { + emit(event: Event, eventData: ?Object) { const threadData = this.getThreadEventData(); super.emit(event, { data: threadData, eventData }); super.emit('threadevent', { event, data: threadData, eventData }); diff --git a/src/doc/DocHighlightThread.js b/src/doc/DocHighlightThread.js index e81577976..2a923cd21 100644 --- a/src/doc/DocHighlightThread.js +++ b/src/doc/DocHighlightThread.js @@ -17,12 +17,11 @@ import { } from '../constants'; class DocHighlightThread extends AnnotationThread { - /** - * Cached page element for the document. - * - * @property {HTMLElement} - */ - pageEl; + /** @property {Location} */ + location: ?Location; + + /** @property {HTMLElement} */ + pageEl: HTMLElement; /** * [constructor] @@ -72,6 +71,8 @@ class DocHighlightThread extends AnnotationThread { */ destroy() { this.threadID = null; + + // $FlowFixMe this.emit(THREAD_EVENT.render, this.location.page); super.destroy(); @@ -108,9 +109,9 @@ class DocHighlightThread extends AnnotationThread { * @param {string} text Text of annotation to save * @return {void} */ - save(type: AnnotationType, text: string) { - super.save(type, text); + save(type: AnnotationType, text: string): Promise { window.getSelection().removeAllRanges(); + return super.save(type, text); } /** @@ -120,14 +121,15 @@ class DocHighlightThread extends AnnotationThread { * @param {boolean} [useServer] Whether or not to delete on server, default true * @return {void} */ - delete(annotation: Object, useServer: boolean = true) { - super.delete(annotation, useServer); + delete(annotation: Object, useServer: boolean = true): Promise { + const promise = super.delete(annotation, useServer); if (!this.threadID) { - return; + return promise; } this.renderAnnotationPopover(); + return promise; } /** @@ -284,6 +286,7 @@ class DocHighlightThread extends AnnotationThread { scrollIntoView() { this.scrollToPage(); + // $FlowFixMe const [yPos] = docUtil.getLowerRightCornerOfLastQuadPoint(this.location.quadPoints); // Adjust scroll to highlight position @@ -311,12 +314,14 @@ class DocHighlightThread extends AnnotationThread { const pageHeight = pageDimensions.height - PAGE_PADDING_TOP - PAGE_PADDING_BOTTOM; const zoomScale = util.getScale(this.annotatedElement); const dimensionScale = util.getDimensionScale( + // $FlowFixMe this.location.dimensions, pageDimensions, zoomScale, PAGE_PADDING_TOP + PAGE_PADDING_BOTTOM ); + // $FlowFixMe this.location.quadPoints.forEach((quadPoint) => { // If needed, scale quad points comparing current dimensions with saved dimensions let scaledQuadPoint = quadPoint; @@ -367,6 +372,7 @@ class DocHighlightThread extends AnnotationThread { const pageTop = pageDimensions.top + PAGE_PADDING_TOP; const zoomScale = util.getScale(this.annotatedElement); const dimensionScale = util.getDimensionScale( + // $FlowFixMe this.location.dimensions, pageDimensions, zoomScale, @@ -392,6 +398,7 @@ class DocHighlightThread extends AnnotationThread { let eventOccurredInHighlight = false; + // $FlowFixMe const points = this.location.quadPoints; const { length } = points; @@ -426,6 +433,7 @@ class DocHighlightThread extends AnnotationThread { */ getPageEl(): HTMLElement { if (!this.pageEl) { + // $FlowFixMe this.pageEl = util.getPageEl(this.annotatedElement, this.location.page); } return this.pageEl; @@ -438,6 +446,7 @@ class DocHighlightThread extends AnnotationThread { * @return {void} */ regenerateBoundary() { + // $FlowFixMe if (!this.location || !this.location.quadPoints) { return; } @@ -520,7 +529,7 @@ class DocHighlightThread extends AnnotationThread { } /** @inheritdoc */ - cleanupAnnotationOnDelete(annotationIDToRemove: number) { + cleanupAnnotationOnDelete(annotationIDToRemove: string) { // Delete matching comment from annotation this.comments = this.comments.filter(({ id }) => id !== annotationIDToRemove); @@ -557,7 +566,7 @@ class DocHighlightThread extends AnnotationThread { } /** @inheritdoc */ - handleThreadSaveError(error: Error, tempAnnotationID: number) { + handleThreadSaveError(error: Error, tempAnnotationID: string) { if (this.type === TYPES.highlight_comment && this.state === STATES.pending) { this.type = TYPES.highlight; this.reset(); diff --git a/src/doc/DocPointThread.js b/src/doc/DocPointThread.js index 1503b920b..416f05eab 100644 --- a/src/doc/DocPointThread.js +++ b/src/doc/DocPointThread.js @@ -21,6 +21,7 @@ class DocPointThread extends AnnotationThread { * @return {void} */ show() { + // $FlowFixMe const pageEl = getPageEl(this.annotatedElement, this.location.page); const [browserX, browserY] = getBrowserCoordinatesFromLocation(this.location, this.annotatedElement); From ebffdd9d2ff275e6e4a91a775705a9da37095a6b Mon Sep 17 00:00:00 2001 From: Sumedha Pramod Date: Tue, 30 Oct 2018 17:15:46 -0700 Subject: [PATCH 2/4] Chore: Cleanup flow types --- src/AnnotationThread.js | 7 ++++--- src/doc/DocHighlightThread.js | 35 ++++++++++++++++++++++------------- src/doc/DocPointThread.js | 1 + 3 files changed, 27 insertions(+), 16 deletions(-) diff --git a/src/AnnotationThread.js b/src/AnnotationThread.js index 90d4ef50d..b9b22a374 100644 --- a/src/AnnotationThread.js +++ b/src/AnnotationThread.js @@ -30,7 +30,7 @@ class AnnotationThread extends EventEmitter { fileVersionId: string; /** @param {Location} */ - location: Location; + location: ?Location; /** @param {string} */ threadID: ?string; @@ -163,6 +163,7 @@ class AnnotationThread extends EventEmitter { getPopoverParent() { return util.shouldDisplayMobileUI(this.container) ? this.container + // $FlowFixMe : util.getPageEl(this.annotatedElement, this.location.page); } @@ -172,7 +173,7 @@ class AnnotationThread extends EventEmitter { * @param {Event} event - Mouse event * @return {void} */ - renderAnnotationPopover(event: Event = null) { + renderAnnotationPopover(event: ?Event = null) { if (event) { event.stopPropagation(); event.preventDefault(); @@ -662,7 +663,7 @@ class AnnotationThread extends EventEmitter { * @param {Object} eventData - Event data * @return {void} */ - emit(event: Event, eventData: Object) { + emit(event: Event, eventData: ?Object) { const threadData = this.getThreadEventData(); super.emit(event, { data: threadData, eventData }); super.emit('threadevent', { event, data: threadData, eventData }); diff --git a/src/doc/DocHighlightThread.js b/src/doc/DocHighlightThread.js index e81577976..2a923cd21 100644 --- a/src/doc/DocHighlightThread.js +++ b/src/doc/DocHighlightThread.js @@ -17,12 +17,11 @@ import { } from '../constants'; class DocHighlightThread extends AnnotationThread { - /** - * Cached page element for the document. - * - * @property {HTMLElement} - */ - pageEl; + /** @property {Location} */ + location: ?Location; + + /** @property {HTMLElement} */ + pageEl: HTMLElement; /** * [constructor] @@ -72,6 +71,8 @@ class DocHighlightThread extends AnnotationThread { */ destroy() { this.threadID = null; + + // $FlowFixMe this.emit(THREAD_EVENT.render, this.location.page); super.destroy(); @@ -108,9 +109,9 @@ class DocHighlightThread extends AnnotationThread { * @param {string} text Text of annotation to save * @return {void} */ - save(type: AnnotationType, text: string) { - super.save(type, text); + save(type: AnnotationType, text: string): Promise { window.getSelection().removeAllRanges(); + return super.save(type, text); } /** @@ -120,14 +121,15 @@ class DocHighlightThread extends AnnotationThread { * @param {boolean} [useServer] Whether or not to delete on server, default true * @return {void} */ - delete(annotation: Object, useServer: boolean = true) { - super.delete(annotation, useServer); + delete(annotation: Object, useServer: boolean = true): Promise { + const promise = super.delete(annotation, useServer); if (!this.threadID) { - return; + return promise; } this.renderAnnotationPopover(); + return promise; } /** @@ -284,6 +286,7 @@ class DocHighlightThread extends AnnotationThread { scrollIntoView() { this.scrollToPage(); + // $FlowFixMe const [yPos] = docUtil.getLowerRightCornerOfLastQuadPoint(this.location.quadPoints); // Adjust scroll to highlight position @@ -311,12 +314,14 @@ class DocHighlightThread extends AnnotationThread { const pageHeight = pageDimensions.height - PAGE_PADDING_TOP - PAGE_PADDING_BOTTOM; const zoomScale = util.getScale(this.annotatedElement); const dimensionScale = util.getDimensionScale( + // $FlowFixMe this.location.dimensions, pageDimensions, zoomScale, PAGE_PADDING_TOP + PAGE_PADDING_BOTTOM ); + // $FlowFixMe this.location.quadPoints.forEach((quadPoint) => { // If needed, scale quad points comparing current dimensions with saved dimensions let scaledQuadPoint = quadPoint; @@ -367,6 +372,7 @@ class DocHighlightThread extends AnnotationThread { const pageTop = pageDimensions.top + PAGE_PADDING_TOP; const zoomScale = util.getScale(this.annotatedElement); const dimensionScale = util.getDimensionScale( + // $FlowFixMe this.location.dimensions, pageDimensions, zoomScale, @@ -392,6 +398,7 @@ class DocHighlightThread extends AnnotationThread { let eventOccurredInHighlight = false; + // $FlowFixMe const points = this.location.quadPoints; const { length } = points; @@ -426,6 +433,7 @@ class DocHighlightThread extends AnnotationThread { */ getPageEl(): HTMLElement { if (!this.pageEl) { + // $FlowFixMe this.pageEl = util.getPageEl(this.annotatedElement, this.location.page); } return this.pageEl; @@ -438,6 +446,7 @@ class DocHighlightThread extends AnnotationThread { * @return {void} */ regenerateBoundary() { + // $FlowFixMe if (!this.location || !this.location.quadPoints) { return; } @@ -520,7 +529,7 @@ class DocHighlightThread extends AnnotationThread { } /** @inheritdoc */ - cleanupAnnotationOnDelete(annotationIDToRemove: number) { + cleanupAnnotationOnDelete(annotationIDToRemove: string) { // Delete matching comment from annotation this.comments = this.comments.filter(({ id }) => id !== annotationIDToRemove); @@ -557,7 +566,7 @@ class DocHighlightThread extends AnnotationThread { } /** @inheritdoc */ - handleThreadSaveError(error: Error, tempAnnotationID: number) { + handleThreadSaveError(error: Error, tempAnnotationID: string) { if (this.type === TYPES.highlight_comment && this.state === STATES.pending) { this.type = TYPES.highlight; this.reset(); diff --git a/src/doc/DocPointThread.js b/src/doc/DocPointThread.js index 1503b920b..416f05eab 100644 --- a/src/doc/DocPointThread.js +++ b/src/doc/DocPointThread.js @@ -21,6 +21,7 @@ class DocPointThread extends AnnotationThread { * @return {void} */ show() { + // $FlowFixMe const pageEl = getPageEl(this.annotatedElement, this.location.page); const [browserX, browserY] = getBrowserCoordinatesFromLocation(this.location, this.annotatedElement); From d39af3e7cfc391cfab99831680d70229ac11c437 Mon Sep 17 00:00:00 2001 From: Sumedha Pramod Date: Tue, 30 Oct 2018 17:54:04 -0700 Subject: [PATCH 3/4] Chore: Handle case where render event doesn't have a page --- src/controllers/AnnotationModeController.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/controllers/AnnotationModeController.js b/src/controllers/AnnotationModeController.js index 2940d9a32..e62da2fb6 100644 --- a/src/controllers/AnnotationModeController.js +++ b/src/controllers/AnnotationModeController.js @@ -500,7 +500,12 @@ class AnnotationModeController extends EventEmitter { this.visibleThreadID = null; break; case THREAD_EVENT.render: - this.renderPage(eventData); + if (eventData) { + this.renderPage(eventData); + } else { + this.render(); + } + break; case THREAD_EVENT.delete: // Thread should be cleaned up, unbind listeners - we From 68c662c8e40d00997ded4af8e8646deb3247e8f3 Mon Sep 17 00:00:00 2001 From: Sumedha Pramod Date: Tue, 30 Oct 2018 18:00:56 -0700 Subject: [PATCH 4/4] Fix: Tests --- .../__tests__/AnnotationModeController-test.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/controllers/__tests__/AnnotationModeController-test.js b/src/controllers/__tests__/AnnotationModeController-test.js index 52c28481b..2847e730d 100644 --- a/src/controllers/__tests__/AnnotationModeController-test.js +++ b/src/controllers/__tests__/AnnotationModeController-test.js @@ -447,6 +447,7 @@ describe('controllers/AnnotationModeController', () => { beforeEach(() => { controller.emit = jest.fn(); controller.renderPage = jest.fn(); + controller.render = jest.fn(); controller.registerThread = jest.fn(); controller.unregisterThread = jest.fn(); controller.localized = { @@ -466,9 +467,12 @@ describe('controllers/AnnotationModeController', () => { expect(controller.hadPendingThreads).toBeFalsy(); }); - it('should re-render the page on render', () => { - controller.handleThreadEvents(thread, { event: THREAD_EVENT.render, data: {} }); + it('should re-render the annotations on render', () => { + controller.handleThreadEvents(thread, { event: THREAD_EVENT.render, eventData: 1 }); expect(controller.renderPage).toBeCalled(); + + controller.handleThreadEvents(thread, { event: THREAD_EVENT.render }); + expect(controller.render).toBeCalled(); }); it('should unregister thread on threadDelete', () => {