Clarify documentation for copyForeignObject regarding pages

Make explicit that copyForeignObject can be used on page objects and will copy them properly but not update the pages tree.

Clarify documentation for copyForeignObject regarding pages
Make explicit that copyForeignObject can be used on page objects and will copy them properly but not update the pages tree.
Jay Berkenbilt
1 parent 2712869c
Showing 5 changed files with 32 additions and 19 deletions
TODO
include/qpdf/QPDF.hh
include/qpdf/QPDFPageDocumentHelper.hh
libqpdf/QPDF.cc
libqpdf/QPDF_pages.cc
@@ -3,12 +3,6 @@ Soon
  * Set up OSS-Fuzz (Google). See starred email in qpdf label.
- * Update the docs for copyForeignObject and addPage to explain the
-   difference between copying a page in the two ways: using addPage
-   updates the pages tree, while using copyForeignObject does not. The
-   latter is appropriate when a page is being converted to a form
-   XObject.
-
 Next ABI
 ========
@@ -25,6 +19,9 @@ Do these things next time we have to break binary compatibility
  * Collapse QPDF::pushInheritedAttributesToPageInternal* and
    QPDF::getAllPagesInternal* into QPDF::*Internal.
+ * Get rid of the version of QPDF::copyForeignObject with the bool
+   unused parameter.
+
 Lexical
 =======
@@ -301,12 +301,18 @@ class QPDF
     //
     // The return value of this method is an indirect reference to the
     // copied object in this file. This method is intended to be used
-    // to copy non-page objects and will not copy page objects. To
-    // copy page objects, pass the foreign page object directly to
-    // addPage (or addPageAt). If you copy objects that contain
-    // references to pages, you should copy the pages first using
-    // addPage(At). Otherwise references to the pages that have not
-    // been copied will be replaced with nulls.
+    // to copy non-page objects. To copy page objects, pass the
+    // foreign page object directly to addPage (or addPageAt). If you
+    // copy objects that contain references to pages, you should copy
+    // the pages first using addPage(At). Otherwise references to the
+    // pages that have not been copied will be replaced with nulls. It
+    // is possible to use copyForeignObject on page objects if you are
+    // not going to use them as pages. Doing so copies the object
+    // normally but does not update the page structure. For example,
+    // it is a valid use case to use copyForeignObject for a page that
+    // you are going to turn into a form XObject, though you can also
+    // use QPDFPageObjectHelper::getFormXObjectForPage for that
+    // purpose.
     // When copying objects with this method, object structure will be
     // preserved, so all indirectly referenced indirect objects will
@@ -930,9 +936,10 @@ class QPDF
 	QPDFObjectHandle& stream_dict, bool is_attachment_stream,
 	std::vector<PointerHolder<Pipeline> >& heap);
-    // Methods to support object copying
+    // Unused copyForeignObject -- remove at next ABI change
     QPDFObjectHandle copyForeignObject(
-        QPDFObjectHandle foreign, bool allow_page);
+        QPDFObjectHandle foreign, bool unused);
+    // Methods to support object copying
     void reserveObjects(QPDFObjectHandle foreign, ObjCopier& obj_copier,
                         bool top);
     QPDFObjectHandle replaceForeignIndirectObjects(
@@ -73,7 +73,12 @@ class QPDFPageDocumentHelper: public QPDFDocumentHelper
     // indirect. If it is an indirect object from another QPDF, this
     // method will call pushInheritedAttributesToPage on the other
     // file and then copy the page to this QPDF using the same
-    // underlying code as copyForeignObject.
+    // underlying code as copyForeignObject. Note that you can call
+    // copyForeignObject directly to copy a page from a different
+    // file, but the resulting object will not be a page in the new
+    // file. You could do this, for example, to convert a page into a
+    // form XObject, though for that, you're better off using
+    // QPDFPageObjectHelper::getFormXObjectForPage.
     QPDF_DLL
     void addPage(QPDFPageObjectHelper newpage, bool first);
@@ -2141,14 +2141,18 @@ QPDF::replaceReserved(QPDFObjectHandle reserved,
 }
 QPDFObjectHandle
-QPDF::copyForeignObject(QPDFObjectHandle foreign)
+QPDF::copyForeignObject(QPDFObjectHandle foreign, bool)
 {
-    return copyForeignObject(foreign, false);
+    // This method will be removed next time the ABI is changed.
+    return copyForeignObject(foreign);
 }
 QPDFObjectHandle
-QPDF::copyForeignObject(QPDFObjectHandle foreign, bool allow_page)
+QPDF::copyForeignObject(QPDFObjectHandle foreign)
 {
+    // Do not preclude use of copyForeignObject on page objects. It is
+    // a documented use case to copy pages this way if the intention
+    // is to not update the pages tree.
     if (! foreign.isIndirect())
     {
         QTC::TC("qpdf", "QPDF copyForeign direct");
@@ -221,7 +221,7 @@ QPDF::insertPage(QPDFObjectHandle newpage, int pos)
     {
         QTC::TC("qpdf", "QPDF insert foreign page");
         newpage.getOwningQPDF()->pushInheritedAttributesToPage();
-        newpage = copyForeignObject(newpage, true);
+        newpage = copyForeignObject(newpage);
     }
     else
     {