Skip to content

Commit

Permalink
close #429: Form field missing from merged result if annotation widge…
Browse files Browse the repository at this point in the history
…t has invalid /P entry
  • Loading branch information
ediweissmann committed Dec 25, 2023
1 parent 2f41684 commit 84f004c
Show file tree
Hide file tree
Showing 4 changed files with 55 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,7 @@
import org.sejda.sambox.cos.COSName;
import org.sejda.sambox.pdmodel.PDDocument;
import org.sejda.sambox.pdmodel.PDPage;
import org.sejda.sambox.pdmodel.interactive.annotation.PDAnnotation;
import org.sejda.sambox.pdmodel.interactive.annotation.PDAnnotationLink;
import org.sejda.sambox.pdmodel.interactive.annotation.PDAnnotationMarkup;
import org.sejda.sambox.pdmodel.interactive.annotation.PDAnnotationPopup;
import org.sejda.sambox.pdmodel.interactive.annotation.*;
import org.sejda.sambox.pdmodel.interactive.documentnavigation.destination.PDDestination;
import org.sejda.sambox.pdmodel.interactive.documentnavigation.destination.PDPageDestination;
import org.slf4j.Logger;
Expand Down Expand Up @@ -80,6 +77,16 @@ public LookupTable<PDAnnotation> retainRelevantAnnotations(LookupTable<PDPage> r
if (nonNull(mapped)) {
keptAnnotations.add(mapped);
} else {
// handle the scenario where the annotation page != actual render page
// TODO: for safety, applying this only to form widgets for now

This comment has been minimized.

Copy link
@ediweissmann

ediweissmann Dec 25, 2023

Author Collaborator

I think this should be applied to all annotations, not just widgets (form fields), but there are some test failures in AnnotationsDistillerTest related to non-link annotations.

This comment has been minimized.

Copy link
@torakiki

torakiki Dec 26, 2023

Owner

I think you are right and we should handle/sanitize all types of annotations. From what I understand in the spec, the /P value is relevant only in the context of Screen annotation (Subtype Screen) and is optional/irrelevant for all the other types, so why don't we set the page to null for all types except Screen annotations and we make that consistent only for Screen annotations?

if(annotation instanceof PDAnnotationWidget) {
PDPage annotationPage = annotation.getPage();
if (annotationPage != null && !annotationPage.equals(page)) {
// inconsistent annotation, this creates problems
annotation.setPage(page);
}
}

if (annotation instanceof PDAnnotationLink) {
processLinkAnnotation(relevantPages, keptAnnotations, (PDAnnotationLink) annotation);
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1030,6 +1030,24 @@ public void atRestEncryptionTest() throws IOException {
testContext.assertPages(13);
}

@Test
public void missingFormFields() throws IOException {
MergeParameters parameters = new MergeParameters();

parameters.setExistingOutputPolicy(ExistingOutputPolicy.OVERWRITE);
parameters.setAcroFormPolicy(AcroFormPolicy.MERGE_RENAMING_EXISTING_FIELDS);

parameters.addInput(new PdfMergeInput(customInput("pdf/sample_form_invalid_widget_page.pdf")));
parameters.addInput(new PdfMergeInput(regularInput()));

testContext.pdfOutputTo(parameters);
execute(parameters);

testContext.assertTaskCompleted();
testContext.assertNoTaskWarnings();
testContext.forEachPdfOutput(doc -> assertEquals(1, doc.getPage(0).getAnnotations().size()));
}

@Test
public void withLargeTocItemsThatWrapAndGenerateMultipleTocPages() throws IOException {
List<String> entries = new ArrayList<>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,16 +30,13 @@
import org.sejda.sambox.pdmodel.PDPage;
import org.sejda.sambox.pdmodel.interactive.action.PDActionGoTo;
import org.sejda.sambox.pdmodel.interactive.action.PDActionJavaScript;
import org.sejda.sambox.pdmodel.interactive.annotation.PDAnnotation;
import org.sejda.sambox.pdmodel.interactive.annotation.PDAnnotationLine;
import org.sejda.sambox.pdmodel.interactive.annotation.PDAnnotationLink;
import org.sejda.sambox.pdmodel.interactive.annotation.PDAnnotationMarkup;
import org.sejda.sambox.pdmodel.interactive.annotation.PDAnnotationPopup;
import org.sejda.sambox.pdmodel.interactive.annotation.PDAnnotationSquareCircle;
import org.sejda.sambox.pdmodel.interactive.annotation.PDAnnotationText;
import org.sejda.sambox.pdmodel.interactive.annotation.*;
import org.sejda.sambox.pdmodel.interactive.documentnavigation.destination.PDNamedDestination;
import org.sejda.sambox.pdmodel.interactive.documentnavigation.destination.PDPageDestination;
import org.sejda.sambox.pdmodel.interactive.documentnavigation.destination.PDPageFitDestination;
import org.sejda.sambox.pdmodel.interactive.form.PDAcroForm;
import org.sejda.sambox.pdmodel.interactive.form.PDField;
import org.sejda.sambox.pdmodel.interactive.form.PDTextField;

import java.io.IOException;
import java.util.Arrays;
Expand Down Expand Up @@ -138,6 +135,8 @@ public void noLinks_PageNotRelevant() {
PDDocument doc = new PDDocument();
doc.addPage(oldPage);
LookupTable<PDAnnotation> annotationsLookup = new AnnotationsDistiller(doc).retainRelevantAnnotations(lookup);

// TODO: review this test; the annotation should be present in the merged result, even if /P is inconsistent

This comment has been minimized.

Copy link
@ediweissmann

ediweissmann Dec 25, 2023

Author Collaborator

This test looks fishy, why would we want to get rid of the annotation in this scenario? Same for the other test below

assertEquals(0, newPage.getAnnotations().size());
assertTrue(annotationsLookup.isEmpty());
}
Expand All @@ -154,6 +153,8 @@ public void noLinks_OnePageNotRelevantOneRelevant() {
PDDocument doc = new PDDocument();
doc.addPage(oldPage);
LookupTable<PDAnnotation> annotationsLookup = new AnnotationsDistiller(doc).retainRelevantAnnotations(lookup);

// TODO: review this test; the fist annotation should be present in the merged result, even if /P is inconsistent
assertEquals(annotationsLookup.lookup(annotation2), newPage.getAnnotations().get(0));
}

Expand Down Expand Up @@ -398,4 +399,22 @@ public void pagesHaveTheSameAnnotations() {
assertEquals(oldPage.getAnnotations().size(), newPage.getAnnotations().size());
assertEquals(secondOld.getAnnotations().size(), secondNew.getAnnotations().size());
}

@Test
public void dontRemoveFormFieldsWhenMergingIfWidgetPageDifferentThanActualDisplayPage() {
PDDocument doc = new PDDocument();
PDAcroForm form = new PDAcroForm(doc);
PDField field = new PDTextField(form);
field.setPartialName("first_name");
PDAnnotationWidget annotation = new PDAnnotationWidget(field.getCOSObject());
// widget page is different than the actual parent page (where the widget is displayed)
annotation.setPage(new PDPage());
List<PDAnnotation> annotations = List.of(annotation);
oldPage.setAnnotations(annotations);

doc.addPage(oldPage);
LookupTable<PDAnnotation> annotationsLookup = new AnnotationsDistiller(doc).retainRelevantAnnotations(lookup);
assertFalse(annotationsLookup.isEmpty());
assertEquals(1, newPage.getAnnotations().size());
}
}
Binary file not shown.

0 comments on commit 84f004c

Please sign in to comment.