[prev in list] [next in list] [prev in thread] [next in thread] 

List:       freedesktop-poppler
Subject:    Re: [poppler] [PATCHES] New annotation features, removal of objects from xref and arrays
From:       Fabio D'Urso <fabiodurso () hotmail ! it>
Date:       2012-05-20 17:04:45
Message-ID: BLU0-SMTP1201D5DCD72FD51C7463CA7AF1C0 () phx ! gbl
[Download RAW message or body]

On Sunday, May 20, 2012 12:27:52 PM Carlos Garcia Campos wrote:
> Why doesn't Page::removeAnnot() also remove the appearances? I've
> noticed that the qt4 implementation removes the popup and appearances
> manually. Appearance streams are only removed if there aren't any
> other annots referencing them, but is it possible to share the same
> popup by several markup annotations?
No, popups can't be shared.

> What happens with all other objects referenced by annotations? I guess they
> are all left in the document, should we remove them too? like the
> appearances?
Correct. As I told you in IRC, I didn't implement code to remove other 
objects, because removing the appearance is enough for the annotation types 
that can be modified via qt4.
Note that appearance stream removal itself is not finished yet. Currently, it 
leaks unreferenced objects in the xref and invalid refs in the AP name tree 
(see TODO comment in AnnotAppearance::removeStream).

> Maybe we could add a Annot::removeAnnotFromPage (or something like
> that) so that every annotation can implement it to remove the objects
> it references and not referenced by others.
As we discussed on chat, I like the idea of adding a virtual method to let 
each annotation type "cleanup". It will be useful when we add removal support 
for annotation types that reference indirect objects (sounds, attachments...).
However, I don't like the Page* argument passed to that method. I'm posting 
the diff here too.

I have some doubts about setting /P if the annotation has no /P or a wrong 
value, because that would mean that annotations are automatically modified as 
soon as the document is loaded. If we decide to go that way, we'll also need 
to be careful not to touch the last-changed timestamp.

What about storing the Page pointer itself in a dedicated field, regardless of 
the /P value?

Fabio

["removeLinkedObjects.diff" (text/x-patch)]

diff --git a/poppler/Annot.cc b/poppler/Annot.cc
index 8e1e760..ad01a27 100644
--- a/poppler/Annot.cc
+++ b/poppler/Annot.cc
@@ -1484,6 +1484,11 @@ void Annot::readArrayNum(Object *pdfArray, int key, double *value) {
   valueObject.free();
 }
 
+void Annot::removeLinkedObjects(Page *) {
+  // Remove appearance streams (if any)
+  invalidateAppearance();
+}
+
 void Annot::incRefCnt() {
   refCnt++;
 }
@@ -1934,6 +1939,16 @@ void AnnotMarkup::setDate(GooString *new_date) {
   update ("CreationDate", &obj1);
 }
 
+void AnnotMarkup::removeLinkedObjects(Page *page) { // <-- I don't like this Page pointer as argument
+  // Remove popup
+  if (popup) {
+    // NOTE: We need the Page here. (We can't rely on this->page, because the /P property is optional)
+    page->removeAnnot(popup);
+  }
+
+  Annot::removeLinkedObjects(page);
+}
+
 //------------------------------------------------------------------------
 // AnnotText
 //------------------------------------------------------------------------
diff --git a/poppler/Annot.h b/poppler/Annot.h
index 04a1301..fbe087c 100644
--- a/poppler/Annot.h
+++ b/poppler/Annot.h
@@ -48,6 +48,7 @@ class Form;
 class FormWidget;
 class FormField;
 class FormFieldChoice;
+class Page;
 class PDFRectangle;
 class Movie;
 class LinkAction;
@@ -471,6 +472,7 @@ private:
 //------------------------------------------------------------------------
 
 class Annot {
+  friend class Page;
 public:
   enum AnnotFlag {
     flagUnknown        = 0x0000,
@@ -591,6 +593,7 @@ private:
 
 protected:
   virtual ~Annot();
+  virtual void removeLinkedObjects(Page *page); // Called by Page::removeAnnot after removal
   void setColor(AnnotColor *color, GBool fill);
   void drawCircle(double cx, double cy, double r, GBool fill);
   void drawCircleTopLeft(double cx, double cy, double r);
@@ -699,6 +702,8 @@ public:
   void setDate(GooString *new_date);
 
 protected:
+  virtual void removeLinkedObjects(Page *page);
+
   GooString *label;             // T            (Default autor)
   AnnotPopup *popup;            // Popup
   double opacity;               // CA           (Default 1.0)
diff --git a/poppler/Page.cc b/poppler/Page.cc
index eccc198..728ff5d 100644
--- a/poppler/Page.cc
+++ b/poppler/Page.cc
@@ -428,6 +428,7 @@ void Page::removeAnnot(Annot *annot) {
     }
   }
   annArray.free();
+  annot->removeLinkedObjects(this); // Note: Might recurse in removeAnnot again
 }
 
 Links *Page::getLinks() {
diff --git a/poppler/Page.h b/poppler/Page.h
index e2e666c..83b6a6d 100644
--- a/poppler/Page.h
+++ b/poppler/Page.h
@@ -173,7 +173,6 @@ public:
   // Add a new annotation to the page
   void addAnnot(Annot *annot);
   // Remove an existing annotation from the page
-  // Note: Caller is responsible for deleting popup and appearance streams too
   void removeAnnot(Annot *annot);
 
   // Return a list of links.
diff --git a/qt4/src/poppler-annotation.cc b/qt4/src/poppler-annotation.cc
index 4d7cd84..59db52d 100644
--- a/qt4/src/poppler-annotation.cc
+++ b/qt4/src/poppler-annotation.cc
@@ -510,14 +510,6 @@ void AnnotationPrivate::removeAnnotationFromPage(::Page *pdfPage, const Annotati
         return;
     }
 
-    // Remove popup window
-    AnnotMarkup *markupann = dynamic_cast<AnnotMarkup*>(ann->d_ptr->pdfAnnot);
-    if (markupann && markupann->getPopup())
-        pdfPage->removeAnnot(markupann->getPopup());
-
-    // Remove appearance streams (if any)
-    ann->d_ptr->pdfAnnot->invalidateAppearance();
-
     // Remove annotation
     pdfPage->removeAnnot(ann->d_ptr->pdfAnnot);
 


_______________________________________________
poppler mailing list
poppler@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/poppler


[prev in list] [next in list] [prev in thread] [next in thread]