Commit d17e2546 authored by Robert Knight's avatar Robert Knight

Simplify annotator plugin initialization

Previously plugin initialization happened in several steps:

 1. The plugin constructor was called with the root element and config
    options
 2. The plugin's `annotator` property was set to a reference to the
    `Guest` instance for the current document
 3. The plugin's `pluginInit` method was called

There was no real need for this multi-step process. This commit
simplifies it by removing the `pluginInit` method and passing the guest
reference as a third argument to the constructor in step 1.

As a result, the `Plugin` base class is no longer needed. Plugins now
extend the parent `Delegator` class instead.
parent edc9713b
......@@ -133,7 +133,6 @@ export default class Guest extends Delegator {
this.addPlugin('CrossFrame', cfOptions);
this.crossframe = this.plugins.CrossFrame;
this.crossframe.onConnect(() => this._setupInitialState(config));
// Whether clicks on non-highlighted text should close the sidebar
......@@ -214,9 +213,7 @@ export default class Guest extends Delegator {
addPlugin(name, options) {
const Klass = this.options.pluginClasses[name];
this.plugins[name] = new Klass(this.element, options);
this.plugins[name].annotator = this;
this.plugins[name].pluginInit?.();
this.plugins[name] = new Klass(this.element, options, this);
}
// Get the document info
......
{ default: Delegator } = require('./delegator')
module.exports = class Plugin extends Delegator
constructor: (element, options) ->
super
pluginInit: () ->
$ = require('jquery')
Plugin = require('../plugin')
{ default: Delegator } = require('../delegator')
scrollIntoView = require('scroll-into-view')
......@@ -18,7 +18,7 @@ scrollToClosest = (anchors, direction) ->
scrollIntoView(closest.highlights[0])
module.exports = class BucketBar extends Plugin
module.exports = class BucketBar extends Delegator
# svg skeleton
html: """
<div class="annotator-bucket-bar">
......@@ -34,7 +34,7 @@ module.exports = class BucketBar extends Plugin
# tab elements
tabs: null
constructor: (element, options) ->
constructor: (element, options, annotator) ->
defaultOptions = {
# gapSize parameter is used by the clustering algorithm
# If an annotation is farther then this gapSize from the next bucket
......@@ -51,9 +51,9 @@ module.exports = class BucketBar extends Plugin
else
$(element).append @element
@annotator = annotator
@highlighter = options.highlighter ? highlighter # Test seam.
pluginInit: ->
$(window).on 'resize scroll', @update
for scrollable in @options.scrollables ? []
......
// @ts-expect-error - `Plugin` base class is still written in CoffeeScript
import Plugin from '../plugin';
import AnnotationSync from '../annotation-sync';
import Bridge from '../../shared/bridge';
import Delegator from '../delegator';
import Discovery from '../../shared/discovery';
import * as frameUtil from '../util/frame-util';
import FrameObserver from '../frame-observer';
......@@ -20,7 +18,7 @@ import FrameObserver from '../frame-observer';
* are added to the page if they have the `enable-annotation` attribute set
* and are same-origin with the current document.
*/
export default class CrossFrame extends Plugin {
export default class CrossFrame extends Delegator {
constructor(element, options) {
super(element, options);
......@@ -58,17 +56,11 @@ export default class CrossFrame extends Plugin {
frameIdentifiers.delete(frame);
};
/**
* Initiate the connection to the sidebar.
*/
this.pluginInit = () => {
// Initiate connection to the sidebar.
const onDiscoveryCallback = (source, origin, token) =>
bridge.createChannel(source, origin, token);
discovery.startDiscovery(onDiscoveryCallback);
frameObserver.observe(injectIntoFrame, iframeUnloaded);
};
/**
* Remove the connection between the sidebar and annotator.
......
......@@ -19,8 +19,7 @@
import baseURI from 'document-base-uri';
// @ts-expect-error - '../plugin' needs to be converted to JS.
import Plugin from '../plugin';
import Delegator from '../delegator';
import { normalizeURI } from '../util/url';
......@@ -65,7 +64,7 @@ function createMetadata() {
* DocumentMeta reads metadata/links from the current HTML document and
* populates the `document` property of new annotations.
*/
export default class DocumentMeta extends Plugin {
export default class DocumentMeta extends Delegator {
constructor(element, options) {
super(element, options);
......@@ -81,20 +80,15 @@ export default class DocumentMeta extends Plugin {
this.beforeAnnotationCreated = annotation => {
annotation.document = this.metadata;
};
// @ts-expect-error - Method comes from CoffeeScript base class.
this.subscribe('beforeAnnotationCreated', this.beforeAnnotationCreated);
this.getDocumentMetadata();
}
destroy() {
// @ts-expect-error - Method comes from CoffeeScript base class.
this.unsubscribe('beforeAnnotationCreated', this.beforeAnnotationCreated);
}
pluginInit() {
this.getDocumentMetadata();
}
/**
* Returns the primary URI for the document being annotated
*
......
import debounce from 'lodash.debounce';
import * as pdfAnchoring from '../anchoring/pdf';
import Delegator from '../delegator';
import RenderingStates from '../pdfjs-rendering-states';
// @ts-expect-error - Plugin module is CoffeeScript
import Plugin from '../plugin';
import PDFMetadata from './pdf-metadata';
......@@ -13,17 +12,15 @@ import PDFMetadata from './pdf-metadata';
* @typedef {import('../../types/annotator').HypothesisWindow} HypothesisWindow
*/
export default class PDF extends Plugin {
constructor(element, config) {
super(element, config);
export default class PDF extends Delegator {
/**
* The `Guest` instance for the current document.
* nb. This is initialized by `Guest` before it calls `pluginInit`.
*
* @type {Annotator}
* @param {Annotator} annotator
*/
this.annotator = /** @type {any} */ (null);
constructor(element, config, annotator) {
super(element, config);
this.annotator = annotator;
annotator.anchoring = pdfAnchoring;
const window_ = /** @type {HypothesisWindow} */ (window);
this.pdfViewer = window_.PDFViewerApplication.pdfViewer;
......@@ -40,10 +37,6 @@ export default class PDF extends Plugin {
});
}
pluginInit() {
this.annotator.anchoring = pdfAnchoring;
}
destroy() {
this.pdfViewer.viewer.classList.remove('has-transparent-text-layer');
this.observer.disconnect();
......
......@@ -22,9 +22,17 @@ createMouseEvent = (type, { ctrlKey, metaKey } = {}) ->
describe 'BucketBar', ->
fakeAnnotator = null
beforeEach ->
fakeAnnotator = {
anchors: [],
selectAnnotations: sinon.stub(),
}
createBucketBar = (options) ->
element = document.createElement('div')
new BucketBar(element, options || {})
new BucketBar(element, options || {}, fakeAnnotator)
# Create a fake anchor, which is a combination of annotation object and
# associated highlight elements.
......@@ -36,7 +44,6 @@ describe 'BucketBar', ->
bucketBar = null
fakeHighlighter = null
fakeAnnotator = null
beforeEach ->
fakeHighlighter =
......@@ -44,12 +51,6 @@ describe 'BucketBar', ->
bucketBar = createBucketBar(highlighter: fakeHighlighter)
# This setup is done by `Guest#addPlugin` in the actual app.
bucketBar.annotator = {
anchors: [],
selectAnnotations: sinon.stub(),
}
# Create fake anchors and render buckets.
anchors = [createAnchor()]
bucketBar.annotator.anchors = anchors
......
import Plugin from '../../plugin';
import CrossFrame from '../cross-frame';
import { $imports } from '../cross-frame';
......@@ -42,7 +41,6 @@ describe('CrossFrame', () => {
proxyBridge = sinon.stub().returns(fakeBridge);
$imports.$mock({
'../plugin': Plugin,
'../annotation-sync': proxyAnnotationSync,
'../../shared/bridge': proxyBridge,
'../../shared/discovery': proxyDiscovery,
......@@ -81,18 +79,14 @@ describe('CrossFrame', () => {
emit: sinon.match.func,
});
});
});
describe('#pluginInit', () => {
it('starts the discovery of new channels', () => {
const bridge = createCrossFrame();
bridge.pluginInit();
createCrossFrame();
assert.called(fakeDiscovery.startDiscovery);
});
it('creates a channel when a new frame is discovered', () => {
const bridge = createCrossFrame();
bridge.pluginInit();
createCrossFrame();
fakeDiscovery.startDiscovery.yield('SOURCE', 'ORIGIN', 'TOKEN');
assert.called(fakeBridge.createChannel);
assert.calledWith(fakeBridge.createChannel, 'SOURCE', 'ORIGIN', 'TOKEN');
......
......@@ -37,7 +37,6 @@ describe('DocumentMeta', function () {
document: tempDocument,
normalizeURI: fakeNormalizeURI,
});
testDocument.pluginInit();
});
afterEach(() => {
......@@ -278,7 +277,6 @@ describe('DocumentMeta', function () {
document: fakeDocument,
baseURI,
});
doc.pluginInit();
return doc;
};
......
......@@ -13,10 +13,7 @@ describe('annotator/plugin/pdf', () => {
let pdfPlugin;
function createPDFPlugin() {
const plugin = new PDF(document.body);
plugin.annotator = fakeAnnotator;
plugin.pluginInit();
return plugin;
return new PDF(document.body, {}, fakeAnnotator);
}
beforeEach(() => {
......
......@@ -2,7 +2,6 @@ import * as adder from '../adder';
import { Observable } from '../util/observable';
import Delegator from '../delegator';
import Guest from '../guest';
import Plugin from '../plugin';
import { $imports } from '../guest';
const scrollIntoView = sinon.stub();
......@@ -19,12 +18,11 @@ class FakeAdder {
}
FakeAdder.instance = null;
class FakePlugin extends Plugin {
constructor(element, config) {
class FakePlugin extends Delegator {
constructor(element, config, annotator) {
super(element, config);
this.annotator = annotator;
FakePlugin.instance = this;
this.pluginInit = sinon.stub();
}
}
FakePlugin.instance = null;
......@@ -114,11 +112,7 @@ describe('Guest', () => {
fakePlugin = FakePlugin.instance;
});
it('calls `pluginInit`', () => {
assert.calledOnce(fakePlugin.pluginInit);
});
it('sets `annotator` property of plugin', () => {
it('passes guest reference to constructor', () => {
assert.equal(fakePlugin.annotator, guest);
});
......
......@@ -45,7 +45,7 @@ describe('CrossFrame multi-frame scenario', function () {
emit: sandbox.stub(),
};
crossFrame = new CrossFrame(container, options);
crossFrame = null;
});
afterEach(function () {
......@@ -56,6 +56,10 @@ describe('CrossFrame multi-frame scenario', function () {
$imports.$restore();
});
function createCrossFrame() {
return new CrossFrame(container, options);
}
it('detects frames on page', function () {
// Create a frame before initializing
const validFrame = document.createElement('iframe');
......@@ -69,7 +73,7 @@ describe('CrossFrame multi-frame scenario', function () {
container.appendChild(invalidFrame);
// Now initialize
crossFrame.pluginInit();
crossFrame = createCrossFrame();
const validFramePromise = new Promise(function (resolve) {
isLoaded(validFrame, function () {
......@@ -100,7 +104,7 @@ describe('CrossFrame multi-frame scenario', function () {
container.appendChild(frame);
// Now initialize
crossFrame.pluginInit();
crossFrame = createCrossFrame();
// Remove the frame
frame.remove();
......@@ -113,7 +117,7 @@ describe('CrossFrame multi-frame scenario', function () {
frame.setAttribute('enable-annotation', '');
container.appendChild(frame);
crossFrame.pluginInit();
crossFrame = createCrossFrame();
return new Promise(function (resolve) {
isLoaded(frame, function () {
......@@ -137,7 +141,7 @@ describe('CrossFrame multi-frame scenario', function () {
container.appendChild(frame);
frame.contentWindow.eval('window.__hypothesis_frame = true');
crossFrame.pluginInit();
crossFrame = createCrossFrame();
return new Promise(function (resolve) {
isLoaded(frame, function () {
......@@ -155,7 +159,7 @@ describe('CrossFrame multi-frame scenario', function () {
it('detects dynamically added frames', function () {
// Initialize with no initial frame, unlike before
crossFrame.pluginInit();
crossFrame = createCrossFrame();
// Add a frame to the DOM
const frame = document.createElement('iframe');
......@@ -183,7 +187,7 @@ describe('CrossFrame multi-frame scenario', function () {
container.appendChild(frame);
// Now initialize
crossFrame.pluginInit();
crossFrame = createCrossFrame();
return new Promise(function (resolve) {
// Yield to let the DOM and CrossFrame catch up
......@@ -206,7 +210,7 @@ describe('CrossFrame multi-frame scenario', function () {
container.appendChild(frame);
// Now initialize
crossFrame.pluginInit();
crossFrame = createCrossFrame();
return new Promise(function (resolve) {
isLoaded(frame, function () {
......@@ -239,7 +243,7 @@ describe('CrossFrame multi-frame scenario', function () {
it('detects a frame dynamically added, removed, and added again', function () {
// Initialize with no initial frame
crossFrame.pluginInit();
crossFrame = createCrossFrame();
// Add a frame to the DOM
const frame = document.createElement('iframe');
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment