From 6f319f65afab1021f86c9f8fb820ba8ed7f01eb3 Mon Sep 17 00:00:00 2001 From: Sumedha Pramod Date: Thu, 3 Aug 2017 10:35:39 -0700 Subject: [PATCH] Chore: Cleaning up annotations methods in BaseViewer (#272) --- src/lib/annotations/Annotator.js | 33 +++-- .../annotations/__tests__/Annotator-test.js | 32 +++++ src/lib/annotations/doc/DocAnnotator.js | 4 + src/lib/viewers/BaseViewer.js | 133 ++++++++---------- src/lib/viewers/__tests__/BaseViewer-test.js | 109 ++++++-------- 5 files changed, 158 insertions(+), 153 deletions(-) diff --git a/src/lib/annotations/Annotator.js b/src/lib/annotations/Annotator.js index c3a27284a..d23ab8c54 100644 --- a/src/lib/annotations/Annotator.js +++ b/src/lib/annotations/Annotator.js @@ -414,24 +414,27 @@ class Annotator extends EventEmitter { } /** - * Binds DOM event listeners. No-op here, but can be overridden by any - * annotator that needs to bind event listeners to the DOM in the normal - * state (ie not in any annotation mode). + * Binds DOM event listeners. Can be overridden by any annotator that + * needs to bind event listeners to the DOM in the normal state (ie not + * in any annotation mode). * - * @protected * @return {void} */ - bindDOMListeners() {} + bindDOMListeners() { + this.addListener('scaleAnnotations', this.scaleAnnotations); + } /** - * Unbinds DOM event listeners. No-op here, but can be overridden by any - * annotator that needs to bind event listeners to the DOM in the normal - * state (ie not in any annotation mode). + * Unbinds DOM event listeners. Can be overridden by any annotator that + * needs to bind event listeners to the DOM in the normal state (ie not + * in any annotation mode). * * @protected * @return {void} */ - unbindDOMListeners() {} + unbindDOMListeners() { + this.removeListener('scaleAnnotations', this.scaleAnnotations); + } /** * Binds custom event listeners for the Annotation Service. @@ -707,6 +710,18 @@ class Annotator extends EventEmitter { // Private //-------------------------------------------------------------------------- + /** + * Orient annotations to the correct scale and orientation of the annotated document. + * + * @protected + * @param {Object} data - Scale and orientation values needed to orient annotations. + * @return {void} + */ + scaleAnnotations(data) { + this.setScale(data.scale); + this.rotateAnnotations(data.rotationAngle, data.pageNum); + } + /** * Destroys pending threads. * diff --git a/src/lib/annotations/__tests__/Annotator-test.js b/src/lib/annotations/__tests__/Annotator-test.js index cef6b0d3d..34b473717 100644 --- a/src/lib/annotations/__tests__/Annotator-test.js +++ b/src/lib/annotations/__tests__/Annotator-test.js @@ -380,6 +380,22 @@ describe('lib/annotations/Annotator', () => { }); }); + describe('bindDOMListeners()', () => { + it('should add a listener for scaling the annotator', () => { + sandbox.stub(annotator, 'addListener'); + annotator.bindDOMListeners(); + expect(annotator.addListener).to.be.calledWith('scaleAnnotations', sinon.match.func); + }); + }); + + describe('unbindDOMListeners()', () => { + it('should add a listener for scaling the annotator', () => { + sandbox.stub(annotator, 'removeListener'); + annotator.unbindDOMListeners(); + expect(annotator.removeListener).to.be.calledWith('scaleAnnotations', sinon.match.func); + }); + }); + describe('bindCustomListenersOnService()', () => { it('should do nothing if the service does not exist', () => { annotator.annotationService = { @@ -713,6 +729,22 @@ describe('lib/annotations/Annotator', () => { }); }); + describe('scaleAnnotations()', () => { + it('should set scale and rotate annotations based on the annotated element', () => { + sandbox.stub(annotator, 'setScale'); + sandbox.stub(annotator, 'rotateAnnotations'); + + const data = { + scale: 5, + rotationAngle: 90, + pageNum: 2 + }; + annotator.scaleAnnotations(data); + expect(annotator.setScale).to.be.calledWith(data.scale); + expect(annotator.rotateAnnotations).to.be.calledWith(data.rotationAngle, data.pageNum); + }); + }); + describe('destroyPendingThreads()', () => { beforeEach(() => { stubs.thread = { diff --git a/src/lib/annotations/doc/DocAnnotator.js b/src/lib/annotations/doc/DocAnnotator.js index bb4495947..1a8180b27 100644 --- a/src/lib/annotations/doc/DocAnnotator.js +++ b/src/lib/annotations/doc/DocAnnotator.js @@ -421,6 +421,8 @@ class DocAnnotator extends Annotator { * @return {void} */ bindDOMListeners() { + super.bindDOMListeners(); + this.annotatedElement.addEventListener('mouseup', this.highlightMouseupHandler); if (this.annotationService.canAnnotate) { @@ -439,6 +441,8 @@ class DocAnnotator extends Annotator { * @return {void} */ unbindDOMListeners() { + super.unbindDOMListeners(); + this.annotatedElement.removeEventListener('mouseup', this.highlightMouseupHandler); if (this.annotationService.canAnnotate) { diff --git a/src/lib/viewers/BaseViewer.js b/src/lib/viewers/BaseViewer.js index 43bc46f6d..0adc9ac8c 100644 --- a/src/lib/viewers/BaseViewer.js +++ b/src/lib/viewers/BaseViewer.js @@ -36,6 +36,16 @@ const ANNOTATION_TYPE_DRAW = 'draw'; const ANNOTATION_TYPE_POINT = 'point'; const LOAD_TIMEOUT_MS = 180000; // 3m const RESIZE_WAIT_TIME_IN_MILLIS = 300; +const ANNOTATION_BUTTONS = { + point: { + title: __('annotation_point_toggle'), + selector: SELECTOR_BOX_PREVIEW_BTN_ANNOTATE_POINT + }, + draw: { + title: __('annotation_draw_toggle'), + selector: SELECTOR_BOX_PREVIEW_BTN_ANNOTATE_DRAW + } +}; @autobind class BaseViewer extends EventEmitter { @@ -162,11 +172,13 @@ class BaseViewer extends EventEmitter { const pointAnnotateButtonEl = container.querySelector(SELECTOR_BOX_PREVIEW_BTN_ANNOTATE_POINT); const drawAnnotateButtonEl = container.querySelector(SELECTOR_BOX_PREVIEW_BTN_ANNOTATE_DRAW); if (pointAnnotateButtonEl) { - pointAnnotateButtonEl.removeEventListener('click', this.getPointModeClickHandler); + const handler = this.getAnnotationModeClickHandler('point'); + pointAnnotateButtonEl.removeEventListener('click', handler); } if (drawAnnotateButtonEl) { - drawAnnotateButtonEl.removeEventListener('click', this.drawAnnotateClickHandler); + const handler = this.getAnnotationModeClickHandler('draw'); + drawAnnotateButtonEl.removeEventListener('click', handler); } } @@ -598,6 +610,28 @@ class BaseViewer extends EventEmitter { return status === STATUS_SUCCESS || status === STATUS_VIEWABLE; } + /** + * Disables viewer controls + * + * @return {void} + */ + disableViewerControls() { + if (this.controls) { + this.controls.disable(); + } + } + + /** + * Enables viewer controls + * + * @return {void} + */ + enableViewerControls() { + if (this.controls) { + this.controls.enable(); + } + } + //-------------------------------------------------------------------------- // Annotations //-------------------------------------------------------------------------- @@ -623,26 +657,16 @@ class BaseViewer extends EventEmitter { // Users can currently only view annotations on mobile this.canAnnotate = checkPermission(file, PERMISSION_ANNOTATE); if (this.canAnnotate) { - this.showPointAnnotateButton(this.getAnnotationModeClickHandler('point')); - // Note: Leave drawing annotation code entry disabled for now - // this.showDrawAnnotateButton(this.getAnnotationModeClickHandler('draw')); + // Show the annotate button for all enabled types for the + // current viewer + this.annotatorConf.TYPE.forEach((type) => { + this.showModeAnnotateButton(type, ANNOTATION_BUTTONS); + }); } this.initAnnotations(); } } - /** - * Orient annotations to the correct scale and orientation of the annotated document. - * - * @protected - * @param {Object} data - Scale and orientation values needed to orient annotations. - * @return {void} - */ - scaleAnnotations(data) { - this.annotator.setScale(data.scale); - this.annotator.rotateAnnotations(data.rotationAngle, data.pageNum); - } - /** * Initializes annotations. * @@ -668,7 +692,6 @@ class BaseViewer extends EventEmitter { locale: location.locale, previewUI: this.previewUI }); - this.annotator.init(this.scale); // Disables controls during annotation mode @@ -681,7 +704,9 @@ class BaseViewer extends EventEmitter { }); // Add a custom listener for events related to scaling/orientation changes - this.addListener('scale', this.scaleAnnotations.bind(this)); + this.addListener('scale', (data) => { + this.annotator.emit('scaleAnnotations', data); + }); // Add a custom listener for events emmited by the annotator this.annotator.addListener('annotatorevent', this.handleAnnotatorNotifications); @@ -724,48 +749,26 @@ class BaseViewer extends EventEmitter { } /** - * Shows the point annotate button. + * Shows the annotate button for the specified mode * - * @param {Function} handler - Point annotation button handler + * @param {string} currentMode - Annotation mode + * @param {Object[]} modeButtons - Annotation modes which require buttons * @return {void} */ - showPointAnnotateButton(handler) { - if (!this.isAnnotatable('point')) { + showModeAnnotateButton(currentMode, modeButtons) { + const mode = modeButtons[currentMode]; + if (!mode || !this.isAnnotatable(currentMode)) { return; } - if (!this.getPointModeClickHandler) { - this.getPointModeClickHandler = () => handler; - } - const { container } = this.options; - const annotateButtonEl = container.querySelector(SELECTOR_BOX_PREVIEW_BTN_ANNOTATE_POINT); - + const annotateButtonEl = container.querySelector(mode.selector); if (annotateButtonEl) { - annotateButtonEl.title = __('annotation_point_toggle'); + annotateButtonEl.title = mode.title; annotateButtonEl.classList.remove(CLASS_HIDDEN); - annotateButtonEl.addEventListener('click', handler); - } - } - /** - * Shows the draw annotate button. - * - * @param {Function} handler - Drawing annotation button handler - * @return {void} - */ - showDrawAnnotateButton(handler) { - if (!this.isAnnotatable('draw')) { - return; - } - - this.drawAnnotateClickHandler = handler; - const { container } = this.options; - const drawAnnotateButtonEl = container.querySelector(SELECTOR_BOX_PREVIEW_BTN_ANNOTATE_DRAW); - if (drawAnnotateButtonEl) { - drawAnnotateButtonEl.title = __('annotation_draw_toggle'); - drawAnnotateButtonEl.classList.remove(CLASS_HIDDEN); - drawAnnotateButtonEl.addEventListener('click', handler); + const handler = this.getAnnotationModeClickHandler(currentMode); + annotateButtonEl.addEventListener('click', handler); } } @@ -786,32 +789,6 @@ class BaseViewer extends EventEmitter { }; } - /** - * Disables viewer controls - * - * @return {void} - */ - /* eslint-disable no-unused-vars */ - disableViewerControls() { - if (this.controls) { - this.controls.disable(); - } - } - /* eslint-enable no-unused-vars */ - - /** - * Enables viewer controls - * - * @return {void} - */ - /* eslint-disable no-unused-vars */ - enableViewerControls() { - if (this.controls) { - this.controls.enable(); - } - } - /* eslint-enable no-unused-vars */ - /** * Handle events emitted by the annotator * @@ -841,7 +818,7 @@ class BaseViewer extends EventEmitter { this.emit('notificationshow', data.data); break; case 'annotationsfetched': - this.scaleAnnotations({ + this.emit('scale', { scale: this.scale, rotationAngle: this.rotationAngle }); diff --git a/src/lib/viewers/__tests__/BaseViewer-test.js b/src/lib/viewers/__tests__/BaseViewer-test.js index 9fdb2537e..3de92d51d 100644 --- a/src/lib/viewers/__tests__/BaseViewer-test.js +++ b/src/lib/viewers/__tests__/BaseViewer-test.js @@ -111,6 +111,7 @@ describe('lib/viewers/BaseViewer', () => { base.finishLoadingSetup(); expect(container.innerHTML).to.equal('icon'); expect(container.classList.add).to.be.called; + base.options.container = null; }); }); @@ -709,8 +710,7 @@ describe('lib/viewers/BaseViewer', () => { }; stubs.isAnnotatable = sandbox.stub(base, 'isAnnotatable').returns(true); sandbox.stub(base, 'initAnnotations'); - sandbox.stub(base, 'showPointAnnotateButton', () => base.getAnnotationModeClickHandler('point')); - sandbox.stub(base, 'showDrawAnnotateButton', () => base.getAnnotationModeClickHandler('draw')); + sandbox.stub(base, 'showModeAnnotateButton'); stubs.checkPermission = sandbox.stub(file, 'checkPermission').returns(false); }); @@ -725,9 +725,7 @@ describe('lib/viewers/BaseViewer', () => { window.BoxAnnotations = BoxAnnotations; base.loadAnnotator(); expect(base.initAnnotations).to.be.called; - expect(base.showPointAnnotateButton).to.be.called; - // NOTE: Enable once drawing annotations are enabled - // expect(base.showDrawAnnotateButton).to.be.called; + expect(base.showModeAnnotateButton).to.be.called; }); it('should not display the point or draw annotation button if the user does not have the appropriate permissions', () => { @@ -738,9 +736,7 @@ describe('lib/viewers/BaseViewer', () => { base.loadAnnotator(); expect(base.initAnnotations).to.not.be.called; - expect(base.showPointAnnotateButton).to.not.be.called; - // NOTE: Enable once drawing annotations are enabled - // expect(base.showDrawAnnotateButton).to.not.be.called; + expect(base.showModeAnnotateButton).to.not.be.called; }); it('should not load an annotator if no loader was found', () => { @@ -750,8 +746,7 @@ describe('lib/viewers/BaseViewer', () => { window.BoxAnnotations = BoxAnnotations; base.loadAnnotator(); expect(base.initAnnotations).to.not.be.called; - expect(base.showPointAnnotateButton).to.not.be.called; - expect(base.showDrawAnnotateButton).to.not.be.called; + expect(base.showModeAnnotateButton).to.not.be.called; }); it('should not load an annotator if the viewer is not annotatable', () => { @@ -764,35 +759,7 @@ describe('lib/viewers/BaseViewer', () => { stubs.isAnnotatable.returns(false); base.loadAnnotator(); expect(base.initAnnotations).to.not.be.called; - expect(base.showPointAnnotateButton).to.not.be.called; - expect(base.showDrawAnnotateButton).to.not.be.called; - }); - }); - - describe('scaleAnnotations()', () => { - const scaleData = { - scale: 0.4321, - rotationAngle: 90, - pageNum: 2 - }; - beforeEach(() => { - base.annotator = { - setScale: sandbox.stub(), - rotateAnnotations: sandbox.stub() - }; - base.annotator.threads = { - 2: [{}] - }; - - base.scaleAnnotations(scaleData); - }); - - it('should invoke setScale() on annotator to scale annotations', () => { - expect(base.annotator.setScale).to.be.calledWith(scaleData.scale); - }); - - it('should invoke rotateAnnotations() on annotator to orient annotations', () => { - expect(base.annotator.rotateAnnotations).to.be.calledWith(scaleData.rotationAngle, scaleData.pageNum); + expect(base.showModeAnnotateButton).to.not.be.called; }); }); @@ -869,31 +836,42 @@ describe('lib/viewers/BaseViewer', () => { }); }); - describe('showPointAnnotateButton()', () => { - it('should set up and show point annotate button', () => { - const buttonEl = document.createElement('div'); - buttonEl.classList.add('bp-btn-annotate-point'); - buttonEl.classList.add(constants.CLASS_HIDDEN); - base.options = { - container: document, - file: { - id: 123 - } - }; - containerEl.appendChild(buttonEl); + describe('showModeAnnotateButton()', () => { + const modes = { + point: { + title: 'Point Annotation Mode', + selector: constants.SELECTOR_BOX_PREVIEW_BTN_ANNOTATE_POINT + }, + draw: { + title: 'Draw Annotation Mode', + selector: constants.SELECTOR_BOX_PREVIEW_BTN_ANNOTATE_DRAW + } + }; + + it('should do nothing if the mode does not require a button', () => { + sandbox.stub(base, 'getAnnotationModeClickHandler'); sandbox.stub(base, 'isAnnotatable').returns(true); - sandbox.mock(buttonEl).expects('addEventListener').withArgs('click', base.handler); + base.showModeAnnotateButton('highlight', modes); + expect(base.getAnnotationModeClickHandler).to.not.be.called; + }); - base.showPointAnnotateButton(base.handler); - expect(buttonEl.title).to.equal('Point annotation mode'); - expect(buttonEl.classList.contains(constants.CLASS_HIDDEN)).to.be.false; + it('should do nothing if the annotation type is not supported ', () => { + sandbox.stub(base, 'getAnnotationModeClickHandler'); + sandbox.stub(base, 'isAnnotatable').returns(false); + base.showModeAnnotateButton('bleh', modes); + expect(base.getAnnotationModeClickHandler).to.not.be.called; + }); + + it('should do nothing if the button is not in the container', () => { + sandbox.stub(base, 'isAnnotatable').returns(true); + sandbox.stub(base, 'getAnnotationModeClickHandler'); + base.showModeAnnotateButton('point', modes); + expect(base.getAnnotationModeClickHandler).to.not.be.called; }); - }); - describe('showDrawAnnotateButton()', () => { - it('should set up and show point annotate button', () => { + it('should set up and show an annotate button', () => { const buttonEl = document.createElement('div'); - buttonEl.classList.add('bp-btn-annotate-draw'); + buttonEl.classList.add('bp-btn-annotate-point'); buttonEl.classList.add(constants.CLASS_HIDDEN); base.options = { container: document, @@ -901,14 +879,14 @@ describe('lib/viewers/BaseViewer', () => { id: 123 } }; - containerEl.appendChild(buttonEl); sandbox.stub(base, 'isAnnotatable').returns(true); - sandbox.mock(buttonEl).expects('addEventListener').withArgs('click', base.handler); + sandbox.stub(base, 'getAnnotationModeClickHandler'); + sandbox.mock(buttonEl).expects('addEventListener').withArgs('click'); - base.showDrawAnnotateButton(base.handler); - expect(buttonEl.title).to.equal('Drawing annotation mode'); - expect(buttonEl.classList.contains(constants.CLASS_HIDDEN)).to.be.false; + base.showModeAnnotateButton('point', modes); + expect(buttonEl.title).to.equal('Point Annotation Mode'); + expect(base.getAnnotationModeClickHandler).to.be.called; }); }); @@ -986,13 +964,12 @@ describe('lib/viewers/BaseViewer', () => { }); it('should scale annotations on annotationsfetched', () => { - sandbox.stub(base, 'scaleAnnotations'); base.scale = 1; base.rotationAngle = 90; base.handleAnnotatorNotifications({ event: 'annotationsfetched' }); - expect(base.scaleAnnotations).to.be.calledWith({ + expect(base.emit).to.be.calledWith('scale', { scale: base.scale, rotationAngle: base.rotationAngle });