Commit cd4039ca authored by Robert Knight's avatar Robert Knight

Display annotations in Annotations tab after timeout

Instead of marking annotations as orphans after the anchoring timeout
expires, instead display them in the Annotations tab but with a visual
indicator that they have not yet anchored. This avoids showing
annotations that are still anchoring in the Orphans tab, which can be
confusing when it happens in long PDF documents with many annotations.

 * Add an `$anchorTimeout` flag to annotations that is set if
   annotations fail to anchor within the timeout period.

 * Display annotations with this flag set in the Annotations tab,
   on the optimistic basis that they will eventually anchor.

 * When rendering annotations with this flag set, display them with
   a quote struck through (same as orphans) to indicate that they are
   pending. The visual representation of the pending state may be
   tweaked in future.
parent 5ad1356c
...@@ -114,9 +114,16 @@ function hasSelector(annotation) { ...@@ -114,9 +114,16 @@ function hasSelector(annotation) {
annotation.target[0].selector); annotation.target[0].selector);
} }
/** Return `true` if the given annotation is not yet anchored. */ /**
* Return `true` if the given annotation is not yet anchored.
*
* Returns false if anchoring is still in process but the flag indicating that
* the initial timeout allowed for anchoring has expired.
*/
function isWaitingToAnchor(annotation) { function isWaitingToAnchor(annotation) {
return hasSelector(annotation) && (typeof annotation.$orphan === 'undefined'); return hasSelector(annotation) &&
(typeof annotation.$orphan === 'undefined') &&
!annotation.$anchorTimeout;
} }
/** Return `true` if the given annotation is an orphan. */ /** Return `true` if the given annotation is an orphan. */
......
...@@ -426,7 +426,13 @@ function AnnotationController( ...@@ -426,7 +426,13 @@ function AnnotationController(
var indicateOrphans = features.flagEnabled('orphans_tab'); var indicateOrphans = features.flagEnabled('orphans_tab');
vm.isOrphan = function() { vm.isOrphan = function() {
return vm.annotation.$orphan && indicateOrphans; if (!indicateOrphans) {
return false;
}
if (typeof vm.annotation.$orphan === 'undefined') {
return vm.annotation.$anchorTimeout;
}
return vm.annotation.$orphan;
}; };
vm.updated = function() { vm.updated = function() {
......
...@@ -698,6 +698,33 @@ describe('annotation', function() { ...@@ -698,6 +698,33 @@ describe('annotation', function() {
}); });
}); });
describe('#isOrphan', function () {
it('returns false if the annotation is not an orphan', function () {
var controller = createDirective().controller;
controller.annotation.$orphan = false;
assert.isFalse(controller.isOrphan());
});
it('returns true if the annotation is an orphan', function () {
var controller = createDirective().controller;
controller.annotation.$orphan = true;
assert.isTrue(controller.isOrphan());
});
it('returns true if the anchoring timeout expired', function () {
var controller = createDirective().controller;
controller.annotation.$anchorTimeout = true;
assert.isTrue(controller.isOrphan());
});
it('returns false if the anchoring timeout expired but anchoring did complete', function () {
var controller = createDirective().controller;
controller.annotation.$orphan = false;
controller.annotation.$anchorTimeout = true;
assert.isFalse(controller.isOrphan());
});
});
describe('saving a new annotation', function() { describe('saving a new annotation', function() {
var annotation; var annotation;
......
...@@ -63,6 +63,8 @@ function initializeAnnot(annotation, tag) { ...@@ -63,6 +63,8 @@ function initializeAnnot(annotation, tag) {
} }
return Object.assign({}, annotation, { return Object.assign({}, annotation, {
// Flag indicating whether waiting for the annotation to anchor timed out.
$anchorTimeout: false,
$tag: annotation.$tag || tag, $tag: annotation.$tag || tag,
$orphan: orphan, $orphan: orphan,
}); });
...@@ -150,6 +152,7 @@ var update = { ...@@ -150,6 +152,7 @@ var update = {
(annot.$tag && annot.$tag === action.tag); (annot.$tag && annot.$tag === action.tag);
if (match) { if (match) {
return Object.assign({}, annot, { return Object.assign({}, annot, {
$anchorTimeout: action.anchorTimeout || annot.$anchorTimeout,
$orphan: action.isOrphan, $orphan: action.isOrphan,
$tag: action.tag, $tag: action.tag,
}); });
...@@ -209,9 +212,9 @@ function addAnnotations(annotations, now) { ...@@ -209,9 +212,9 @@ function addAnnotations(annotations, now) {
.forEach(function (orphan) { .forEach(function (orphan) {
dispatch({ dispatch({
type: actions.UPDATE_ANCHOR_STATUS, type: actions.UPDATE_ANCHOR_STATUS,
anchorTimeout: true,
id: orphan.id, id: orphan.id,
tag: orphan.$tag, tag: orphan.$tag,
isOrphan: true,
}); });
}); });
}, ANCHORING_TIMEOUT); }, ANCHORING_TIMEOUT);
......
...@@ -287,5 +287,12 @@ describe('annotation-metadata', function () { ...@@ -287,5 +287,12 @@ describe('annotation-metadata', function () {
it('returns false for page notes', function () { it('returns false for page notes', function () {
assert.isFalse(isWaitingToAnchor(fixtures.oldPageNote())); assert.isFalse(isWaitingToAnchor(fixtures.oldPageNote()));
}); });
it('returns false if the anchoring timeout flag was set', function () {
var pending = Object.assign({}, fixtures.defaultAnnotation(), {
$anchorTimeout: true,
});
assert.isFalse(isWaitingToAnchor(pending));
});
}); });
}); });
...@@ -146,32 +146,23 @@ describe('annotationUI', function () { ...@@ -146,32 +146,23 @@ describe('annotationUI', function () {
assert.isFalse(updatedAnnot.$orphan); assert.isFalse(updatedAnnot.$orphan);
}); });
it('marks annotations as orphans if they fail to anchor within a time limit', function () { it('sets the timeout flag on annotations that fail to anchor within a time limit', function () {
var isOrphan = function () {
return !!metadata.isOrphan(annotationUI.getState().annotations[0]);
};
var annot = defaultAnnotation(); var annot = defaultAnnotation();
annotationUI.addAnnotations([annot]); annotationUI.addAnnotations([annot]);
assert.isFalse(isOrphan());
clock.tick(ANCHOR_TIME_LIMIT); clock.tick(ANCHOR_TIME_LIMIT);
assert.isTrue(isOrphan()); assert.isTrue(annotationUI.getState().annotations[0].$anchorTimeout);
}); });
it('does not mark annotations as orphans if they do anchor within a time limit', function () { it('does not set the timeout flag on annotations that do anchor within a time limit', function () {
var isOrphan = function () {
return !!metadata.isOrphan(annotationUI.getState().annotations[0]);
};
var annot = defaultAnnotation(); var annot = defaultAnnotation();
annotationUI.addAnnotations([annot]); annotationUI.addAnnotations([annot]);
annotationUI.updateAnchorStatus(annot.id, 'atag', false); annotationUI.updateAnchorStatus(annot.id, 'atag', false);
clock.tick(ANCHOR_TIME_LIMIT); clock.tick(ANCHOR_TIME_LIMIT);
assert.isFalse(isOrphan()); assert.isFalse(annotationUI.getState().annotations[0].$anchorTimeout);
}); });
it('does not attempt to modify orphan status if annotations are removed before anchoring timeout expires', function () { it('does not attempt to modify orphan status if annotations are removed before anchoring timeout expires', function () {
......
...@@ -46,6 +46,7 @@ describe('tabs', function () { ...@@ -46,6 +46,7 @@ describe('tabs', function () {
describe('shouldShowInTab', function () { describe('shouldShowInTab', function () {
unroll('returns true if the annotation should be shown', function (testCase) { unroll('returns true if the annotation should be shown', function (testCase) {
var ann = fixtures.defaultAnnotation(); var ann = fixtures.defaultAnnotation();
ann.$anchorTimeout = testCase.anchorTimeout;
ann.$orphan = testCase.orphan; ann.$orphan = testCase.orphan;
assert.equal(tabs.shouldShowInTab(ann, uiConstants.TAB_ANNOTATIONS, assert.equal(tabs.shouldShowInTab(ann, uiConstants.TAB_ANNOTATIONS,
...@@ -53,18 +54,51 @@ describe('tabs', function () { ...@@ -53,18 +54,51 @@ describe('tabs', function () {
assert.equal(tabs.shouldShowInTab(ann, uiConstants.TAB_ORPHANS, assert.equal(tabs.shouldShowInTab(ann, uiConstants.TAB_ORPHANS,
testCase.separateOrphans), testCase.expectedTab === uiConstants.TAB_ORPHANS); testCase.separateOrphans), testCase.expectedTab === uiConstants.TAB_ORPHANS);
}, [{ }, [{
// Orphans tab disabled, anchoring in progress.
anchorTimeout: false,
orphan: undefined, orphan: undefined,
separateOrphans: false, separateOrphans: false,
expectedTab: uiConstants.TAB_ANNOTATIONS, expectedTab: uiConstants.TAB_ANNOTATIONS,
},{ },{
// Orphans tab disabled, anchoring succeeded.
anchorTimeout: false,
orphan: false,
separateOrphans: false,
expectedTab: uiConstants.TAB_ANNOTATIONS,
},{
// Orphans tab disabled, anchoring failed
anchorTimeout: false,
orphan: true,
separateOrphans: false,
expectedTab: uiConstants.TAB_ANNOTATIONS,
},{
// Orphans tab enabled, anchoring in progress.
anchorTimeout: false,
orphan: undefined, orphan: undefined,
separateOrphans: true, separateOrphans: true,
expectedTab: null, expectedTab: null,
},{ },{
// Orphans tab enabled, anchoring succeeded.
anchorTimeout: false,
orphan: false, orphan: false,
separateOrphans: true, separateOrphans: true,
expectedTab: uiConstants.TAB_ANNOTATIONS, expectedTab: uiConstants.TAB_ANNOTATIONS,
},{ },{
// Orphans tab enabled, anchoring failed.
anchorTimeout: false,
orphan: true,
separateOrphans: true,
expectedTab: uiConstants.TAB_ORPHANS,
},{
// Orphans tab enabled, anchoring timed out.
anchorTimeout: true,
orphan: undefined,
separateOrphans: true,
expectedTab: uiConstants.TAB_ANNOTATIONS,
},{
// Orphans tab enabled, anchoring initially timed out but eventually
// failed.
anchorTimeout: true,
orphan: true, orphan: true,
separateOrphans: true, separateOrphans: true,
expectedTab: uiConstants.TAB_ORPHANS, expectedTab: uiConstants.TAB_ORPHANS,
......
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