Fix: Add Annotations bundling changes back #95

pramodsum · 2017-05-01T22:15:59Z

No description provided.

jeremypress · 2017-05-03T16:45:07Z

src/lib/viewers/BaseViewer.js

@@ -255,6 +268,12 @@ class BaseViewer extends EventEmitter {
        this.addListener('togglepointannotationmode', () => {
            this.annotator.togglePointModeHandler();
        });
+
+        this.addListener('load', () => {


Remove this in "destroy"

Shouldn't need to specifically remove this when you have removeAllListeners() without a param.

- Reverts the commit (176ad13) which reverted the annotations bundling changes - Fixes performance problems from 'load' event

tonyjin · 2017-05-08T19:40:04Z

src/lib/annotations/Annotator.js

+            fileId,
+            token,
+            canAnnotate: this.canAnnotate
+        });
    }


I don't think breaking this out into a function is super valuable.

tonyjin · 2017-05-08T19:41:42Z

src/lib/annotations/Annotator.js

@@ -65,8 +65,29 @@ class Annotator extends EventEmitter {
     * @return {void}
     */
    init() {
+        this.annotatedElement = this.getAnnotatedEl(this.container);
+        this.annotationService = this.initAnnotationService(this.options);
+        this.notification = new Notification(this.annotatedElement);


const { apiHost, fileId, token } = this.options; this.annotatedEl = this.getAnnotatedEl(this.container); this.notification = new Notification(this.annotatedEl); this.annotationService = new AnnotationService({ apiHost, fileId, token, canAnnotation: this.canAnnotate });

tonyjin · 2017-05-08T19:45:12Z

src/lib/viewers/BaseViewer.js

@@ -83,6 +96,8 @@ class BaseViewer extends EventEmitter {
        fullscreen.removeAllListeners();
        document.defaultView.removeEventListener('resize', this.debouncedResizeHandler);
        this.removeAllListeners();
+        this.removeAllListeners('togglepointannotationmode');
+        this.removeAllListeners('load');


Don't need these two - removeAllListeners() without an argument removes all listeners.

tonyjin · 2017-05-08T19:45:38Z

src/lib/viewers/BaseViewer.js

@@ -255,6 +268,12 @@ class BaseViewer extends EventEmitter {
        this.addListener('togglepointannotationmode', () => {
            this.annotator.togglePointModeHandler();
        });
+
+        this.addListener('load', () => {


Shouldn't need to specifically remove this when you have removeAllListeners() without a param.

tonyjin · 2017-05-08T19:47:27Z

src/lib/viewers/BaseViewer.js

+    }
+
+    /**
+     * Returns whether or not viewer has the option to annotate


Returns whether or not annotations are enabled for this viewer.

tonyjin · 2017-05-08T20:02:48Z

src/lib/viewers/BaseViewer.js

+        // Construct and init annotator
+        this.annotator = new this.annotatorLoader.CONSTRUCTOR({
+            canAnnotate,
+            container: this.options.container,


No need for this.options when we destructure above

tonyjin · 2017-05-08T20:03:17Z

src/lib/viewers/BaseViewer.js

+            apiHost,
+            fileId: file.id,
+            token
+        };


Just move these into the annotator constructor options.

tonyjin · 2017-05-08T20:03:31Z

src/lib/viewers/BaseViewer.js

+        this.annotator = new this.annotatorLoader.CONSTRUCTOR({
+            canAnnotate,
+            container: this.options.container,
+            options: annotationOptions,


options: { apiHost, fileId: file.id, token, }

tonyjin · 2017-05-08T20:04:36Z

src/lib/viewers/BaseViewer.js

+
+    /**
+     * Returns whether or not viewer is annotatable. If an optional type is
+     * passed in, we check if that type of annotation is allowed.


Returns whether or not viewer both supports annotations and has annotations enabled.

tonyjin · 2017-05-08T20:05:30Z

src/lib/viewers/BaseViewer.js

+     * @return {boolean} Whether or not viewer is annotatable
+     */
+    isAnnotatable(type) {
+        if (type && this.annotationTypes) {


Do we pass isAnnotatable with a type anywhere anymore?

getPointModeClickHandler() in DocBaseViewer and ImageViewer

Use const { TYPE: annotationTypes } = this.annotatorConf; after you remove this.annotationTypes

jeremypress · 2017-05-08T19:05:06Z

src/lib/viewers/BaseViewer.js

+        // Attempts to load annotations assets and initializes annotations if
+        // the assets are available, the showAnnotations flag is true, and the
+        // expiring embed is not a shared link
+        // TODO(@spramod): Determine the expected behavior on shared links


Has there been any update on this?

asked by Tony...

jeremypress · 2017-05-08T19:08:09Z

src/lib/viewers/BaseViewer.js

@@ -83,6 +96,8 @@ class BaseViewer extends EventEmitter {
        fullscreen.removeAllListeners();
        document.defaultView.removeEventListener('resize', this.debouncedResizeHandler);
        this.removeAllListeners();
+        this.removeAllListeners('togglepointannotationmode');


Actually I think I was wrong here. It looks like when you call removeAllListeners with no arguments it removes all listeners bound. https://nodejs.org/api/events.html#events_emitter_removealllisteners_eventname

asked by Tony...

jeremypress · 2017-05-08T20:05:04Z

src/lib/viewers/BaseViewer.js

@@ -255,6 +270,12 @@ class BaseViewer extends EventEmitter {
        this.addListener('togglepointannotationmode', () => {
            this.annotator.togglePointModeHandler();
        });
+
+        this.addListener('load', () => {
+            if (window.BoxAnnotations !== undefined && this.isViewerAnnotatable()) {


Can we just do (if window.boxAnnotations && this.isViewerAnnotatable())?

jeremypress · 2017-05-08T20:07:49Z

src/lib/viewers/BaseViewer.js

+        const fileVersionID = file.file_version.id;
+
+        // Users can currently only view annotations on mobile
+        const canAnnotate = checkPermission(file, PERMISSION_ANNOTATE) && !Browser.isMobile();


Maybe extract this out to a class value since we do it twice?

jeremypress · 2017-05-08T20:10:33Z

src/lib/viewers/BaseViewer.js

+    isViewerAnnotatable() {
+        // Respect viewer-specific annotation option if it is set
+        const viewerAnnotations = this.getViewerOption('annotations');
+        if (typeof viewerAnnotations === 'boolean') {


What else could viewerAnnotations be?

The only other option would be undefined, this is really just double checking against if developers set it to anything other than a boolean

jeremypress · 2017-05-08T20:13:10Z

src/lib/viewers/BaseViewer.js

+
+        annotateButtonEl.title = __('annotation_point_toggle');
+        annotateButtonEl.classList.remove(CLASS_HIDDEN);
+        annotateButtonEl.addEventListener('click', handler);


This one should be removed separately since it's a DOM event listener.

You can't actually remove the event listener unless you also save a reference to the handler. Unlike event emitter event listeners, DOM listeners can't be removed unless you specific the handler as the second param.

I don't think we ever removed this handler in the previous implementation... to do so we'd have to do:

this.annotateClickHandler = handler; ... annotateButtonEl.addEventListener('click', this.annotateClickHandler);

And in the cleanup, remove with:

annotateButtonEl.removeEventListener('click', this.annotateClickHandler);

Don't have to null check for this.annotateClickHandler - if it doesn't exist, removeEventListener doesn't do anything.

jeremypress · 2017-05-08T20:14:36Z

src/lib/viewers/BaseViewer.js

+     * @return {Function|null} Click handler
+     */
+    /* eslint-disable no-unused-vars */
+    getPointModeClickHandler(containerEl) {}


Missing the containerEl param in the JSDoc, maybe use @inheritdoc here?

tonyjin · 2017-05-08T23:28:14Z

src/lib/viewers/BaseViewer.js

+        // the assets are available, the showAnnotations flag is true, and the
+        // expiring embed is not a shared link
+        if (this.areAnnotationsEnabled() && !this.options.sharedLink) {
+            this.loadAssets(ANNOTATIONS_JS);


There's potentially a timing issue if the document loads before annotations JS is ready. Doesn't seem super likely but it's possible.

Let's leave this for now - I think we could hook into each viewer's loadAssets if needed but it gets a bit complicated.

Yea it happens very rarely but it does happen. I can look into this during the refactor

tonyjin · 2017-05-08T23:29:36Z

src/lib/viewers/BaseViewer.js

+        /* global BoxAnnotations */
+        this.loader = new BoxAnnotations();
+
+        this.annotatorLoader = this.loader.determineAnnotator(this.options.viewer.NAME);


Naming is confusing between this.loader and this.annotatorLoader.

Do you use this.loader anywhere else? If not I'd do:

const boxAnnotations = new BoxAnnotations; this.annotatorConf = boxAnnotations.determineAnnotator(this.options.viewer.NAME); if (!this.annotatorConf) { return; }

tonyjin · 2017-05-08T23:32:21Z

src/lib/viewers/BaseViewer.js

+            return;
+        }
+
+        this.annotationTypes = this.annotatorLoader.TYPE;


If it's already available on this.annotatorConf.TYPE, I wouldn't define it again

tonyjin · 2017-05-08T23:37:14Z

src/lib/viewers/BaseViewer.js

+     */
+    initAnnotations() {
+        const { apiHost, container, file, location, token } = this.options;
+        const fileVersionID = file.file_version.id;


const { id: fileId, file_version: { id: fileVersionId } } = file;

This creates two const variables, fileId and fileVersionId, set to file.id and file.file_version.id respectively. Use camelCase Id instead of ID - I know annotations code base uses fileVersionID and threadID and annotationID right now, we can update that later, but the rest of preview uses Id

oh cool! Didn't know I could do that!

I also made sure to fix fileID and fileVersionID everywhere in BoxAnnotations.js so this shouldn't be anywhere in annotations anymore

tonyjin · 2017-05-08T23:37:33Z

src/lib/viewers/BaseViewer.js

+     * @return {boolean} Whether or not viewer is annotatable
+     */
+    isAnnotatable(type) {
+        if (type && this.annotationTypes) {


Use const { TYPE: annotationTypes } = this.annotatorConf; after you remove this.annotationTypes

tonyjin · 2017-05-08T23:39:06Z

src/lib/viewers/BaseViewer.js

+            }
+        }
+
+        // Respect viewer-specific annotation option if it is set


// Return whether or not annotations are enabled for this viewer

tonyjin · 2017-05-08T23:40:59Z

src/lib/viewers/BaseViewer.js

+
+            if (!supportedType) {
+                return false;
+            }


You can simplify this to:

if (!annotationTypes.some((annotationType) => type === annotationType)) { return false; }

tonyjin · 2017-05-08T23:45:46Z

src/lib/viewers/BaseViewer.js

+
+        annotateButtonEl.title = __('annotation_point_toggle');
+        annotateButtonEl.classList.remove(CLASS_HIDDEN);
+        annotateButtonEl.addEventListener('click', handler);


You can't actually remove the event listener unless you also save a reference to the handler. Unlike event emitter event listeners, DOM listeners can't be removed unless you specific the handler as the second param.

I don't think we ever removed this handler in the previous implementation... to do so we'd have to do:

this.annotateClickHandler = handler; ... annotateButtonEl.addEventListener('click', this.annotateClickHandler);

And in the cleanup, remove with:

annotateButtonEl.removeEventListener('click', this.annotateClickHandler);

Don't have to null check for this.annotateClickHandler - if it doesn't exist, removeEventListener doesn't do anything.

- Replacing fileVersionID with fileVersionId (same with fileID) - Making sure isAnnotatable() is not overridden in Viewers

jeremypress suggested changes May 3, 2017

View reviewed changes

pramodsum added 4 commits May 7, 2017 12:53

Fix: Add Annotations bundling changes back

9700a23

- Reverts the commit (176ad13) which reverted the annotations bundling changes - Fixes performance problems from 'load' event

Fix: No longer emit extra 'load' event on 'textlayerrendered'

8edc335

Fix: Clean up check for BoxAnnotations in window

3b89a6c

Chore: Cleaning up listeners in BaseViewer

52e1e36

pramodsum mentioned this pull request May 7, 2017

New: Multi Image Annotations #73

Closed

tonyjin reviewed May 8, 2017

View reviewed changes

jeremypress reviewed May 8, 2017

View reviewed changes

Chore: Fixes after comments

2016df9

tonyjin reviewed May 8, 2017

View reviewed changes

jeremypress approved these changes May 9, 2017

View reviewed changes

Chore: Cleaning up commits from comments

db448d8

- Replacing fileVersionID with fileVersionId (same with fileID) - Making sure isAnnotatable() is not overridden in Viewers

pramodsum merged commit bbc6ea3 into box:master May 9, 2017

Fix: Add Annotations bundling changes back #95

Fix: Add Annotations bundling changes back #95

Conversation

pramodsum commented May 1, 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

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

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