Commit 0c16351c authored by Robert Knight's avatar Robert Knight

Only consider part of CFI up to step indirection

When filtering annotations against CFIs or matching the current segment
against a focus filter, only consider the part of the CFI up to the
first step indirection ("!"). This means that matching only considers whether
CFIs refer to the same content document (which the client also refers to more
generically as a "segment"), and not the precise part of the content document.

This resolves an issue where a CFI focus filter with a step indirection (eg.
"/2/4!/8-/4!/2") did not match the expected segment ("/2").
parent 2392ad46
...@@ -82,10 +82,11 @@ export function stripCFIAssertions(cfi: string): string { ...@@ -82,10 +82,11 @@ export function stripCFIAssertions(cfi: string): string {
* *
* The full sorting rules for CFIs are specified by https://idpf.org/epub/linking/cfi/#sec-sorting. * The full sorting rules for CFIs are specified by https://idpf.org/epub/linking/cfi/#sec-sorting.
* *
* This function currently only implements what is necessary to compare simple * This function only considers the part of the CFI up to the first step
* CFIs that specify a location within an EPUB's Package Document, without any step * indirection ("!"), which identify a location within the EPUB's Package
* indirections ("!"). These CFIs consist of a "/"-delimited sequence of numbers, * Document. These portions of CFIs consist of a "/"-delimited sequence of
* with optional assertions in `[...]` brackets (eg. "/2/4[chapter2ref]"). * numbers, with optional assertions in `[...]` brackets (eg.
* "/2/4[chapter2ref]").
* *
* Per the sorting rules linked above, the input CFIs are assumed to be * Per the sorting rules linked above, the input CFIs are assumed to be
* unescaped. This means that they may contain circumflex (^) escape characters, * unescaped. This means that they may contain circumflex (^) escape characters,
...@@ -102,7 +103,7 @@ export function stripCFIAssertions(cfi: string): string { ...@@ -102,7 +103,7 @@ export function stripCFIAssertions(cfi: string): string {
*/ */
export function compareCFIs(a: string, b: string): number { export function compareCFIs(a: string, b: string): number {
const parseCFI = (cfi: string) => { const parseCFI = (cfi: string) => {
return stripCFIAssertions(cfi) return documentCFI(cfi)
.split('/') .split('/')
.map(str => { .map(str => {
// CFI step values _should_ always be integers. We currently handle // CFI step values _should_ always be integers. We currently handle
...@@ -117,6 +118,9 @@ export function compareCFIs(a: string, b: string): number { ...@@ -117,6 +118,9 @@ export function compareCFIs(a: string, b: string): number {
/** /**
* Return true if the CFI `cfi` lies in the range [start, end). * Return true if the CFI `cfi` lies in the range [start, end).
*
* Only the part of the CFI up to the first step indirection ("!") is
* considered. See {@link documentCFI}.
*/ */
export function cfiInRange(cfi: string, start: string, end: string): boolean { export function cfiInRange(cfi: string, start: string, end: string): boolean {
return compareCFIs(cfi, start) >= 0 && compareCFIs(cfi, end) < 0; return compareCFIs(cfi, start) >= 0 && compareCFIs(cfi, end) < 0;
......
...@@ -97,6 +97,23 @@ describe('sidebar/util/cfi', () => { ...@@ -97,6 +97,23 @@ describe('sidebar/util/cfi', () => {
b: '', b: '',
expected: 0, expected: 0,
}, },
// CFIs with step indirections. Only the part before the step indirection
// is considered.
{
a: '/2!/4',
b: '/2!/8',
expected: 0,
},
{
a: '/2!/4',
b: '/4!/8',
expected: -1,
},
{
a: '/4!/8',
b: '/2!/4',
expected: 1,
},
].forEach(({ a, b, expected }) => { ].forEach(({ a, b, expected }) => {
it('compares CFIs', () => { it('compares CFIs', () => {
assert.equal( assert.equal(
...@@ -162,6 +179,14 @@ describe('sidebar/util/cfi', () => { ...@@ -162,6 +179,14 @@ describe('sidebar/util/cfi', () => {
end: '/4', end: '/4',
expected: false, expected: false,
}, },
// CFIs with step indirections. Only the part before the first step
// indirection is considered.
{
cfi: '/6',
start: '/6!/2',
end: '/8!/4',
expected: true,
},
].forEach(({ cfi, start, end, expected }) => { ].forEach(({ cfi, start, end, expected }) => {
it('should return true if the cfi is in the range [start, end)', () => { it('should return true if the cfi is in the range [start, end)', () => {
assert.equal(cfiInRange(cfi, start, end), expected); assert.equal(cfiInRange(cfi, start, end), expected);
......
...@@ -122,6 +122,13 @@ describe('segmentMatchesFocusFilters', () => { ...@@ -122,6 +122,13 @@ describe('segmentMatchesFocusFilters', () => {
filters: { cfi: { value: '/2-/4' } }, filters: { cfi: { value: '/2-/4' } },
expected: true, expected: true,
}, },
// Filter present, matches segment. Filter CFI has step indirections,
// which should be ignored.
{
segment: { cfi: '/2' },
filters: { cfi: { value: '/2!/2-/4!/4' } },
expected: true,
},
// Filter present, does not match segment // Filter present, does not match segment
{ {
segment: { cfi: '/6' }, segment: { cfi: '/6' },
......
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