From 4bcd248ad9f18079b30fa114feeb94c46312a7e4 Mon Sep 17 00:00:00 2001 From: Sumedha Pramod Date: Thu, 5 Apr 2018 11:04:23 -0700 Subject: [PATCH 1/2] Chore: Removing DrawingModeController dependency on DocDrawingThread - Required to enable drawing annotations on image files --- src/controllers/AnnotationModeController.js | 4 + src/controllers/DrawingModeController.js | 104 +++++++++++++------- src/controllers/PointModeController.js | 24 +---- src/doc/DocDrawingThread.js | 2 +- src/drawing/DrawingThread.js | 53 +++++++++- 5 files changed, 124 insertions(+), 63 deletions(-) diff --git a/src/controllers/AnnotationModeController.js b/src/controllers/AnnotationModeController.js index fefb29fee..e48f8a161 100644 --- a/src/controllers/AnnotationModeController.js +++ b/src/controllers/AnnotationModeController.js @@ -277,6 +277,10 @@ class AnnotationModeController extends EventEmitter { */ getThreadByID(threadID) { let thread = null; + if (!threadID) { + return thread; + } + Object.keys(this.threads).some((pageNum) => { const pageThreads = this.threads[pageNum]; if (!pageThreads) { diff --git a/src/controllers/DrawingModeController.js b/src/controllers/DrawingModeController.js index cb8c49e4c..e8352cc30 100644 --- a/src/controllers/DrawingModeController.js +++ b/src/controllers/DrawingModeController.js @@ -1,17 +1,10 @@ import AnnotationModeController from './AnnotationModeController'; import shell from './drawingShell.html'; -import DocDrawingThread from '../doc/DocDrawingThread'; -import { - replaceHeader, - enableElement, - disableElement, - eventToLocationHandler, - clearCanvas, - hasValidBoundaryCoordinates -} from '../util'; +import { replaceHeader, enableElement, disableElement, clearCanvas, hasValidBoundaryCoordinates } from '../util'; import { TYPES, STATES, + THREAD_EVENT, SELECTOR_ANNOTATION_BUTTON_DRAW_CANCEL, SELECTOR_ANNOTATION_BUTTON_DRAW_POST, SELECTOR_ANNOTATION_BUTTON_DRAW_UNDO, @@ -107,40 +100,18 @@ class DrawingModeController extends AnnotationModeController { /** @inheritdoc */ setupHandlers() { /* eslint-disable require-jsdoc */ - const locationFunction = (event) => this.annotator.getLocationFromEvent(event, TYPES.point); + this.locationFunction = (event) => this.annotator.getLocationFromEvent(event, TYPES.point); /* eslint-enable require-jsdoc */ - // Setup - const threadParams = this.annotator.getThreadParams([], {}, TYPES.draw); - this.currentThread = new DocDrawingThread(threadParams); - this.currentThread.addListener('threadevent', (data) => { - const thread = this.currentThread || this.selectedThread; - this.handleThreadEvents(thread, data); - }); - - // Get handlers - this.pushElementHandler( - this.annotatedElement, - ['mousemove', 'touchmove'], - eventToLocationHandler(locationFunction, this.currentThread.handleMove) - ); - - this.pushElementHandler( - this.annotatedElement, - ['mousedown', 'touchstart'], - eventToLocationHandler(locationFunction, this.currentThread.handleStart) - ); - - this.pushElementHandler( - this.annotatedElement, - ['mouseup', 'touchcancel', 'touchend'], - eventToLocationHandler(locationFunction, this.currentThread.handleStop) - ); + this.drawingStartHandler = this.drawingStartHandler.bind(this); + this.pushElementHandler(this.annotatedElement, ['mousedown', 'touchstart'], this.drawingStartHandler); this.pushElementHandler(this.cancelButtonEl, 'click', () => { if (this.currentThread) { this.currentThread.cancelUnsavedAnnotation(); } + + this.currentThread = undefined; this.toggleMode(); }); @@ -149,11 +120,61 @@ class DrawingModeController extends AnnotationModeController { this.saveThread(this.currentThread); } + this.currentThread = undefined; this.toggleMode(); }); - this.pushElementHandler(this.undoButtonEl, 'click', this.currentThread.undo); - this.pushElementHandler(this.redoButtonEl, 'click', this.currentThread.redo); + this.pushElementHandler(this.undoButtonEl, 'click', () => { + if (this.currentThread) { + this.currentThread.undo(); + } + }); + + this.pushElementHandler(this.redoButtonEl, 'click', () => { + if (this.currentThread) { + this.currentThread.redo(); + } + }); + } + + /** + * Start a drawing stroke + * + * @param {Event} event - DOM event + * @return {void} + */ + drawingStartHandler(event) { + event.stopPropagation(); + event.preventDefault(); + + // Get annotation location from click event, ignore click if location is invalid + const location = this.annotator.getLocationFromEvent(event, TYPES.point); + if (!location) { + return; + } + + if (this.currentThread) { + this.currentThread.handleStart(location); + return; + } + + // Add initial drawing boundary based on starting location + location.minX = location.x; + location.maxX = location.x; + location.minY = location.y; + location.maxY = location.y; + + // Create new thread with no annotations, show indicator, and show dialog + const thread = this.annotator.createAnnotationThread([], location, TYPES.draw); + if (!thread) { + return; + } + + this.currentThread = thread; + this.emit(THREAD_EVENT.pending, thread.getThreadEventData()); + thread.bindDrawingListeners(this.locationFunction); + thread.addListener('threadevent', (data) => this.handleThreadEvents(thread, data)); + thread.handleStart(location); } /** @@ -171,6 +192,9 @@ class DrawingModeController extends AnnotationModeController { const { eventData } = data; switch (data.event) { case 'softcommit': + thread.removeListener('threadevent', this.handleThreadEvents); + thread.unbindDrawingListeners(); + this.currentThread = undefined; this.saveThread(thread); this.unbindListeners(); @@ -187,6 +211,10 @@ class DrawingModeController extends AnnotationModeController { return; } + this.currentThread = undefined; + thread.removeListener('threadevent', this.handleThreadEvents); + thread.unbindDrawingListeners(); + 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 diff --git a/src/controllers/PointModeController.js b/src/controllers/PointModeController.js index 90e987ccd..6579eb694 100644 --- a/src/controllers/PointModeController.js +++ b/src/controllers/PointModeController.js @@ -16,12 +16,6 @@ class PointModeController extends AnnotationModeController { /** @property {HTMLElement} - The button to exit point annotation mode */ exitButtonEl; - /** @property {HTMLElement} - The button to cancel the pending thread */ - cancelButtonEl; - - /** @property {HTMLElement} - The button to commit the pending thread */ - postButtonEl; - /** @inheritdoc */ init(data) { super.init(data); @@ -123,23 +117,7 @@ class PointModeController extends AnnotationModeController { // Get handlers this.pushElementHandler(this.annotatedElement, ['mousedown', 'touchstart'], this.pointClickHandler); - this.pushElementHandler(this.exitButtonEl, 'click', () => { - if (this.currentThread) { - this.currentThread.cancelUnsavedAnnotation(); - } - - this.toggleMode(); - }); - - this.pushElementHandler(this.cancelButtonEl, 'click', () => { - this.currentThread.cancelUnsavedAnnotation(); - this.emit(CONTROLLER_EVENT.toggleMode); - }); - - this.pushElementHandler(this.postButtonEl, 'click', () => { - this.currentThread.saveAnnotation(TYPES.point); - this.emit(CONTROLLER_EVENT.toggleMode); - }); + this.pushElementHandler(this.exitButtonEl, 'click', this.toggleMode); } /** @inheritdoc */ diff --git a/src/doc/DocDrawingThread.js b/src/doc/DocDrawingThread.js index 09bd815c0..f685dc54e 100644 --- a/src/doc/DocDrawingThread.js +++ b/src/doc/DocDrawingThread.js @@ -78,8 +78,8 @@ class DocDrawingThread extends DrawingThread { page: location.page, dimensions: location.dimensions }; - this.checkAndHandleScaleUpdate(); } + this.checkAndHandleScaleUpdate(); this.drawingFlag = DRAW_STATES.drawing; if (!this.pendingPath) { diff --git a/src/drawing/DrawingThread.js b/src/drawing/DrawingThread.js index 967dec362..2e4a2af63 100644 --- a/src/drawing/DrawingThread.js +++ b/src/drawing/DrawingThread.js @@ -1,7 +1,7 @@ import AnnotationThread from '../AnnotationThread'; import DrawingPath from './DrawingPath'; import DrawingContainer from './DrawingContainer'; -import { getFirstAnnotation } from '../util'; +import { eventToLocationHandler, getFirstAnnotation } from '../util'; import { STATES, DRAW_STATES, @@ -121,6 +121,57 @@ class DrawingThread extends AnnotationThread { this.clearBoundary(); } + /** + * Binds DOM event listeners for drawing new thread using + * mode specific location getter + * + * @protected + * @param {Function} locationFunction - Location getter method + * @return {void} + */ + bindDrawingListeners(locationFunction) { + if (!this.isMobile) { + this.annotatedElement.addEventListener( + 'mousemove', + eventToLocationHandler(locationFunction, this.handleMove) + ); + this.annotatedElement.addEventListener( + 'mouseup', + eventToLocationHandler(locationFunction, this.handleStop) + ); + } + + if (this.hasTouch) { + this.annotatedElement.addEventListener( + 'touchmove', + eventToLocationHandler(locationFunction, this.handleMove) + ); + this.annotatedElement.addEventListener( + 'touchcancel', + eventToLocationHandler(locationFunction, this.handleStop) + ); + this.annotatedElement.addEventListener( + 'touchend', + eventToLocationHandler(locationFunction, this.handleStop) + ); + } + } + + /** + * Unbinds DOM event listeners for drawing new threads. + * + * @protected + * @return {void} + */ + unbindDrawingListeners() { + this.annotatedElement.removeEventListener('mousemove', eventToLocationHandler); + this.annotatedElement.removeEventListener('mouseup', eventToLocationHandler); + + this.annotatedElement.removeEventListener('touchmove', eventToLocationHandler); + this.annotatedElement.removeEventListener('touchcancel', eventToLocationHandler); + this.annotatedElement.removeEventListener('touchend', eventToLocationHandler); + } + /* eslint-disable no-unused-vars */ /** * Handle a pointer movement From a1c4ac61936751adae05a027cfac3b3db4d9b086 Mon Sep 17 00:00:00 2001 From: Sumedha Pramod Date: Thu, 5 Apr 2018 18:19:57 -0700 Subject: [PATCH 2/2] Update: tests --- .../__tests__/DrawingModeController-test.js | 92 ++++++++++++++----- .../__tests__/PointModeController-test.js | 4 +- src/doc/__tests__/DocDrawingThread-test.js | 1 + src/drawing/__tests__/DrawingThread-test.js | 59 ++++++++++++ 4 files changed, 131 insertions(+), 25 deletions(-) diff --git a/src/controllers/__tests__/DrawingModeController-test.js b/src/controllers/__tests__/DrawingModeController-test.js index 077752945..0fd3d82e3 100644 --- a/src/controllers/__tests__/DrawingModeController-test.js +++ b/src/controllers/__tests__/DrawingModeController-test.js @@ -12,7 +12,8 @@ import { SELECTOR_ANNOTATION_BUTTON_DRAW_REDO, SELECTOR_DRAW_MODE_HEADER, CLASS_ANNOTATION_MODE, - CLASS_ACTIVE + CLASS_ACTIVE, + THREAD_EVENT } from '../../constants'; let controller; @@ -39,6 +40,9 @@ describe('controllers/DrawingModeController', () => { deleteThread: () => {}, clearBoundary: () => {}, drawBoundary: () => {}, + bindDrawingListeners: () => {}, + unbindDrawingListeners: () => {}, + getThreadEventData: () => {}, show: () => {} }; stubs.threadMock = sandbox.mock(stubs.thread); @@ -186,22 +190,63 @@ describe('controllers/DrawingModeController', () => { describe('setupHandlers()', () => { it('should successfully contain draw mode handlers if undo and redo buttons exist', () => { - controller.annotator = { - getThreadParams: sandbox.stub(), - getLocationFromEvent: sandbox.stub() - }; controller.annotatedElement = {}; - stubs.getParams = controller.annotator.getThreadParams.returns({}); - stubs.getLocation = controller.annotator.getLocationFromEvent; - controller.postButtonEl = 'not undefined'; controller.undoButtonEl = 'also not undefined'; controller.redoButtonEl = 'additionally not undefined'; controller.cancelButtonEl = 'definitely not undefined'; controller.setupHandlers(); - expect(stubs.getParams).to.be.called; - expect(controller.handlers.length).to.equal(7); + expect(controller.handlers.length).to.equal(5); + }); + }); + + describe('drawingStartHandler()', () => { + const event = { + stopPropagation: () => {}, + preventDefault: () => {} + }; + const eventMock = sandbox.mock(event); + const location = {}; + + beforeEach(() => { + controller.annotator = { + getLocationFromEvent: () => {}, + createAnnotationThread: () => {} + }; + stubs.annotatorMock = sandbox.mock(controller.annotator); + controller.currentThread = undefined; + controller.locationFunction = sandbox.stub(); + + eventMock.expects('stopPropagation'); + eventMock.expects('preventDefault'); + }); + + it('should do nothing if drawing start is invalid', () => { + stubs.annotatorMock.expects('getLocationFromEvent'); + stubs.annotatorMock.expects('createAnnotationThread').never(); + controller.drawingStartHandler(event); + }); + + it('should continue drawing if in the middle of creating a new drawing', () => { + controller.currentThread = stubs.thread; + stubs.threadMock.expects('handleStart').withArgs(location); + stubs.annotatorMock.expects('getLocationFromEvent').returns(location); + stubs.annotatorMock.expects('createAnnotationThread').never(); + controller.drawingStartHandler(event); + }); + + it('should begin a new drawing thread if none exist already', () => { + stubs.annotatorMock.expects('getLocationFromEvent').returns(location); + stubs.annotatorMock.expects('createAnnotationThread').returns(stubs.thread); + stubs.threadMock.expects('bindDrawingListeners').withArgs(controller.locationFunction); + stubs.threadMock.expects('addListener').withArgs('threadevent', sinon.match.func); + stubs.threadMock.expects('handleStart').withArgs(location); + stubs.threadMock.expects('getThreadEventData').returns({}); + + controller.drawingStartHandler(event); + expect(controller.currentThread).to.not.be.undefined; + expect(controller.emit).to.be.calledWith(THREAD_EVENT.pending, {}); }); }); @@ -234,12 +279,15 @@ describe('controllers/DrawingModeController', () => { it('should save thread on softcommit', () => { stubs.threadMock.expects('handleStart').never(); + stubs.threadMock.expects('removeListener').withArgs('threadevent', sinon.match.func); + stubs.threadMock.expects('unbindDrawingListeners'); controller.handleThreadEvents(stubs.thread, { event: 'softcommit' }); expect(controller.unbindListeners).to.be.called; expect(controller.bindListeners).to.be.called; expect(controller.saveThread).to.be.called; + expect(controller.currentThread).to.be.undefined; }); it('should start a new thread on pagechanged', () => { @@ -252,7 +300,9 @@ describe('controllers/DrawingModeController', () => { page: 1 }, info: 'I am a thread', - saveAnnotation: sandbox.stub() + saveAnnotation: sandbox.stub(), + removeListener: sandbox.stub(), + unbindDrawingListeners: sandbox.stub() }; const thread2 = { minX: 10, @@ -280,27 +330,20 @@ describe('controllers/DrawingModeController', () => { expect(controller.unbindListeners).to.be.called; expect(controller.bindListeners).to.be.called; expect(thread2.handleStart).to.be.calledWith(data.eventData.location); - }); - - it('should update undo and redo buttons on availableactions', () => { - controller.handleThreadEvents(stubs.thread, { - event: 'availableactions', - eventData: { - undo: 1, - redo: 2 - } - }); - expect(controller.updateUndoRedoButtonEls).to.be.calledWith(1, 2); + expect(controller.currentThread).to.not.be.undefined; }); it('should soft delete a pending thread and restart mode listeners', () => { stubs.thread.state = 'pending'; stubs.threadMock.expects('destroy'); + stubs.threadMock.expects('removeListener').withArgs('threadevent', sinon.match.func); + stubs.threadMock.expects('unbindDrawingListeners'); controller.handleThreadEvents(stubs.thread, { event: 'dialogdelete' }); expect(controller.unbindListeners).to.be.called; expect(controller.bindListeners).to.be.called; + expect(controller.currentThread).to.be.undefined; }); it('should delete a non-pending thread', () => { @@ -311,10 +354,13 @@ describe('controllers/DrawingModeController', () => { const unregisterThreadStub = sandbox.stub(controller, 'unregisterThread'); stubs.threadMock.expects('deleteThread'); + stubs.threadMock.expects('removeListener').withArgs('threadevent', sinon.match.func); + stubs.threadMock.expects('unbindDrawingListeners'); controller.handleThreadEvents(stubs.thread, { event: 'dialogdelete' }); expect(unregisterThreadStub).to.be.called; + expect(controller.currentThread).to.be.undefined; }); it('should not delete a thread if the dialog no longer exists', () => { @@ -323,6 +369,8 @@ describe('controllers/DrawingModeController', () => { controller.threads[1] = new rbush(); controller.registerThread(stubs.thread); const unregisterThreadStub = sandbox.stub(controller, 'unregisterThread'); + stubs.threadMock.expects('removeListener').never(); + stubs.threadMock.expects('unbindDrawingListeners').never(); controller.handleThreadEvents(stubs.thread, { event: 'dialogdelete' diff --git a/src/controllers/__tests__/PointModeController-test.js b/src/controllers/__tests__/PointModeController-test.js index 5c190474d..5d7140e6a 100644 --- a/src/controllers/__tests__/PointModeController-test.js +++ b/src/controllers/__tests__/PointModeController-test.js @@ -129,12 +129,10 @@ describe('controllers/PointModeController', () => { describe('setupHandlers()', () => { it('should successfully contain mode handlers', () => { - controller.postButtonEl = 'not undefined'; - controller.cancelButtonEl = 'definitely not undefined'; controller.exitButtonEl = 'also definitely not undefined'; controller.setupHandlers(); - expect(controller.handlers.length).to.equal(4); + expect(controller.handlers.length).to.equal(2); }); }); diff --git a/src/doc/__tests__/DocDrawingThread-test.js b/src/doc/__tests__/DocDrawingThread-test.js index 4946d8e34..27f007564 100644 --- a/src/doc/__tests__/DocDrawingThread-test.js +++ b/src/doc/__tests__/DocDrawingThread-test.js @@ -112,6 +112,7 @@ describe('doc/DocDrawingThread', () => { expect(window.requestAnimationFrame).to.be.called; expect(thread.drawingFlag).to.equal(DRAW_STATES.drawing); expect(thread.hasPageChanged).to.be.called; + expect(thread.checkAndHandleScaleUpdate).to.be.called; expect(thread.pendingPath).to.be.an.instanceof(DrawingPath); expect(thread.state).to.equal(STATES.pending); }); diff --git a/src/drawing/__tests__/DrawingThread-test.js b/src/drawing/__tests__/DrawingThread-test.js index 36a29e4b3..d9fb16d3d 100644 --- a/src/drawing/__tests__/DrawingThread-test.js +++ b/src/drawing/__tests__/DrawingThread-test.js @@ -94,6 +94,65 @@ describe('drawing/DrawingThread', () => { }); }); + describe('bindDrawingListeners()', () => { + beforeEach(() => { + thread.isMobile = false; + thread.hasTouch = false; + thread.annotatedElement = { + addEventListener: sandbox.stub() + }; + stubs.add = thread.annotatedElement.addEventListener; + }); + + it('should add only mouse listeners for desktop devices', () => { + thread.bindDrawingListeners(); + expect(stubs.add).to.be.calledWith('mousemove', sinon.match.func); + expect(stubs.add).to.be.calledWith('mouseup', sinon.match.func); + expect(stubs.add).to.not.be.calledWith('touchmove', sinon.match.func); + expect(stubs.add).to.not.be.calledWith('touchcancel', sinon.match.func); + expect(stubs.add).to.not.be.calledWith('touchend', sinon.match.func); + }); + + it('should add all listeners for touch enabled laptop devices', () => { + thread.hasTouch = true; + thread.bindDrawingListeners(); + expect(stubs.add).to.be.calledWith('mousemove', sinon.match.func); + expect(stubs.add).to.be.calledWith('mouseup', sinon.match.func); + expect(stubs.add).to.be.calledWith('touchmove', sinon.match.func); + expect(stubs.add).to.be.calledWith('touchcancel', sinon.match.func); + expect(stubs.add).to.be.calledWith('touchend', sinon.match.func); + }); + + it('should add only touch listeners for touch enabled mobile devices', () => { + thread.isMobile = true; + thread.hasTouch = true; + thread.bindDrawingListeners(); + expect(stubs.add).to.not.be.calledWith('mousemove', sinon.match.func); + expect(stubs.add).to.not.be.calledWith('mouseup', sinon.match.func); + expect(stubs.add).to.be.calledWith('touchmove', sinon.match.func); + expect(stubs.add).to.be.calledWith('touchcancel', sinon.match.func); + expect(stubs.add).to.be.calledWith('touchend', sinon.match.func); + }); + }); + + describe('unbindDrawingListeners()', () => { + beforeEach(() => { + thread.annotatedElement = { + removeEventListener: sandbox.stub() + }; + stubs.remove = thread.annotatedElement.removeEventListener; + }); + + it('should add only mouse listeners for desktop devices', () => { + thread.unbindDrawingListeners(); + expect(stubs.remove).to.be.calledWith('mousemove', sinon.match.func); + expect(stubs.remove).to.be.calledWith('mouseup', sinon.match.func); + expect(stubs.remove).to.be.calledWith('touchmove', sinon.match.func); + expect(stubs.remove).to.be.calledWith('touchcancel', sinon.match.func); + expect(stubs.remove).to.be.calledWith('touchend', sinon.match.func); + }); + }); + describe('setContextStyles()', () => { it('should set configurable context properties', () => { thread.drawingContext = {