-
Notifications
You must be signed in to change notification settings - Fork 116
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature: drawing annotation deletion #337
Changes from 97 commits
12cc4ed
1872d82
6db4184
48af9ac
533030c
7f661ea
7230510
b90659a
000b503
48b5589
8c88200
f3b557c
ed403b8
22d4faa
4be971e
51b0201
0757c0b
e4a097b
1075f78
f25c34c
70d619c
833a42a
fe3587d
924cd25
3549473
1857b90
cb87a9e
5402f45
67e250f
bf29d9f
7bd6fdb
74c85fc
10d1616
18f0444
60e259b
034e37a
8edb906
f79244b
1fea19d
a1909fe
bb56ac4
d225a4a
6d0b4cc
b216523
bd25a6e
08f44c1
41a5d51
5d05d52
06e4f24
abdc1ac
55817f4
1e31e26
c827c3e
bf550bd
e44a151
52642bf
746d291
ee25fa4
cffba79
7f31e8c
0046949
41287af
1df105b
94bf934
2a39df7
455549f
98d8e3b
fb0f5cb
d8f9f80
6ac7878
bad8368
2509bd6
08c24bc
94e92c0
ab8358b
ae4e82c
73aaa87
cdf8b6b
15e0e72
2612395
5bcc4a4
d02ac11
c3d3ee5
82012d1
a8a9be5
e6e77fd
53aed0e
45ce77b
91bffcc
b369165
8c8e158
303bb2d
d2eddee
f9d083d
f85d2e8
75b021e
c7b2b35
9d1ac3d
b8fdc3d
9402fa2
48e4bfb
9268715
9314a41
d5f99a6
34ff020
d0c4427
4d94bf4
beb8150
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,146 @@ | ||
import EventEmitter from 'events'; | ||
|
||
class AnnotationController extends EventEmitter { | ||
/** @property {Array} - The array of annotation threads */ | ||
threads = []; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So would we be storing all the annotation threads both in the annotator and the controller then? Seems redundant to have it stored in 2 places There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The goal is to have each controller manage their respective type of threads. Right now we are at an in-between state where highlight and point annotation doesn't use a controller so we still need to store threads in the annotator and controller. In addition, the threadmap is in use in other functions that I did not want to modify. The good news is that these data structures simply store a reference to the thread so the memory overhead isn't that bad. The downside is that the thread reference needs to be removed from both (this is accomplished by listening to 'threaddeleted'). |
||
|
||
/** @property {Array} - The array of annotation handlers */ | ||
handlers = []; | ||
|
||
/** | ||
* [constructor] | ||
* | ||
* @return {AnnotationController} Annotation controller instance | ||
*/ | ||
constructor() { | ||
super(); | ||
|
||
this.handleAnnotationEvent = this.handleAnnotationEvent.bind(this); | ||
} | ||
|
||
/** | ||
* Register the annotator and any information associated with the annotator | ||
* | ||
* @public | ||
* @param {Annotator} annotator - The annotator to be associated with the controller | ||
* @return {void} | ||
*/ | ||
registerAnnotator(annotator) { | ||
// TODO (@minhnguyen): remove the need to register an annotator. Ideally, the annotator should know about the | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
// controller and the controller does not know about the annotator. | ||
this.annotator = annotator; | ||
} | ||
|
||
/** | ||
* Bind the mode listeners and store each handler for future unbinding | ||
* | ||
* @public | ||
* @return {void} | ||
*/ | ||
bindModeListeners() { | ||
const handlers = this.setupAndGetHandlers(); | ||
handlers.forEach((handler) => { | ||
const eventNames = handler.type.split(' '); | ||
eventNames.forEach((eventName) => handler.eventObj.addEventListener(eventName, handler.func)); | ||
this.handlers.push(handler); | ||
}); | ||
} | ||
|
||
/** | ||
* Unbind the previously bound mode listeners | ||
* | ||
* @public | ||
* @return {void} | ||
*/ | ||
unbindModeListeners() { | ||
while (this.handlers.length > 0) { | ||
const handler = this.handlers.pop(); | ||
const eventNames = handler.type.split(' '); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would just save the event types as an array so you can just loop through them instead of parsing a string into an array |
||
eventNames.forEach((eventName) => { | ||
handler.eventObj.removeEventListener(eventName, handler.func); | ||
}); | ||
} | ||
} | ||
|
||
/** | ||
* Register a thread with the controller so that the controller can keep track of relevant threads | ||
* | ||
* @public | ||
* @param {AnnotationThread} thread - The thread to register with the controller | ||
* @return {void} | ||
*/ | ||
registerThread(thread) { | ||
this.threads.push(thread); | ||
} | ||
|
||
/** | ||
* Unregister a previously registered thread | ||
* | ||
* @public | ||
* @param {AnnotationThread} thread - The thread to unregister with the controller | ||
* @return {void} | ||
*/ | ||
unregisterThread(thread) { | ||
this.threads = this.threads.filter((item) => item !== thread); | ||
} | ||
|
||
/** | ||
* Binds custom event listeners for a thread. | ||
* | ||
* @protected | ||
* @param {AnnotationThread} thread - Thread to bind events to | ||
* @return {void} | ||
*/ | ||
bindCustomListenersOnThread(thread) { | ||
if (!thread) { | ||
return; | ||
} | ||
|
||
// TODO (@minhnguyen): Move annotator.bindCustomListenersOnThread logic to AnnotationController | ||
this.annotator.bindCustomListenersOnThread(thread); | ||
thread.addListener('annotationevent', (data) => { | ||
this.handleAnnotationEvent(thread, data); | ||
}); | ||
} | ||
|
||
/** | ||
* Unbinds custom event listeners for the thread. | ||
* | ||
* @protected | ||
* @param {AnnotationThread} thread - Thread to unbind events from | ||
* @return {void} | ||
*/ | ||
unbindCustomListenersOnThread(thread) { | ||
if (!thread) { | ||
return; | ||
} | ||
|
||
thread.removeAllListeners('threaddeleted'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pretty sure you can call this once without args and it will remove all listeners for the thread. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like you're right about this, but there is a comment in annotator regarding calling unbindCustomListenersOnThread on 'threadcleanup':
|
||
thread.removeAllListeners('threadcleanup'); | ||
thread.removeAllListeners('annotationsaved'); | ||
thread.removeAllListeners('annotationevent'); | ||
} | ||
|
||
/** | ||
* Set up and return the necessary handlers for the annotation mode | ||
* | ||
* @protected | ||
* @return {Array} An array where each element is an object containing the object that will emit the event, | ||
* the type of events to listen for, and the callback | ||
*/ | ||
setupAndGetHandlers() {} | ||
|
||
/** | ||
* Handle an annotation event. | ||
* | ||
* @protected | ||
* @param {AnnotationThread} thread - The thread that emitted the event | ||
* @param {Object} data - Extra data related to the annotation event | ||
* @return {void} | ||
*/ | ||
/* eslint-disable no-unused-vars */ | ||
handleAnnotationEvent(thread, data = {}) {} | ||
/* eslint-enable no-unused-vars */ | ||
} | ||
|
||
export default AnnotationController; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
import EventEmitter from 'events'; | ||
import autobind from 'autobind-decorator'; | ||
import AnnotationService from './AnnotationService'; | ||
import DrawingController from './drawing/DrawingController'; | ||
import * as annotatorUtil from './annotatorUtil'; | ||
import { ICON_CLOSE } from '../icons/icons'; | ||
import './Annotator.scss'; | ||
|
@@ -84,7 +85,10 @@ class Annotator extends EventEmitter { | |
Object.keys(this.modeButtons).forEach((type) => { | ||
const handler = this.getAnnotationModeClickHandler(type); | ||
const buttonEl = this.container.querySelector(this.modeButtons[type].selector); | ||
buttonEl.removeEventListener('click', handler); | ||
|
||
if (buttonEl) { | ||
buttonEl.removeEventListener('click', handler); | ||
} | ||
}); | ||
|
||
this.unbindDOMListeners(); | ||
|
@@ -167,6 +171,11 @@ class Annotator extends EventEmitter { | |
|
||
const handler = this.getAnnotationModeClickHandler(currentMode); | ||
annotateButtonEl.addEventListener('click', handler); | ||
|
||
// TODO (@minhnguyen): Implement controller for point mode annotation and remove this check | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
if (mode.controller && mode.controller.constructor.name === DrawingController.name) { | ||
mode.controller.registerAnnotator(this); | ||
} | ||
} | ||
} | ||
|
||
|
@@ -536,6 +545,17 @@ class Annotator extends EventEmitter { | |
// Bind events on valid annotation thread | ||
const thread = this.createAnnotationThread(annotations, firstAnnotation.location, firstAnnotation.type); | ||
this.bindCustomListenersOnThread(thread); | ||
|
||
const modeData = this.modeButtons[firstAnnotation.type]; | ||
if ( | ||
modeData && | ||
modeData.controller && | ||
modeData.controller.constructor.name === DrawingController.name | ||
) { | ||
modeData.controller.bindCustomListenersOnThread(thread); | ||
modeData.controller.registerThread(thread); | ||
this.addThreadToMap(thread); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remind me why we need to bind the thread so early and not on mode entry? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This particular binding is done on AnnotationThreads fetched from the server. This binding allows the annotator to react to the deletion of saved AnnotationThreads. |
||
} | ||
}); | ||
|
||
this.emit('annotationsfetched'); | ||
|
@@ -667,6 +687,7 @@ class Annotator extends EventEmitter { | |
unbindCustomListenersOnThread(thread) { | ||
thread.removeAllListeners('threaddeleted'); | ||
thread.removeAllListeners('threadcleanup'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I'm right about removeAllListeners then it applies here as well. |
||
thread.removeAllListeners('annotationsaved'); | ||
thread.removeAllListeners('annotationevent'); | ||
} | ||
|
||
|
@@ -694,98 +715,12 @@ class Annotator extends EventEmitter { | |
} | ||
); | ||
} else if (mode === TYPES.draw) { | ||
const drawingThread = this.createAnnotationThread([], {}, TYPES.draw); | ||
this.bindCustomListenersOnThread(drawingThread); | ||
|
||
/* eslint-disable require-jsdoc */ | ||
const locationFunction = (event) => this.getLocationFromEvent(event, TYPES.point); | ||
/* eslint-enable require-jsdoc */ | ||
|
||
const postButtonEl = this.getAnnotateButton(SELECTOR_ANNOTATION_BUTTON_DRAW_POST); | ||
const undoButtonEl = this.getAnnotateButton(SELECTOR_ANNOTATION_BUTTON_DRAW_UNDO); | ||
const redoButtonEl = this.getAnnotateButton(SELECTOR_ANNOTATION_BUTTON_DRAW_REDO); | ||
|
||
// NOTE (@minhnguyen): Move this logic to a new controller class | ||
const that = this; | ||
drawingThread.addListener('annotationevent', (data = {}) => { | ||
switch (data.type) { | ||
case 'drawcommit': | ||
drawingThread.removeAllListeners('annotationevent'); | ||
break; | ||
case 'pagechanged': | ||
drawingThread.saveAnnotation(TYPES.draw); | ||
that.unbindModeListeners(); | ||
that.bindModeListeners(TYPES.draw); | ||
break; | ||
case 'availableactions': | ||
if (data.undo === 1) { | ||
annotatorUtil.enableElement(undoButtonEl); | ||
} else if (data.undo === 0) { | ||
annotatorUtil.disableElement(undoButtonEl); | ||
} | ||
|
||
if (data.redo === 1) { | ||
annotatorUtil.enableElement(redoButtonEl); | ||
} else if (data.redo === 0) { | ||
annotatorUtil.disableElement(redoButtonEl); | ||
} | ||
break; | ||
default: | ||
} | ||
}); | ||
|
||
handlers.push( | ||
{ | ||
type: 'mousemove', | ||
func: annotatorUtil.eventToLocationHandler(locationFunction, drawingThread.handleMove), | ||
eventObj: this.annotatedElement | ||
}, | ||
{ | ||
type: 'mousedown', | ||
func: annotatorUtil.eventToLocationHandler(locationFunction, drawingThread.handleStart), | ||
eventObj: this.annotatedElement | ||
}, | ||
{ | ||
type: 'mouseup', | ||
func: annotatorUtil.eventToLocationHandler(locationFunction, drawingThread.handleStop), | ||
eventObj: this.annotatedElement | ||
} | ||
); | ||
|
||
if (postButtonEl) { | ||
handlers.push({ | ||
type: 'click', | ||
func: () => { | ||
drawingThread.saveAnnotation(mode); | ||
this.toggleAnnotationHandler(mode); | ||
}, | ||
eventObj: postButtonEl | ||
}); | ||
} | ||
|
||
if (undoButtonEl) { | ||
handlers.push({ | ||
type: 'click', | ||
func: () => { | ||
drawingThread.undo(); | ||
}, | ||
eventObj: undoButtonEl | ||
}); | ||
} | ||
|
||
if (redoButtonEl) { | ||
handlers.push({ | ||
type: 'click', | ||
func: () => { | ||
drawingThread.redo(); | ||
}, | ||
eventObj: redoButtonEl | ||
}); | ||
} | ||
const controller = this.modeButtons[TYPES.draw].controller; | ||
controller.bindModeListeners(); | ||
} | ||
|
||
handlers.forEach((handler) => { | ||
handler.eventObj.addEventListener(handler.type, handler.func); | ||
handler.eventObj.addEventListener(handler.type, handler.func, false); | ||
this.annotationModeHandlers.push(handler); | ||
}); | ||
} | ||
|
@@ -836,12 +771,26 @@ class Annotator extends EventEmitter { | |
* Unbinds event listeners for annotation modes. | ||
* | ||
* @protected | ||
* @param {string} mode - Annotation mode to be unbound | ||
* @return {void} | ||
*/ | ||
unbindModeListeners() { | ||
unbindModeListeners(mode) { | ||
while (this.annotationModeHandlers.length > 0) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What would be the difference between removing these listeners vs. calling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This part delegates the unbinding to the controller rather than the annotator. Meaning once a point annotation controller is in (and preferably highlight as well) this call just needs to be a call to controller[mode].unbindModeListeners() |
||
const handler = this.annotationModeHandlers.pop(); | ||
handler.eventObj.removeEventListener(handler.type, handler.func); | ||
const eventNames = handler.type.split(' '); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are these for different events then the unbind func on line 55 of annotationController? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was initially checking out a way to bind the same listener to multiple events ie. 'mousedown', 'touchstart' by simply adding it to the string in bindModeListeners. This is done in AnnotationController (see drawingcontroller line 119 for an example) and doesn't need to be in annotator. I'll remove this. |
||
eventNames.forEach((eventName) => { | ||
handler.eventObj.removeEventListener(eventName, handler.func); | ||
}); | ||
} | ||
|
||
if ( | ||
mode && | ||
this.modeButtons && | ||
this.modeButtons[mode] && | ||
this.modeButtons[mode].controller && | ||
this.modeButtons[mode].controller.constructor.name === DrawingController.name | ||
) { | ||
this.modeButtons[mode].controller.unbindModeListeners(); | ||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be clearer to label this AnnotationModeController since it only applies to annotation types that would enter a 'mode'. AKA not highlight annotations