diff --git a/src/core/editor/pdf_editor.js b/src/core/editor/pdf_editor.js index 9aa0d7fbd..c39df045c 100644 --- a/src/core/editor/pdf_editor.js +++ b/src/core/editor/pdf_editor.js @@ -57,6 +57,8 @@ class PageData { this.annotations = null; // Named destinations which points to this page. this.pointingNamedDestinations = null; + // Rank of this page among the output pages sharing the same source page. + this.copyLevel = 0; documentData.pagesMap.put(page.ref, this); } @@ -1097,10 +1099,18 @@ class PDFEditor { } } await Promise.all(promises); + const copyCounts = new Map(); for (let i = 0, ii = this.oldPages.length; i < ii; i++) { - if (this.oldPages[i] === undefined) { + const pageData = this.oldPages[i]; + if (pageData === undefined) { throw new Error("extractPages: sparse pageIndices."); } + if (pageData) { + const { page } = pageData; + const copyLevel = copyCounts.get(page) ?? 0; + copyCounts.set(page, copyLevel + 1); + pageData.copyLevel = copyLevel; + } } promises.length = 0; @@ -2398,12 +2408,18 @@ class PDFEditor { /** * Create a copy of a page. - * @param {number} pageIndex + * @param {number} pageIndex - Index of the page slot in the new document + * (the index of the source page is `page.pageIndex`). * @returns {Promise} the page reference in the new PDF document. */ async #makePageCopy(pageIndex) { - const { page, documentData, annotations, pointingNamedDestinations } = - this.oldPages[pageIndex]; + const { + page, + documentData, + annotations, + pointingNamedDestinations, + copyLevel, + } = this.oldPages[pageIndex]; this.currentDocument = documentData; const { dedupNamedDestinations, oldRefMapping } = documentData; const { xref, rotate, mediaBox, resources, ref: oldPageRef } = page; @@ -2472,9 +2488,11 @@ class PDFEditor { const newAnnotations = documentData.document === this.#primaryDocument - ? this.#newAnnotationsParams?.newAnnotationsByPage?.get(page.pageIndex) + ? this.#newAnnotationsParams?.newAnnotationsByPage + ?.get(page.pageIndex) + ?.filter(({ copyLevel: level }) => (level ?? 0) === copyLevel) : null; - if (newAnnotations) { + if (newAnnotations?.length) { const { handler, task, imagesPromises } = this.#newAnnotationsParams; const changes = new RefSetCache(); const newData = await AnnotationFactory.saveNewAnnotations( diff --git a/src/display/api.js b/src/display/api.js index 908b2af6b..6ebe0a0e9 100644 --- a/src/display/api.js +++ b/src/display/api.js @@ -1009,11 +1009,15 @@ class PDFDocumentProxy { /** * @param {Array} pageInfos - The pages to extract. + * @param {Int32Array} [copyLevels] - For each viewer page, its rank among the + * extracted pages sharing the same source page, or -1 if it isn't extracted. + * This routes editor annotations when the viewer contains multiple copies + * of a source page. * @returns {Promise} A promise that is resolved with a * {Uint8Array} containing the full data of the saved document. */ - extractPages(pageInfos) { - return this._transport.extractPages(pageInfos); + extractPages(pageInfos, copyLevels = null) { + return this._transport.extractPages(pageInfos, copyLevels); } /** @@ -2936,7 +2940,7 @@ class WorkerTransport { }); } - extractPages(pageInfos) { + extractPages(pageInfos, copyLevels = null) { const params = { pageInfos, }; @@ -2963,6 +2967,8 @@ class WorkerTransport { // Annotation pageIndex tracks the editor's current viewer position; the // worker keys lookups by source index. Remap UI -> source via pagesMapper // so reorganized pages still receive their annotations after extraction. + // Multiple viewer pages can share a source page. The copy level routes + // each editor annotation to the corresponding extracted copy. const mapping = this.pagesMapper.getMapping(); if (mapping) { const remapped = new Map(); @@ -2972,9 +2978,13 @@ class WorkerTransport { v.pageIndex >= 0 && v.pageIndex < mapping.length ) { + // copyLevels uses -1 for non-extracted pages. Keep their entries + // because an extracted stamp may share their bitmapId; the worker + // uses the negative level to skip the annotation itself. + const copyLevel = copyLevels?.[v.pageIndex] ?? 0; const sourceIdx = mapping[v.pageIndex] - 1; - if (sourceIdx !== v.pageIndex) { - remapped.set(k, { ...v, pageIndex: sourceIdx }); + if (sourceIdx !== v.pageIndex || copyLevel !== 0) { + remapped.set(k, { ...v, pageIndex: sourceIdx, copyLevel }); continue; } } diff --git a/src/display/editor/tools.js b/src/display/editor/tools.js index d33829c2c..af98c6940 100644 --- a/src/display/editor/tools.js +++ b/src/display/editor/tools.js @@ -748,6 +748,8 @@ class AnnotationEditorUIManager { #savedAllLayers = null; + #savedEditorsByPage = null; + #altTextManager = null; #annotationStorage = null; @@ -1913,7 +1915,7 @@ class AnnotationEditorUIManager { } updatePageIndex(oldPageIndex, newPageIndex) { - for (const editor of this.getEditors(oldPageIndex)) { + for (const editor of this.#savedEditorsByPage.get(oldPageIndex) || []) { editor.pageIndex = newPageIndex; } const layer = this.#savedAllLayers.get(oldPageIndex); @@ -1931,10 +1933,32 @@ class AnnotationEditorUIManager { startUpdatePages() { this.#savedAllLayers = new Map(this.#allLayers); this.#allLayers.clear(); + + const savedEditorsByPage = (this.#savedEditorsByPage = new Map()); + const saveEditor = editor => { + savedEditorsByPage + .getOrInsertComputed(editor.pageIndex, makeArr) + .push(editor); + }; + for (const editor of this.#allEditors.values()) { + saveEditor(editor); + } + // Clones are initially kept serialized until their editor layer is + // rendered, hence they're not present in #allEditors yet. + for (const [id, editor] of this.#annotationStorage) { + if ( + id.startsWith(AnnotationEditorPrefix) && + !this.#allEditors.has(id) && + Number.isInteger(editor?.pageIndex) + ) { + saveEditor(editor); + } + } } endUpdatePages() { this.#savedAllLayers = null; + this.#savedEditorsByPage = null; } clonePage(pageIndex, newPageIndex) { diff --git a/src/display/pages_mapper.js b/src/display/pages_mapper.js index 3dca542c2..0ffe3966e 100644 --- a/src/display/pages_mapper.js +++ b/src/display/pages_mapper.js @@ -111,7 +111,6 @@ class PagesMapper { movePages(selectedPages, pagesToMove, index) { this.#ensureInit(); const pageNumberToId = this.#pageNumberToId; - const prevIdToPageNumber = this.#buildIdToPageNumber(); const movedCount = pagesToMove.length; const mappedPagesToMove = new Uint32Array(movedCount); let removedBeforeTarget = 0; @@ -126,6 +125,7 @@ class PagesMapper { const pagesNumber = this.#pagesNumber; const remainingLen = pagesNumber - movedCount; + const prevPageNumbers = new Int32Array(pagesNumber); const adjustedTarget = MathClamp( index - removedBeforeTarget, 0, @@ -135,7 +135,8 @@ class PagesMapper { // Compact: keep only non-moved pages. for (let i = 0, r = 0; i < pagesNumber; i++) { if (!selectedPages.has(i + 1)) { - pageNumberToId[r++] = pageNumberToId[i]; + pageNumberToId[r] = pageNumberToId[i]; + prevPageNumbers[r++] = i + 1; } } @@ -146,8 +147,13 @@ class PagesMapper { remainingLen ); pageNumberToId.set(mappedPagesToMove, adjustedTarget); - - this.#updatePrevPageNumbers(prevIdToPageNumber); + prevPageNumbers.copyWithin( + adjustedTarget + movedCount, + adjustedTarget, + remainingLen + ); + prevPageNumbers.set(pagesToMove, adjustedTarget); + this.#prevPageNumbers = prevPageNumbers; if (pageNumberToId.every((id, i) => id === i + 1)) { this.#pageNumberToId = null; @@ -310,12 +316,48 @@ class PagesMapper { return this.#pageNumberToId !== null; } + /** + * Builds the copy level of every page. Extracted pages are ranked, in page + * order, among the extracted pages sharing their page ID. Pages which aren't + * extracted get -1. + * @param {Array} [extractedPageNumbers] - Sorted 1-based page + * numbers to extract, or null to extract everything. + * @returns {Int32Array|null} null when the page mapping is the identity. + */ + #buildCopyLevels(extractedPageNumbers = null) { + if (!this.#pageNumberToId) { + return null; + } + const copyLevels = new Int32Array(this.#pagesNumber).fill(-1); + const counts = new Map(); + if (extractedPageNumbers) { + for (const pageNumber of extractedPageNumbers) { + const id = this.getPageId(pageNumber); + const level = counts.get(id) ?? 0; + counts.set(id, level + 1); + copyLevels[pageNumber - 1] = level; + } + } else { + for (let i = 0, ii = this.#pagesNumber; i < ii; i++) { + const id = this.#pageNumberToId[i]; + const level = counts.get(id) ?? 0; + counts.set(id, level + 1); + copyLevels[i] = level; + } + } + return copyLevels; + } + /** * Gets the current page mapping suitable for saving. * @param {Map>} [idToPageNumber] - * @returns {Array} + * @param {Int32Array} [copyLevels] + * @returns {{pageInfos: Array, copyLevels: Int32Array|null}} */ - getPageMappingForSaving(idToPageNumber = null) { + getPageMappingForSaving( + idToPageNumber = null, + copyLevels = this.#buildCopyLevels() + ) { idToPageNumber ??= this.#buildIdToPageNumber(); // idToPageNumber maps used 1-based IDs to 1-based page numbers. // For example if the final pdf contains page 3 twice and they are moved at @@ -364,7 +406,7 @@ class PagesMapper { } } - return extractParams; + return { pageInfos: extractParams, copyLevels }; } extractPages(extractedPageNumbers) { @@ -377,7 +419,10 @@ class PagesMapper { const usedPageNumbers = usedIds.getOrInsertComputed(id, makeArr); usedPageNumbers.push(i + 1); } - return this.getPageMappingForSaving(usedIds); + return this.getPageMappingForSaving( + usedIds, + this.#buildCopyLevels(extractedPageNumbers) + ); } /** diff --git a/test/integration/reorganize_pages_spec.mjs b/test/integration/reorganize_pages_spec.mjs index 2a63fdf64..dd4e261f6 100644 --- a/test/integration/reorganize_pages_spec.mjs +++ b/test/integration/reorganize_pages_spec.mjs @@ -815,19 +815,13 @@ describe("Reorganize Pages View", () => { await page.waitForSelector("#viewsManagerStatusActionButton", { visible: true, }); - const rect1 = await getRect(page, getThumbnailSelector(1)); - const rect2 = await getRect(page, getThumbnailSelector(2)); - - await dragAndDrop( - page, - getThumbnailSelector(1), - [[0, rect2.y - rect1.y + rect2.height / 2]], - 10 - ); + // Drag-and-drop behavior is covered separately. Use an exact move + // here so this test only checks the save payload. + await movePages(page, [1], 2); const handleSave = await createPromise(page, resolve => { window.PDFViewerApplication.onSavePages = async ({ data }) => { - resolve(Array.from(data[0].pageIndices)); + resolve(Array.from(data.pageInfos[0].pageIndices)); }; }); @@ -2032,9 +2026,12 @@ describe("Reorganize Pages View", () => { const pagesData = await awaitPromise(handleExport); expect(pagesData) .withContext(`In ${browserName}`) - .toEqual([ - { document: null, pageIndices: [0, 1], includePages: [0, 2] }, - ]); + .toEqual({ + pageInfos: [ + { document: null, pageIndices: [0, 1], includePages: [0, 2] }, + ], + copyLevels: null, + }); await waitForTextToBe(page, labelSelector, "Select pages"); // All checkboxes should be unchecked. @@ -2865,7 +2862,7 @@ describe("Reorganize Pages View", () => { await closePages(pages); }); - it("should check that the pasted page has an ink annotation in the DOM", async () => { + it("should keep an ink annotation on a pasted and moved page", async () => { await Promise.all( pages.map(async ([browserName, page]) => { // Enable ink editor mode and draw a line on page 1. @@ -2899,20 +2896,43 @@ describe("Reorganize Pages View", () => { // Both the original and the cloned annotation must now be in storage. await waitForStorageEntries(page, 2); - // Close the reorganize view and navigate to page 3 (the pasted copy) - // to trigger rendering of its annotation editor layer. + const editorIds = await page.evaluate(() => { + const entries = Array.from( + window.PDFViewerApplication.pdfDocument.annotationStorage + ); + return { + original: entries.find(([, editor]) => editor.pageIndex === 0)[0], + clone: entries.find(([, editor]) => editor.pageIndex === 2)[0], + }; + }); + + // Move the pasted copy before the original and verify that both + // stored page indices follow the new page order. + await movePages(page, [3], 0); + const editorPageIndices = await page.evaluate(ids => { + const storage = + window.PDFViewerApplication.pdfDocument.annotationStorage; + return { + original: storage.getRawValue(ids.original).pageIndex, + clone: storage.getRawValue(ids.clone).pageIndex, + }; + }, editorIds); + expect(editorPageIndices) + .withContext(`In ${browserName}`) + .toEqual({ original: 1, clone: 0 }); + + // Show the moved copy and verify that its cloned editor survived. await page.click("#viewsManagerToggleButton"); await page.waitForSelector("#viewsManager", { hidden: true }); await page.evaluate(() => { - window.PDFViewerApplication.pdfViewer.currentPageNumber = 3; + window.PDFViewerApplication.pdfViewer.currentPageNumber = 1; }); - // The cloned ink annotation must appear in the DOM of page 3. - await page.waitForSelector(`.page[data-page-number="3"] .inkEditor`, { + await page.waitForSelector(`.page[data-page-number="1"] .inkEditor`, { visible: true, }); const inkEditors = await page.$$( - `.page[data-page-number="3"] .inkEditor` + `.page[data-page-number="1"] .inkEditor` ); expect(inkEditors.length).withContext(`In ${browserName}`).toBe(1); }) diff --git a/test/unit/api_spec.js b/test/unit/api_spec.js index b9d173bfc..73b2d03cf 100644 --- a/test/unit/api_spec.js +++ b/test/unit/api_spec.js @@ -7302,9 +7302,14 @@ small scripts as well as for`); let loadingTask = getDocument(buildGetDocumentParams("empty.pdf")); let pdfDoc = await loadingTask.promise; - // Simulate what clonePage() puts in annotationStorage when a page is - // copied: the original annotation stays on pageIndex 0 and the clone - // is placed on pageIndex 1 (the new position of the pasted copy). + // Mirror the viewer: copy page 1 and paste the copy right after it. + pdfDoc.pagesMapper.copyPages(new Uint32Array([1])); + pdfDoc.pagesMapper.pastePages(1); + expect(pdfDoc.pagesMapper.pagesNumber).toEqual(2); + + // Model the annotationStorage entries after clonePage(): the original + // stays at viewer page index 0 and the copy gets an entry at index 1. + // Both viewer pages map to source page 0. const inkAnnotation = { annotationType: AnnotationEditorType.INK, rect: [50, 50, 200, 200], @@ -7342,15 +7347,15 @@ small scripts as well as for`); }); pdfDoc.annotationStorage.setValue("pdfjs_internal_editor_1", { ...inkAnnotation, + color: [255, 0, 0], pageIndex: 1, + isClone: true, }); - // Extract page 0 twice: once at output position 0 (original) and once - // at output position 1 (clone), mirroring copy+paste in the UI. - const data = await pdfDoc.extractPages([ - { document: null, includePages: [0], pageIndices: [0] }, - { document: null, includePages: [0], pageIndices: [1] }, - ]); + const { pageInfos, copyLevels } = pdfDoc.pagesMapper.extractPages( + new Set([1, 2]) + ); + const data = await pdfDoc.extractPages(pageInfos, copyLevels); await loadingTask.destroy(); loadingTask = getDocument({ data }); @@ -7358,7 +7363,8 @@ small scripts as well as for`); expect(pdfDoc.numPages).toEqual(2); - // Both pages should carry the ink annotation. + // Each page must have the expected color and a distinct annotation ID. + const annotationIds = []; for (let i = 1; i <= 2; i++) { const pdfPage = await pdfDoc.getPage(i); const annotations = await pdfPage.getAnnotations(); @@ -7366,7 +7372,174 @@ small scripts as well as for`); expect(annotations[0].annotationType) .withContext(`Page ${i}`) .toEqual(AnnotationType.INK); + expect(Array.from(annotations[0].color)) + .withContext(`Page ${i}`) + .toEqual(i === 1 ? [0, 0, 255] : [255, 0, 0]); + annotationIds.push(annotations[0].id); } + expect(new Set(annotationIds).size).toEqual(2); + + await loadingTask.destroy(); + }); + + it("keeps distinct annotations when a clone moves before its original", async function () { + let loadingTask = getDocument(buildGetDocumentParams("empty.pdf")); + let pdfDoc = await loadingTask.promise; + + pdfDoc.pagesMapper.copyPages(new Uint32Array([1])); + pdfDoc.pagesMapper.pastePages(1); + + const freeText = { + annotationType: AnnotationEditorType.FREETEXT, + rect: [12, 34, 56, 78], + rotation: 0, + fontSize: 10, + color: [0, 0, 0], + }; + const original = { + ...freeText, + value: "on the original", + pageIndex: 0, + }; + const clone = { + ...freeText, + value: "on the clone", + pageIndex: 1, + isClone: true, + }; + pdfDoc.annotationStorage.setValue("pdfjs_internal_editor_0", original); + pdfDoc.annotationStorage.setValue("pdfjs_internal_editor_1", clone); + + // Move the clone before the original and update both editor page + // indices to match. + pdfDoc.pagesMapper.movePages(new Set([2]), [2], 0); + expect(pdfDoc.pagesMapper.getPrevPageNumber(1)).toEqual(2); + expect(pdfDoc.pagesMapper.getPrevPageNumber(2)).toEqual(1); + clone.pageIndex = 0; + original.pageIndex = 1; + + const { pageInfos, copyLevels } = + pdfDoc.pagesMapper.getPageMappingForSaving(); + const data = await pdfDoc.extractPages(pageInfos, copyLevels); + await loadingTask.destroy(); + + loadingTask = getDocument({ data }); + pdfDoc = await loadingTask.promise; + expect(pdfDoc.numPages).toEqual(2); + + for (let i = 1; i <= 2; i++) { + const pdfPage = await pdfDoc.getPage(i); + const annotations = await pdfPage.getAnnotations(); + expect(annotations.map(a => a.contentsObj?.str)).toEqual([ + i === 1 ? "on the clone" : "on the original", + ]); + } + + await loadingTask.destroy(); + }); + + it("only keeps the annotations of the extracted copy of a page", async function () { + let loadingTask = getDocument( + buildGetDocumentParams("three_pages_with_number.pdf") + ); + let pdfDoc = await loadingTask.promise; + + // Copy page 1 and paste it at the end: source page 0 is now at the + // viewer positions 1 and 4. + pdfDoc.pagesMapper.copyPages(new Uint32Array([1])); + pdfDoc.pagesMapper.pastePages(3); + expect(pdfDoc.pagesMapper.pagesNumber).toEqual(4); + + const freeText = { + annotationType: AnnotationEditorType.FREETEXT, + rect: [12, 34, 56, 78], + rotation: 0, + fontSize: 10, + color: [0, 0, 0], + }; + pdfDoc.annotationStorage.setValue("pdfjs_internal_editor_0", { + ...freeText, + value: "on the original", + pageIndex: 0, + }); + pdfDoc.annotationStorage.setValue("pdfjs_internal_editor_1", { + ...freeText, + value: "on the clone", + pageIndex: 3, + isClone: true, + }); + + // Extract the clone only: the annotation of the original must not be + // written on it. + const { pageInfos, copyLevels } = pdfDoc.pagesMapper.extractPages( + new Set([4]) + ); + const data = await pdfDoc.extractPages(pageInfos, copyLevels); + await loadingTask.destroy(); + + loadingTask = getDocument({ data }); + pdfDoc = await loadingTask.promise; + expect(pdfDoc.numPages).toEqual(1); + + const pdfPage = await pdfDoc.getPage(1); + const annotations = await pdfPage.getAnnotations(); + expect(annotations.map(a => a.contentsObj?.str)).toEqual([ + "on the clone", + ]); + + await loadingTask.destroy(); + }); + + it("keeps a shared stamp image when extracting only a copied page", async function () { + if (isNodeJS) { + pending("Cannot create a bitmap from Node.js."); + } + const bitmap = await getImageBitmap("firefox_logo.png"); + + let loadingTask = getDocument(buildGetDocumentParams("empty.pdf")); + let pdfDoc = await loadingTask.promise; + + pdfDoc.pagesMapper.copyPages(new Uint32Array([1])); + pdfDoc.pagesMapper.pastePages(1); + + const stamp = { + annotationType: AnnotationEditorType.STAMP, + rect: [12, 34, 56, 78], + rotation: 0, + bitmapId: "im1", + }; + // Model stamp serialization: both entries share a bitmapId, but only + // one carries the bitmap data. + pdfDoc.annotationStorage.setValue("pdfjs_internal_editor_0", { + ...stamp, + bitmap, + pageIndex: 0, + }); + pdfDoc.annotationStorage.setValue("pdfjs_internal_editor_1", { + ...stamp, + pageIndex: 1, + isClone: true, + isCopy: true, + }); + + const { pageInfos, copyLevels } = pdfDoc.pagesMapper.extractPages( + new Set([2]) + ); + const data = await pdfDoc.extractPages(pageInfos, copyLevels); + expect(data).not.toBeNull(); + await loadingTask.destroy(); + + loadingTask = getDocument({ data }); + pdfDoc = await loadingTask.promise; + expect(pdfDoc.numPages).toEqual(1); + + const pdfPage = await pdfDoc.getPage(1); + const annotations = await pdfPage.getAnnotations(); + expect(annotations.length).toEqual(1); + expect(annotations[0].annotationType).toEqual(AnnotationType.STAMP); + + const opList = await pdfPage.getOperatorList(); + expect(opList.fnArray).toContain(OPS.paintImageXObject); await loadingTask.destroy(); }); diff --git a/web/app.js b/web/app.js index 9c033d85f..9b5667f01 100644 --- a/web/app.js +++ b/web/app.js @@ -2563,7 +2563,7 @@ const PDFViewerApplication = { this.pdfViewer.onPagesEdited(data); }, - async onSavePages({ data: extractParams }) { + async onSavePages({ data: { pageInfos, copyLevels } }) { if (typeof PDFJSDev !== "undefined" && PDFJSDev.test("TESTING")) { return; } @@ -2573,7 +2573,10 @@ const PDFViewerApplication = { if (!this.pdfDocument) { return; } - const modifiedPdfBytes = await this.pdfDocument.extractPages(extractParams); + const modifiedPdfBytes = await this.pdfDocument.extractPages( + pageInfos, + copyLevels + ); if (!modifiedPdfBytes) { console.error( "Something wrong happened when saving the edited PDF.\nPlease file a bug." @@ -2587,11 +2590,14 @@ const PDFViewerApplication = { ); }, - async onSaveAndLoad({ data: extractParams }) { + async onSaveAndLoad({ data: { pageInfos, copyLevels } }) { if (!this.pdfDocument) { return; } - const modifiedPdfBytes = await this.pdfDocument.extractPages(extractParams); + const modifiedPdfBytes = await this.pdfDocument.extractPages( + pageInfos, + copyLevels + ); if (!modifiedPdfBytes) { console.error( "Something wrong happened when saving the edited PDF.\nPlease file a bug." diff --git a/web/pdf_thumbnail_viewer.js b/web/pdf_thumbnail_viewer.js index 709039257..6c4fb410c 100644 --- a/web/pdf_thumbnail_viewer.js +++ b/web/pdf_thumbnail_viewer.js @@ -386,8 +386,8 @@ class PDFThumbnailViewer { const pagesCount = this.#pagesMapper.pagesNumber; const data = this.hasStructuralChanges() ? this.getStructuralChanges() - : [{ document: null }]; - data.push(...entries); + : { pageInfos: [{ document: null }], copyLevels: null }; + data.pageInfos.push(...entries); this.eventBus.on( "pagesloaded", () => {