From 2a3076070c4065549a0ed9eaa392a665f7cf8c93 Mon Sep 17 00:00:00 2001 From: Minh Nguyen Date: Wed, 30 Aug 2017 12:38:32 -0700 Subject: [PATCH 1/6] Update: drawing boundary update --- src/lib/annotations/doc/DocDrawingThread.js | 12 ++++++++++-- src/lib/annotations/drawing/DrawingPath.js | 7 +++---- src/lib/annotations/drawing/DrawingThread.js | 6 ++++++ 3 files changed, 19 insertions(+), 6 deletions(-) diff --git a/src/lib/annotations/doc/DocDrawingThread.js b/src/lib/annotations/doc/DocDrawingThread.js index 537ae6516..cc0e4d716 100644 --- a/src/lib/annotations/doc/DocDrawingThread.js +++ b/src/lib/annotations/doc/DocDrawingThread.js @@ -97,8 +97,16 @@ class DocDrawingThread extends DrawingThread { handleStop() { this.drawingFlag = DRAW_STATES.idle; - if (!this.pendingPath || this.pendingPath.isEmpty()) { - return; + if (this.pendingPath && !this.pendingPath.isEmpty()) { + this.pathContainer.insert(this.pendingPath); + this.updateBoundary(this.pendingPath); + this.setBoundary(); + this.emitAvailableActions(); + this.pendingPath = null; + + if (!this.dialog) { + this.createDialog(); + } } this.pathContainer.insert(this.pendingPath); diff --git a/src/lib/annotations/drawing/DrawingPath.js b/src/lib/annotations/drawing/DrawingPath.js index 1829e1504..440202667 100644 --- a/src/lib/annotations/drawing/DrawingPath.js +++ b/src/lib/annotations/drawing/DrawingPath.js @@ -154,11 +154,10 @@ class DrawingPath { */ static extractDrawingInfo(pathA, accumulator) { let paths = accumulator.paths; - const apath = { path: pathA.path }; - if (!paths) { - paths = [apath]; + if (paths) { + paths.push(pathA.path); } else { - paths.push(apath); + paths = [pathA.path]; } return { diff --git a/src/lib/annotations/drawing/DrawingThread.js b/src/lib/annotations/drawing/DrawingThread.js index f7272bfd3..2e05f619b 100644 --- a/src/lib/annotations/drawing/DrawingThread.js +++ b/src/lib/annotations/drawing/DrawingThread.js @@ -192,6 +192,9 @@ class DrawingThread extends AnnotationThread { this.setBoundary(); this.drawBoundary(); this.emitAvailableActions(); + this.updateBoundary(); + this.setBoundary(); + this.drawBoundary(); } } @@ -211,6 +214,9 @@ class DrawingThread extends AnnotationThread { this.setBoundary(); this.drawBoundary(); this.emitAvailableActions(); + this.updateBoundary(); + this.setBoundary(); + this.drawBoundary(); } } From 59f68a6864e8ab2bdd056238a126f37e772fda85 Mon Sep 17 00:00:00 2001 From: Minh Nguyen Date: Wed, 30 Aug 2017 17:48:03 -0700 Subject: [PATCH 2/6] Update: merge from master --- src/lib/annotations/drawing/DrawingPath.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/lib/annotations/drawing/DrawingPath.js b/src/lib/annotations/drawing/DrawingPath.js index 440202667..1829e1504 100644 --- a/src/lib/annotations/drawing/DrawingPath.js +++ b/src/lib/annotations/drawing/DrawingPath.js @@ -154,10 +154,11 @@ class DrawingPath { */ static extractDrawingInfo(pathA, accumulator) { let paths = accumulator.paths; - if (paths) { - paths.push(pathA.path); + const apath = { path: pathA.path }; + if (!paths) { + paths = [apath]; } else { - paths = [pathA.path]; + paths.push(apath); } return { From cf356cb97f4de3ce71949158a5721f5b50606574 Mon Sep 17 00:00:00 2001 From: Minh Nguyen Date: Wed, 30 Aug 2017 18:35:19 -0700 Subject: [PATCH 3/6] Update: tests for drawing boundary on each stroke --- src/lib/annotations/doc/DocDrawingThread.js | 12 ++---------- src/lib/annotations/drawing/DrawingThread.js | 6 ------ 2 files changed, 2 insertions(+), 16 deletions(-) diff --git a/src/lib/annotations/doc/DocDrawingThread.js b/src/lib/annotations/doc/DocDrawingThread.js index cc0e4d716..537ae6516 100644 --- a/src/lib/annotations/doc/DocDrawingThread.js +++ b/src/lib/annotations/doc/DocDrawingThread.js @@ -97,16 +97,8 @@ class DocDrawingThread extends DrawingThread { handleStop() { this.drawingFlag = DRAW_STATES.idle; - if (this.pendingPath && !this.pendingPath.isEmpty()) { - this.pathContainer.insert(this.pendingPath); - this.updateBoundary(this.pendingPath); - this.setBoundary(); - this.emitAvailableActions(); - this.pendingPath = null; - - if (!this.dialog) { - this.createDialog(); - } + if (!this.pendingPath || this.pendingPath.isEmpty()) { + return; } this.pathContainer.insert(this.pendingPath); diff --git a/src/lib/annotations/drawing/DrawingThread.js b/src/lib/annotations/drawing/DrawingThread.js index 2e05f619b..f7272bfd3 100644 --- a/src/lib/annotations/drawing/DrawingThread.js +++ b/src/lib/annotations/drawing/DrawingThread.js @@ -192,9 +192,6 @@ class DrawingThread extends AnnotationThread { this.setBoundary(); this.drawBoundary(); this.emitAvailableActions(); - this.updateBoundary(); - this.setBoundary(); - this.drawBoundary(); } } @@ -214,9 +211,6 @@ class DrawingThread extends AnnotationThread { this.setBoundary(); this.drawBoundary(); this.emitAvailableActions(); - this.updateBoundary(); - this.setBoundary(); - this.drawBoundary(); } } From c18c2959883eee3bf664a3b0f4709a27140b132d Mon Sep 17 00:00:00 2001 From: Minh Nguyen Date: Fri, 1 Sep 2017 21:55:51 -0700 Subject: [PATCH 4/6] Update: drawing dialog --- src/i18n/en-US.properties | 6 ++ .../annotations/AnnotationModeController.js | 7 ++ src/lib/annotations/Annotator.js | 2 - src/lib/annotations/annotationConstants.js | 5 ++ src/lib/annotations/annotatorUtil.js | 19 ++++- src/lib/annotations/doc/DocAnnotator.js | 10 +++ src/lib/annotations/doc/DocDrawingDialog.js | 6 ++ src/lib/annotations/doc/DocDrawingThread.js | 30 +++++-- .../doc/__tests__/DocDrawingThread-test.js | 2 +- .../annotations/drawing/DrawingContainer.js | 11 ++- .../drawing/DrawingModeController.js | 84 ++++++++++++------- src/lib/annotations/drawing/DrawingThread.js | 40 +++++++-- .../__tests__/DrawingModeController-test.js | 2 +- src/lib/icons/draw_delete.svg | 1 + src/lib/icons/draw_save.svg | 1 + src/lib/icons/icons.js | 4 + 16 files changed, 180 insertions(+), 50 deletions(-) create mode 100644 src/lib/icons/draw_delete.svg create mode 100644 src/lib/icons/draw_save.svg diff --git a/src/i18n/en-US.properties b/src/i18n/en-US.properties index 951cf365a..22f978826 100644 --- a/src/i18n/en-US.properties +++ b/src/i18n/en-US.properties @@ -135,6 +135,12 @@ annotation_highlight_toggle=Highlight text annotation_highlight_comment=Add comment to highlighted text # Text for which user made the highlight annotation annotation_who_highlighted={1} highlighted +# Text for which user made the drawn annotation +annotation_who_drew={1} drew +# Accessibilty text for button that soft commits drawing +annotation_draw_save=Save drawing +# Accessibilty text for button that soft deletes drawing +annotation_draw_delete=Delete drawing # Text for when annotations fail to load annotations_load_error=We're sorry, annotations failed to load for this file. # Text for when the annotation can't be created diff --git a/src/lib/annotations/AnnotationModeController.js b/src/lib/annotations/AnnotationModeController.js index f644550c6..08eb0106f 100644 --- a/src/lib/annotations/AnnotationModeController.js +++ b/src/lib/annotations/AnnotationModeController.js @@ -88,6 +88,13 @@ class AnnotationModeController extends EventEmitter { this.threads = this.threads.filter((item) => item !== thread); } + /** + * Clean up the annotation selector + * + * @return {void} + */ + cleanSelector() {} + /** * Binds custom event listeners for a thread. * diff --git a/src/lib/annotations/Annotator.js b/src/lib/annotations/Annotator.js index 70ed8f37c..062e319f7 100644 --- a/src/lib/annotations/Annotator.js +++ b/src/lib/annotations/Annotator.js @@ -413,8 +413,6 @@ class Annotator extends EventEmitter { annotatorUtil.hideElement(postButtonEl); annotatorUtil.hideElement(undoButtonEl); annotatorUtil.hideElement(redoButtonEl); - annotatorUtil.disableElement(undoButtonEl); - annotatorUtil.disableElement(redoButtonEl); } } diff --git a/src/lib/annotations/annotationConstants.js b/src/lib/annotations/annotationConstants.js index 3e5c19425..4b140c90b 100644 --- a/src/lib/annotations/annotationConstants.js +++ b/src/lib/annotations/annotationConstants.js @@ -1,3 +1,5 @@ +export const USER_ANONYMOUS = 'Anonymous'; + export const CLASS_ACTIVE = 'bp-is-active'; export const CLASS_HIDDEN = 'bp-is-hidden'; export const CLASS_INVISIBLE = 'bp-is-invisible'; @@ -32,6 +34,9 @@ export const CLASS_ANNOTATION_BUTTON_DRAW_REDO = 'bp-btn-annotate-draw-redo'; export const CLASS_ANNOTATION_BUTTON_DRAW_POST = 'bp-btn-annotate-draw-post'; export const CLASS_ANNOTATION_BUTTON_DRAW_CANCEL = 'bp-btn-annotate-draw-cancel'; export const CLASS_ANNOTATION_BUTTON_DRAW_ENTER = 'bp-btn-annotate-draw-enter'; +export const CLASS_ANNOTATION_DRAWING_LABEL = 'bp-annotation-drawing-label'; +export const CLASS_ADD_DRAWING_BTN = 'bp-btn-annotate-draw-add'; +export const CLASS_DELETE_DRAWING_BTN = 'bp-btn-annotate-draw-delete'; export const DATA_TYPE_ANNOTATION_DIALOG = 'annotation-dialog'; export const DATA_TYPE_ANNOTATION_INDICATOR = 'annotation-indicator'; diff --git a/src/lib/annotations/annotatorUtil.js b/src/lib/annotations/annotatorUtil.js index d9984da68..fae43d8e1 100644 --- a/src/lib/annotations/annotatorUtil.js +++ b/src/lib/annotations/annotatorUtil.js @@ -439,7 +439,7 @@ export function validateThreadParams(thread) { } /** - * Returns a function that passes a callback a location when given an event + * Returns a function that passes a callback a location when given an event on the document text layer * * @param {Function} locationFunction - The function to get a location from an event * @param {Function} callback - Callback to be called upon receiving an event @@ -448,7 +448,8 @@ export function validateThreadParams(thread) { export function eventToLocationHandler(locationFunction, callback) { return (event) => { const evt = event || window.event; - if (!evt) { + // Do nothing when the target isn't the text layer in case the text layer receives event precedence over buttons + if (!evt || (event.target && event.target.className !== 'textLayer')) { return; } @@ -459,6 +460,20 @@ export function eventToLocationHandler(locationFunction, callback) { }; } +/** + * Call preventDefault and stopPropagation on an event + * + * @param {event} event - Event object to stop event bubbling + * @return {void} + */ +export function prevDefAndStopProp(event) { + if (!event) { + return; + } + + event.preventDefault(); + event.stopPropagation(); +} /** * Create a JSON object containing x/y coordinates and optionally dimensional information * diff --git a/src/lib/annotations/doc/DocAnnotator.js b/src/lib/annotations/doc/DocAnnotator.js index 6f7435627..983bd6ce2 100644 --- a/src/lib/annotations/doc/DocAnnotator.js +++ b/src/lib/annotations/doc/DocAnnotator.js @@ -454,6 +454,10 @@ class DocAnnotator extends Annotator { bindDOMListeners() { super.bindDOMListeners(); + if(!this.canAnnotate) { + return; + } + this.annotatedElement.addEventListener('mouseup', this.highlightMouseupHandler); if (this.hasTouch && this.isMobile) { @@ -485,6 +489,12 @@ class DocAnnotator extends Annotator { this.highlightThrottleHandle = null; } + if (!this.canAnnotate) { + return; + } + + Object.values(this.modeControllers).forEach((controller) => controller.cleanSelector()); + if (this.hasTouch && this.isMobile) { document.removeEventListener('selectionchange', this.onSelectionChange); this.annotatedElement.removeEventListener('touchstart', this.drawingSelectionHandler); diff --git a/src/lib/annotations/doc/DocDrawingDialog.js b/src/lib/annotations/doc/DocDrawingDialog.js index f2f6fb37d..940228c94 100644 --- a/src/lib/annotations/doc/DocDrawingDialog.js +++ b/src/lib/annotations/doc/DocDrawingDialog.js @@ -1,6 +1,8 @@ import AnnotationDialog from '../AnnotationDialog'; class DocDrawingDialog extends AnnotationDialog { + visible = false; + /** * Empty stub to avoid unexpected behavior. * @@ -36,6 +38,10 @@ class DocDrawingDialog extends AnnotationDialog { * @return {void} */ show() {} + + isVisible() { + return this.visible; + } } export default DocDrawingDialog; diff --git a/src/lib/annotations/doc/DocDrawingThread.js b/src/lib/annotations/doc/DocDrawingThread.js index 537ae6516..c28b6bfe0 100644 --- a/src/lib/annotations/doc/DocDrawingThread.js +++ b/src/lib/annotations/doc/DocDrawingThread.js @@ -84,6 +84,10 @@ class DocDrawingThread extends DrawingThread { this.pendingPath = new DrawingPath(); } + if (this.dialog && this.dialog.isVisible()) { + this.dialog.hide(); + } + // Start drawing rendering this.lastAnimationRequestId = window.requestAnimationFrame(this.render); } @@ -134,9 +138,6 @@ class DocDrawingThread extends DrawingThread { * @return {void} */ saveAnnotation(type, text) { - this.emit('annotationevent', { - type: 'drawcommit' - }); this.reset(); // Only make save request to server if there exist paths to save @@ -149,8 +150,7 @@ class DocDrawingThread extends DrawingThread { super.saveAnnotation(type, text); // Move the in-progress drawing to the concrete context - const inProgressCanvas = this.drawingContext.canvas; - this.drawingContext.clearRect(0, 0, inProgressCanvas.width, inProgressCanvas.height); + this.createDialog(); this.show(); } @@ -167,7 +167,7 @@ class DocDrawingThread extends DrawingThread { // Get the annotation layer context to draw with const context = this.selectContext(); - if (this.state === STATES.pending) { + if (this.dialog && this.dialog.isVisible()) { this.drawBoundary(); } @@ -215,7 +215,7 @@ class DocDrawingThread extends DrawingThread { onPageChange(location) { this.handleStop(); this.emit('annotationevent', { - type: 'pagechanged', + type: 'softcommit', location }); } @@ -285,6 +285,10 @@ class DocDrawingThread extends DrawingThread { * @return {void} */ createDialog() { + if (this.dialog) { + this.dialog.destroy(); + } + this.dialog = new DocDrawingDialog({ annotatedElement: this.annotatedElement, container: this.container, @@ -293,6 +297,18 @@ class DocDrawingThread extends DrawingThread { location: this.location, canAnnotate: this.annotationService.canAnnotate }); + + const drawingThread = this; + this.dialog.addListener('save', () => + drawingThread.emit('annotationevent', { + type: 'softcommit' + }) + ); + this.dialog.addListener('delete', () => + drawingThread.emit('annotationevent', { + type: 'dialogdelete' + }) + ); } } diff --git a/src/lib/annotations/doc/__tests__/DocDrawingThread-test.js b/src/lib/annotations/doc/__tests__/DocDrawingThread-test.js index f465dedfa..22fe04aa7 100644 --- a/src/lib/annotations/doc/__tests__/DocDrawingThread-test.js +++ b/src/lib/annotations/doc/__tests__/DocDrawingThread-test.js @@ -158,7 +158,7 @@ describe('lib/annotations/doc/DocDrawingThread', () => { sandbox.stub(docDrawingThread, 'handleStop'); docDrawingThread.addListener('annotationevent', (data) => { expect(docDrawingThread.handleStop).to.be.called; - expect(data.type).to.equal('pagechanged'); + expect(data.type).to.equal('softcommit'); done(); }); diff --git a/src/lib/annotations/drawing/DrawingContainer.js b/src/lib/annotations/drawing/DrawingContainer.js index 965a90398..55400db14 100644 --- a/src/lib/annotations/drawing/DrawingContainer.js +++ b/src/lib/annotations/drawing/DrawingContainer.js @@ -36,7 +36,7 @@ class DrawingContainer { * @return {boolean} Whether or not an undo was done. */ undo() { - if (this.undoStack.length === 0) { + if (this.isUndoEmpty()) { return false; } @@ -60,6 +60,15 @@ class DrawingContainer { return true; } + /** + * Return whether or not there are objects that can be undone + * + * @return {boolean} Whether or not there exists committed items + */ + isUndoEmpty() { + return this.undoStack.length === 0; + } + /** * Retrieve a JSON blob containing the number of undo and redo in each stack. * diff --git a/src/lib/annotations/drawing/DrawingModeController.js b/src/lib/annotations/drawing/DrawingModeController.js index 1f2a57623..8482223ea 100644 --- a/src/lib/annotations/drawing/DrawingModeController.js +++ b/src/lib/annotations/drawing/DrawingModeController.js @@ -3,6 +3,7 @@ import AnnotationModeController from '../AnnotationModeController'; import * as annotatorUtil from '../annotatorUtil'; import { TYPES, + STATES, SELECTOR_ANNOTATION_BUTTON_DRAW_POST, SELECTOR_ANNOTATION_BUTTON_DRAW_UNDO, SELECTOR_ANNOTATION_BUTTON_DRAW_REDO, @@ -95,6 +96,34 @@ class DrawingModeController extends AnnotationModeController { thread.addListener('threaddeleted', () => this.unregisterThread(thread)); } + /** + * Unbind drawing mode listeners. Resets the undo and redo buttons to be disabled if they exist + * + * @inheritdoc + * @protected + * @return {void} + */ + unbindModeListeners() { + super.unbindModeListeners(); + + annotatorUtil.disableElement(this.undoButtonEl); + annotatorUtil.disableElement(this.redoButtonEl); + } + + /** + * Deselect a saved and selected thread + * + * @return {void} + */ + cleanSelector() { + if (!this.selectedThread) { + return; + } + + this.selectedThread.clearBoundary(); + this.selectedThread = undefined; + } + /** * Set up and return the necessary handlers for the annotation mode * @@ -151,19 +180,33 @@ class DrawingModeController extends AnnotationModeController { // Register the thread to the threadmap when a starting location is assigned. Should only occur once. this.annotator.addThreadToMap(thread); break; - case 'drawcommit': - // Upon a commit, remove the listeners on the thread. - // Adding the thread to the Rbush only happens upon a successful save - thread.removeAllListeners('annotationevent'); - break; - case 'pagechanged': - // On page change, save the original thread, create a new thread and + case 'softcommit': + // Save the original thread, create a new thread and // start drawing at the location indicating the page change this.currentThread = undefined; thread.saveAnnotation(TYPES.draw); this.unbindModeListeners(); - this.bindModeListeners(TYPES.draw); - this.currentThread.handleStart(data.location); + this.bindModeListeners(); + + // Given a location (page change) start drawing at the provided location + if (data.location) { + this.currentThread.handleStart(data.location); + } + + break; + case 'dialogdelete': + if (thread.state === STATES.pending) { + // Soft delete, in-progress thread doesn't require a redraw or a delete on the server + // Clear in-progress thread and restart drawing + thread.destroy(); + this.unbindModeListeners(); + this.bindModeListeners(); + } else { + // Redraw any threads that the deleted thread could have been overlapping + thread.deleteThread(); + this.threads.search(thread).forEach((drawingThread) => drawingThread.show()); + } + break; case 'availableactions': this.updateUndoRedoButtonEls(data.undo, data.redo); @@ -202,10 +245,7 @@ class DrawingModeController extends AnnotationModeController { .filter((drawingThread) => drawingThread.location.page === location.page); // Clear boundary on previously selected thread - if (this.selectedThread) { - const canvas = this.selectedThread.drawingContext.canvas; - this.selectedThread.drawingContext.clearRect(0, 0, canvas.width, canvas.height); - } + this.cleanSelector(); // Selected a region with no drawing threads, remove the reference to the previously selected thread if (intersectingThreads.length === 0) { @@ -213,7 +253,7 @@ class DrawingModeController extends AnnotationModeController { return; } - // Randomly select a thread in case there are multiple + // Randomly select a thread in case there are multiple overlapping threads (use canvas hitmap to avoid this) const index = Math.floor(Math.random() * intersectingThreads.length); const selected = intersectingThreads[index]; this.select(selected); @@ -227,20 +267,8 @@ class DrawingModeController extends AnnotationModeController { * @return {void} */ select(selectedDrawingThread) { - if (this.selectedThread && this.selectedThread === selectedDrawingThread) { - // Selected the same thread twice, delete the thread - const toDelete = this.selectedThread; - toDelete.deleteThread(); - - // Redraw any threads that the deleted thread could have been covering - const toRedraw = this.threads.search(toDelete); - toRedraw.forEach((drawingThread) => drawingThread.show()); - this.selectedThread = undefined; - } else { - // Selected the thread for the first time, select the thread (TODO @minhnguyen: show UI on select) - selectedDrawingThread.drawBoundary(); - this.selectedThread = selectedDrawingThread; - } + selectedDrawingThread.drawBoundary(); + this.selectedThread = selectedDrawingThread; } /** diff --git a/src/lib/annotations/drawing/DrawingThread.js b/src/lib/annotations/drawing/DrawingThread.js index f7272bfd3..7e37dd40b 100644 --- a/src/lib/annotations/drawing/DrawingThread.js +++ b/src/lib/annotations/drawing/DrawingThread.js @@ -90,16 +90,16 @@ class DrawingThread extends AnnotationThread { window.cancelAnimationFrame(this.lastAnimationRequestId); } - if (this.drawingContext) { - const canvas = this.drawingContext.canvas; - this.drawingContext.clearRect(0, 0, canvas.width, canvas.height); - } - super.destroy(); this.reset(); this.emit('threadcleanup'); } + reset() { + super.reset(); + + this.clearBoundary(); + } /* eslint-disable no-unused-vars */ /** * Handle a pointer movement @@ -140,7 +140,7 @@ class DrawingThread extends AnnotationThread { // Calculate the bounding rectangle const [x, y, width, height] = this.getBrowserRectangularBoundary(); - // Clear the drawn thread and destroy it + // Clear the drawn thread and its boundary this.concreteContext.clearRect( x - DRAW_BORDER_OFFSET, y + DRAW_BORDER_OFFSET, @@ -148,8 +148,7 @@ class DrawingThread extends AnnotationThread { height - DRAW_BORDER_OFFSET * 2 ); - // Notifies that the thread was destroyed so that observers can react accordingly - this.destroy(); + this.clearBoundary(); } /** @@ -276,6 +275,10 @@ class DrawingThread extends AnnotationThread { */ emitAvailableActions() { const availableActions = this.pathContainer.getNumberOfItems(); + if (this.dialog && availableActions.undoCount === 0) { + this.dialog.hide(); + } + this.emit('annotationevent', { type: 'availableactions', undo: availableActions.undoCount, @@ -307,6 +310,14 @@ class DrawingThread extends AnnotationThread { // Restore context style this.drawingContext.restore(); + + if (this.dialog) { + if (!this.dialog.isVisible() && !this.pathContainer.isUndoEmpty()) { + this.showDialog(); + } + + this.dialog.position(x + width, y); + } } /** @@ -382,6 +393,19 @@ class DrawingThread extends AnnotationThread { * @return {void} */ getBrowserRectangularBoundary() {} + + clearBoundary() { + if (this.drawingContext) { + const canvas = this.drawingContext.canvas; + this.drawingContext.clearRect(0, 0, canvas.width, canvas.height); + } + + if (!this.dialog || !this.dialog.isVisible()) { + return; + } + + this.dialog.hide(); + } } export default DrawingThread; diff --git a/src/lib/annotations/drawing/__tests__/DrawingModeController-test.js b/src/lib/annotations/drawing/__tests__/DrawingModeController-test.js index 886f447f7..23dcb7ee2 100644 --- a/src/lib/annotations/drawing/__tests__/DrawingModeController-test.js +++ b/src/lib/annotations/drawing/__tests__/DrawingModeController-test.js @@ -178,7 +178,7 @@ describe('lib/annotations/drawing/DrawingModeController', () => { handleStart: sandbox.stub() }; const data = { - type: 'pagechanged', + type: 'softcommit', location: 'not empty' }; sandbox.stub(drawingModeController, 'unbindModeListeners'); diff --git a/src/lib/icons/draw_delete.svg b/src/lib/icons/draw_delete.svg new file mode 100644 index 000000000..bb4bfdd3a --- /dev/null +++ b/src/lib/icons/draw_delete.svg @@ -0,0 +1 @@ + \ No newline at end of file diff --git a/src/lib/icons/draw_save.svg b/src/lib/icons/draw_save.svg new file mode 100644 index 000000000..3fd90af6c --- /dev/null +++ b/src/lib/icons/draw_save.svg @@ -0,0 +1 @@ + \ No newline at end of file diff --git a/src/lib/icons/icons.js b/src/lib/icons/icons.js index cc88816de..c969c9bc6 100644 --- a/src/lib/icons/icons.js +++ b/src/lib/icons/icons.js @@ -40,6 +40,8 @@ import FIND_DROP_UP from './arrow_drop_up.svg'; import CLOSE from './close.svg'; import SEARCH from './search.svg'; import PRINT_CHECKMARK from './print_checkmark.svg'; +import DRAW_SAVE from './draw_save.svg'; +import DRAW_DELETE from './draw_delete.svg'; export const ICON_ANNOTATION = ANNOTATION; export const ICON_HIGHLIGHT_COMMENT = ANNOTATION_HIGHLIGHT_COMMENT; @@ -83,3 +85,5 @@ export const ICON_FIND_DROP_UP = FIND_DROP_UP; export const ICON_CLOSE = CLOSE; export const ICON_SEARCH = SEARCH; export const ICON_PRINT_CHECKMARK = PRINT_CHECKMARK; +export const ICON_DRAW_SAVE = DRAW_SAVE; +export const ICON_DRAW_DELETE = DRAW_DELETE; From 2035dfa0f90a897d01ebb28ee901f301b715f527 Mon Sep 17 00:00:00 2001 From: Minh Nguyen Date: Sat, 2 Sep 2017 15:27:15 -0700 Subject: [PATCH 5/6] Update: tests --- src/lib/annotations/Annotator.js | 2 +- .../__tests__/annotatorUtil-test.js | 67 +++++++++----- src/lib/annotations/doc/DocAnnotator.js | 4 +- src/lib/annotations/doc/DocDrawingThread.js | 84 +++++++++-------- .../doc/__tests__/DocAnnotator-test.js | 18 +++- .../doc/__tests__/DocDrawingThread-test.js | 90 ++++++++++++++----- src/lib/annotations/drawing/DrawingThread.js | 18 +++- .../__tests__/DrawingContainer-test.js | 12 +++ .../__tests__/DrawingModeController-test.js | 73 ++++++++++++++- .../drawing/__tests__/DrawingThread-test.js | 45 ++++++++-- 10 files changed, 310 insertions(+), 103 deletions(-) diff --git a/src/lib/annotations/Annotator.js b/src/lib/annotations/Annotator.js index 062e319f7..cfe7f1c19 100644 --- a/src/lib/annotations/Annotator.js +++ b/src/lib/annotations/Annotator.js @@ -541,7 +541,7 @@ class Annotator extends EventEmitter { // Do not load any pre-existing annotations if the user does not have // the correct permissions if (!this.permissions.canViewAllAnnotations || !this.permissions.canViewOwnAnnotations) { - return this.threads; + return Promise.resolve(this.threads); } return this.annotationService.getThreadMap(this.fileVersionId).then((threadMap) => { diff --git a/src/lib/annotations/__tests__/annotatorUtil-test.js b/src/lib/annotations/__tests__/annotatorUtil-test.js index d47b2ccc5..9bbe3ba7c 100644 --- a/src/lib/annotations/__tests__/annotatorUtil-test.js +++ b/src/lib/annotations/__tests__/annotatorUtil-test.js @@ -25,7 +25,8 @@ import { getHeaders, replacePlaceholders, createLocation, - round + round, + prevDefAndStopProp } from '../annotatorUtil'; import { STATES, @@ -36,6 +37,8 @@ import { const DIALOG_WIDTH = 81; +const sandbox = sinon.sandbox.create(); + describe('lib/annotations/annotatorUtil', () => { let childEl; let parentEl; @@ -52,6 +55,7 @@ describe('lib/annotations/annotatorUtil', () => { }); afterEach(() => { + sandbox.verifyAndRestore(); fixture.cleanup(); }); @@ -403,39 +407,54 @@ describe('lib/annotations/annotatorUtil', () => { }); describe('eventToLocationHandler()', () => { - it('should not call the callback when the location is valid', () => { - const annotator = { - isChanged: false - } - const getLocation = ((event) => 'location'); - const callback = ((location) => { - annotator.isChanged = true - }); - const locationHandler = eventToLocationHandler(getLocation, callback); + let getLocation; + let annotator; + let callback; + let locationHandler; + let event; + + beforeEach(() => { + getLocation = ((event) => 'location'); + callback = sandbox.stub(); + locationHandler = eventToLocationHandler(getLocation, callback); + event = { + preventDefault: () => {}, + stopPropagation: () => {} + }; + }); + it('should not call the callback when the location is valid', () => { locationHandler(undefined); - expect(annotator.isChanged).to.be.false; + expect(callback).to.not.be.called; }); it('should call the callback when the location is valid', () => { - const annotator = { - isChanged: false - } - const getLocation = ((event) => 'location'); - const callback = ((location) => { - annotator.isChanged = true - }); - const locationHandler = eventToLocationHandler(getLocation, callback); - const event = { - preventDefault: () => {}, - stopPropagation: () => {} - }; + locationHandler(event); + expect(callback).to.be.calledWith('location'); + }); + it('should do nothing when the target exists and it is not the textLayer', () => { + event.target = { + className: 'button' + }; locationHandler(event); - expect(annotator.isChanged).to.be.true; + expect(callback).to.not.be.called; }); }); + describe('prevDefAndStopProp()', () => { + it('should prevent default and stop propogation on an event', () => { + const event = { + preventDefault: sandbox.stub(), + stopPropagation: sandbox.stub() + }; + + prevDefAndStopProp(event); + expect(event.preventDefault).to.be.called; + expect(event.stopPropagation).to.be.called; + }) + }); + describe('createLocation()', () => { it('should create a location object without dimensions', () => { const location = createLocation(1,2, undefined); diff --git a/src/lib/annotations/doc/DocAnnotator.js b/src/lib/annotations/doc/DocAnnotator.js index 983bd6ce2..e06d54533 100644 --- a/src/lib/annotations/doc/DocAnnotator.js +++ b/src/lib/annotations/doc/DocAnnotator.js @@ -454,7 +454,7 @@ class DocAnnotator extends Annotator { bindDOMListeners() { super.bindDOMListeners(); - if(!this.canAnnotate) { + if (!this.permissions.canAnnotate) { return; } @@ -489,7 +489,7 @@ class DocAnnotator extends Annotator { this.highlightThrottleHandle = null; } - if (!this.canAnnotate) { + if (!this.permissions.canAnnotate) { return; } diff --git a/src/lib/annotations/doc/DocDrawingThread.js b/src/lib/annotations/doc/DocDrawingThread.js index c28b6bfe0..f402475ed 100644 --- a/src/lib/annotations/doc/DocDrawingThread.js +++ b/src/lib/annotations/doc/DocDrawingThread.js @@ -30,6 +30,7 @@ class DocDrawingThread extends DrawingThread { this.onPageChange = this.onPageChange.bind(this); this.reconstructBrowserCoordFromLocation = this.reconstructBrowserCoordFromLocation.bind(this); } + /** * Handle a pointer movement * @@ -141,8 +142,7 @@ class DocDrawingThread extends DrawingThread { this.reset(); // Only make save request to server if there exist paths to save - const { undoCount } = this.pathContainer.getNumberOfItems(); - if (undoCount === 0) { + if (this.pathContainer.isUndoEmpty()) { return; } @@ -183,6 +183,53 @@ class DocDrawingThread extends DrawingThread { this.draw(context, false); } + /** + * Binds custom event listeners for the dialog. + * + * @protected + * @return {void} + */ + bindCustomListenersOnDialog() { + if (!this.dialog) { + return; + } + + this.dialog.addListener('annotationcreate', () => { + this.emit('annotationevent', { + type: 'softcommit' + }); + }); + this.dialog.addListener('annotationdelete', () => { + this.emit('annotationevent', { + type: 'dialogdelete' + }); + }); + } + + /** + * Creates the document drawing annotation dialog for the thread. + * + * @protected + * @override + * @return {void} + */ + createDialog() { + if (this.dialog) { + this.dialog.destroy(); + } + + this.dialog = new DocDrawingDialog({ + annotatedElement: this.annotatedElement, + container: this.container, + annotations: this.annotations, + locale: this.locale, + location: this.location, + canAnnotate: this.annotationService.canAnnotate + }); + + this.bindCustomListenersOnDialog(); + } + /** * Prepare the pending drawing canvas if the scale factor has changed since the last render. Will do nothing if * the thread has not been assigned a page. @@ -277,39 +324,6 @@ class DocDrawingThread extends DrawingThread { return [x1, y1, width, height]; } - - /** - * Creates the document drawing annotation dialog for the thread. - * - * @override - * @return {void} - */ - createDialog() { - if (this.dialog) { - this.dialog.destroy(); - } - - this.dialog = new DocDrawingDialog({ - annotatedElement: this.annotatedElement, - container: this.container, - annotations: this.annotations, - locale: this.locale, - location: this.location, - canAnnotate: this.annotationService.canAnnotate - }); - - const drawingThread = this; - this.dialog.addListener('save', () => - drawingThread.emit('annotationevent', { - type: 'softcommit' - }) - ); - this.dialog.addListener('delete', () => - drawingThread.emit('annotationevent', { - type: 'dialogdelete' - }) - ); - } } export default DocDrawingThread; diff --git a/src/lib/annotations/doc/__tests__/DocAnnotator-test.js b/src/lib/annotations/doc/__tests__/DocAnnotator-test.js index 8a9243048..f0b03fc85 100644 --- a/src/lib/annotations/doc/__tests__/DocAnnotator-test.js +++ b/src/lib/annotations/doc/__tests__/DocAnnotator-test.js @@ -531,7 +531,7 @@ describe('lib/annotations/doc/DocAnnotator', () => { }); it('should bind DOM listeners if user can annotate', () => { - annotator.canAnnotate = true; + annotator.permissions.canAnnotate = true; stubs.elMock.expects('addEventListener').withArgs('mouseup', sinon.match.func); stubs.elMock.expects('addEventListener').withArgs('dblclick', sinon.match.func); @@ -566,7 +566,7 @@ describe('lib/annotations/doc/DocAnnotator', () => { }); it('should unbind DOM listeners if user can annotate', () => { - annotator.canAnnotate = true; + annotator.permissions.canAnnotate = true; stubs.elMock.expects('removeEventListener').withArgs('mouseup', sinon.match.func); stubs.elMock.expects('removeEventListener').withArgs('mousedown', sinon.match.func); @@ -579,7 +579,7 @@ describe('lib/annotations/doc/DocAnnotator', () => { it('should stop and destroy the requestAnimationFrame handle created by getHighlightMousemoveHandler()', () => { const rafHandle = 12; // RAF handles are integers - annotator.canAnnotate = true; + annotator.permissions.canAnnotate = true; annotator.highlightThrottleHandle = rafHandle; sandbox.stub(annotator, 'getHighlightMouseMoveHandler').returns(sandbox.stub()); @@ -602,6 +602,18 @@ describe('lib/annotations/doc/DocAnnotator', () => { expect(docStopListen).to.be.calledWith('selectionchange', sinon.match.func); expect(annotatedElementStopListen).to.be.calledWith('touchstart', sinon.match.func); }); + + it('should tell controllers to clean up selections', () => { + annotator.permissions.canAnnotate = true; + annotator.modeControllers = { + 'test': { + cleanSelector: sandbox.stub() + } + }; + + annotator.unbindDOMListeners(); + expect(annotator.modeControllers['test'].cleanSelector).to.be.called; + }); }); describe('bindCustomListenersOnThread()', () => { diff --git a/src/lib/annotations/doc/__tests__/DocDrawingThread-test.js b/src/lib/annotations/doc/__tests__/DocDrawingThread-test.js index 22fe04aa7..04660f2f0 100644 --- a/src/lib/annotations/doc/__tests__/DocDrawingThread-test.js +++ b/src/lib/annotations/doc/__tests__/DocDrawingThread-test.js @@ -1,6 +1,7 @@ import * as docAnnotatorUtil from '../docAnnotatorUtil'; import * as annotatorUtil from '../../annotatorUtil'; import DocDrawingThread from '../DocDrawingThread'; +import DocDrawingDialog from '../DocDrawingDialog'; import AnnotationThread from '../../AnnotationThread'; import DrawingPath from '../../drawing/DrawingPath'; import { @@ -113,6 +114,7 @@ describe('lib/annotations/doc/DocDrawingThread', () => { expect(docDrawingThread.onPageChange).to.be.called; expect(docDrawingThread.checkAndHandleScaleUpdate).to.not.be.called; }); + }); describe('handleStop()', () => { @@ -145,7 +147,8 @@ describe('lib/annotations/doc/DocDrawingThread', () => { docDrawingThread.dialog = { value: 'non-empty', removeAllListeners: () => {}, - destroy: () => {} + destroy: () => {}, + isVisible: () => false } docDrawingThread.handleStop(); @@ -221,6 +224,7 @@ describe('lib/annotations/doc/DocDrawingThread', () => { beforeEach(() => { stubs.setBoundary = sandbox.stub(docDrawingThread, 'setBoundary'); stubs.show = sandbox.stub(docDrawingThread, 'show'); + stubs.createDialog = sandbox.stub(docDrawingThread, 'createDialog'); Object.defineProperty(AnnotationThread.prototype, 'saveAnnotation', { value: sandbox.stub() }); }); @@ -230,20 +234,14 @@ describe('lib/annotations/doc/DocDrawingThread', () => { it('should clean up without committing when there are no paths to be saved', () => { sandbox.stub(docDrawingThread, 'reset'); - sandbox.stub(docDrawingThread, 'emit'); - sandbox.stub(docDrawingThread.pathContainer, 'getNumberOfItems').returns({ - undoCount: 0, - redoCount: 1 - }); + sandbox.stub(docDrawingThread.pathContainer, 'isUndoEmpty').returns(true); docDrawingThread.saveAnnotation('draw'); - expect(docDrawingThread.pathContainer.getNumberOfItems).to.be.called; + expect(docDrawingThread.pathContainer.isUndoEmpty).to.be.called; expect(AnnotationThread.prototype.saveAnnotation).to.not.be.called; expect(docDrawingThread.reset).to.be.called; - expect(docDrawingThread.emit).to.be.calledWith('annotationevent', { - type: 'drawcommit' - }); expect(stubs.show).to.not.be.called; + expect(stubs.createDialog).to.not.be.called; }); it('should clean up and commit in-progress drawings when there are paths to be saved', () => { @@ -259,17 +257,15 @@ describe('lib/annotations/doc/DocDrawingThread', () => { clearRect: sandbox.stub() }; - sandbox.stub(docDrawingThread.pathContainer, 'getNumberOfItems').returns({ - undoCount: 1, - redoCount: 0 - }); + sandbox.stub(docDrawingThread.pathContainer, 'isUndoEmpty').returns(false); docDrawingThread.saveAnnotation('draw'); - expect(stubs.show).to.be.called; - expect(stubs.setBoundary).to.be.called; - expect(docDrawingThread.pathContainer.getNumberOfItems).to.be.called; + expect(docDrawingThread.pathContainer.isUndoEmpty).to.be.called; expect(docDrawingThread.drawingContext.clearRect).to.be.called; expect(AnnotationThread.prototype.saveAnnotation).to.be.called; + expect(stubs.show).to.be.called; + expect(stubs.setBoundary).to.be.called; + expect(stubs.createDialog).to.be.called; }); }); @@ -317,31 +313,43 @@ describe('lib/annotations/doc/DocDrawingThread', () => { docDrawingThread.pathContainer = { applyToItems: sandbox.stub() }; + + docDrawingThread.annotatedElement = 'annotatedEl'; + docDrawingThread.location = 'loc'; }); it('should do nothing when no element is assigned to the DocDrawingThread', () => { docDrawingThread.annotatedElement = undefined; - docDrawingThread.location = 'loc'; docDrawingThread.show(); expect(docDrawingThread.selectContext).to.not.be.called; }); it('should do nothing when no location is assigned to the DocDrawingThread', () => { - docDrawingThread.annotatedElement = 'annotatedEl'; docDrawingThread.location = undefined; docDrawingThread.show(); expect(docDrawingThread.selectContext).to.not.be.called; }); it('should draw the paths in the thread', () => { - docDrawingThread.annotatedElement = 'annotatedEl'; - docDrawingThread.location = 'loc'; docDrawingThread.state = 'not pending'; - - docDrawingThread.show() + docDrawingThread.show(); expect(docDrawingThread.selectContext).to.be.called; expect(docDrawingThread.draw).to.be.called; }); + + it('should draw the boundary when a dialog exists and is visible', () => { + sandbox.stub(docDrawingThread, 'drawBoundary'); + docDrawingThread.dialog = { + isVisible: sandbox.stub().returns(true), + destroy: () => {}, + removeAllListeners: () => {}, + hide: () => {} + } + + docDrawingThread.show(); + expect(docDrawingThread.dialog.isVisible).to.be.called; + expect(docDrawingThread.drawBoundary).to.be.called; + }) }); describe('selectContext()', () => { @@ -412,4 +420,40 @@ describe('lib/annotations/doc/DocDrawingThread', () => { expect(value).to.deep.equal([5, 5, 45, 40]); }); }); + + describe('createDialog()', () => { + it('should create a new doc drawing dialog', () => { + const existingDialog = { + destroy: sandbox.stub() + }; + + sandbox.stub(docDrawingThread, 'bindCustomListenersOnDialog'); + docDrawingThread.dialog = existingDialog; + docDrawingThread.annotationService = { + canAnnotate: true + }; + + docDrawingThread.createDialog(); + + expect(existingDialog.destroy).to.be.called; + expect(docDrawingThread.dialog instanceof DocDrawingDialog).to.be.truthy; + expect(docDrawingThread.bindCustomListenersOnDialog).to.be.called; + }); + }); + + describe('bindCustomListenersOnDialog', () => { + it('should bind listeners on the dialog', () => { + docDrawingThread.dialog = { + addListener: sandbox.stub(), + removeAllListeners: sandbox.stub(), + hide: sandbox.stub(), + destroy: sandbox.stub(), + isVisible: sandbox.stub() + }; + + docDrawingThread.bindCustomListenersOnDialog(); + expect(docDrawingThread.dialog.addListener).to.be.calledWith('annotationcreate', sinon.match.func); + expect(docDrawingThread.dialog.addListener).to.be.calledWith('annotationdelete', sinon.match.func); + }); + }); }); diff --git a/src/lib/annotations/drawing/DrawingThread.js b/src/lib/annotations/drawing/DrawingThread.js index 7e37dd40b..2b220bcda 100644 --- a/src/lib/annotations/drawing/DrawingThread.js +++ b/src/lib/annotations/drawing/DrawingThread.js @@ -90,6 +90,11 @@ class DrawingThread extends AnnotationThread { window.cancelAnimationFrame(this.lastAnimationRequestId); } + if (this.dialog) { + this.dialog.destroy(); + this.dialog = null; + } + super.destroy(); this.reset(); this.emit('threadcleanup'); @@ -275,10 +280,6 @@ class DrawingThread extends AnnotationThread { */ emitAvailableActions() { const availableActions = this.pathContainer.getNumberOfItems(); - if (this.dialog && availableActions.undoCount === 0) { - this.dialog.hide(); - } - this.emit('annotationevent', { type: 'availableactions', undo: availableActions.undoCount, @@ -383,6 +384,10 @@ class DrawingThread extends AnnotationThread { this.maxX = boundaryData.maxX; this.minY = boundaryData.minY; this.maxY = boundaryData.maxY; + + if (this.dialog && this.pathContainer.isUndoEmpty()) { + this.dialog.hide(); + } } /** @@ -394,6 +399,11 @@ class DrawingThread extends AnnotationThread { */ getBrowserRectangularBoundary() {} + /** + * Clear any drawn boundary and associated dialog + * + * @return {void} + */ clearBoundary() { if (this.drawingContext) { const canvas = this.drawingContext.canvas; diff --git a/src/lib/annotations/drawing/__tests__/DrawingContainer-test.js b/src/lib/annotations/drawing/__tests__/DrawingContainer-test.js index dd297ab64..2dc79040c 100644 --- a/src/lib/annotations/drawing/__tests__/DrawingContainer-test.js +++ b/src/lib/annotations/drawing/__tests__/DrawingContainer-test.js @@ -167,4 +167,16 @@ describe('lib/annotations/drawing/DrawingContainer', () => { }); }); }); + + describe('isUndoEmpty()', () => { + it('should return true when no items are in the undoStack', () => { + drawingContainer.undoStack = []; + expect(drawingContainer.isUndoEmpty()).to.be.truthy; + }); + + it('should return false when there are items are in the undoStack', () => { + drawingContainer.undoStack = ['one']; + expect(drawingContainer.isUndoEmpty()).to.be.falsy; + }); + }); }); diff --git a/src/lib/annotations/drawing/__tests__/DrawingModeController-test.js b/src/lib/annotations/drawing/__tests__/DrawingModeController-test.js index 23dcb7ee2..07f09585f 100644 --- a/src/lib/annotations/drawing/__tests__/DrawingModeController-test.js +++ b/src/lib/annotations/drawing/__tests__/DrawingModeController-test.js @@ -146,6 +146,18 @@ describe('lib/annotations/drawing/DrawingModeController', () => { }); }); + describe('unbindModeListeners()', () => { + it('should disable undo and redo buttons', () => { + sandbox.stub(annotatorUtil, 'disableElement'); + + drawingModeController.undoButtonEl = 'test1'; + drawingModeController.redoButtonEl = 'test2'; + drawingModeController.unbindModeListeners(); + expect(annotatorUtil.disableElement).to.be.calledWith(drawingModeController.undoButtonEl); + expect(annotatorUtil.disableElement).to.be.calledWith(drawingModeController.redoButtonEl); + }); + }); + describe('handleAnnotationEvent()', () => { it('should add thread to map on locationassigned', () => { const thread = 'obj'; @@ -159,15 +171,21 @@ describe('lib/annotations/drawing/DrawingModeController', () => { expect(drawingModeController.annotator.addThreadToMap).to.be.called; }); - it('should remove annotationevent listeners from the thread on drawcommit', () => { + it('should restart mode listeners from the thread on softcommit', () => { const thread = { - removeAllListeners: sandbox.stub() + saveAnnotation: sandbox.stub(), + handleStart: sandbox.stub() }; + sandbox.stub(drawingModeController, 'unbindModeListeners'); + sandbox.stub(drawingModeController, 'bindModeListeners'); drawingModeController.handleAnnotationEvent(thread, { - type: 'drawcommit' + type: 'softcommit' }); - expect(thread.removeAllListeners).to.be.calledWith('annotationevent'); + expect(drawingModeController.unbindModeListeners).to.be.called; + expect(drawingModeController.bindModeListeners).to.be.called; + expect(thread.saveAnnotation).to.be.called; + expect(thread.handleStart).to.not.be.called; }); it('should start a new thread on pagechanged', () => { @@ -204,6 +222,38 @@ describe('lib/annotations/drawing/DrawingModeController', () => { }); expect(drawingModeController.updateUndoRedoButtonEls).to.be.calledWith(1, 2); }); + + it('should soft delete a pending thread and restart mode listeners', () => { + const thread = { + state: 'pending', + destroy: sandbox.stub() + }; + + sandbox.stub(drawingModeController, 'unbindModeListeners'); + sandbox.stub(drawingModeController, 'bindModeListeners'); + drawingModeController.handleAnnotationEvent(thread, { + type: 'dialogdelete' + }); + expect(thread.destroy).to.be.called; + expect(drawingModeController.unbindModeListeners).to.be.called; + expect(drawingModeController.bindModeListeners).to.be.called; + }); + + it('should delete a non-pending thread', () => { + const thread = { + state: 'idle', + deleteThread: sandbox.stub() + }; + drawingModeController.threads = { + search: sandbox.stub().returns([]) + }; + + drawingModeController.handleAnnotationEvent(thread, { + type: 'dialogdelete' + }); + expect(thread.deleteThread).to.be.called; + expect(drawingModeController.threads.search).to.be.called; + }); }); describe('handleSelection()', () => { @@ -221,6 +271,7 @@ describe('lib/annotations/drawing/DrawingModeController', () => { it('should call select on an thread found in the data store', () => { stubs.select = sandbox.stub(drawingModeController, 'select'); + stubs.clean = sandbox.stub(drawingModeController, 'cleanSelector'); stubs.getLoc.returns({ x: 5, y: 5 @@ -237,10 +288,24 @@ describe('lib/annotations/drawing/DrawingModeController', () => { drawingModeController.handleSelection('event'); expect(drawingModeController.threads.search).to.be.called; expect(filterObjects.filter).to.be.called; + expect(stubs.clean).to.be.called; expect(stubs.select).to.be.calledWith(filteredObject); }); }); + describe('cleanSelector()', () => { + it('should clean a selected thread boundary', () => { + const thread = { + clearBoundary: sandbox.stub() + }; + drawingModeController.selectedThread = thread; + + drawingModeController.cleanSelector(); + expect(thread.clearBoundary).to.be.called; + expect(drawingModeController.selectedThread).to.be.undefined; + }); + }); + describe('select()', () => { it('should draw the boundary', () => { const thread = { diff --git a/src/lib/annotations/drawing/__tests__/DrawingThread-test.js b/src/lib/annotations/drawing/__tests__/DrawingThread-test.js index c98ab687b..cd5df6156 100644 --- a/src/lib/annotations/drawing/__tests__/DrawingThread-test.js +++ b/src/lib/annotations/drawing/__tests__/DrawingThread-test.js @@ -58,26 +58,33 @@ describe('lib/annotations/drawing/DrawingThread', () => { expect(window.cancelAnimationFrame).to.be.calledWith(1); expect(drawingThread.reset).to.be.called; - expect(drawingThread.drawingContext.clearRect).to.be.called; }) }); + describe('reset()', () => { + it('should clear the boundary', () => { + sandbox.stub(drawingThread, 'clearBoundary'); + drawingThread.reset(); + expect(drawingThread.clearBoundary).to.be.called; + }); + }) + describe('deleteThread()', () => { it('should delete all attached annotations, clear the drawn rectangle, and call destroy', () => { sandbox.stub(drawingThread, 'getBrowserRectangularBoundary').returns(['a', 'b', 'c', 'd']); - sandbox.stub(drawingThread, 'destroy'); + sandbox.stub(drawingThread, 'deleteAnnotationWithID'); + sandbox.stub(drawingThread, 'clearBoundary'); drawingThread.concreteContext = { clearRect: sandbox.stub() }; - drawingThread.annotations = { - forEach: sandbox.stub() - }; + drawingThread.annotations = ['annotation']; + drawingThread.deleteThread(); expect(drawingThread.getBrowserRectangularBoundary).to.be.called; expect(drawingThread.concreteContext.clearRect).to.be.called; - expect(drawingThread.annotations.forEach).to.be.called; - expect(drawingThread.destroy).to.be.called; + expect(drawingThread.clearBoundary).to.be.called; + expect(drawingThread.deleteAnnotationWithID).to.be.calledWith('annotation'); }); }); @@ -340,4 +347,28 @@ describe('lib/annotations/drawing/DrawingThread', () => { expect(drawingThread.maxY).to.equal(drawingThread.location.maxY); }); }); + + describe('clearBoundary()', () => { + it('should clear the drawing context and hide any dialog', () => { + drawingThread.drawingContext = { + canvas: { + width: 100, + height: 100 + }, + clearRect: sandbox.stub() + }; + + drawingThread.dialog = { + isVisible: sandbox.stub().returns(true), + hide: sandbox.stub(), + destroy: () => {}, + removeAllListeners: () => {} + }; + + drawingThread.clearBoundary(); + expect(drawingThread.drawingContext.clearRect).to.be.called; + expect(drawingThread.dialog.isVisible).to.be.called; + expect(drawingThread.dialog.hide).to.be.called; + }); + }); }); From 84e277f50f54be7e72d45c4ef65bbc1f2bc21a47 Mon Sep 17 00:00:00 2001 From: Minh Nguyen Date: Tue, 5 Sep 2017 12:57:46 -0700 Subject: [PATCH 6/6] Update: PR feedback --- src/lib/annotations/AnnotationModeController.js | 4 ++-- src/lib/annotations/annotatorUtil.js | 1 + src/lib/annotations/doc/DocAnnotator.js | 10 +--------- src/lib/annotations/doc/__tests__/DocAnnotator-test.js | 7 ++++--- src/lib/annotations/drawing/DrawingModeController.js | 4 ++-- .../drawing/__tests__/DrawingModeController-test.js | 6 +++--- 6 files changed, 13 insertions(+), 19 deletions(-) diff --git a/src/lib/annotations/AnnotationModeController.js b/src/lib/annotations/AnnotationModeController.js index 08eb0106f..126a8a2ea 100644 --- a/src/lib/annotations/AnnotationModeController.js +++ b/src/lib/annotations/AnnotationModeController.js @@ -89,11 +89,11 @@ class AnnotationModeController extends EventEmitter { } /** - * Clean up the annotation selector + * Clean up any selected annotations * * @return {void} */ - cleanSelector() {} + removeSelection() {} /** * Binds custom event listeners for a thread. diff --git a/src/lib/annotations/annotatorUtil.js b/src/lib/annotations/annotatorUtil.js index fae43d8e1..b5cf46bb7 100644 --- a/src/lib/annotations/annotatorUtil.js +++ b/src/lib/annotations/annotatorUtil.js @@ -449,6 +449,7 @@ export function eventToLocationHandler(locationFunction, callback) { return (event) => { const evt = event || window.event; // Do nothing when the target isn't the text layer in case the text layer receives event precedence over buttons + // NOTE: Currently only applicable to documents. Need to find a better way to ensure button event precedence. if (!evt || (event.target && event.target.className !== 'textLayer')) { return; } diff --git a/src/lib/annotations/doc/DocAnnotator.js b/src/lib/annotations/doc/DocAnnotator.js index e06d54533..9a955612e 100644 --- a/src/lib/annotations/doc/DocAnnotator.js +++ b/src/lib/annotations/doc/DocAnnotator.js @@ -454,10 +454,6 @@ class DocAnnotator extends Annotator { bindDOMListeners() { super.bindDOMListeners(); - if (!this.permissions.canAnnotate) { - return; - } - this.annotatedElement.addEventListener('mouseup', this.highlightMouseupHandler); if (this.hasTouch && this.isMobile) { @@ -489,11 +485,7 @@ class DocAnnotator extends Annotator { this.highlightThrottleHandle = null; } - if (!this.permissions.canAnnotate) { - return; - } - - Object.values(this.modeControllers).forEach((controller) => controller.cleanSelector()); + Object.values(this.modeControllers).forEach((controller) => controller.removeSelection()); if (this.hasTouch && this.isMobile) { document.removeEventListener('selectionchange', this.onSelectionChange); diff --git a/src/lib/annotations/doc/__tests__/DocAnnotator-test.js b/src/lib/annotations/doc/__tests__/DocAnnotator-test.js index f0b03fc85..15eebdbac 100644 --- a/src/lib/annotations/doc/__tests__/DocAnnotator-test.js +++ b/src/lib/annotations/doc/__tests__/DocAnnotator-test.js @@ -607,12 +607,12 @@ describe('lib/annotations/doc/DocAnnotator', () => { annotator.permissions.canAnnotate = true; annotator.modeControllers = { 'test': { - cleanSelector: sandbox.stub() + removeSelection: sandbox.stub() } }; annotator.unbindDOMListeners(); - expect(annotator.modeControllers['test'].cleanSelector).to.be.called; + expect(annotator.modeControllers['test'].removeSelection).to.be.called; }); }); @@ -1348,7 +1348,8 @@ describe('lib/annotations/doc/DocAnnotator', () => { describe('drawingSelectionHandler()', () => { it('should use the controller to select with the event', () => { const drawController = { - handleSelection: sandbox.stub() + handleSelection: sandbox.stub(), + removeSelection: sandbox.stub() }; annotator.modeControllers = { [TYPES.draw]: drawController diff --git a/src/lib/annotations/drawing/DrawingModeController.js b/src/lib/annotations/drawing/DrawingModeController.js index 8482223ea..b5b69b596 100644 --- a/src/lib/annotations/drawing/DrawingModeController.js +++ b/src/lib/annotations/drawing/DrawingModeController.js @@ -115,7 +115,7 @@ class DrawingModeController extends AnnotationModeController { * * @return {void} */ - cleanSelector() { + removeSelection() { if (!this.selectedThread) { return; } @@ -245,7 +245,7 @@ class DrawingModeController extends AnnotationModeController { .filter((drawingThread) => drawingThread.location.page === location.page); // Clear boundary on previously selected thread - this.cleanSelector(); + this.removeSelection(); // Selected a region with no drawing threads, remove the reference to the previously selected thread if (intersectingThreads.length === 0) { diff --git a/src/lib/annotations/drawing/__tests__/DrawingModeController-test.js b/src/lib/annotations/drawing/__tests__/DrawingModeController-test.js index 07f09585f..62e98f64f 100644 --- a/src/lib/annotations/drawing/__tests__/DrawingModeController-test.js +++ b/src/lib/annotations/drawing/__tests__/DrawingModeController-test.js @@ -271,7 +271,7 @@ describe('lib/annotations/drawing/DrawingModeController', () => { it('should call select on an thread found in the data store', () => { stubs.select = sandbox.stub(drawingModeController, 'select'); - stubs.clean = sandbox.stub(drawingModeController, 'cleanSelector'); + stubs.clean = sandbox.stub(drawingModeController, 'removeSelection'); stubs.getLoc.returns({ x: 5, y: 5 @@ -293,14 +293,14 @@ describe('lib/annotations/drawing/DrawingModeController', () => { }); }); - describe('cleanSelector()', () => { + describe('removeSelection()', () => { it('should clean a selected thread boundary', () => { const thread = { clearBoundary: sandbox.stub() }; drawingModeController.selectedThread = thread; - drawingModeController.cleanSelector(); + drawingModeController.removeSelection(); expect(thread.clearBoundary).to.be.called; expect(drawingModeController.selectedThread).to.be.undefined; });