On Tuesday, September 18, 2012 07:52:23 PM Albert Astals Cid wrote: > El Dimarts, 18 de setembre de 2012, a les 07:49:05, vau escriure: > > Hi Albert, Hi Fabio, > > > > the solution from Fabio worked quite well to me. > > There are still some PDFs remaining which do not have their application > > stream changed - at least it looks like that. > > > > But for most of the files I tried it works fine. > > Hence, a big "thank you" to Fabio and I would recommend to apply that > > patch to an upcoming release. > > Spoke with Fabio in private and he seems not to be happy with it, he'll be > working in a better one once he gets a bit of time.
Here we go. I've rewritten the patches in a cleaner way and now I'm satisfied with them. Short recap: After filling and saving a PDF form, filled text fields are still shown with their old value (usually blank) in acroread (and in poppler too, when you open the resulting PDF file) because we don't update the annotation widgets' appearance streams. With these patches, the appearance stream gets rewritten for text and choice fields (for button fields it's not necessary). Note that choice fields still don't work in acroread, because of some unrelated issues that I'll discuss in a new thread. Patches in detail: 0001 Patch 0001 adds a AnnotWidget::updateAppearanceStream() method that is called by form handling code to inform the widget annotation that its appearance stream is no longer valid and needs to be rebuilt. This information was previously obtained by the widget annotation by testing field->isModified() before drawing. This new "push" approach has two advantages: 1) The appearance is now rebuilt only once after the contents are modified. Previously, field->isModified() stayed always true, even after the first draw() call, causing the apperance stream to be rebuilt on every subsequent draw() call too, even if contents stayed the same. 2) It gives the possibility to react to modifications immediatly, without having to wait for the next rendering. 0002 This patch creates the new appearance stream in AnnotWidget::updateAppearanceStream() and writes it in the xref. This patch fixes the original bug, but... Note: ...every time the field is modified (eg in okular it happens every time the the user adds or removes a character) the old apprearance stream is deleted and a new one is generated. This means that the xref entry for the old appearance is freed and a new one is allocated. Since every time a xref entry is freed its generation number gets incremented, repeated edits result in generation numbers rapidily growing. The next patch solves this issue. 0003 This patch avoids going through xref entry deallocation+allocation on every modification. Now it only happens the first time, and references to the newly allocated xref entry are kept, so that subsequent updateAppearanceStream invocations can do a simple overwrite instead. These patches are supposed to fix exactly the same documents that the previous proof-of-concept patch fixed. Please let me know if you find any difference. Thank you, Fabio
From e6d3614f7bb886aff05dab7a6ccd562921c702f7 Mon Sep 17 00:00:00 2001 From: Fabio D'Urso <[email protected]> Date: Tue, 9 Oct 2012 10:47:40 +0200 Subject: [PATCH 1/3] Killed FormField::isModified() in favor of a new AnnotWidget callback Instead of having to ask FormField from AnnotWidget::draw if the widget's appearance needs to be rebuilt, now AnnotWidget gets notified of changes via the new AnnotWidget::updateAppearanceStream() callback. --- poppler/Annot.cc | 16 ++++++++++------ poppler/Annot.h | 1 + poppler/Form.cc | 52 ++++++++++++++++++++++++++++++++++++++-------------- poppler/Form.h | 12 +++++++++--- 4 files changed, 58 insertions(+), 23 deletions(-) diff --git a/poppler/Annot.cc b/poppler/Annot.cc index 53cbb67..d55ebc7 100644 --- a/poppler/Annot.cc +++ b/poppler/Annot.cc @@ -4905,6 +4905,14 @@ void AnnotWidget::generateFieldAppearance() { appearStream->setNeedFree(gTrue); } +// NOTE: This is a temporary implementation! +// TODO: Generate new appearance stream *here* and write it +void AnnotWidget::updateAppearanceStream() +{ + // Remove the old appearance so that AnnotWidget::draw will rebuild it next time + appearance.free(); + appearance.initNull(); +} void AnnotWidget::draw(Gfx *gfx, GBool printing) { Object obj; @@ -4916,13 +4924,9 @@ void AnnotWidget::draw(Gfx *gfx, GBool printing) { // Only construct the appearance stream when // - annot doesn't have an AP or - // - it's a field containing text (text and choices) and - // - NeedAppearances is true or - // - widget has been modified or + // - NeedAppearances is true if (field) { - if (appearance.isNull() || (form && form->getNeedAppearances()) || - ((field->getType() == formText || field->getType() == formChoice) && - field->isModified())) + if (appearance.isNull() || (form && form->getNeedAppearances())) generateFieldAppearance(); } diff --git a/poppler/Annot.h b/poppler/Annot.h index e495d06..978af47 100644 --- a/poppler/Annot.h +++ b/poppler/Annot.h @@ -1291,6 +1291,7 @@ public: void drawFormFieldText(GfxResources *resources, GooString *da); void drawFormFieldChoice(GfxResources *resources, GooString *da); void generateFieldAppearance (); + void updateAppearanceStream (); AnnotWidgetHighlightMode getMode() { return mode; } AnnotAppearanceCharacs *getAppearCharacs() { return appearCharacs; } diff --git a/poppler/Form.cc b/poppler/Form.cc index 309b065..67e6735 100644 --- a/poppler/Form.cc +++ b/poppler/Form.cc @@ -14,6 +14,7 @@ // Copyright 2009 Matthias Drochner <[email protected]> // Copyright 2009 KDAB via Guillermo Amaral <[email protected]> // Copyright 2010, 2012 Mark Riedesel <[email protected]> +// Copyright 2012 Fabio D'Urso <[email protected]> // //======================================================================== @@ -124,10 +125,6 @@ bool FormWidget::isReadOnly() const return field->isReadOnly(); } -GBool FormWidget::isModified() const { - return field->isModified(); -} - int FormWidget::encodeID (unsigned pageNum, unsigned fieldNum) { return (pageNum << 4*sizeof(unsigned)) + fieldNum; @@ -211,6 +208,11 @@ void FormWidgetButton::setAppearanceState(const char *state) { widget->setAppearanceState(state); } +void FormWidgetButton::updateWidgetAppearance() +{ + // The appearance stream must NOT be regenerated for this widget type +} + void FormWidgetButton::setState (GBool astate) { //pushButtons don't have state @@ -247,7 +249,13 @@ GooString* FormWidgetText::getContentCopy () { return parent->getContentCopy(); } - + +void FormWidgetText::updateWidgetAppearance() +{ + if (widget) + widget->updateAppearanceStream(); +} + bool FormWidgetText::isMultiline () const { return parent->isMultiline(); @@ -356,6 +364,12 @@ GooString* FormWidgetChoice::getEditChoice () return parent->getEditChoice(); } +void FormWidgetChoice::updateWidgetAppearance() +{ + if (widget) + widget->updateAppearanceStream(); +} + bool FormWidgetChoice::isSelected (int i) { if (!_checkRange(i)) return false; @@ -423,6 +437,11 @@ FormWidgetSignature::FormWidgetSignature(PDFDoc *docA, Object *aobj, unsigned nu parent = static_cast<FormFieldSignature*>(field); } +void FormWidgetSignature::updateWidgetAppearance() +{ + // Unimplemented +} + //======================================================================== // FormField @@ -446,7 +465,6 @@ FormField::FormField(PDFDoc *docA, Object *aobj, const Ref& aref, FormField *par fullyQualifiedName = NULL; quadding = quaddingLeftJustified; hasQuadding = gFalse; - modified = gFalse; ref = aref; @@ -623,10 +641,6 @@ void FormField::createWidgetAnnotations() { } } -GBool FormField::isModified() const { - return modified ? gTrue : parent ? parent->isModified() : gFalse; -} - void FormField::_createWidget (Object *obj, Ref aref) { terminal = true; @@ -758,6 +772,18 @@ GooString* FormField::getFullyQualifiedName() { return fullyQualifiedName; } +void FormField::updateChildrenAppearance() +{ + // Recursively update each child's appearance + if (terminal) { + for (int i = 0; i < numChildren; i++) + widgets[i]->updateWidgetAppearance(); + } else { + for (int i = 0; i < numChildren; i++) + children[i]->updateChildrenAppearance(); + } +} + //------------------------------------------------------------------------ // FormFieldButton //------------------------------------------------------------------------ @@ -864,7 +890,6 @@ GBool FormFieldButton::setState(char *state) if (terminal && parent && parent->getType() == formButton && appearanceState.isNull()) { // It's button in a set, set state on parent if (static_cast<FormFieldButton*>(parent)->setState(state)) { - modified = gTrue; return gTrue; } return gFalse; @@ -910,7 +935,6 @@ GBool FormFieldButton::setState(char *state) } updateState(state); - modified = gTrue; return gTrue; } @@ -1024,7 +1048,7 @@ void FormFieldText::setContentCopy (GooString* new_content) obj1.initString(content ? content->copy() : new GooString("")); obj.getDict()->set("V", &obj1); xref->setModifiedObject(&obj, ref); - modified = gTrue; + updateChildrenAppearance(); } FormFieldText::~FormFieldText() @@ -1199,7 +1223,7 @@ void FormFieldChoice::updateSelection() { obj.getDict()->set("V", &obj1); xref->setModifiedObject(&obj, ref); - modified = gTrue; + updateChildrenAppearance(); } void FormFieldChoice::unselectAll () diff --git a/poppler/Form.h b/poppler/Form.h index 303936a..ef67748 100644 --- a/poppler/Form.h +++ b/poppler/Form.h @@ -9,6 +9,7 @@ // Copyright 2007-2010, 2012 Albert Astals Cid <[email protected]> // Copyright 2010 Mark Riedesel <[email protected]> // Copyright 2011 Pino Toscano <[email protected]> +// Copyright 2012 Fabio D'Urso <[email protected]> // //======================================================================== @@ -109,6 +110,8 @@ public: void createWidgetAnnotation(); AnnotWidget *getWidgetAnnotation() const { return widget; } + virtual void updateWidgetAppearance() = 0; + #ifdef DEBUG_FORMS void print(int indent = 0); #endif @@ -154,6 +157,7 @@ public: char* getOnStr(); void setAppearanceState(const char *state); + void updateWidgetAppearance(); protected: GooString *onStr; @@ -175,6 +179,8 @@ public: //except a UTF16BE string void setContent(GooString* new_content); + void updateWidgetAppearance(); + bool isMultiline () const; bool isPassword () const; bool isFileSelect () const; @@ -214,6 +220,7 @@ public: GooString* getEditChoice (); + void updateWidgetAppearance(); bool isSelected (int i); bool isCombo () const; @@ -234,6 +241,7 @@ protected: class FormWidgetSignature: public FormWidget { public: FormWidgetSignature(PDFDoc *docA, Object *dict, unsigned num, Ref ref, FormField *p); + void updateWidgetAppearance(); protected: FormFieldSignature *parent; }; @@ -259,8 +267,6 @@ public: void setReadOnly (bool b) { readOnly = b; } bool isReadOnly () const { return readOnly; } - GBool isModified () const; - GooString* getDefaultAppearance() const { return defaultAppearance; } GBool hasTextQuadding() const { return hasQuadding; } VariableTextQuadding getTextQuadding() const { return quadding; } @@ -288,6 +294,7 @@ public: protected: void _createWidget (Object *obj, Ref aref); void createChildren(std::set<int> *usedParents); + void updateChildrenAppearance(); FormFieldType type; // field type Ref ref; @@ -300,7 +307,6 @@ public: int numChildren; FormWidget **widgets; bool readOnly; - GBool modified; GooString *partialName; // T field GooString *alternateUiName; // TU field -- 1.7.6.5
From af9c3ec47f101b8381fd573a7b7ad7a943938acb Mon Sep 17 00:00:00 2001 From: Fabio D'Urso <[email protected]> Date: Tue, 9 Oct 2012 12:49:26 +0200 Subject: [PATCH 2/3] Generate and write the appearance stream in AnnotWidget::updateWidgetApperance() Note: At the moment the old appearance is deleted and a *new* xref entry is created every time AnnotWidget::updateWidgetApperance() is called. --- poppler/Annot.cc | 30 +++++++++++++++++++++++++----- 1 files changed, 25 insertions(+), 5 deletions(-) diff --git a/poppler/Annot.cc b/poppler/Annot.cc index d55ebc7..bde6aae 100644 --- a/poppler/Annot.cc +++ b/poppler/Annot.cc @@ -4905,13 +4905,33 @@ void AnnotWidget::generateFieldAppearance() { appearStream->setNeedFree(gTrue); } -// NOTE: This is a temporary implementation! -// TODO: Generate new appearance stream *here* and write it void AnnotWidget::updateAppearanceStream() { - // Remove the old appearance so that AnnotWidget::draw will rebuild it next time - appearance.free(); - appearance.initNull(); + // Destroy the old appearance if any + invalidateAppearance(); + + // There's no need to create a new appearance stream if NeedAppearances is + // set, because it will be ignored next time anyway. + if (form && form->getNeedAppearances()) + return; + + // Create the new appearance + generateFieldAppearance(); + + // Fetch the appearance stream we've just created + Object obj1, obj2; + Ref apRef; + appearance.fetch(xref, &obj1); + apRef = xref->addIndirectObject(&obj1); + obj1.free(); + + // Write the AP dictionary + obj1.initDict(xref); + obj1.dictAdd(copyString("N"), obj2.initRef(apRef.num, apRef.gen)); + update("AP", &obj1); + + // Update our internal pointers to the appearance dictionary + appearStreams = new AnnotAppearance(doc, &obj1); } void AnnotWidget::draw(Gfx *gfx, GBool printing) { -- 1.7.6.5
From d82975a564dc9144b43d6f6cc09b8908ae13ec78 Mon Sep 17 00:00:00 2001 From: Fabio D'Urso <[email protected]> Date: Tue, 9 Oct 2012 15:24:02 +0200 Subject: [PATCH 3/3] AnnotWidget: Avoid repeatedly deleting and creating xref entries for appearance streams Previously updating the appearance stream always involved deleting the old stream's xref entry and creating a new one. Since xref entry deletion causes the generation number to be incremented, this behavior caused the generation number to quickly rise during user input. This patch stops it by reusing the same entry as the old appearance stream in case of repeated modifications. --- poppler/Annot.cc | 40 ++++++++++++++++++++++++++++------------ poppler/Annot.h | 1 + 2 files changed, 29 insertions(+), 12 deletions(-) diff --git a/poppler/Annot.cc b/poppler/Annot.cc index bde6aae..aa34197 100644 --- a/poppler/Annot.cc +++ b/poppler/Annot.cc @@ -3810,6 +3810,8 @@ void AnnotWidget::initialize(PDFDoc *docA, Dict *dict) { parent = NULL; } obj1.free(); + + updatedAppearanceStream.num = updatedAppearanceStream.gen = -1; } LinkAction* AnnotWidget::getAdditionalAction(AdditionalActionsType type) @@ -4907,8 +4909,11 @@ void AnnotWidget::generateFieldAppearance() { void AnnotWidget::updateAppearanceStream() { - // Destroy the old appearance if any - invalidateAppearance(); + // If this the first time updateAppearanceStream() is called on this widget, + // destroy the AP dictionary because we are going to create a new one. + if (updatedAppearanceStream.num == -1) { + invalidateAppearance(); // Delete AP dictionary and all referenced streams + } // There's no need to create a new appearance stream if NeedAppearances is // set, because it will be ignored next time anyway. @@ -4919,19 +4924,30 @@ void AnnotWidget::updateAppearanceStream() generateFieldAppearance(); // Fetch the appearance stream we've just created - Object obj1, obj2; - Ref apRef; + Object obj1; appearance.fetch(xref, &obj1); - apRef = xref->addIndirectObject(&obj1); - obj1.free(); - // Write the AP dictionary - obj1.initDict(xref); - obj1.dictAdd(copyString("N"), obj2.initRef(apRef.num, apRef.gen)); - update("AP", &obj1); + // If this the first time updateAppearanceStream() is called on this widget, + // create a new AP dictionary containing the new appearance stream. + // Otherwise, just update the stream we had created previously. + if (updatedAppearanceStream.num == -1) { + // Write the appearance stream + updatedAppearanceStream = xref->addIndirectObject(&obj1); + obj1.free(); + + // Write the AP dictionary + Object obj2; + obj1.initDict(xref); + obj1.dictAdd(copyString("N"), obj2.initRef(updatedAppearanceStream.num, updatedAppearanceStream.gen)); + update("AP", &obj1); - // Update our internal pointers to the appearance dictionary - appearStreams = new AnnotAppearance(doc, &obj1); + // Update our internal pointers to the appearance dictionary + appearStreams = new AnnotAppearance(doc, &obj1); + } else { + // Replace the existing appearance stream + xref->setModifiedObject(&obj1, updatedAppearanceStream); + obj1.free(); + } } void AnnotWidget::draw(Gfx *gfx, GBool printing) { diff --git a/poppler/Annot.h b/poppler/Annot.h index 978af47..68ddeb7 100644 --- a/poppler/Annot.h +++ b/poppler/Annot.h @@ -1320,6 +1320,7 @@ private: // AnnotBorderBS border; // BS Dict *parent; // Parent GBool addDingbatsResource; + Ref updatedAppearanceStream; // {-1,-1} if updateAppearanceStream has never been called }; //------------------------------------------------------------------------ -- 1.7.6.5
_______________________________________________ poppler mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/poppler
