Skip to content

Commit

Permalink
Fix: init annotations after file rep is fetched (#842)
Browse files Browse the repository at this point in the history 
* Fix: construct annotator when BoxAnnotations loads

* Chore: address PR comment

* Chore: addressing CR comments

* Chore: fetch annotations lib after rep is fetched

* Chore: make loading of annotations work with custom BoxAnnotations
  • Loading branch information
Conrad Chan authored Sep 17, 2018
1 parent 912c9c5 commit 20cc3c5
Show file tree
Hide file tree
Showing 5 changed files with 93 additions and 72 deletions.
69 changes: 43 additions & 26 deletions src/lib/viewers/BaseViewer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -138,8 +138,10 @@ class BaseViewer extends EventEmitter {
this.mobileZoomChangeHandler = this.mobileZoomChangeHandler.bind(this);
this.mobileZoomEndHandler = this.mobileZoomEndHandler.bind(this);
this.handleAnnotatorEvents = this.handleAnnotatorEvents.bind(this);
this.annotationsLoadHandler = this.annotationsLoadHandler.bind(this);
this.createAnnotator = this.createAnnotator.bind(this);
this.viewerLoadHandler = this.viewerLoadHandler.bind(this);
this.initAnnotations = this.initAnnotations.bind(this);
this.loadBoxAnnotations = this.loadBoxAnnotations.bind(this);
}

/**
Expand Down Expand Up @@ -176,11 +178,12 @@ class BaseViewer extends EventEmitter {
this.containerEl.classList.add(CLASS_BOX_PREVIEW_MOBILE);
}

// 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
// Creates a promise that the annotator will be constructed if annotations are
// enabled and the expiring embed is not a shared link
if (this.areAnnotationsEnabled() && !this.options.sharedLink) {
this.loadAnnotator();
this.annotatorPromise = new Promise((resolve) => {
this.annotatorPromiseResolver = resolve;
});
}
}

Expand Down Expand Up @@ -232,6 +235,8 @@ class BaseViewer extends EventEmitter {
}

this.destroyed = true;
this.annotatorPromise = null;
this.annotatorPromiseResolver = null;
this.emit('destroy');
}

Expand Down Expand Up @@ -482,8 +487,9 @@ class BaseViewer extends EventEmitter {
this.scale = event.scale;
}

if (this.annotationsLoadPromise) {
this.annotationsLoadPromise.then(this.annotationsLoadHandler).catch(() => {});
// Ensures that the annotator has been created first
if (this.annotatorPromise) {
this.annotatorPromise.then(this.initAnnotations);
}
}

Expand Down Expand Up @@ -826,27 +832,34 @@ class BaseViewer extends EventEmitter {
//--------------------------------------------------------------------------

/**
* Loads the appropriate annotator and loads the file's annotations
* Loads the BoxAnnotations static assets
*
* @protected
* @return {void}
* @return {Promise} promise that is resolved when the assets are loaded
*/
loadAnnotator() {
// Auto-resolves promise if BoxAnnotations is passed in as a Preview option
this.annotationsLoadPromise =
window.BoxAnnotations && this.options.boxAnnotations instanceof window.BoxAnnotations
? Promise.resolve()
: this.loadAssets([ANNOTATIONS_JS], [ANNOTATIONS_CSS], false);
loadBoxAnnotations() {
if (
!this.options.showAnnotations ||
(window.BoxAnnotations && this.options.boxAnnotations instanceof window.BoxAnnotations)
) {
return Promise.resolve();
}

return this.loadAssets([ANNOTATIONS_JS], [ANNOTATIONS_CSS], false);
}

/**
* Fetches the Box Annotations library. Creates an instance of BoxAnnotations
* if one isn't passed in to the preview options
* Creates an instance of BoxAnnotations if one isn't passed in to the preview options
* and instantiates the appropriate annotator
*
* @protected
* @return {void}
*/
annotationsLoadHandler() {
createAnnotator() {
if (!this.options.showAnnotations) {
return;
}

// Set viewer-specific annotation options
const viewerOptions = {};
viewerOptions[this.options.viewer.NAME] = this.viewerConfig;
Expand All @@ -860,8 +873,18 @@ class BaseViewer extends EventEmitter {
const boxAnnotations = this.options.boxAnnotations || new global.BoxAnnotations(viewerOptions);
this.annotatorConf = boxAnnotations.determineAnnotator(this.options, this.viewerConfig);

if (this.annotatorConf) {
this.initAnnotations();
if (!this.annotatorConf) {
return;
}

const annotatorOptions = this.createAnnotatorOptions({
annotator: this.annotatorConf,
modeButtons: ANNOTATION_BUTTONS
});
this.annotator = new this.annotatorConf.CONSTRUCTOR(annotatorOptions);

if (this.annotatorPromiseResolver) {
this.annotatorPromiseResolver();
}
}

Expand All @@ -872,12 +895,6 @@ class BaseViewer extends EventEmitter {
* @return {void}
*/
initAnnotations() {
// Construct and init annotator
const annotatorOptions = this.createAnnotatorOptions({
annotator: this.annotatorConf,
modeButtons: ANNOTATION_BUTTONS
});
this.annotator = new this.annotatorConf.CONSTRUCTOR(annotatorOptions);
this.annotator.init(this.scale);

// Once the annotator instance has been created, emit it so that clients can attach their events.
Expand Down
90 changes: 44 additions & 46 deletions src/lib/viewers/__tests__/BaseViewer-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ describe('lib/viewers/BaseViewer', () => {
sandbox.stub(base, 'addCommonListeners');
sandbox.stub(base, 'areAnnotationsEnabled').returns(true);
sandbox.stub(base, 'finishLoadingSetup');
sandbox.stub(base, 'loadAnnotator');
sandbox.stub(base, 'loadBoxAnnotations').returns(Promise.resolve());
base.options.showAnnotations = true;

base.setup();
Expand All @@ -79,12 +79,13 @@ describe('lib/viewers/BaseViewer', () => {
expect(base.addCommonListeners).to.be.called;
expect(getIconFromExtensionStub).to.be.called;
expect(base.loadTimeout).to.be.a('number');
expect(base.loadAnnotator).to.be.called;
expect(base.annotatorPromise).to.not.be.undefined;
expect(base.annotatorPromiseResolver).to.not.be.undefined;
});

it('should add a mobile class to the container if on mobile', () => {
base.isMobile = true;
sandbox.stub(base, 'loadAnnotator');
sandbox.stub(base, 'loadBoxAnnotations').returns(Promise.resolve());
sandbox.stub(base, 'finishLoadingSetup');
sandbox.stub(base, 'areAnnotationsEnabled').returns(true);

Expand All @@ -97,25 +98,25 @@ describe('lib/viewers/BaseViewer', () => {
it('should not load annotations assets if global preview showAnnotations option is false', () => {
sandbox.stub(base, 'addCommonListeners');
sandbox.stub(base, 'areAnnotationsEnabled').returns(false);
sandbox.stub(base, 'loadAnnotator');
sandbox.stub(base, 'loadBoxAnnotations').returns(Promise.resolve());
sandbox.stub(base, 'finishLoadingSetup');
base.options.showAnnotations = false;

base.setup();

expect(base.loadAnnotator).to.not.be.called;
expect(base.loadBoxAnnotations).to.not.be.called;
});

it('should not load annotations assets if expiring embed is a shared link', () => {
sandbox.stub(base, 'addCommonListeners');
sandbox.stub(base, 'areAnnotationsEnabled').returns(true);
sandbox.stub(base, 'loadAnnotator');
sandbox.stub(base, 'loadBoxAnnotations').returns(Promise.resolve());
sandbox.stub(base, 'finishLoadingSetup');
base.options.sharedLink = 'url';

base.setup();

expect(base.loadAnnotator).to.not.be.called;
expect(base.loadBoxAnnotations).to.not.be.called;
});
});

Expand Down Expand Up @@ -453,8 +454,6 @@ describe('lib/viewers/BaseViewer', () => {

describe('viewerLoadHandler()', () => {
beforeEach(() => {
base.annotationsLoadPromise = Promise.resolve();
stubs.annotationsLoadHandler = sandbox.stub(base, 'annotationsLoadHandler');
base.options.representation = {
content: {
url_template: 'dl.boxcloud.com'
Expand All @@ -463,6 +462,7 @@ describe('lib/viewers/BaseViewer', () => {
stubs.getDownloadNotificationToShow = sandbox
.stub(DownloadReachability, 'getDownloadNotificationToShow')
.returns(undefined);
sandbox.stub(base, 'initAnnotations');
});

it('should show the notification if downloads are degraded and we have not shown the notification yet', () => {
Expand All @@ -485,27 +485,19 @@ describe('lib/viewers/BaseViewer', () => {
expect(base.scale).to.equal(1.5);
});

it('should handle the annotations load promise', () => {
base.viewerLoadHandler({ scale: 1.5 });
return base.annotationsLoadPromise.then(() => {
expect(base.annotationsLoadHandler).to.be.called;
it('should show annotations if annotatorPromise exists', (done) => {
base.annotatorPromise = new Promise((resolve) => {
resolve();
done();
});
base.viewerLoadHandler({ scale: 1.5 });
expect(base.initAnnotations).to.be.called;
});

it('should handle the annotations load promise', () => {
stubs.annotationsLoadHandler.callsFake(() => {
throw new Error('message');
});

it('should show annotations if annotatorPromise does not exist', () => {
base.annotatorPromise = null;
base.viewerLoadHandler({ scale: 1.5 });
return base.annotationsLoadPromise
.then(() => {
sinon.assert.failException;
})
.catch((error) => {
expect(error).to.be.an('error');
expect(error.message).to.equal('message');
});
expect(base.initAnnotations).to.not.be.called;
});
});

Expand Down Expand Up @@ -580,7 +572,7 @@ describe('lib/viewers/BaseViewer', () => {

it('should cleanup the base viewer', () => {
sandbox.stub(base, 'loadAssets').returns(Promise.resolve());
sandbox.stub(base, 'loadAnnotator');
sandbox.stub(base, 'loadBoxAnnotations').returns(Promise.resolve());
sandbox.stub(base, 'finishLoadingSetup');
base.setup();

Expand Down Expand Up @@ -667,7 +659,7 @@ describe('lib/viewers/BaseViewer', () => {
});
sandbox.stub(base, 'loadAssets').returns(Promise.resolve());
sandbox.stub(base, 'areAnnotationsEnabled').returns(false);
sandbox.stub(base, 'loadAnnotator');
sandbox.stub(base, 'loadBoxAnnotations').returns(Promise.resolve());
sandbox.stub(base, 'finishLoadingSetup');
base.setup();
event = {
Expand Down Expand Up @@ -999,7 +991,7 @@ describe('lib/viewers/BaseViewer', () => {
});
});

describe('loadAnnotator()', () => {
describe('loadBoxAnnotations()', () => {
const conf = {
annotationsEnabled: true,
types: {
Expand All @@ -1009,61 +1001,67 @@ describe('lib/viewers/BaseViewer', () => {
};

beforeEach(() => {
sandbox.stub(base, 'loadAssets');
sandbox.stub(base, 'loadAssets').returns(Promise.resolve());
window.BoxAnnotations = function BoxAnnotations() {
this.determineAnnotator = sandbox.stub().returns(conf);
};
});

it('should resolve the promise if a BoxAnnotations instance was passed into Preview', (done) => {
it('should resolve the promise if a BoxAnnotations instance was passed into Preview', () => {
base.options.boxAnnotations = new window.BoxAnnotations({});

base.loadAnnotator();
base.loadBoxAnnotations();
expect(base.loadAssets).to.not.be.calledWith(['annotations.js']);
base.annotationsLoadPromise.then(() => done());
});

it('should load the annotations assets', () => {
base.loadAnnotator();
it('should load the annotations assets if showAnnotations option is true', () => {
base.options.showAnnotations = true;
base.loadBoxAnnotations();
expect(base.loadAssets).to.be.calledWith(['annotations.js'], ['annotations.css'], false);
});

it('should not load the annotations assets if showAnnotations option is false', () => {
base.options.showAnnotations = false;
base.loadBoxAnnotations();
expect(base.loadAssets).to.not.be.called;
});
});

describe('annotationsLoadHandler()', () => {
describe('createAnnotator()', () => {
const annotatorMock = {};
const conf = {
annotationsEnabled: true,
types: {
point: true,
highlight: false
}
},
CONSTRUCTOR: sandbox.stub().returns(annotatorMock)
};

beforeEach(() => {
base.options.viewer = { NAME: 'viewerName' };
base.options.location = { locale: 'en-US' };
base.options.showAnnotations = true;
window.BoxAnnotations = function BoxAnnotations() {
this.determineAnnotator = sandbox.stub().returns(conf);
};

sandbox.stub(base, 'initAnnotations');
});

it('should determine the annotator', () => {
base.annotationsLoadHandler();
it('should determine and instantiate the annotator', () => {
base.createAnnotator();
expect(base.annotatorConf).to.equal(conf);
expect(base.annotator).to.equal(annotatorMock);
});

it('should not instantiate an instance of BoxAnnotations if one is already passed in', () => {
base.options.boxAnnotations = {
determineAnnotator: sandbox.stub()
determineAnnotator: sandbox.stub().returns(conf)
};
base.annotationsLoadHandler();
base.createAnnotator();
expect(base.options.boxAnnotations.determineAnnotator).to.be.called;
});

it('should init annotations if a conf is present', () => {
base.annotationsLoadHandler();
expect(base.initAnnotations).to.be.called;
});
});

describe('initAnnotations()', () => {
Expand Down
2 changes: 2 additions & 0 deletions src/lib/viewers/doc/DocBaseViewer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,8 @@ class DocBaseViewer extends BaseViewer {

return Promise.all([this.loadAssets(JS, CSS), this.getRepStatus().getPromise()])
.then(this.handleAssetAndRepLoad)
.then(this.loadBoxAnnotations)
.then(this.createAnnotator)
.catch(this.handleAssetError);
}

Expand Down
2 changes: 2 additions & 0 deletions src/lib/viewers/doc/__tests__/DocBaseViewer-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -410,12 +410,14 @@ describe('src/lib/viewers/doc/DocBaseViewer', () => {
sandbox.stub(docBase, 'handleAssetAndRepLoad');
sandbox.stub(docBase, 'getRepStatus').returns({ getPromise: () => Promise.resolve() });
sandbox.stub(docBase, 'loadAssets');
sandbox.stub(docBase, 'loadBoxAnnotations');

return docBase.load().then(() => {
expect(docBase.loadAssets).to.be.called;
expect(docBase.setup).to.be.called;
expect(docBase.createContentUrlWithAuthParams).to.be.calledWith('foo');
expect(docBase.handleAssetAndRepLoad).to.be.called;
expect(docBase.loadBoxAnnotations).to.be.called;
});
});
});
Expand Down
2 changes: 2 additions & 0 deletions src/lib/viewers/image/ImageViewer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ class ImageViewer extends ImageBaseViewer {
this.startLoadTimer();
this.imageEl.src = downloadUrl;
})
.then(this.loadBoxAnnotations)
.then(this.createAnnotator)
.catch(this.handleAssetError);
}

Expand Down

0 comments on commit 20cc3c5

Please sign in to comment.