Hi, now patch 1 contains the memleak fix. and patch2 is the updated patch addressing review comments.
Greets José On Thu, Mar 31, 2011 at 6:57 AM, Carlos Garcia Campos <[email protected]> wrote: > Excerpts from [email protected]'s message of mié mar 30 20:50:44 +0200 > 2011: >> Carlos, thanks for the review. >> >> Here is an updated patch. I finally decided to have only one enum for >> TriggerEvents, so we can have methods >> AnnotScreen::getAdditionAction(AnnotTriggerEvent event) >> and AnnotWidget::getAdditionAction(AnnotTriggerEvent event) >> of giving eventFocusOut or eventFocusIn to a screen Annotation will >> just give you a NULL. >> >> I changed to use a stack allocated pointer of LinkActions. If you feel >> there is a better data struct for this, let me know. >> > >> From e1576c8d6be31721ffeab52c4aa936eef1195d19 Mon Sep 17 00:00:00 2001 >> From: =?UTF-8?q?Jos=C3=A9=20Aliste?= <[email protected]> >> Date: Tue, 29 Mar 2011 04:27:15 -0400 >> Subject: [PATCH] Parse additionActions dictionary for Widget annots. > >> --- >> poppler/Annot.cc | 102 >> +++++++++++++++++++++++++++++++++++++++++++++++++----- >> poppler/Annot.h | 31 +++++++++++++--- >> 2 files changed, 118 insertions(+), 15 deletions(-) > >> diff --git a/poppler/Annot.cc b/poppler/Annot.cc >> index 1f77c71..faab471 100644 >> --- a/poppler/Annot.cc >> +++ b/poppler/Annot.cc >> @@ -1350,6 +1350,72 @@ void Annot::draw(Gfx *gfx, GBool printing) { >> obj.free(); >> } > >> +// Parse Actions in AdditionActions dictionary that are common to Screen >> and Widget annots >> +void Annot::parseAdditionActions (LinkAction **additionActions, Object >> *addActionDict, GooString *baseURI) >> +{ >> + Object obj2; >> + >> + if (addActionDict->dictLookup("E", &obj2)->isDict()) >> + additionActions[eventCursorEnter] = LinkAction::parseAction (&obj2, >> baseURI); >> + obj2.free(); >> + >> + if (addActionDict->dictLookup("X", &obj2)->isDict()) >> + additionActions[eventCursorExit] = LinkAction::parseAction (&obj2, >> baseURI); >> + obj2.free(); >> + >> + if (addActionDict->dictLookup("D", &obj2)->isDict()) >> + additionActions[eventMouseDown] = LinkAction::parseAction (&obj2, >> baseURI); >> + obj2.free(); >> + >> + if (addActionDict->dictLookup("U", &obj2)->isDict()) >> + additionActions[eventMouseUp] = LinkAction::parseAction (&obj2, >> baseURI); >> + obj2.free(); >> + >> + if (addActionDict->dictLookup("PO", &obj2)->isDict()) >> + additionActions[eventPageOpen] = LinkAction::parseAction (&obj2, >> baseURI); >> + obj2.free(); >> + >> + if (addActionDict->dictLookup("PC", &obj2)->isDict()) >> + additionActions[eventPageClose] = LinkAction::parseAction (&obj2, >> baseURI); >> + obj2.free(); >> + >> + if (addActionDict->dictLookup("PV", &obj2)->isDict()) >> + additionActions[eventPageVisible] = LinkAction::parseAction (&obj2, >> baseURI); >> + obj2.free(); >> + >> + if (addActionDict->dictLookup("PI", &obj2)->isDict()) >> + additionActions[eventPageInvisible] = LinkAction::parseAction (&obj2, >> baseURI); >> + obj2.free(); >> + >> + } >> + >> +// Delete additionActions that are common to Screen and Widget annots >> +void Annot::deleteAdditionActions(LinkAction **additionActions) { > > You don't need this. See below > >> + if (additionActions[eventCursorEnter]) >> + delete additionActions[eventCursorEnter]; >> + >> + if (additionActions[eventCursorExit]) >> + delete additionActions[eventCursorExit]; >> + >> + if (additionActions[eventMouseDown]) >> + delete additionActions[eventMouseDown]; >> + >> + if (additionActions[eventMouseUp]) >> + delete additionActions[eventMouseUp]; >> + >> + if (additionActions[eventPageOpen]) >> + delete additionActions[eventPageOpen]; >> + >> + if (additionActions[eventPageClose]) >> + delete additionActions[eventPageClose]; >> + >> + if (additionActions[eventPageVisible]) >> + delete additionActions[eventPageVisible]; >> + >> + if (additionActions[eventPageInvisible]) >> + delete additionActions[eventPageInvisible]; >> +} >> + >> //------------------------------------------------------------------------ >> // AnnotPopup >> //------------------------------------------------------------------------ >> @@ -2715,12 +2781,16 @@ AnnotWidget::~AnnotWidget() { > >> if (action) >> delete action; >> - >> - if (additionActions) >> - delete additionActions; >> - >> + >> + Annot::deleteAdditionActions(additionActions); >> + if (additionActions[eventFocusIn]) >> + delete additionActions[eventFocusIn]; >> + if (additionActions[eventFocusOut]) >> + delete additionActions[eventFocusOut]; >> + > > delete is null-safe so here you only need a for loop > > for (int i = 0; i < EventsNumber; i++) > delete additionActions[i]; > >> if (parent) >> delete parent; >> + >> } > > Remove this new line please. > >> void AnnotWidget::initialize(XRef *xrefA, Catalog *catalog, Dict *dict) { >> @@ -2758,10 +2828,20 @@ void AnnotWidget::initialize(XRef *xrefA, Catalog >> *catalog, Dict *dict) { >> } >> obj1.free(); > >> + memset (additionActions, 0, EventsNumber); >> + >> if(dict->lookup("AA", &obj1)->isDict()) { >> - additionActions = NULL; >> - } else { >> - additionActions = NULL; >> + Object obj2; >> + >> + Annot::parseAdditionActions(additionActions, &obj1, >> catalog->getBaseURI()); > > You don't need the Annot:: prefix here. > >> + if (obj1.dictLookup("Fo", &obj2)->isDict()) >> + additionActions[eventFocusIn] = LinkAction::parseAction(&obj2, >> catalog->getBaseURI()); >> + obj2.free(); >> + >> + if (obj1.dictLookup("Bl", &obj2)->isDict()) >> + additionActions[eventFocusOut] = LinkAction::parseAction(&obj2, >> catalog->getBaseURI()); >> + obj2.free(); >> } >> obj1.free(); > >> @@ -4066,7 +4146,7 @@ AnnotScreen::~AnnotScreen() { >> if (action) >> delete action; > >> - additionAction.free(); >> + Annot::deleteAdditionActions(additionActions); > > Ditto. > >> } > >> void AnnotScreen::initialize(XRef *xrefA, Catalog *catalog, Dict* dict) { >> @@ -4088,8 +4168,12 @@ void AnnotScreen::initialize(XRef *xrefA, Catalog >> *catalog, Dict* dict) { >> ok = gFalse; >> } >> } >> + obj1.free(); > > This looks unrelated, it there's a memleak there, please use a > different commit, so that we can push it now. > >> - dict->lookup("AA", &additionAction); >> + memset(additionActions, 0, EventsNumber); >> + if (dict->lookup("AA", &obj1)->isDict()) { >> + Annot::parseAdditionActions (additionActions, &obj1, >> catalog->getBaseURI()); > > Don't need Annot:: here either. > >> + } > >> appearCharacs = NULL; >> if(dict->lookup("MK", &obj1)->isDict()) { >> diff --git a/poppler/Annot.h b/poppler/Annot.h >> index 93f82bf..ad31118 100644 >> --- a/poppler/Annot.h >> +++ b/poppler/Annot.h >> @@ -453,6 +453,21 @@ public: >> type3D // 3D 25 >> }; > >> + enum AnnotTriggerEvent { >> + eventCursorEnter, // E >> + eventCursorExit, // X >> + eventMouseDown, // D >> + eventMouseUp, // U >> + eventFocusIn, // Fo >> + eventFocusOut, // Bl >> + eventPageOpen, // PO >> + eventPageClose, // PC >> + eventPageVisible, // PV >> + eventPageInvisible, // PI >> + EventsNumber >> + }; >> + >> + >> Annot(XRef *xrefA, PDFRectangle *rectA, Catalog *catalog); >> Annot(XRef *xrefA, Dict *dict, Catalog *catalog); >> Annot(XRef *xrefA, Dict *dict, Catalog *catalog, Object *obj); >> @@ -525,7 +540,9 @@ protected: >> void createResourcesDict(char *formName, Object *formStream, char >> *stateName, >> double opacity, char *blendMode, Object *resDict); >> GBool isVisible(GBool printing); >> - >> + static void parseAdditionActions(LinkAction **additionActions, Object >> *additionActionsDict, >> + GooString *baseURI); >> + static void deleteAdditionActions (LinkAction **additionActions); >> // Updates the field key of the annotation dictionary >> // and sets M to the current time >> void update(const char *key, Object *value); >> @@ -721,8 +738,9 @@ class AnnotScreen: public Annot { > >> AnnotAppearanceCharacs *getAppearCharacs() { return appearCharacs; } >> LinkAction* getAction() { return action; } >> - Object* getAdditionActions() { return &additionAction; } >> - >> + LinkAction **getAdditionActions() { return additionActions; } >> + LinkAction *getAdditionAction(AnnotTriggerEvent event) { return >> additionActions[event]; } > > Please, rename it to AdditionalActions. > >> private: >> void initialize(XRef *xrefA, Catalog *catalog, Dict *dict); > > Regards, > -- > Carlos Garcia Campos > PGP key: http://pgp.mit.edu:11371/pks/lookup?op=get&search=0x523E6462 >
From 79568f0114154dab3153012736cb7dc925d1a43b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Aliste?= <[email protected]> Date: Thu, 31 Mar 2011 08:09:41 -0400 Subject: [PATCH 1/2] Fix a memleak in AnnotScreen::initialize. --- poppler/Annot.cc | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/poppler/Annot.cc b/poppler/Annot.cc index 1f77c71..7d1136c 100644 --- a/poppler/Annot.cc +++ b/poppler/Annot.cc @@ -4088,6 +4088,7 @@ void AnnotScreen::initialize(XRef *xrefA, Catalog *catalog, Dict* dict) { ok = gFalse; } } + obj1.free(); dict->lookup("AA", &additionAction); -- 1.7.3.5
From 967cb1ef4acba2b517eb97cace9fab770388bb58 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Aliste?= <[email protected]> Date: Tue, 29 Mar 2011 04:27:15 -0400 Subject: [PATCH 2/2] Parse additionActions dictionary for Widget annots. --- poppler/Annot.cc | 71 ++++++++++++++++++++++++++++++++++++++++++++++------- poppler/Annot.h | 29 +++++++++++++++++---- 2 files changed, 84 insertions(+), 16 deletions(-) diff --git a/poppler/Annot.cc b/poppler/Annot.cc index 7d1136c..dbf1778 100644 --- a/poppler/Annot.cc +++ b/poppler/Annot.cc @@ -1350,6 +1350,45 @@ void Annot::draw(Gfx *gfx, GBool printing) { obj.free(); } +// Parse Actions in AdditionalActionss dictionary that are common to Screen and Widget annots +void Annot::parseAdditionalActionss (LinkAction **additionActions, Object *addActionDict, GooString *baseURI) +{ + Object obj2; + + if (addActionDict->dictLookup("E", &obj2)->isDict()) + additionActions[eventCursorEnter] = LinkAction::parseAction (&obj2, baseURI); + obj2.free(); + + if (addActionDict->dictLookup("X", &obj2)->isDict()) + additionActions[eventCursorExit] = LinkAction::parseAction (&obj2, baseURI); + obj2.free(); + + if (addActionDict->dictLookup("D", &obj2)->isDict()) + additionActions[eventMouseDown] = LinkAction::parseAction (&obj2, baseURI); + obj2.free(); + + if (addActionDict->dictLookup("U", &obj2)->isDict()) + additionActions[eventMouseUp] = LinkAction::parseAction (&obj2, baseURI); + obj2.free(); + + if (addActionDict->dictLookup("PO", &obj2)->isDict()) + additionActions[eventPageOpen] = LinkAction::parseAction (&obj2, baseURI); + obj2.free(); + + if (addActionDict->dictLookup("PC", &obj2)->isDict()) + additionActions[eventPageClose] = LinkAction::parseAction (&obj2, baseURI); + obj2.free(); + + if (addActionDict->dictLookup("PV", &obj2)->isDict()) + additionActions[eventPageVisible] = LinkAction::parseAction (&obj2, baseURI); + obj2.free(); + + if (addActionDict->dictLookup("PI", &obj2)->isDict()) + additionActions[eventPageInvisible] = LinkAction::parseAction (&obj2, baseURI); + obj2.free(); + + } + //------------------------------------------------------------------------ // AnnotPopup //------------------------------------------------------------------------ @@ -2715,10 +2754,10 @@ AnnotWidget::~AnnotWidget() { if (action) delete action; - - if (additionActions) - delete additionActions; - + + for (int i = 0; i < EventsNumber; i++) + delete additionActions[i]; + if (parent) delete parent; } @@ -2758,10 +2797,19 @@ void AnnotWidget::initialize(XRef *xrefA, Catalog *catalog, Dict *dict) { } obj1.free(); + memset (additionActions, 0, EventsNumber); if(dict->lookup("AA", &obj1)->isDict()) { - additionActions = NULL; - } else { - additionActions = NULL; + Object obj2; + + parseAdditionalActionss(additionActions, &obj1, catalog->getBaseURI()); + + if (obj1.dictLookup("Fo", &obj2)->isDict()) + additionActions[eventFocusIn] = LinkAction::parseAction(&obj2, catalog->getBaseURI()); + obj2.free(); + + if (obj1.dictLookup("Bl", &obj2)->isDict()) + additionActions[eventFocusOut] = LinkAction::parseAction(&obj2, catalog->getBaseURI()); + obj2.free(); } obj1.free(); @@ -4065,8 +4113,8 @@ AnnotScreen::~AnnotScreen() { delete appearCharacs; if (action) delete action; - - additionAction.free(); + for (int i = 0; i < EventsNumber; i++) + delete additionActions[i]; } void AnnotScreen::initialize(XRef *xrefA, Catalog *catalog, Dict* dict) { @@ -4090,7 +4138,10 @@ void AnnotScreen::initialize(XRef *xrefA, Catalog *catalog, Dict* dict) { } obj1.free(); - dict->lookup("AA", &additionAction); + memset(additionActions, 0, EventsNumber); + if (dict->lookup("AA", &obj1)->isDict()) { + parseAdditionalActionss (additionActions, &obj1, catalog->getBaseURI()); + } appearCharacs = NULL; if(dict->lookup("MK", &obj1)->isDict()) { diff --git a/poppler/Annot.h b/poppler/Annot.h index 93f82bf..040a02a 100644 --- a/poppler/Annot.h +++ b/poppler/Annot.h @@ -453,6 +453,20 @@ public: type3D // 3D 25 }; + enum AnnotTriggerEvent { + eventCursorEnter, // E + eventCursorExit, // X + eventMouseDown, // D + eventMouseUp, // U + eventFocusIn, // Fo + eventFocusOut, // Bl + eventPageOpen, // PO + eventPageClose, // PC + eventPageVisible, // PV + eventPageInvisible, // PI + EventsNumber + }; + Annot(XRef *xrefA, PDFRectangle *rectA, Catalog *catalog); Annot(XRef *xrefA, Dict *dict, Catalog *catalog); Annot(XRef *xrefA, Dict *dict, Catalog *catalog, Object *obj); @@ -525,7 +539,8 @@ protected: void createResourcesDict(char *formName, Object *formStream, char *stateName, double opacity, char *blendMode, Object *resDict); GBool isVisible(GBool printing); - + static void parseAdditionalActions(LinkAction **additionActions, Object *additionActionsDict, + GooString *baseURI); // Updates the field key of the annotation dictionary // and sets M to the current time void update(const char *key, Object *value); @@ -721,8 +736,9 @@ class AnnotScreen: public Annot { AnnotAppearanceCharacs *getAppearCharacs() { return appearCharacs; } LinkAction* getAction() { return action; } - Object* getAdditionActions() { return &additionAction; } - + LinkAction **getAdditionalActions() { return additionActions; } + LinkAction *getAdditionalAction(AnnotTriggerEvent event) { return additionActions[event]; } + private: void initialize(XRef *xrefA, Catalog *catalog, Dict *dict); @@ -732,7 +748,7 @@ class AnnotScreen: public Annot { AnnotAppearanceCharacs* appearCharacs; // MK LinkAction *action; // A - Object additionAction; // AA + LinkAction *additionActions[EventsNumber]; // AA }; //------------------------------------------------------------------------ @@ -1147,7 +1163,8 @@ public: AnnotWidgetHighlightMode getMode() { return mode; } AnnotAppearanceCharacs *getAppearCharacs() { return appearCharacs; } LinkAction *getAction() { return action; } - Dict *getAdditionActions() { return additionActions; } + LinkAction **getAdditionalActions() { return additionActions; } + LinkAction *getAdditionalAction(AnnotTriggerEvent event) { return additionActions[event]; } Dict *getParent() { return parent; } private: @@ -1170,7 +1187,7 @@ private: AnnotWidgetHighlightMode mode; // H (Default I) AnnotAppearanceCharacs *appearCharacs; // MK LinkAction *action; // A - Dict *additionActions; // AA + LinkAction *additionActions[EventsNumber]; // AA // inherited from Annot // AnnotBorderBS border; // BS Dict *parent; // Parent -- 1.7.3.5
_______________________________________________ poppler mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/poppler
