Skip to content

C
coopwire-hypothesis
Commit e63a0be7 authored by Eduardo Sanz García's avatar Eduardo Sanz García Committed by Eduardo
Browse files
Options

Remove `AnnotationSync` utility 

AnnotationSync utility didn't have a clear purpose, except for assigning a
temporarily `$tag` to newly created annotations. For that, it used an
internal cache of annotation objects from the Guest, that were mutated
in-place. `AnnotationSync` used a very indirect method of communication
to the `Guest`, using an event emitter, even though their mutual
relationship was quite straightforward.

In this PR, we have:

* Simplified the messages that are sent between the host and the
  sidebar. We have assumed that a `$tag` value is always present if it
  comes from the `sidebar`, and a temporarily `$tag` is assigned in the
  `Guest` class for newly created annotations. Therefore, the type of
  the message sent between these two frames can be simplified to always
  be of the shape of `AnnotationData`, a safe version of `Annotation`.

* Added a few missing types.

* Changed the RPC method `deleteAnnotation` to send astring tag, similar
  to other RPC methods, like `focusAnnotations` or `scrollToAnnoation`.
  As a consequence of that we changed the `Guest#detach` method
  signature.
parent 13d0073c
Show whitespace changes
Inline Side-by-side
Showing with 117 additions and 455 deletions
src/annotator/annotation-sync.js deleted 100644 → 0
View file @ 13d0073c
import { generateHexString } from '../shared/random';
/**
* @typedef {import('../shared/bridge').Bridge<GuestToSidebarEvent,SidebarToGuestEvent>} SidebarBridge
* @typedef {import('../types/annotator').AnnotationData} AnnotationData
* @typedef {import('../types/annotator').Destroyable} Destroyable
* @typedef {import('../types/bridge-events').GuestToSidebarEvent} GuestToSidebarEvent
* @typedef {import('../types/bridge-events').SidebarToGuestEvent} SidebarToGuestEvent
* @typedef {import('./util/emitter').EventBus} EventBus
*/
/**
* Message sent between the sidebar and annotator about an annotation that has
* changed.
*
* @typedef RpcMessage
* @prop {string} tag
* @prop {AnnotationData} msg
*/
/**
* AnnotationSync listens for messages from the sidebar app indicating that
* annotations have been added or removed and relays them to Guest.
*
* It also listens for events from Guest when new annotations are created or
* annotations successfully anchor and relays these to the sidebar app.
*
* @implements Destroyable
*/
export class AnnotationSync {
/**
* @param {EventBus} eventBus - Event bus for communicating with the annotator code (eg. the Guest)
* @param {SidebarBridge} sidebarRPC - Channel for communicating with the sidebar
*/
constructor(eventBus, sidebarRPC) {
this._emitter = eventBus.createEmitter();
this._sidebarRPC = sidebarRPC;
/**
* Mapping from annotation tags to annotation objects for annotations which
* have been sent to or received from the sidebar.
*
* @type {{ [tag: string]: AnnotationData }}
*/
this.cache = {};
this.destroyed = false;
// Relay events from the sidebar to the rest of the annotator.
this._sidebarRPC.on('deleteAnnotation', (body, callback) => {
if (this.destroyed) {
callback(null);
return;
}
const annotation = this._parse(body);
delete this.cache[annotation.$tag];
this._emitter.publish('annotationDeleted', annotation);
this._format(annotation);
callback(null);
});
this._sidebarRPC.on('loadAnnotations', (bodies, callback) => {
if (this.destroyed) {
callback(null);
return;
}
const annotations = bodies.map(body => this._parse(body));
this._emitter.publish('annotationsLoaded', annotations);
callback(null);
});
// Relay events from annotator to sidebar.
this._emitter.subscribe('beforeAnnotationCreated', annotation => {
if (annotation.$tag) {
return;
}
this._sidebarRPC.call('createAnnotation', this._format(annotation));
});
}
/**
* Relay updated annotations from the annotator to the sidebar.
*
* This is called for example after annotations are anchored to notify the
* sidebar about the current anchoring status.
*
* @param {AnnotationData[]} annotations
*/
sync(annotations) {
if (this.destroyed) {
return;
}
this._sidebarRPC.call(
'syncAnchoringStatus',
annotations.map(ann => this._format(ann))
);
}
/**
* Assign a non-enumerable "tag" to identify annotations exchanged between
* the sidebar and annotator and associate the tag with the `annotation` instance
* in the local cache.
*
* @param {AnnotationData} annotation
* @param {string} [tag] - The tag to assign
* @return {AnnotationData}
*/
_tag(annotation, tag) {
if (annotation.$tag) {
return annotation;
}
tag = tag || 'a:' + generateHexString(8);
Object.defineProperty(annotation, '$tag', {
value: tag,
});
this.cache[tag] = annotation;
return annotation;
}
/**
* Copy annotation data from an RPC message into a local copy (in `this.cache`)
* and return the local copy.
*
* @param {RpcMessage} body
* @return {AnnotationData}
*/
_parse(body) {
const merged = Object.assign(this.cache[body.tag] || {}, body.msg);
return this._tag(merged, body.tag);
}
/**
* Format an annotation into an RPC message body.
*
* @param {AnnotationData} annotation
* @return {RpcMessage}
*/
_format(annotation) {
this._tag(annotation);
return {
tag: annotation.$tag,
msg: annotation,
};
}
destroy() {
this.destroyed = true;
this._emitter.destroy();
}
}
src/annotator/guest.js
View file @ e63a0be7
import { Bridge } from '../shared/bridge';
import { PortFinder } from '../shared/port-finder';
import { ListenerCollection } from '../shared/listener-collection';
import { PortFinder } from '../shared/port-finder';
import { generateHexString } from '../shared/random';
import { Adder } from './adder';
import { AnnotationSync } from './annotation-sync';
import { TextRange } from './anchoring/text-range';
import {
getHighlightsContainingNode,
......@@ -180,11 +180,6 @@ export default class Guest {
this._sidebarRPC = new Bridge();
this._connectSidebarEvents();
// Set up listeners for when the sidebar asks us to add or remove annotations
// in this frame.
this._annotationSync = new AnnotationSync(eventBus, this._sidebarRPC);
this._connectAnnotationSync();
// Set up automatic and integration-triggered injection of client into
// iframes in this frame.
this._hypothesisInjector = new HypothesisInjector(this.element, config);
......@@ -305,20 +300,13 @@ export default class Guest {
}
}
_connectAnnotationSync() {
this._emitter.subscribe('annotationDeleted', annotation => {
this.detach(annotation);
});
this._emitter.subscribe('annotationsLoaded', annotations => {
annotations.map(annotation => this.anchor(annotation));
});
}
async _connectSidebarEvents() {
// Handlers for events sent when user hovers or clicks on an annotation card
// in the sidebar.
this._sidebarRPC.on('focusAnnotations', (tags = []) => {
this._sidebarRPC.on(
'focusAnnotations',
/** @param {string[]} tags */
(tags = []) => {
this._focusedAnnotations.clear();
tags.forEach(tag => this._focusedAnnotations.add(tag));
......@@ -328,9 +316,13 @@ export default class Guest {
setHighlightsFocused(anchor.highlights, toggle);
}
}
});
}
);
this._sidebarRPC.on('scrollToAnnotation', tag => {
this._sidebarRPC.on(
'scrollToAnnotation',
/** @param {string} tag */
tag => {
const anchor = this.anchors.find(a => a.annotation.$tag === tag);
if (!anchor?.highlights) {
return;
......@@ -353,7 +345,8 @@ export default class Guest {
if (defaultNotPrevented) {
this._integration.scrollToAnchor(anchor);
}
});
}
);
// Handler for when sidebar requests metadata for the current document
this._sidebarRPC.on('getDocumentInfo', cb => {
......@@ -367,6 +360,18 @@ export default class Guest {
this.setHighlightsVisible(showHighlights);
});
this._sidebarRPC.on(
'deleteAnnotation',
/** @param {string} tag */
tag => this.detach(tag)
);
this._sidebarRPC.on(
'loadAnnotations',
/** @param {AnnotationData[]} annotations */
annotations => annotations.forEach(annotation => this.anchor(annotation))
);
// Discover and connect to the sidebar frame. All RPC events must be
// registered before creating the channel.
const sidebarPort = await this._portFinder.discover('sidebar');
......@@ -386,7 +391,6 @@ export default class Guest {
this._integration.destroy();
this._emitter.destroy();
this._annotationSync.destroy();
this._sidebarRPC.destroy();
}
......@@ -478,7 +482,7 @@ export default class Guest {
};
// Remove existing anchors for this annotation.
this.detach(annotation, false /* notify */);
this.detach(annotation.$tag, false /* notify */);
// Resolve selectors to ranges and insert highlights.
if (!annotation.target) {
......@@ -499,7 +503,7 @@ export default class Guest {
this._updateAnchors(this.anchors.concat(anchors), true /* notify */);
// Let other frames (eg. the sidebar) know about the new annotation.
this._annotationSync.sync([annotation]);
this._sidebarRPC.call('syncAnchoringStatus', annotation);
return anchors;
}
......@@ -507,13 +511,14 @@ export default class Guest {
/**
* Remove the anchors and associated highlights for an annotation from the document.
*
* @param {AnnotationData} annotation
* @param {string} tag
* @param {boolean} [notify] - For internal use. Whether to emit an `anchorsChanged` notification
*/
detach(annotation, notify = true) {
detach(tag, notify = true) {
/** @type {Anchor[]} */
const anchors = [];
for (let anchor of this.anchors) {
if (anchor.annotation !== annotation) {
if (anchor.annotation.$tag !== tag) {
anchors.push(anchor);
} else if (anchor.highlights) {
removeHighlights(anchor.highlights);
......@@ -566,12 +571,10 @@ export default class Guest {
document: info.metadata,
target,
$highlight: highlight,
// nb. `$tag` is assigned by `AnnotationSync`.
$tag: '',
$tag: 'a:' + generateHexString(8),
};
this._emitter.publish('beforeAnnotationCreated', annotation);
this._sidebarRPC.call('createAnnotation', annotation);
this.anchor(annotation);
return annotation;
......
src/annotator/test/annotation-sync-test.js deleted 100644 → 0
View file @ 13d0073c
import { AnnotationSync } from '../annotation-sync';
import { EventBus } from '../util/emitter';
describe('AnnotationSync', () => {
let createAnnotationSync;
let emitter;
let fakeBridge;
let publish;
const sandbox = sinon.createSandbox();
beforeEach(() => {
const eventBus = new EventBus();
emitter = eventBus.createEmitter();
const listeners = {};
fakeBridge = {
on: sandbox.spy((method, fn) => {
listeners[method] = fn;
}),
call: sandbox.stub(),
onConnect: sandbox.stub(),
links: [],
};
createAnnotationSync = () => {
return new AnnotationSync(eventBus, fakeBridge);
};
publish = (method, ...args) => listeners[method](...args);
});
afterEach(() => {
sandbox.restore();
emitter.destroy();
});
describe('handling events from the sidebar', () => {
describe('on "deleteAnnotation" event', () => {
it('publish "annotationDeleted" to the annotator', () => {
const ann = { id: 1, $tag: 'tag1' };
const eventStub = sinon.stub();
emitter.subscribe('annotationDeleted', eventStub);
createAnnotationSync();
publish('deleteAnnotation', { msg: ann }, () => {});
assert.calledWith(eventStub, ann);
});
it('calls the "deleteAnnotation" event\'s callback function', done => {
const ann = { id: 1, $tag: 'tag1' };
const callback = function (err, result) {
assert.isNull(err);
assert.isUndefined(result);
done();
};
createAnnotationSync();
publish('deleteAnnotation', { msg: ann }, callback);
});
it('deletes any existing annotation from its cache before publishing event to the annotator', done => {
const annSync = createAnnotationSync();
const ann = { id: 1, $tag: 'tag1' };
annSync.cache.tag1 = ann;
emitter.subscribe('annotationDeleted', () => {
assert.isUndefined(annSync.cache.tag1);
done();
});
publish('deleteAnnotation', { msg: ann }, () => {});
});
it('deletes any existing annotation from its cache', () => {
const ann = { id: 1, $tag: 'tag1' };
const annSync = createAnnotationSync();
annSync.cache.tag1 = ann;
publish('deleteAnnotation', { msg: ann }, () => {});
assert.isUndefined(annSync.cache.tag1);
});
});
describe('on "loadAnnotations" event', () => {
it('publishes "annotationsLoaded" to the annotator', () => {
const annotations = [
{ id: 1, $tag: 'tag1' },
{ id: 2, $tag: 'tag2' },
{ id: 3, $tag: 'tag3' },
];
const bodies = [
{ msg: annotations[0], tag: annotations[0].$tag },
{ msg: annotations[1], tag: annotations[1].$tag },
{ msg: annotations[2], tag: annotations[2].$tag },
];
const loadedStub = sinon.stub();
emitter.subscribe('annotationsLoaded', loadedStub);
createAnnotationSync();
publish('loadAnnotations', bodies, () => {});
assert.calledWith(loadedStub, annotations);
});
});
});
describe('handling events from the annotator', () => {
describe('on "beforeAnnotationCreated" event', () => {
it('calls "createAnnotation" RPC method in the sidebar', () => {
// nb. Setting an empty `$tag` here matches what `Guest#createAnnotation`
// does.
const ann = { id: 1, $tag: '' };
createAnnotationSync();
emitter.publish('beforeAnnotationCreated', ann);
assert.called(fakeBridge.call);
assert.calledWith(fakeBridge.call, 'createAnnotation', {
msg: ann,
tag: ann.$tag,
});
});
it('assigns a non-empty tag to the annotation', () => {
const ann = { id: 1, $tag: '' };
createAnnotationSync();
emitter.publish('beforeAnnotationCreated', ann);
assert.notEmpty(ann.$tag);
});
context('if the annotation already has a $tag', () => {
it('does not call bridge.call()', () => {
const ann = { id: 1, $tag: 'tag1' };
createAnnotationSync();
emitter.publish('beforeAnnotationCreated', ann);
assert.notCalled(fakeBridge.call);
});
it('does not modify the tag', () => {
const ann = { id: 1, $tag: 'sometag' };
createAnnotationSync();
emitter.publish('beforeAnnotationCreated', ann);
assert.equal(ann.$tag, 'sometag');
});
});
});
});
describe('#sync', () => {
it('calls "syncAnchoringStatus" RPC method in the sidebar', () => {
const ann = { id: 1 };
const annotationSync = createAnnotationSync();
annotationSync.sync([ann]);
assert.calledWith(fakeBridge.call, 'syncAnchoringStatus', [
{ msg: ann, tag: ann.$tag },
]);
});
});
describe('#destroy', () => {
it('ignore `loadAnnotations` and `deleteAnnotation` events from the sidebar', () => {
const ann = { msg: { id: 1 }, tag: 'tag1' };
const annotationSync = createAnnotationSync();
annotationSync.destroy();
const cb = sinon.stub();
publish('loadAnnotations', [ann], cb);
publish('deleteAnnotation', ann, cb);
assert.calledTwice(cb);
assert.calledWith(cb.firstCall, null);
assert.calledWith(cb.secondCall, null);
});
it('disables annotation syncing with the sidebar', () => {
const annotationSync = createAnnotationSync();
annotationSync.destroy();
annotationSync.sync([{ id: 1 }]);
assert.notCalled(fakeBridge.call);
});
it('ignores "beforeAnnotationCreated" events from the annotator', () => {
const annotationSync = createAnnotationSync();
annotationSync.destroy();
emitter.publish('beforeAnnotationCreated', { id: 1, $tag: '' });
assert.notCalled(fakeBridge.call);
});
});
});
src/sidebar/services/frame-sync.js
View file @ e63a0be7
......@@ -8,29 +8,33 @@ import { isReply, isPublic } from '../helpers/annotation-metadata';
import { watch } from '../util/watch';
/**
* @typedef {import('../../types/annotator').AnnotationData} AnnotationData
* @typedef {import('../../types/api').Annotation} Annotation
* @typedef {import('../../types/bridge-events').SidebarToHostEvent} SidebarToHostEvent
* @typedef {import('../../types/bridge-events').HostToSidebarEvent} HostToSidebarEvent
* @typedef {import('../../types/bridge-events').SidebarToGuestEvent} SidebarToGuestEvent
* @typedef {import('../../types/bridge-events').GuestToSidebarEvent} GuestToSidebarEvent
* @typedef {import('../../shared/port-rpc').PortRPC} PortRPC
* @typedef {import('../store/modules/frames').Frame} Frame
*/
/**
* Return a minimal representation of an annotation that can be sent from the
* sidebar app to a connected frame.
* sidebar app to the host frame.
*
* Because this representation will be exposed to untrusted third-party
* JavaScript, it includes only the information needed to uniquely identify it
* within the current session and anchor it in the document.
*
* @param {Annotation} annotation
* @returns {AnnotationData}
*/
export function formatAnnot(ann) {
export function formatAnnot({ $tag, document, target, uri }) {
return {
tag: ann.$tag,
msg: {
document: ann.document,
target: ann.target,
uri: ann.uri,
},
$tag,
document: { title: document.title, link: [{ href: target[0].source }] },
target,
uri,
};
}
......@@ -109,9 +113,15 @@ export class FrameSyncService {
watch(
this._store.subscribe,
[() => this._store.allAnnotations(), () => this._store.frames()],
/**
* @param {[Annotation[], Frame[]]} current
* @param {[Annotation[]]} previous
*/
([annotations, frames], [prevAnnotations]) => {
let publicAnns = 0;
/** @type {Set<string>} */
const inSidebar = new Set();
/** @type {Annotation[]} */
const added = [];
annotations.forEach(annot => {
......@@ -143,7 +153,7 @@ export class FrameSyncService {
});
}
deleted.forEach(annot => {
this._guestRPC.call('deleteAnnotation', formatAnnot(annot));
this._guestRPC.call('deleteAnnotation', annot.$tag);
this._inFrame.delete(annot.$tag);
});
......@@ -165,8 +175,10 @@ export class FrameSyncService {
*/
_setupSyncFromGuests() {
// A new annotation, note or highlight was created in the frame
this._guestRPC.on('createAnnotation', event => {
const annot = Object.assign({}, event.msg, { $tag: event.tag });
this._guestRPC.on(
'createAnnotation',
/** @param {AnnotationData} annot */
annot => {
// If user is not logged in, we can't really create a meaningful highlight
// or annotation. Instead, we need to open the sidebar, show an error,
// and delete the (unsaved) annotation so it gets un-selected in the
......@@ -174,10 +186,10 @@ export class FrameSyncService {
if (!this._store.isLoggedIn()) {
this._hostRPC.call('openSidebar');
this._store.openSidebarPanel('loginPrompt');
this._guestRPC.call('deleteAnnotation', formatAnnot(annot));
this._guestRPC.call('deleteAnnotation', annot.$tag);
return;
}
this._inFrame.add(event.tag);
this._inFrame.add(annot.$tag);
// Open the sidebar so that the user can immediately edit the draft
// annotation.
......@@ -192,7 +204,8 @@ export class FrameSyncService {
// Create the new annotation in the sidebar.
this._annotationsService.create(annot);
});
}
);
// Map of annotation tag to anchoring status
// ('anchored'|'orphan'|'timeout').
......@@ -208,15 +221,15 @@ export class FrameSyncService {
}, 10);
// Anchoring an annotation in the frame completed
this._guestRPC.on('syncAnchoringStatus', events_ => {
events_.forEach(event => {
this._inFrame.add(event.tag);
anchoringStatusUpdates[event.tag] = event.msg.$orphan
? 'orphan'
: 'anchored';
this._guestRPC.on(
'syncAnchoringStatus',
/** @param {AnnotationData} annotation */
({ $tag, $orphan }) => {
this._inFrame.add($tag);
anchoringStatusUpdates[$tag] = $orphan ? 'orphan' : 'anchored';
scheduleAnchoringStatusUpdate();
});
});
}
);
this._guestRPC.on('showAnnotations', tags => {
this._store.selectAnnotations(this._store.findIDsForTags(tags));
......
src/sidebar/store/modules/frames.js
View file @ e63a0be7
......@@ -18,6 +18,7 @@ import { createStoreModule } from '../create-store';
* - Sub-frames will all have a id (frame identifier) set. The main frame's id is always `null`
* @prop {DocumentMetadata} metadata - Metadata about the document currently loaded in this frame
* @prop {string} uri - Current primary URI of the document being displayed
* @prop {boolean} [isAnnotationFetchComplete]
*/
/** @type {Frame[]} */
......
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