Feature: drawing annotation deletion #337

Minh-Ng · 2017-08-25T04:56:19Z

Unfortunately I didn't have the foresight to split selection and deletion into separate pull requests, but ~900 lines are tests.

I'm proposing the ability to pass a controller into the annotation object (see BaseViewer.js for an example). This will make more sense with custom annotations in the sense that only a controller needs to be accounted for within the official library.
The controller will be able to take over some of the responsibilities currently done in annotator. Annotator should be able to be made more generic as any annotation type specific logic should be delegated to a controller. Eventually the controller should not need to know about the annotator. Instead should emit events and the annotator should observe these events.
Deletion is currently done upon selecting a drawing twice. Next up is to make UI for selection and deletion. An additional improvement is to use two canvases for hit detection so that it can be done in O(1) time, allowing selection to be done on hover without a huge loss of performance.

…notationScaling

…n/box-content-preview into branch/rebaseToRemote

…aling

…/MinhHNguyen/box-content-preview into feature/drawingAnnotationScaling

…-content-preview into feature/undoredo

…letion

pramodsum · 2017-08-25T21:49:53Z

src/lib/viewers/BaseViewer.js

@@ -3,6 +3,8 @@ import EventEmitter from 'events';
 import debounce from 'lodash.debounce';
 import fullscreen from '../Fullscreen';
 import RepStatus from '../RepStatus';
+// TODO (@minhnguyen): Import controller from annotations npm module once the library is separate
+import DrawingController from '../annotations/drawing/DrawingController';


I really want to avoid calling annotation specific code in BaseViewer. Is it possible to move this into Annotator or BoxAnnotations?

Unsure if it makes sense to allow external controllers. Any other annotation type that gets added would be added through the annotations codebase, so any needed controllers would also be added in the annotations codebase.

Good point, I'll move it into annotator as discussed.

jeremypress · 2017-08-26T23:27:49Z

src/lib/annotations/AnnotationController.js

+            return;
+        }
+
+        thread.removeAllListeners('threaddeleted');


Pretty sure you can call this once without args and it will remove all listeners for the thread.

Looks like you're right about this, but there is a comment in annotator regarding calling unbindCustomListenersOnThread on 'threadcleanup':

// Thread should be cleaned up, unbind listeners - we don't do
// in threaddeleted listener since thread may still need to respond
// to error messages

jeremypress · 2017-08-26T23:47:15Z

src/lib/annotations/Annotator.js

+                ) {
+                    modeData.controller.bindCustomListenersOnThread(thread);
+                    modeData.controller.registerThread(thread);
+                    this.addThreadToMap(thread);


Remind me why we need to bind the thread so early and not on mode entry?

This particular binding is done on AnnotationThreads fetched from the server. This binding allows the annotator to react to the deletion of saved AnnotationThreads.

jeremypress · 2017-08-26T23:47:42Z

src/lib/annotations/Annotator.js

@@ -667,6 +687,7 @@ class Annotator extends EventEmitter {
    unbindCustomListenersOnThread(thread) {
        thread.removeAllListeners('threaddeleted');
        thread.removeAllListeners('threadcleanup');


If I'm right about removeAllListeners then it applies here as well.

jeremypress · 2017-08-26T23:53:14Z

src/lib/annotations/Annotator.js

        while (this.annotationModeHandlers.length > 0) {
            const handler = this.annotationModeHandlers.pop();
-            handler.eventObj.removeEventListener(handler.type, handler.func);
+            const eventNames = handler.type.split(' ');


Are these for different events then the unbind func on line 55 of annotationController?

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.

jeremypress · 2017-08-26T23:57:59Z

src/lib/annotations/doc/DocAnnotator.js

@@ -436,7 +437,9 @@ class DocAnnotator extends Annotator {

        if (this.hasTouch && this.isMobile) {
            document.addEventListener('selectionchange', this.onSelectionChange);
+            document.addEventListener('touchstart', this.drawingSelectionHandler);


can we add these to the annotated element as well?

I'm not quite sure. I will test it out but it might be the case that mobile browsers are functioning a bit differently.

Just tested it out, looks like highlight selection needs to be bound to document but the drawing selection can be bound to annotated element.

jeremypress · 2017-08-27T00:01:58Z

src/lib/annotations/drawing/DrawingController.js

+     * @param {Object} data - Extra data related to the annotation event
+     * @return {void}
+     */
+    handleAnnotationEvent(thread, data = {}) {


+1 on the comments here, nice.

jeremypress · 2017-08-27T00:03:28Z

src/lib/annotations/drawing/DrawingController.js

+        }
+
+        const eventBoundary = {
+            minX: +location.x - DRAW_BORDER_OFFSET,


What do these +'s do?

Sometimes location.x/y are strings (I suspect this is when they are returned from getLocation) and + (unary plus operator) ensures they are parsed as numbers. We can also use parseFloat() here if that is more clear.

It looks like +number is actually the fastest. https://jsperf.com/number-vs-plus-vs-toint-vs-tofloat/16

jeremypress · 2017-08-27T00:04:51Z

src/lib/annotations/drawing/DrawingController.js

+            return;
+        }
+
+        // Randomly select a thread in case there are multiple


This will change with better hit detection 👍 (using a hitmap canvas)

jeremypress · 2017-08-27T00:05:24Z

src/lib/annotations/drawing/DrawingController.js

+    }
+
+    /**
+     * Select the indicated drawing thread. Deletes a drawing thread upon the second consecutive selection


This will change once we introduce UI, correct?

Yes, with the UI, a selection should show the GUI. A deletion will be done by clicking delete on the GUI option.

jeremypress · 2017-08-27T00:05:59Z

src/lib/annotations/drawing/DrawingController.js

+     */
+    updateUndoRedoButtonEls(undoCount, redoCount) {
+        if (this.undoButtonEl) {
+            if (undoCount === 1) {


Can these numbers be > 1?

Yes, in which case we don't need to do anything because they should already be enabled. In theory, the undo and redo counts will always start at 0. When they hit 1 they will be enabled and stay enabled for values > 1. If they drop to 0 they will return to the starting state (of being disabled).

pramodsum · 2017-08-25T21:59:07Z

src/lib/viewers/BaseViewer.js

@@ -3,6 +3,8 @@ import EventEmitter from 'events';
 import debounce from 'lodash.debounce';
 import fullscreen from '../Fullscreen';
 import RepStatus from '../RepStatus';
+// TODO (@minhnguyen): Import controller from annotations npm module once the library is separate
+import DrawingController from '../annotations/drawing/DrawingController';


Unsure if it makes sense to allow external controllers. Any other annotation type that gets added would be added through the annotations codebase, so any needed controllers would also be added in the annotations codebase.

pramodsum · 2017-08-25T22:00:33Z

src/lib/annotations/AnnotationController.js

+     * @return {void}
+     */
+    registerAnnotator(annotator) {
+        // TODO (@minhnguyen): remove the need to register an annotator. Ideally, the annotator should know about the


pramodsum · 2017-08-25T22:02:22Z

src/lib/annotations/drawing/DrawingController.js

+        // Get handlers
+        handlers.push(
+            {
+                type: 'mousemove touchmove',


maybe store this as type: ['mousemove', 'touchmove']

pramodsum · 2017-08-25T22:07:14Z

src/lib/annotations/AnnotationController.js

+    unbindModeListeners() {
+        while (this.handlers.length > 0) {
+            const handler = this.handlers.pop();
+            const eventNames = handler.type.split(' ');


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

pramodsum · 2017-08-25T22:08:30Z

src/lib/annotations/AnnotationController.js

+
+class AnnotationController extends EventEmitter {
+    /** @property {Array} - The array of annotation threads */
+    threads = [];


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

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').

pramodsum · 2017-08-25T22:09:57Z

src/lib/annotations/Annotator.js

@@ -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


pramodsum · 2017-08-25T22:11:52Z

src/lib/annotations/AnnotationController.js

@@ -0,0 +1,146 @@
+import EventEmitter from 'events';
+
+class AnnotationController extends EventEmitter {


Might be clearer to label this AnnotationModeController since it only applies to annotation types that would enter a 'mode'. AKA not highlight annotations

Minh-Ng · 2017-08-29T03:50:48Z

Changed controller names
Controllers instantiated inside of BoxAnnotations and loaded through annotatorConf
Handler array uses an array of event names rather than one string being parsed
Bind selection handler to annotator.annotatedElement

…letion

…yen/box-content-preview into feature/drawingDeletion

pramodsum

🎉

pramodsum · 2017-08-30T16:29:55Z

src/lib/annotations/AnnotationModeController.js

+        const handlers = this.setupAndGetHandlers();
+        handlers.forEach((handler) => {
+            const types = handler.type instanceof Array ? handler.type : [handler.type];
+            types.forEach((eventName) => handler.eventObj.addEventListener(eventName, handler.func));


new line before forEach and maybe style it like this

types.forEach((eventName) => { ... });

pramodsum · 2017-08-30T16:31:00Z

src/lib/annotations/AnnotationModeController.js

+        while (this.handlers.length > 0) {
+            const handler = this.handlers.pop();
+            const types = handler.type instanceof Array ? handler.type : [handler.type];
+            types.forEach((eventName) => {


new line before forEach

pramodsum · 2017-08-30T16:33:12Z

src/lib/annotations/Annotator.js

@@ -167,6 +170,11 @@ class Annotator extends EventEmitter {

            const handler = this.getAnnotationModeClickHandler(currentMode);
            annotateButtonEl.addEventListener('click', handler);
+
+            const { CONTROLLERS } = this.options.annotator;


What if this.options.annotator is null?

Also might make sense to store each valid mode controller as this.modeControllersso you're not constantly checking if this.options.annotator and CONTROLLERS exist

This.options.annotator can't be null because of the isModeAnnotatable check above

pramodsum · 2017-08-30T16:37:34Z

src/lib/annotations/Annotator.js

     * @return {void}
     */
-    unbindModeListeners() {
+    unbindModeListeners(mode) {
        while (this.annotationModeHandlers.length > 0) {


What would be the difference between removing these listeners vs. calling CONTROLLERS[mode]. unbindModeListeners()? Does this part just remove point annotation listeners (I assume until we create a modeController for point annotation mode as well)?

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()

pramodsum · 2017-08-30T16:43:32Z

src/lib/annotations/BoxAnnotations.js

+        annotatorConfig.CONTROLLERS = {};
+        annotatorConfig.TYPE.forEach((type) => {
+            if (type in ANNOTATOR_TYPE_CONTROLLERS) {
+                annotatorConfig.CONTROLLERS[type] = new ANNOTATOR_TYPE_CONTROLLERS[type]();


annotatorConfig.CONTROLLERS[type] = new ANNOTATOR_TYPE_CONTROLLERS[type].CONSTRUCTOR();

pramodsum · 2017-08-30T16:51:32Z

src/lib/annotations/doc/DocDrawingThread.js

     * @param {Object} location - The location information of the pointer
     * @return {void}
     */
    handleMove(location) {
-        if (this.drawingFlag !== DRAW_STATES.drawing) {
+        if (this.drawingFlag !== DRAW_STATES.drawing || !location) {


Would this not have a valid location at some point?

It would not have a valid location upon moving the mouse on the side of the document.

pramodsum · 2017-08-30T16:56:53Z

src/lib/annotations/drawing/DrawingContainer.js

+     *
+     * @return {Object} The object with the boundaries and the path
+     */
+    getAABB() {


getAxisAlignedBoundingBox() or getAlignedBoundingBox()

pramodsum · 2017-08-30T17:03:05Z

src/lib/annotations/drawing/DrawingModeController.js

+            }
+        );
+
+        if (this.postButtonEl) {


move these into their own helper method and just call it for each button. May even make sense to move it to the base class!

addButtonHandlers(buttonEl, handler) { if (buttonEl) { const buttonHandler = { type: 'click, func: handler, eventObj: buttonEl }; handlers.push(buttonHandler); }

pramodsum · 2017-08-30T17:08:19Z

src/lib/annotations/drawing/DrawingModeController.js

+        };
+
+        // Get the threads that correspond to the point that was clicked on
+        const intersectingThreads = this.threads


is drawController.threads an array?

It is an array by default (AnnotationModeController) and an RTree in DrawingModeController

pramodsum · 2017-08-30T17:10:11Z

src/lib/annotations/drawing/DrawingThread.js

+
+        // Calculate the bounding rectangle
+        const [x, y, width, height] = this.getRectangularBoundary();
+        // Clear the drawn thread and destroy it


new line before comment

Minh Nguyen and others added 30 commits July 25, 2017 21:36

Fix: thread save incorrectly rejects when no dialogue exists

12cc4ed

Fix: error attempting to set dialogue threadnum when no dialogue exists

1872d82

Merge branch 'master' into branch/rebaseToRemote

6db4184

Merge branch 'master' into branch/rebaseToRemote

48af9ac

Update: drawing annotations now rescale correctly

533030c

Fix: annotator is now loaded with the correct initial scale

7f661ea

Fix: update annotator tests for constructor change

7230510

Update: drawingannotation rescaling

b90659a

Merge branch 'master' into branch/rebaseToRemote

000b503

Merge remote-tracking branch 'upstream/master' into feature/drawingAn…

48b5589

…notationScaling

Update: tests for updating local annotations on annotationthreads

8c88200

Merge branch 'branch/rebaseToRemote' of https://github.com/MinhHNguye…

f3b557c

…n/box-content-preview into branch/rebaseToRemote

Fix: typo

ed403b8

Update: merge initial scaling changes

22d4faa

Merge branch 'branch/rebaseToRemote' into feature/drawingAnnotationSc…

4be971e

…aling

Update: fix tests

51b0201

Fix: clean up anonymous functions

0757c0b

Fix: clean up anonymous functions

e4a097b

Update: merge from master

1075f78

Update: tests for docdrawingthread

f25c34c

Update: change page change method

70d619c

Fix: various cleanup for PR

833a42a

Fix: removing fixes coming in a different PR

fe3587d

Fix: sentenced to 1 year of unit tests

924cd25

Merge branch 'master' into feature/drawingAnnotationScaling

3549473

Update: docdrawingthread documentation

1857b90

Merge branch 'feature/drawingAnnotationScaling' of https://github.com…

cb87a9e

…/MinhHNguyen/box-content-preview into feature/drawingAnnotationScaling

Merge branch 'master' into feature/drawingAnnotationScaling

5402f45

Merge branch 'master' into feature/drawingAnnotationScaling

67e250f

Update: merge from upstream master

bf29d9f

Minh Nguyen added 11 commits August 15, 2017 15:36

Merge branch 'feature/undoredo' of https://github.com/MinhHNguyen/box…

53aed0e

…-content-preview into feature/undoredo

New: drawing deletion

45ce77b

Update: resolve merge conflict

91bffcc

Update: updates

b369165

Update: resolve merge from master

8c8e158

Merge remote-tracking branch 'upstream/master' into feature/drawingDe…

303bb2d

…letion

Update: separate drawing methods

d2eddee

Merge remote-tracking branch 'upstream/master' into feature/drawingDe…

f9d083d

…letion

Update: tests

f85d2e8

Merge branch 'master' into feature/drawingDeletion

75b021e

Update: complete tests

c7b2b35

pramodsum suggested changes Aug 25, 2017

View reviewed changes

jeremypress reviewed Aug 27, 2017

View reviewed changes

pramodsum reviewed Aug 28, 2017

View reviewed changes

Minh Nguyen added 2 commits August 28, 2017 17:56

Update: Change how controllers are instantiated

9d1ac3d

Update: resolve merge conflict in BoxAnnotations

b8fdc3d

Minh Nguyen and others added 7 commits August 29, 2017 13:02

Merge remote-tracking branch 'upstream/master' into feature/drawingDe…

9402fa2

…letion

Update: bind draw selection to annotated element

48e4bfb

Merge branch 'master' into feature/drawingDeletion

9268715

Update: resolve merge conflict from master

9314a41

Merge branch 'master' into feature/drawingDeletion

d5f99a6

Merge remote-tracking branch 'upstream/master' into feature/drawingDe…

34ff020

…letion

Merge branch 'feature/drawingDeletion' of https://github.com/MinhHNgu…

d0c4427

…yen/box-content-preview into feature/drawingDeletion

pramodsum reviewed Aug 30, 2017

View reviewed changes

Minh Nguyen and others added 2 commits August 30, 2017 15:33

Update: pr updates

4d94bf4

Merge branch 'master' into feature/drawingDeletion

beb8150

pramodsum approved these changes Aug 30, 2017

View reviewed changes

Minh-Ng merged commit 34b9da1 into box:master Aug 30, 2017

Minh-Ng deleted the feature/drawingDeletion branch August 30, 2017 23:52

		@@ -0,0 +1,146 @@
		import EventEmitter from 'events';

		class AnnotationController extends EventEmitter {

Feature: drawing annotation deletion #337

Feature: drawing annotation deletion #337

Conversation

Minh-Ng commented Aug 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Minh-Ng Aug 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Minh-Ng Aug 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Minh-Ng commented Aug 29, 2017 • edited Loading

pramodsum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Minh-Ng Aug 28, 2017 •

edited

Loading

Minh-Ng Aug 28, 2017 •

edited

Loading

Minh-Ng commented Aug 29, 2017 •

edited

Loading