diff --git a/src/core/editor/pdf_editor.js b/src/core/editor/pdf_editor.js index 5c2288920..9c536e21e 100644 --- a/src/core/editor/pdf_editor.js +++ b/src/core/editor/pdf_editor.js @@ -660,11 +660,88 @@ class PDFEditor { } } - // Sort by insertAfter value so that each value is interpreted relative to - // the same base sequential sequence, then insert into the sequence. - // The offset accumulates the number of pages already inserted, converting - // base-relative positions to current-sequence positions. insertAfterList.sort((a, b) => a.insertAfter - b.insertAfter); + + // Partial pageIndices would auto-fill into free slots at extraction time, + // which collides with the positions insertAfter shifts/assigns. Reject. + if (insertAfterList.length > 0) { + for (const info of pageInfos) { + if (!info.document || !info.pageIndices) { + continue; + } + const filteredCount = this.#getFilteredPageIndices(info).length; + if (info.pageIndices.length < filteredCount) { + throw new Error( + "extractPages: partial pageIndices cannot be combined with insertAfter entries." + ); + } + } + } + + // If the base sequential sequence is empty but some entries carry explicit + // pageIndices (e.g. after a deletion), resolve each insertAfter against + // that explicit layout: shift existing positions past the insertion point + // and fill the gap. Entries without a document are ignored. With no + // explicit entries we fall through to the sequential branch, whose + // Array.splice already clamps to position 0 on an empty sequence. + const hasExplicitLayout = + insertAfterList.length > 0 && + pageInfos.some(info => info.document && info.pageIndices); + if (sequence.length === 0 && hasExplicitLayout) { + const updatedPageInfos = pageInfos.slice(); + let maxExistingPos = -1; + for (const info of pageInfos) { + if (info.document && info.pageIndices) { + for (const idx of info.pageIndices) { + if (idx > maxExistingPos) { + maxExistingPos = idx; + } + } + } + } + let offset = 0; + for (const { i, insertAfter, count } of insertAfterList) { + // Clamp to [-1, maxExistingPos] so out-of-range values don't produce + // negative or gap-creating positions. + const threshold = Math.min( + Math.max(insertAfter, -1) + offset, + maxExistingPos + ); + for (let j = 0; j < updatedPageInfos.length; j++) { + const existingInfo = updatedPageInfos[j]; + if ( + !existingInfo.document || + !existingInfo.pageIndices || + existingInfo.pageIndices.every(idx => idx <= threshold) + ) { + continue; + } + updatedPageInfos[j] = { + ...existingInfo, + pageIndices: existingInfo.pageIndices.map(idx => + idx > threshold ? idx + count : idx + ), + }; + } + const insertedIndices = []; + for (let k = 0; k < count; k++) { + insertedIndices.push(threshold + 1 + k); + } + const newInfo = { + ...updatedPageInfos[i], + pageIndices: insertedIndices, + }; + delete newInfo.insertAfter; + updatedPageInfos[i] = newInfo; + offset += count; + maxExistingPos += count; + } + return updatedPageInfos; + } + + // insertAfter values are relative to the base sequential sequence (stable + // across entries thanks to the sort above); offset converts them to + // current-sequence positions as we splice. let offset = 0; for (const { i, insertAfter, count } of insertAfterList) { const insertPos = insertAfter + 1 + offset; diff --git a/src/core/worker.js b/src/core/worker.js index 3f2e677b9..93b87e399 100644 --- a/src/core/worker.js +++ b/src/core/worker.js @@ -652,8 +652,7 @@ class WorkerMessageHandler { ); return buffer; } catch (reason) { - // eslint-disable-next-line no-console - console.error(reason); + warn(`extractPages: "${reason}".`); return null; } finally { if (task) { diff --git a/src/display/api.js b/src/display/api.js index 3cf541917..8b8977bd4 100644 --- a/src/display/api.js +++ b/src/display/api.js @@ -982,6 +982,26 @@ class PDFDocumentProxy { * included ranges or indices. * @property {Array|number>} [excludePages] * excluded ranges or indices. + * @property {Array} [pageIndices] Explicit 0-based positions in the + * final document for pages contributed by this entry. If shorter than the + * filtered page list, the remaining pages are placed in the first free + * slots at extraction time. Positions must not overlap with those of + * other entries, and the union of all explicit/auto-filled positions + * across the call must form a dense `[0, N)` range (where `N` is the + * total page count of the final document) — sparse layouts leave empty + * slots and are not supported. Cannot be combined with `insertAfter` on + * the same entry, and must fully cover the filtered page list when any + * entry in the same call specifies `insertAfter` (partial arrays are + * rejected in that case). + * @property {number} [insertAfter] 0-based index in the base sequential + * sequence (the concatenation of entries that have neither `pageIndices` + * nor `insertAfter`) after which to insert the pages. When every + * contributing entry carries explicit `pageIndices`, this is interpreted + * against that explicit layout instead, shifting any existing positions + * beyond the insertion point to make room. Use `-1` to insert before + * everything. Values beyond the current layout are clamped so the pages + * are appended at the end. Cannot be combined with `pageIndices` on the + * same entry. */ /** diff --git a/test/integration/reorganize_pages_spec.mjs b/test/integration/reorganize_pages_spec.mjs index ab0a6aa4c..cfc16a9f2 100644 --- a/test/integration/reorganize_pages_spec.mjs +++ b/test/integration/reorganize_pages_spec.mjs @@ -3097,6 +3097,58 @@ describe("Reorganize Pages View", () => { ); }); + it("should merge a PDF after the current page when first page was deleted (bug 2034804)", async () => { + await Promise.all( + pages.map(async ([browserName, page]) => { + await waitForThumbnailVisible(page, 1); + + // Select page 1 and delete it. + await waitAndClick( + page, + `.thumbnail:has(${getThumbnailSelector(1)}) input` + ); + const handlePagesEdited = await waitForPagesEdited(page); + await waitAndClick(page, "#viewsManagerStatusActionButton"); + await waitAndClick(page, "#viewsManagerStatusActionDelete"); + await awaitPromise(handlePagesEdited); + + // After deletion page 1 is the former page 2; navigate to it. + await page.waitForFunction( + () => window.PDFViewerApplication.page === 1 + ); + await waitAndClick(page, getThumbnailSelector(1)); + + const handleMerged = await createPromise(page, resolve => { + window.PDFViewerApplication.eventBus._on( + "thumbnailsloaded", + resolve, + { once: true } + ); + }); + + const picker = await page.$("#viewsManagerAddFilePicker"); + await picker.uploadFile( + path.join(__dirname, "../pdfs/three_pages_with_number.pdf") + ); + await awaitPromise(handleMerged); + + // 2 remaining original + 3 merged = 5 pages. + await page.waitForFunction( + () => parseInt(document.getElementById("pageNumber").max, 10) === 5 + ); + + // Former page 2, the 3 merged pages, then former page 3. + await waitForHavingContents(page, [2, 1, 2, 3, 3]); + + await waitForTextToBe( + page, + "#viewsManagerStatusActionLabel", + `${FSI}3${PDI} selected` + ); + }) + ); + }); + it("must mark document as needing save after merge (bug 2034461)", async () => { await Promise.all( pages.map(async ([browserName, page]) => { diff --git a/test/unit/api_spec.js b/test/unit/api_spec.js index b0c4cd95b..dfa59c3b5 100644 --- a/test/unit/api_spec.js +++ b/test/unit/api_spec.js @@ -7124,6 +7124,183 @@ small scripts as well as for`); await loadingTask.destroy(); }); + + it("insertAfter works when all base pages have explicit pageIndices", async function () { + // Mirrors a post-deletion merge: explicit pages 0 and 2 at pos 0/1, + // insert page 1 after pos 0 → "1"·"2"·"3". + let loadingTask = getDocument( + buildGetDocumentParams("page_with_number.pdf") + ); + let pdfDoc = await loadingTask.promise; + const data = await pdfDoc.extractPages([ + { document: null, includePages: [0, 2], pageIndices: [0, 1] }, + { document: null, includePages: [1], insertAfter: 0 }, + ]); + await loadingTask.destroy(); + + loadingTask = getDocument(data); + pdfDoc = await loadingTask.promise; + expect(pdfDoc.numPages).toEqual(3); + + for (const [pageNum, expected] of [ + [1, "1"], + [2, "2"], + [3, "3"], + ]) { + const pdfPage = await pdfDoc.getPage(pageNum); + const { items } = await pdfPage.getTextContent(); + expect(mergeText(items)) + .withContext(`Page ${pageNum}`) + .toEqual(expected); + } + + await loadingTask.destroy(); + }); + + it("two insertAfter entries at the same position with explicit pageIndices", async function () { + // Two insertAfter: 0 entries must land consecutively in entry order, + // yielding "1"·"2"·"3"·"4" (explicit pages 0 and 3 around them). + let loadingTask = getDocument( + buildGetDocumentParams("page_with_number.pdf") + ); + let pdfDoc = await loadingTask.promise; + const data = await pdfDoc.extractPages([ + { document: null, includePages: [0, 3], pageIndices: [0, 1] }, + { document: null, includePages: [1], insertAfter: 0 }, + { document: null, includePages: [2], insertAfter: 0 }, + ]); + await loadingTask.destroy(); + + loadingTask = getDocument(data); + pdfDoc = await loadingTask.promise; + expect(pdfDoc.numPages).toEqual(4); + + for (const [pageNum, expected] of [ + [1, "1"], + [2, "2"], + [3, "3"], + [4, "4"], + ]) { + const pdfPage = await pdfDoc.getPage(pageNum); + const { items } = await pdfPage.getTextContent(); + expect(mergeText(items)) + .withContext(`Page ${pageNum}`) + .toEqual(expected); + } + + await loadingTask.destroy(); + }); + + it("insertAfter: -1 prepends pages when all base pages have explicit pageIndices", async function () { + // insertAfter: -1 prepends pages 0 and 1 before the explicit page 2 + // → "1"·"2"·"3". + let loadingTask = getDocument( + buildGetDocumentParams("page_with_number.pdf") + ); + let pdfDoc = await loadingTask.promise; + const data = await pdfDoc.extractPages([ + { document: null, includePages: [2], pageIndices: [0] }, + { document: null, includePages: [0, 1], insertAfter: -1 }, + ]); + await loadingTask.destroy(); + + loadingTask = getDocument(data); + pdfDoc = await loadingTask.promise; + expect(pdfDoc.numPages).toEqual(3); + + for (const [pageNum, expected] of [ + [1, "1"], + [2, "2"], + [3, "3"], + ]) { + const pdfPage = await pdfDoc.getPage(pageNum); + const { items } = await pdfPage.getTextContent(); + expect(mergeText(items)) + .withContext(`Page ${pageNum}`) + .toEqual(expected); + } + + await loadingTask.destroy(); + }); + + it("insertAfter beyond the explicit layout appends pages at the end", async function () { + // Out-of-range insertAfter (5) must clamp to end rather than leave a + // gap → "1"·"2"·"3". + let loadingTask = getDocument( + buildGetDocumentParams("page_with_number.pdf") + ); + let pdfDoc = await loadingTask.promise; + const data = await pdfDoc.extractPages([ + { document: null, includePages: [0, 1], pageIndices: [0, 1] }, + { document: null, includePages: [2], insertAfter: 5 }, + ]); + await loadingTask.destroy(); + + loadingTask = getDocument(data); + pdfDoc = await loadingTask.promise; + expect(pdfDoc.numPages).toEqual(3); + + for (const [pageNum, expected] of [ + [1, "1"], + [2, "2"], + [3, "3"], + ]) { + const pdfPage = await pdfDoc.getPage(pageNum); + const { items } = await pdfPage.getTextContent(); + expect(mergeText(items)) + .withContext(`Page ${pageNum}`) + .toEqual(expected); + } + + await loadingTask.destroy(); + }); + + it("rejects partial pageIndices combined with insertAfter", async function () { + // Partial pageIndices + insertAfter could race over the same slots; + // the resolver throws and the worker surfaces it as a null result. + const loadingTask = getDocument( + buildGetDocumentParams("page_with_number.pdf") + ); + const pdfDoc = await loadingTask.promise; + const data = await pdfDoc.extractPages([ + { document: null, includePages: [0, 2], pageIndices: [0] }, + { document: null, includePages: [1], insertAfter: 0 }, + ]); + expect(data).toBeNull(); + + await loadingTask.destroy(); + }); + + it("insertAfter alone (no base sequential or explicit layout) places pages from position 0", async function () { + // With no base layout, insertAfter must still land pages from pos 0 + // (splice clamps on the empty sequence) → "1"·"2". + let loadingTask = getDocument( + buildGetDocumentParams("page_with_number.pdf") + ); + let pdfDoc = await loadingTask.promise; + const data = await pdfDoc.extractPages([ + { document: null, includePages: [0], insertAfter: 0 }, + { document: null, includePages: [1], insertAfter: 0 }, + ]); + await loadingTask.destroy(); + + loadingTask = getDocument(data); + pdfDoc = await loadingTask.promise; + expect(pdfDoc.numPages).toEqual(2); + + for (const [pageNum, expected] of [ + [1, "1"], + [2, "2"], + ]) { + const pdfPage = await pdfDoc.getPage(pageNum); + const { items } = await pdfPage.getTextContent(); + expect(mergeText(items)) + .withContext(`Page ${pageNum}`) + .toEqual(expected); + } + + await loadingTask.destroy(); + }); }); }); });