Merge pull request #1538 from m-holger/afdh5

Refactor QPDFAcroFormDocumentHelper::transformAnnotations

Merge pull request #1538 from m-holger/afdh5
Refactor QPDFAcroFormDocumentHelper::transformAnnotations
m-holger · GitHub
2 parents f4589458 8803fede
Showing 2 changed files with 154 additions and 156 deletions
libqpdf/QPDFAcroFormDocumentHelper.cc
qpdf/qpdf.testcov
@@ -8,6 +8,7 @@
 #include <qpdf/QUtil.hh>
 #include <qpdf/ResourceFinder.hh>
+#include <deque>
 #include <utility>
 using namespace qpdf;
@@ -480,6 +481,9 @@ QPDFAcroFormDocumentHelper::adjustInheritedFields(
     bool override_q,
     int from_default_q)
 {
+    if (!(override_da || override_q)) {
+        return;
+    }
     // Override /Q or /DA if needed. If this object has a field type, directly or inherited, it is a
     // field and not just an annotation. In that case, we need to override if we are getting a value
     // from the document that is different from the value we would have gotten from the old
@@ -719,7 +723,7 @@ QPDFAcroFormDocumentHelper::adjustAppearanceStream(
 void
 QPDFAcroFormDocumentHelper::transformAnnotations(
-    QPDFObjectHandle old_annots,
+    QPDFObjectHandle a_old_annots,
     std::vector<QPDFObjectHandle>& new_annots,
     std::vector<QPDFObjectHandle>& new_fields,
     std::set<QPDFObjGen>& old_fields,
@@ -727,94 +731,87 @@ QPDFAcroFormDocumentHelper::transformAnnotations(
     QPDF* from_qpdf,
     QPDFAcroFormDocumentHelper* from_afdh)
 {
-    std::shared_ptr<QPDFAcroFormDocumentHelper> afdhph;
+    Array old_annots = std::move(a_old_annots);
     if (!from_qpdf) {
         // Assume these are from the same QPDF.
         from_qpdf = &qpdf;
         from_afdh = this;
-    } else if ((from_qpdf != &qpdf) && (!from_afdh)) {
-        afdhph = std::make_shared<QPDFAcroFormDocumentHelper>(*from_qpdf);
-        from_afdh = afdhph.get();
+    } else if (from_qpdf != &qpdf && !from_afdh) {
+        from_afdh = &QPDFAcroFormDocumentHelper::get(*from_qpdf);
     }
-    bool foreign = (from_qpdf != &qpdf);
+    const bool foreign = from_qpdf != &qpdf;
     // It's possible that we will transform annotations that don't include any form fields. This
     // code takes care not to muck around with /AcroForm unless we have to.
-    QPDFObjectHandle acroform = qpdf.getRoot().getKey("/AcroForm");
-    QPDFObjectHandle from_acroform = from_qpdf->getRoot().getKey("/AcroForm");
+    Dictionary acroform = qpdf.getRoot()["/AcroForm"];
+    Dictionary from_acroform = from_qpdf->getRoot()["/AcroForm"];
     // /DA and /Q may be inherited from the document-level /AcroForm dictionary. If we are copying a
     // foreign stream and the stream is getting one of these values from its document's /AcroForm,
     // we will need to copy the value explicitly so that it doesn't start getting its default from
     // the destination document.
-    bool override_da = false;
-    bool override_q = false;
     std::string from_default_da;
     int from_default_q = 0;
     // If we copy any form fields, we will need to merge the source document's /DR into this
     // document's /DR.
-    QPDFObjectHandle from_dr = QPDFObjectHandle::newNull();
+    Dictionary from_dr;
+    std::string default_da;
+    int default_q = 0;
     if (foreign) {
-        std::string default_da;
-        int default_q = 0;
-        if (acroform.isDictionary()) {
-            if (acroform.getKey("/DA").isString()) {
-                default_da = acroform.getKey("/DA").getUTF8Value();
+        if (acroform) {
+            if (acroform["/DA"].isString()) {
+                default_da = acroform["/DA"].getUTF8Value();
             }
-            if (acroform.getKey("/Q").isInteger()) {
-                default_q = acroform.getKey("/Q").getIntValueAsInt();
+            if (Integer Q = acroform["/Q"]) {
+                default_q = Q;
             }
         }
-        if (from_acroform.isDictionary()) {
-            if (from_acroform.getKey("/DR").isDictionary()) {
-                from_dr = from_acroform.getKey("/DR");
-                if (!from_dr.isIndirect()) {
+        if (from_acroform) {
+            from_dr = from_acroform["/DR"];
+            if (from_dr) {
+                if (!from_dr.indirect()) {
                     from_dr = from_qpdf->makeIndirectObject(from_dr);
                 }
                 from_dr = qpdf.copyForeignObject(from_dr);
             }
-            if (from_acroform.getKey("/DA").isString()) {
-                from_default_da = from_acroform.getKey("/DA").getUTF8Value();
+            if (from_acroform["/DA"].isString()) {
+                from_default_da = from_acroform["/DA"].getUTF8Value();
             }
-            if (from_acroform.getKey("/Q").isInteger()) {
-                from_default_q = from_acroform.getKey("/Q").getIntValueAsInt();
+            if (Integer Q = from_acroform["/Q"]) {
+                from_default_q = Q;
             }
         }
-        if (from_default_da != default_da) {
-            override_da = true;
-        }
-        if (from_default_q != default_q) {
-            override_q = true;
-        }
     }
+    const bool override_da = from_acroform ? from_default_da != default_da : false;
+    const bool override_q = from_acroform ? from_default_q != default_q : false;
     // If we have to merge /DR, we will need a mapping of conflicting keys for rewriting /DA. Set
     // this up for lazy initialization in case we encounter any form fields.
     std::map<std::string, std::map<std::string, std::string>> dr_map;
-    bool initialized_dr_map = false;
-    QPDFObjectHandle dr = QPDFObjectHandle::newNull();
+    Dictionary dr;
+
     auto init_dr_map = [&]() {
-        if (!initialized_dr_map) {
-            initialized_dr_map = true;
+        if (!dr) {
             // Ensure that we have a /DR that is an indirect
             // dictionary object.
-            if (!acroform.isDictionary()) {
+            if (!acroform) {
                 acroform = getOrCreateAcroForm();
             }
-            dr = acroform.getKey("/DR");
-            if (!dr.isDictionary()) {
-                dr = QPDFObjectHandle::newDictionary();
+            dr = acroform["/DR"];
+            if (!dr) {
+                dr = Dictionary::empty();
             }
-            dr.makeResourcesIndirect(qpdf);
-            if (!dr.isIndirect()) {
-                dr = acroform.replaceKeyAndGetNew("/DR", qpdf.makeIndirectObject(dr));
+            QPDFObjectHandle(dr).makeResourcesIndirect(qpdf);
+            if (!dr.indirect()) {
+                acroform.replaceKey("/DR", qpdf.makeIndirectObject(dr));
+                dr = acroform["/DR"];
             }
             // Merge the other document's /DR, creating a conflict map. mergeResources checks to
             // make sure both objects are dictionaries. By this point, if this is foreign, from_dr
             // has been copied, so we use the target qpdf as the owning qpdf.
-            from_dr.makeResourcesIndirect(qpdf);
-            dr.mergeResources(from_dr, &dr_map);
+            QPDFObjectHandle(from_dr).makeResourcesIndirect(qpdf);
+            QPDFObjectHandle(dr).mergeResources(from_dr, &dr_map);
             if (from_afdh->getNeedAppearances()) {
                 setNeedAppearances(true);
@@ -836,15 +833,59 @@ QPDFAcroFormDocumentHelper::transformAnnotations(
         }
     };
-    // Now do the actual copies.
-
-    QPDFObjGen::set added_new_fields;
-    for (auto annot: old_annots.aitems()) {
-        if (annot.isStream()) {
-            annot.warn("ignoring annotation that's a stream");
-            continue;
+    // Traverse the field, copying kids, and preserving integrity.
+    auto traverse_field = [&](QPDFObjectHandle& top_field) -> void {
+        std::deque<Dictionary> queue({top_field});
+        QPDFObjGen::set seen;
+        for (auto it = queue.begin(); it != queue.end(); ++it) {
+            auto& obj = *it;
+            if (seen.add(obj)) {
+                Dictionary parent = obj["/Parent"];
+                if (parent.indirect()) {
+                    auto parent_og = parent.id_gen();
+                    if (orig_to_copy.contains(parent_og)) {
+                        obj.replaceKey("/Parent", orig_to_copy[parent_og]);
+                    } else {
+                        parent.warn(
+                            "while traversing field " + obj.id_gen().unparse(',') +
+                            ", found parent (" + parent_og.unparse(',') +
+                            ") that had not been seen, indicating likely invalid field structure");
+                    }
+                }
+                size_t i = 0;
+                Array Kids = obj["/Kids"];
+                for (auto& kid: Kids) {
+                    if (maybe_copy_object(kid)) {
+                        Kids.set(i, kid);
+                        queue.emplace_back(kid);
+                    }
+                    ++i;
+                }
+                adjustInheritedFields(
+                    obj, override_da, from_default_da, override_q, from_default_q);
+                if (foreign) {
+                    // Lazily initialize our /DR and the conflict map.
+                    init_dr_map();
+                    // The spec doesn't say anything about /DR on the field, but lots of writers
+                    // put one there, and it is frequently the same as the document-level /DR.
+                    // To avoid having the field's /DR point to information that we are not
+                    // maintaining, just reset it to that if it exists. Empirical evidence
+                    // suggests that many readers, including Acrobat, Adobe Acrobat Reader,
+                    // chrome, firefox, the mac Preview application, and several of the free
+                    // readers on Linux all ignore /DR at the field level.
+                    if (obj.contains("/DR")) {
+                        obj.replaceKey("/DR", dr);
+                    }
+                    if (obj["/DA"].isString() && !dr_map.empty()) {
+                        adjustDefaultAppearances(obj, dr_map);
+                    }
+                }
+            }
         }
+    };
+    auto transform_annotation =
+        [&](QPDFObjectHandle& annot) -> std::tuple<QPDFObjectHandle, bool, bool> {
         // Make copies of annotations and fields down to the appearance streams, preserving all
         // internal referential integrity. When the incoming annotations are from a different file,
         // we first copy them locally. Then, whether local or foreign, we copy them again so that if
@@ -870,98 +911,57 @@ QPDFAcroFormDocumentHelper::transformAnnotations(
         auto ffield = from_afdh->getFieldForAnnotation(annot);
         auto ffield_oh = ffield.getObjectHandle();
-        QPDFObjectHandle top_field;
-        bool have_field = false;
-        bool have_parent = false;
+        if (ffield.null()) {
+            return {{}, false, false};
+        }
         if (ffield_oh.isStream()) {
             ffield.warn("ignoring form field that's a stream");
-        } else if (!ffield_oh.null() && !ffield_oh.isIndirect()) {
+            return {{}, false, false};
+        }
+        if (!ffield_oh.isIndirect()) {
             ffield.warn("ignoring form field not indirect");
-        } else if (!ffield.null()) {
-            // A field and its associated annotation can be the same object. This matters because we
-            // don't want to clone the annotation and field separately in this case.
-            have_field = true;
-            // Find the top-level field. It may be the field itself.
-            top_field = ffield.getTopLevelField(&have_parent).getObjectHandle();
-            if (foreign) {
-                // copyForeignObject returns the same value if called multiple times with the same
-                // field. Create/retrieve the local copy of the original field. This pulls over
-                // everything the field references including annotations and appearance streams, but
-                // it's harmless to call copyForeignObject on them too. They will already be copied,
-                // so we'll get the right object back.
-
-                // top_field and ffield_oh are known to be indirect.
-                top_field = qpdf.copyForeignObject(top_field);
-                ffield_oh = qpdf.copyForeignObject(ffield_oh);
-            } else {
-                // We don't need to add top_field to old_fields if it's foreign because the new copy
-                // of the foreign field won't be referenced anywhere. It's just the starting point
-                // for us to make an additional local copy of.
-                old_fields.insert(top_field.getObjGen());
-            }
+            return {{}, false, false};
+        }
+        // A field and its associated annotation can be the same object. This matters because we
+        // don't want to clone the annotation and field separately in this case.
+        // Find the top-level field. It may be the field itself.
+        bool have_parent = false;
+        QPDFObjectHandle top_field = ffield.getTopLevelField(&have_parent).getObjectHandle();
+        if (foreign) {
+            // copyForeignObject returns the same value if called multiple times with the same
+            // field. Create/retrieve the local copy of the original field. This pulls over
+            // everything the field references including annotations and appearance streams, but
+            // it's harmless to call copyForeignObject on them too. They will already be copied,
+            // so we'll get the right object back.
+
+            // top_field and ffield_oh are known to be indirect.
+            top_field = qpdf.copyForeignObject(top_field);
+            ffield_oh = qpdf.copyForeignObject(ffield_oh);
+        } else {
+            // We don't need to add top_field to old_fields if it's foreign because the new copy
+            // of the foreign field won't be referenced anywhere. It's just the starting point
+            // for us to make an additional local copy of.
+            old_fields.insert(top_field.getObjGen());
+        }
-            // Traverse the field, copying kids, and preserving integrity.
-            std::list<QPDFObjectHandle> queue;
-            QPDFObjGen::set seen;
-            if (maybe_copy_object(top_field)) {
-                queue.push_back(top_field);
-            }
-            for (; !queue.empty(); queue.pop_front()) {
-                auto& obj = queue.front();
-                if (seen.add(obj)) {
-                    auto parent = obj.getKey("/Parent");
-                    if (parent.isIndirect()) {
-                        auto parent_og = parent.getObjGen();
-                        if (orig_to_copy.contains(parent_og)) {
-                            obj.replaceKey("/Parent", orig_to_copy[parent_og]);
-                        } else {
-                            parent.warn(
-                                "while traversing field " + obj.getObjGen().unparse(',') +
-                                ", found parent (" + parent_og.unparse(',') +
-                                ") that had not been seen, indicating likely invalid field "
-                                "structure");
-                        }
-                    }
-                    auto kids = obj.getKey("/Kids");
-                    int sz = static_cast<int>(kids.size());
-                    if (sz != 1 || kids.isArray()) {
-                        for (int i = 0; i < sz; ++i) {
-                            auto kid = kids.getArrayItem(i);
-                            if (maybe_copy_object(kid)) {
-                                kids.setArrayItem(i, kid);
-                                queue.emplace_back(kid);
-                            }
-                        }
-                    }
+        if (maybe_copy_object(top_field)) {
+            traverse_field(top_field);
+        }
-                    if (override_da || override_q) {
-                        adjustInheritedFields(
-                            obj, override_da, from_default_da, override_q, from_default_q);
-                    }
-                    if (foreign) {
-                        // Lazily initialize our /DR and the conflict map.
-                        init_dr_map();
-                        // The spec doesn't say anything about /DR on the field, but lots of writers
-                        // put one there, and it is frequently the same as the document-level /DR.
-                        // To avoid having the field's /DR point to information that we are not
-                        // maintaining, just reset it to that if it exists. Empirical evidence
-                        // suggests that many readers, including Acrobat, Adobe Acrobat Reader,
-                        // chrome, firefox, the mac Preview application, and several of the free
-                        // readers on Linux all ignore /DR at the field level.
-                        if (obj.hasKey("/DR")) {
-                            obj.replaceKey("/DR", dr);
-                        }
-                    }
-                    if (foreign && obj.getKey("/DA").isString() && (!dr_map.empty())) {
-                        adjustDefaultAppearances(obj, dr_map);
-                    }
-                }
-            }
+        // Now switch to copies. We already switched for top_field
+        maybe_copy_object(ffield_oh);
+        return {top_field, true, have_parent};
+    };
+
+    // Now do the actual copies.
-            // Now switch to copies. We already switched for top_field
-            maybe_copy_object(ffield_oh);
-            ffield = QPDFFormFieldObjectHelper(ffield_oh);
+    QPDFObjGen::set added_new_fields;
+    for (auto annot: old_annots) {
+        if (annot.isStream()) {
+            annot.warn("ignoring annotation that's a stream");
+            continue;
         }
+        auto [top_field, have_field, have_parent] = transform_annotation(annot);
         QTC::TC(
             "qpdf",
@@ -983,24 +983,25 @@ QPDFAcroFormDocumentHelper::transformAnnotations(
         // Now we have copies, so we can safely mutate.
         if (have_field && added_new_fields.add(top_field)) {
-            new_fields.push_back(top_field);
+            new_fields.emplace_back(top_field);
         }
-        new_annots.push_back(annot);
+        new_annots.emplace_back(annot);
         // Identify and copy any appearance streams
         auto ah = QPDFAnnotationObjectHelper(annot);
-        auto apdict = ah.getAppearanceDictionary();
+        Dictionary apdict = ah.getAppearanceDictionary();
         std::vector<QPDFObjectHandle> streams;
         auto replace_stream = [](auto& dict, auto& key, auto& old) {
-            return dict.replaceKeyAndGetNew(key, old.copyStream());
+            dict.replaceKey(key, old.copyStream());
+            return dict[key];
         };
-        for (auto& [key1, value1]: apdict.as_dictionary()) {
+        for (auto& [key1, value1]: apdict) {
             if (value1.isStream()) {
                 streams.emplace_back(replace_stream(apdict, key1, value1));
             } else {
-                for (auto& [key2, value2]: value1.as_dictionary()) {
+                for (auto& [key2, value2]: Dictionary(value1)) {
                     if (value2.isStream()) {
                         streams.emplace_back(replace_stream(value1, key2, value2));
                     }
@@ -1010,25 +1011,23 @@ QPDFAcroFormDocumentHelper::transformAnnotations(
         // Now we can safely mutate the annotation and its appearance streams.
         for (auto& stream: streams) {
-            auto dict = stream.getDict();
-            auto omatrix = dict.getKey("/Matrix");
+            Dictionary dict = stream.getDict();
+
             QPDFMatrix apcm;
-            if (omatrix.isArray()) {
-                QTC::TC("qpdf", "QPDFAcroFormDocumentHelper modify ap matrix");
-                auto m1 = omatrix.getArrayAsMatrix();
-                apcm = QPDFMatrix(m1);
+            Array omatrix = dict["/Matrix"];
+            if (omatrix) {
+                apcm = QPDFMatrix(QPDFObjectHandle(omatrix).getArrayAsMatrix());
             }
             apcm.concat(cm);
-            auto new_matrix = QPDFObjectHandle::newFromMatrix(apcm);
-            if (omatrix.isArray() || (apcm != QPDFMatrix())) {
-                dict.replaceKey("/Matrix", new_matrix);
+            if (omatrix || apcm != QPDFMatrix()) {
+                dict.replaceKey("/Matrix", QPDFObjectHandle::newFromMatrix(apcm));
             }
-            auto resources = dict.getKey("/Resources");
-            if ((!dr_map.empty()) && resources.isDictionary()) {
+            Dictionary resources = dict["/Resources"];
+            if (!dr_map.empty() && resources) {
                 adjustAppearanceStream(stream, dr_map);
             }
         }
-        auto rect = cm.transformRectangle(annot.getKey("/Rect").getArrayAsRectangle());
+        auto rect = cm.transformRectangle(annot["/Rect"].getArrayAsRectangle());
         annot.replaceKey("/Rect", QPDFObjectHandle::newFromRectangle(rect));
     }
 }
@@ -487,7 +487,6 @@ QPDFFileSpecObjectHelper empty compat_name 0
 QPDFFileSpecObjectHelper non-empty compat_name 0
 QPDFAcroFormDocumentHelper copy annotation 3
 QPDFAcroFormDocumentHelper field with parent 3
-QPDFAcroFormDocumentHelper modify ap matrix 0
 QPDFJob pages keeping field from original 0
 QPDFObjectHandle merge reuse 0
 QPDFObjectHandle merge generate 0