Skip to content

C
coopwire-hypothesis
Commit d9085810 authored by Robert Knight's avatar Robert Knight
Browse files
Options

Refactor `Guest#anchor` 

Rewrite the `Guest#anchor` method to simplify the control flow and
generally make it easier to understand and change. There are no
functional changes for annotations with zero or one entry in the
`target` field (all existing Hypothesis annotations).

There is a functional change to handling of annotations with multiple
targets. Previously `anchor` would try to be smart about only
re-anchoring targets which were not already anchored. In the new
implementation all targets for an annotation are re-anchored. This will have
no effect in practice because the Hypothesis client only supports
creating annotations with a single target and the h backend can only
store a single target per annotation. However the Hypothesis API does
allow for multiple targets per annotation, in line with the W3C Web
Annotations specs in which a single annotation can refer to multiple
parts of a document.

 - Convert `anchor` method to async and replace Promise chains with async/await

 - Replace the logic that removes existing anchors and highlights for an
   annotation with a call to the `detach` method. This required adding
   an internal parameter to `detach` to control whether `anchorsChanged`
   is emitted, so that `anchor` only emits `anchorsChanged` once.

 - Add an explicit error to `Promise.reject` calls in tests so that the
   tests are easier to debug if they fail
parent 3a784bce
Hide whitespace changes
Inline Side-by-side
Showing with 76 additions and 141 deletions
src/annotator/guest.js
View file @ d9085810
...@@ -335,83 +335,64 @@ export default class Guest { ...@@ -335,83 +335,64 @@ export default class Guest {
} }
/** /**
* Anchor (locate) an annotation's selectors in the document. * Anchor an annotation's selectors in the document.
*
* _Anchoring_ resolves a set of selectors to a concrete region of the document
* which is then highlighted. The results of anchoring are broadcast to the
* rest of the application via `CrossFrame#sync`.
*
* Any existing anchors associated with `annotation` will be removed before
* re-anchoring the annotation.
* *
* @param {AnnotationData} annotation * @param {AnnotationData} annotation
* @return {Promise<Anchor[]>} * @return {Promise<Anchor[]>}
*/ */
anchor(annotation) { async anchor(annotation) {
let anchor;
const root = this.element;
// Anchors for all annotations are in the `anchors` instance property. These
// are anchors for this annotation only. After all the targets have been
// processed these will be appended to the list of anchors known to the
// instance. Anchors hold an annotation, a target of that annotation, a
// document range for that target and an Array of highlights.
const anchors = [];
// The targets that are already anchored. This function consults this to
// determine which targets can be left alone.
const anchoredTargets = [];
// These are the highlights for existing anchors of this annotation with
// targets that have since been removed from the annotation. These will
// be removed by this function.
let deadHighlights = [];
// Initialize the target array.
if (!annotation.target) {
annotation.target = [];
}
/** /**
* Locate the region of the current document that the annotation refers to. * Resolve an annotation's selectors to a concrete range.
* *
* @param {Target} target * @param {Target} target
* @return {Promise<Anchor>}
*/ */
const locate = target => { const locate = async target => {
// Check that the anchor has a TextQuoteSelector -- without a // Only annotations with an associated quote can currently be anchored.
// TextQuoteSelector we have no basis on which to verify that we have // This is because the quote is used to verify anchoring with other selector
// reanchored correctly and so we shouldn't even try. // types.
//
// Returning an anchor without a range will result in this annotation being
// treated as an orphan (assuming no other targets anchor).
if ( if (
!target.selector || !target.selector ||
!target.selector.some(s => s.type === 'TextQuoteSelector') !target.selector.some(s => s.type === 'TextQuoteSelector')
) { ) {
return Promise.resolve({ annotation, target }); return { annotation, target };
} }
// Find a target using the anchoring module. /** @type {Anchor} */
return this.integration let anchor;
.anchor(root, target.selector) try {
.then(range => ({ const range = await this.integration.anchor(
annotation, this.element,
target, target.selector
);
// Convert the `Range` to a `TextRange` which can be converted back to // Convert the `Range` to a `TextRange` which can be converted back to
// a `Range` later. The `TextRange` representation allows for highlights // a `Range` later. The `TextRange` representation allows for highlights
// to be inserted during anchoring other annotations without "breaking" // to be inserted during anchoring other annotations without "breaking"
// this anchor. // this anchor.
range: TextRange.fromRange(range), const textRange = TextRange.fromRange(range);
})) anchor = { annotation, target, range: textRange };
.catch(() => ({ } catch (err) {
annotation, anchor = { annotation, target };
target, }
})); return anchor;
}; };
/** /**
* Highlight the range for an anchor. * Highlight the text range that `anchor` refers to.
* *
* @param {Anchor} anchor * @param {Anchor} anchor
*/ */
const highlight = anchor => { const highlight = anchor => {
const range = resolveAnchor(anchor); const range = resolveAnchor(anchor);
if (!range) { if (!range) {
return anchor; return;
} }
const highlights = /** @type {AnnotationHighlight[]} */ (highlightRange( const highlights = /** @type {AnnotationHighlight[]} */ (highlightRange(
...@@ -421,98 +402,62 @@ export default class Guest { ...@@ -421,98 +402,62 @@ export default class Guest {
h._annotation = anchor.annotation; h._annotation = anchor.annotation;
}); });
anchor.highlights = highlights; anchor.highlights = highlights;
return anchor;
}; };
/** // Remove existing anchors for this annotation.
* Inform other parts of Hypothesis (eg. the sidebar and bucket bar) about this.detach(annotation, false /* notify */);
* the results of anchoring.
*
* @param {Anchor[]} anchors
*/
const sync = anchors => {
// An annotation is considered to be an orphan if it has at least one
// target with selectors, and all targets with selectors failed to anchor
// (i.e. we didn't find it in the page and thus it has no range).
let hasAnchorableTargets = false;
let hasAnchoredTargets = false;
for (let anchor of anchors) {
if (anchor.target.selector) {
hasAnchorableTargets = true;
if (anchor.range) {
hasAnchoredTargets = true;
break;
}
}
}
annotation.$orphan = hasAnchorableTargets && !hasAnchoredTargets;
// Add the anchors for this annotation to instance storage.
this._updateAnchors(this.anchors.concat(anchors));
// Let other frames (eg. the sidebar) know about the new annotation.
this.crossframe.sync([annotation]);
return anchors;
};
// Remove all the anchors for this annotation from the instance storage. // Resolve selectors to ranges and insert highlights.
for (anchor of this.anchors.splice(0, this.anchors.length)) { if (!annotation.target) {
if (anchor.annotation === annotation) { annotation.target = [];
// Anchors are valid as long as they still have a range and their target }
// is still in the list of targets for this annotation. const anchors = await Promise.all(annotation.target.map(locate));
if (anchor.range && annotation.target.includes(anchor.target)) { for (let anchor of anchors) {
anchors.push(anchor); highlight(anchor);
anchoredTargets.push(anchor.target);
} else if (anchor.highlights) {
// These highlights are no longer valid and should be removed.
deadHighlights = deadHighlights.concat(anchor.highlights);
delete anchor.highlights;
delete anchor.range;
}
} else {
// These can be ignored, so push them back onto the new list.
this.anchors.push(anchor);
}
} }
// Remove all the highlights that have no corresponding target anymore. // Set flag indicating whether anchoring succeeded. For each target,
removeHighlights(deadHighlights); // anchoring is successful either if there are no selectors (ie. this is a
// Page Note) or we successfully resolved the selectors to a range.
annotation.$orphan =
anchors.length > 0 &&
anchors.every(anchor => anchor.target.selector && !anchor.range);
// Anchor any targets of this annotation that are not anchored already. this._updateAnchors(this.anchors.concat(anchors), true /* notify */);
for (let target of annotation.target) {
if (!anchoredTargets.includes(target)) { // Let other frames (eg. the sidebar) know about the new annotation.
anchor = locate(target).then(highlight); this.crossframe.sync([annotation]);
anchors.push(anchor);
}
}
return Promise.all(anchors).then(sync); return anchors;
} }
/** /**
* Remove the anchors and associated highlights for an annotation from the document. * Remove the anchors and associated highlights for an annotation from the document.
* *
* @param {AnnotationData} annotation * @param {AnnotationData} annotation
* @param {boolean} [notify] - For internal use. Whether to emit an `anchorsChanged` notification
*/ */
detach(annotation) { detach(annotation, notify = true) {
const anchors = []; const anchors = [];
let unhighlight = [];
for (let anchor of this.anchors) { for (let anchor of this.anchors) {
if (anchor.annotation === annotation) { if (anchor.annotation !== annotation) {
unhighlight.push(...(anchor.highlights ?? []));
} else {
anchors.push(anchor); anchors.push(anchor);
} else if (anchor.highlights) {
removeHighlights(anchor.highlights);
} }
} }
removeHighlights(unhighlight); this._updateAnchors(anchors, notify);
this._updateAnchors(anchors);
} }
_updateAnchors(anchors) { /**
* @param {Anchor[]} anchors
* @param {boolean} notify
*/
_updateAnchors(anchors, notify) {
this.anchors = anchors; this.anchors = anchors;
this._emitter.publish('anchorsChanged', this.anchors); if (notify) {
this._emitter.publish('anchorsChanged', this.anchors);
}
} }
/** /**
......
src/annotator/test/guest-test.js
View file @ d9085810
...@@ -785,7 +785,7 @@ describe('Guest', () => { ...@@ -785,7 +785,7 @@ describe('Guest', () => {
}; };
fakeHTMLIntegration.anchor fakeHTMLIntegration.anchor
.onFirstCall() .onFirstCall()
.returns(Promise.reject()) .returns(Promise.reject(new Error('Failed to anchor')))
.onSecondCall() .onSecondCall()
.returns(Promise.resolve(range)); .returns(Promise.resolve(range));
...@@ -801,7 +801,9 @@ describe('Guest', () => { ...@@ -801,7 +801,9 @@ describe('Guest', () => {
{ selector: [{ type: 'TextQuoteSelector', exact: 'notinhere' }] }, { selector: [{ type: 'TextQuoteSelector', exact: 'notinhere' }] },
], ],
}; };
fakeHTMLIntegration.anchor.returns(Promise.reject()); fakeHTMLIntegration.anchor.returns(
Promise.reject(new Error('Failed to anchor'))
);
return guest return guest
.anchor(annotation) .anchor(annotation)
...@@ -816,7 +818,9 @@ describe('Guest', () => { ...@@ -816,7 +818,9 @@ describe('Guest', () => {
{ selector: [{ type: 'TextQuoteSelector', exact: 'neitherami' }] }, { selector: [{ type: 'TextQuoteSelector', exact: 'neitherami' }] },
], ],
}; };
fakeHTMLIntegration.anchor.returns(Promise.reject()); fakeHTMLIntegration.anchor.returns(
Promise.reject(new Error('Failed to anchor'))
);
return guest return guest
.anchor(annotation) .anchor(annotation)
...@@ -916,20 +920,6 @@ describe('Guest', () => { ...@@ -916,20 +920,6 @@ describe('Guest', () => {
assert.calledWith(removeHighlights, highlights); assert.calledWith(removeHighlights, highlights);
}); });
}); });
it('does not reanchor targets that are already anchored', () => {
const guest = createGuest();
const annotation = {
target: [{ selector: [{ type: 'TextQuoteSelector', exact: 'hello' }] }],
};
fakeHTMLIntegration.anchor.returns(Promise.resolve(range));
return guest.anchor(annotation).then(() =>
guest.anchor(annotation).then(() => {
assert.equal(guest.anchors.length, 1);
assert.calledOnce(fakeHTMLIntegration.anchor);
})
);
});
}); });
describe('#detach', () => { describe('#detach', () => {
......
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