From 2d0885bc119af035ab2df4d8c19000408223ae7f Mon Sep 17 00:00:00 2001
From: Jay Berkenbilt <ejb@ql.org>
Date: Mon, 28 Jan 2019 21:53:55 -0500
Subject: [PATCH] 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.
---
 TODO                                   |  9 +++------
 include/qpdf/QPDF.hh                   | 23 +++++++++++++++--------
 include/qpdf/QPDFPageDocumentHelper.hh |  7 ++++++-
 libqpdf/QPDF.cc                        | 10 +++++++---
 libqpdf/QPDF_pages.cc                  |  2 +-
 5 files changed, 32 insertions(+), 19 deletions(-)

diff --git a/TODO b/TODO
index 42a348ed..a4554456 100644
--- a/TODO
+++ b/TODO
@@ -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
 =======
 
diff --git a/include/qpdf/QPDF.hh b/include/qpdf/QPDF.hh
index cfb10aac..329c2756 100644
--- a/include/qpdf/QPDF.hh
+++ b/include/qpdf/QPDF.hh
@@ -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(
diff --git a/include/qpdf/QPDFPageDocumentHelper.hh b/include/qpdf/QPDFPageDocumentHelper.hh
index b49d9652..096a401c 100644
--- a/include/qpdf/QPDFPageDocumentHelper.hh
+++ b/include/qpdf/QPDFPageDocumentHelper.hh
@@ -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);
 
diff --git a/libqpdf/QPDF.cc b/libqpdf/QPDF.cc
index 0dbce669..f49c0216 100644
--- a/libqpdf/QPDF.cc
+++ b/libqpdf/QPDF.cc
@@ -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");
diff --git a/libqpdf/QPDF_pages.cc b/libqpdf/QPDF_pages.cc
index 7b6b369e..397c175a 100644
--- a/libqpdf/QPDF_pages.cc
+++ b/libqpdf/QPDF_pages.cc
@@ -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
     {