Skip to content

Commit 63ee8f3

Browse files
WiFuchsWiFuchs
authored
Make doodles not orphans (#13)
* refactor doodle loading * add doodles tab * fix test cases * clean up some smells
1 parent 6152b8c commit 63ee8f3

File tree

13 files changed

+117
-76
lines changed

13 files changed

+117
-76
lines changed

‎src/annotator/guest.js‎

Lines changed: 19 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -306,8 +306,7 @@ export default class Guest extends Delegator {
306306
});
307307

308308
this.subscribe('annotationsLoaded', annotations => {
309-
this.loadDoodles(annotations.filter(this.isDoodleAnnotation));
310-
annotations.map(annotation => this.anchor(annotation));
309+
annotations.forEach(annotation => this.anchor(annotation));
311310
});
312311
}
313312

@@ -499,6 +498,17 @@ export default class Guest extends Delegator {
499498
}
500499
}
501500
annotation.$orphan = hasAnchorableTargets && !hasAnchoredTargets;
501+
// An annotation is a doodle if any of its selectors have the type 'DoodleSelector'
502+
annotation.$doodle = anchors.some(a => {
503+
return (
504+
a.target && a.target.selector?.some(s => s.type === 'DoodleSelector')
505+
);
506+
});
507+
508+
// Display the doodle on the canvas
509+
if (annotation.$doodle) {
510+
this.loadDoodle(annotation);
511+
}
502512

503513
// Add the anchors for this annotation to instance storage.
504514
this.anchors = this.anchors.concat(anchors);
@@ -796,43 +806,17 @@ export default class Guest extends Delegator {
796806
}
797807
}
798808

799-
/**
800-
*
801-
* @param {*} annotation
802-
* @returns true if the annotation is a doodleAnnotation
803-
*/
804-
805-
isDoodleAnnotation(annotation) {
806-
// If any of the targets have a DoodleSelector, this is a doodle annotation. Otherwise, it is not.
807-
if (annotation.target) {
808-
for (let targ of annotation.target) {
809-
// not all targets have selectors at this point
810-
if (targ.selector) {
811-
for (let selector of targ.selector) {
812-
if (selector.type === 'DoodleSelector') {
813-
return true;
814-
}
815-
}
816-
}
817-
}
818-
}
819-
return false;
820-
}
821-
822-
loadDoodles(doodleAnnotations) {
823-
// First, make sure there are doodleAnnotations and a Controller
824-
if (!doodleAnnotations.length || !this.doodleCanvasController) {
809+
loadDoodle(doodleAnnotation) {
810+
if (!this.doodleCanvasController) {
825811
return;
826812
}
827813

828-
// Then, load the lines into our doodleCanvasController.
814+
//load the lines into our doodleCanvasController.
829815
let newLines = [];
830-
for (let doodle of doodleAnnotations) {
831-
for (let targ of doodle.target) {
832-
for (let sel of targ.selector) {
833-
if (sel.type === 'DoodleSelector') {
834-
newLines = [...newLines, sel.line];
835-
}
816+
for (let targ of doodleAnnotation.target) {
817+
for (let sel of targ.selector) {
818+
if (sel.type === 'DoodleSelector') {
819+
newLines = [...newLines, sel.line];
836820
}
837821
}
838822
}

‎src/annotator/test/guest-test.js‎

Lines changed: 30 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -206,36 +206,6 @@ describe('Guest', () => {
206206
assert.calledWith(guest.anchor, ann2);
207207
});
208208

209-
it('calls loadDoodles with only doodle annotations on "annotationsLoaded"', () => {
210-
const ann1 = {
211-
id: 1,
212-
$tag: 'tag1',
213-
target: [
214-
{
215-
selector: [
216-
{
217-
type: 'DoodleSelector',
218-
line: {
219-
points: [
220-
[1, 1],
221-
[2, 2],
222-
],
223-
size: 5,
224-
color: '#FF0000',
225-
tool: 'pen',
226-
},
227-
},
228-
],
229-
},
230-
],
231-
};
232-
const ann2 = { id: 2, $tag: 'tag2' };
233-
sandbox.stub(guest, 'anchor');
234-
options.emit('annotationsLoaded', [ann1, ann2]);
235-
assert.calledOnce(guest.loadDoodles);
236-
assert.calledWith(guest.loadDoodles, [ann1]);
237-
});
238-
239209
it('proxies all other events into the annotator event system', () => {
240210
const fooHandler = sandbox.stub();
241211
const barHandler = sandbox.stub();
@@ -691,6 +661,36 @@ describe('Guest', () => {
691661
.then(() => assert.isFalse(annotation.$orphan));
692662
});
693663

664+
it('marks an annotation with DoodleSelectors as a doodle', () => {
665+
const guest = createGuest();
666+
const annotation = {
667+
id: 1,
668+
$tag: 'tag1',
669+
target: [
670+
{
671+
selector: [
672+
{
673+
type: 'DoodleSelector',
674+
line: {
675+
points: [
676+
[1, 1],
677+
[2, 2],
678+
],
679+
size: 5,
680+
color: '#FF0000',
681+
tool: 'pen',
682+
},
683+
},
684+
],
685+
},
686+
],
687+
};
688+
689+
return guest
690+
.anchor(annotation)
691+
.then(() => assert.isTrue(annotation.$doodle));
692+
});
693+
694694
it("doesn't mark an annotation with a selectorless target as an orphan", () => {
695695
const guest = createGuest();
696696
const annotation = { target: [{ source: 'wibble' }] };

‎src/sidebar/components/SelectionTabs.js‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,7 @@ function SelectionTabs({ isLoading, settings }) {
9696
const noteCount = store.noteCount();
9797
const annotationCount = store.annotationCount();
9898
const orphanCount = store.orphanCount();
99+
const doodleCount = store.doodleCount();
99100
const isWaitingToAnchorAnnotations = store.isWaitingToAnchorAnnotations();
100101

101102
/**
@@ -145,6 +146,17 @@ function SelectionTabs({ isLoading, settings }) {
145146
Orphans
146147
</Tab>
147148
)}
149+
{doodleCount > 0 && (
150+
<Tab
151+
count={doodleCount}
152+
isWaitingToAnchor={isWaitingToAnchorAnnotations}
153+
isSelected={selectedTab === 'doodle'}
154+
label="Doodles"
155+
onSelect={() => selectTab('doodle')}
156+
>
157+
Doodles
158+
</Tab>
159+
)}
148160
</div>
149161
{selectedTab === 'note' && settings.enableExperimentalNewNoteButton && (
150162
<NewNoteBtn />

‎src/sidebar/components/test/SelectionTabs-test.js‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ describe('SelectionTabs', function () {
3333
annotationCount: sinon.stub().returns(123),
3434
noteCount: sinon.stub().returns(456),
3535
orphanCount: sinon.stub().returns(0),
36+
doodleCount: sinon.stub().returns(1),
3637
isWaitingToAnchorAnnotations: sinon.stub().returns(false),
3738
selectedTab: sinon.stub().returns('annotation'),
3839
};
@@ -92,7 +93,7 @@ describe('SelectionTabs', function () {
9293
fakeStore.selectedTab.returns('orphan');
9394
const wrapper = createComponent({});
9495
const tabs = wrapper.find('button');
95-
assert.equal(tabs.length, 2);
96+
assert.equal(tabs.length, 3);
9697
});
9798

9899
it('should render `title` and `aria-label` attributes for tab buttons, with counts', () => {
@@ -200,6 +201,7 @@ describe('SelectionTabs', function () {
200201
{ label: 'Annotations', tab: 'annotation' },
201202
{ label: 'Page Notes', tab: 'note' },
202203
{ label: 'Orphans', tab: 'orphan' },
204+
{ label: 'Doodles', tab: 'doodle' },
203205
].forEach(({ label, tab }) => {
204206
it(`should change the selected tab when "${label}" tab is clicked`, () => {
205207
// Pre-select a different tab than the one we are about to click.

‎src/sidebar/helpers/annotation-metadata.js‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -235,6 +235,15 @@ export function isOrphan(annotation) {
235235
return hasSelector(annotation) && annotation.$orphan === true;
236236
}
237237

238+
/**
239+
* Return `true` if the given annotation is a doodle.
240+
*
241+
* @param {Annotation} annotation
242+
*/
243+
export function isDoodle(annotation) {
244+
return hasSelector(annotation) && annotation.$doodle === true;
245+
}
246+
238247
/**
239248
* Return `true` if the given annotation is a page note.
240249
*

‎src/sidebar/helpers/tabs.js‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@ export function tabForAnnotation(ann) {
1818
return 'orphan';
1919
} else if (metadata.isPageNote(ann)) {
2020
return 'note';
21+
} else if (metadata.isDoodle(ann)) {
22+
return 'doodle';
2123
} else {
2224
return 'annotation';
2325
}

‎src/sidebar/helpers/thread-annotations.js‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import { sorters } from './thread-sorters';
99
/** @typedef {import('../../types/api').Annotation} Annotation */
1010
/** @typedef {import('./build-thread').Thread} Thread */
1111
/** @typedef {import('./build-thread').BuildThreadOptions} BuildThreadOptions */
12+
/** @typedef {import('../../types/sidebar').TabName} TabName */
1213

1314
/**
1415
* @typedef ThreadState
@@ -20,7 +21,7 @@ import { sorters } from './thread-sorters';
2021
* @prop {string[]} selection.forcedVisible
2122
* @prop {string[]} selection.selected
2223
* @prop {string} selection.sortKey
23-
* @prop {'annotation'|'note'|'orphan'} selection.selectedTab
24+
* @prop {TabName} selection.selectedTab
2425
* @prop {string|null} route
2526
*/
2627

‎src/sidebar/services/annotations.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@ export default function annotationsService(api, store) {
119119
} else if (metadata.isAnnotation(annotation)) {
120120
// if this is a doodle, select the doodle tab
121121
if (annotation.$doodle) {
122-
store.selectTab('orphan');
122+
store.selectTab('doodle');
123123
} else {
124124
store.selectTab('annotation');
125125
}

‎src/sidebar/services/frame-sync.js‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -145,11 +145,21 @@ export default function FrameSync(annotationsService, bridge, store) {
145145

146146
// Anchoring an annotation in the frame completed
147147
bridge.on('sync', function (events_) {
148+
const getAnchoringStatus = (orphan, doodle) => {
149+
if (orphan) {
150+
if (doodle) {
151+
return 'doodle';
152+
}
153+
return 'orphan';
154+
}
155+
return 'anchored';
156+
};
148157
events_.forEach(function (event) {
149158
inFrame.add(event.tag);
150-
anchoringStatusUpdates[event.tag] = event.msg.$orphan
151-
? 'orphan'
152-
: 'anchored';
159+
anchoringStatusUpdates[event.tag] = getAnchoringStatus(
160+
event.msg.$orphan,
161+
event.msg.$doodle
162+
);
153163
scheduleAnchoringStatusUpdate();
154164
});
155165
});

‎src/sidebar/services/test/frame-sync-test.js‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -288,14 +288,22 @@ describe('sidebar/services/frame-sync', function () {
288288
});
289289

290290
it('coalesces multiple "sync" messages', () => {
291-
fakeBridge.emit('sync', [{ tag: 't1', msg: { $orphan: false } }]);
292-
fakeBridge.emit('sync', [{ tag: 't2', msg: { $orphan: true } }]);
291+
fakeBridge.emit('sync', [
292+
{ tag: 't1', msg: { $orphan: false, $doodle: false } },
293+
]);
294+
fakeBridge.emit('sync', [
295+
{ tag: 't2', msg: { $orphan: true, $doodle: false } },
296+
]);
297+
fakeBridge.emit('sync', [
298+
{ tag: 't3', msg: { $orphan: true, $doodle: true } },
299+
]);
293300

294301
expireDebounceTimeout();
295302

296303
assert.calledWith(fakeStore.updateAnchorStatus, {
297304
t1: 'anchored',
298305
t2: 'orphan',
306+
t3: 'doodle',
299307
});
300308
});
301309
});

0 commit comments

Comments
 (0)