[poppler] Form-Reset and Print [patches]

[Pino Toscano]

Hi,

Alle lunedì 14 giugno 2010, Guillermo Amaral ha scritto:
>   Anyway, I added the missing features to the reset form patch today,
> check it out.

Some more notes other than what Albert and Carlos said already:

> +LinkResetForm::LinkResetForm(Object *obj) {
> +  Object obj1;
> +
> +  fieldList = new GooList();
> +  flags = 0;
> +
> +  if (obj->dictLookup("Fields", &obj1)->isArray()) {
> +    for (int i = 0; i < obj1.arrayGetLength(); ++i) {
> +      Object obj2;
> +      obj1.arrayGetNF(i, &obj2);
> +      fieldList->append(obj2.getString()->copy());
> +      obj2.free();
> +    }
> +  } else {
> +    error (-1, "Invalid ResetForm action");
> +    delete fieldList;
> +    fieldList = NULL;
> +  }
> +  obj1.free();
> [...]
> +  // Was the LinkResetForm create successfully?
> +  virtual GBool isOk() { return fieldList != NULL; }

Basically this code makes the action valid only if Fields is specified 
and valid, while that key is optional.

> +class LinkFormActionPrivate : public LinkPrivate
> +{
> +       public:
> +               LinkFormActionPrivate( const QRectF &area,
> LinkFormAction::ActionType actionType );
> +
> +               LinkFormAction::ActionType type;

+ const here

> +       bool LinkResetFormAction::isExcludeFieldList() const
> +       {
> +               Q_D( const LinkResetFormAction );
> +               return (1 == (d->flags % 1));

Err, you mean 'd->flags & 1'?

> @@ -182,7 +184,8 @@ class POPPLER_QT4_EXPORT Link
>                     Action,   ///< A "standard" action to be executed
> in the viewer
>                     Sound,    ///< A link representing a sound to be
> played
>                     Movie,    ///< An action to be executed on a movie
> -                   JavaScript    ///< A JavaScript code to be
> interpreted \since 0.10
> +                   JavaScript,   ///< A
> JavaScript code to be interpreted \since 0.10
> +                   FormAction    ///< A "form" action to be
> executed in the viewer

+ \since 0.16

> @@ -484,6 +487,95 @@ class POPPLER_QT4_EXPORT LinkMovie : public Link
>  };
>  #endif
>  
> +/**
> + * \brief "Form" action request.
> + *
> + * The LinkFormAction class represents a link that request a "form"
> action
> + * to be performed by the viewer on the displayed document.
> + */

as above, add \since 0.16

> +class POPPLER_QT4_EXPORT LinkFormAction : public Link
> +{
> +       public:
> +               /**
> +                * The possible types of actions
> +                */
> +               enum ActionType { Submit = 1,
> +                                 Reset = 2,
> +                                 ImportData = 3 };

I'd use a slightly better name for the enum, like FormActionType.

> +
> +               /**
> +                * The action of the current LinkFormAction
> +                */
> +               ActionType actionType() const;

likewise for the method naming

> +               /**
> +                * Create a new Action link, that executes a
> specified action +                * on the document.
> +                *
> +                * \param linkArea the active area of the link
> +                * \param actionType which action should be executed
> +                */
> +               LinkFormAction( const QRectF &linkArea, ActionType
> actionType );

I'd make this constructor protected, as LinkFormAction looks more an 
intermediate action type which has no sense used alone.

> +/**
> + * \brief "Form" action reset request.
> + *
> + * The LinkResetFormAction class represents a link that request a
> "ResetForm" action
> + * to be performed by the viewer on the displayed document.
> + */

... guess what? ;)

> +class POPPLER_QT4_EXPORT LinkResetFormAction : public LinkFormAction
> +{
> +       public:
> +               typedef QList<QString> FieldList;

Why not just a simple QStringList instead of this typedef?

> +               LinkResetFormAction( const QRectF &linkArea, const
> FieldList &_fieldList, int _flags );
> [...]
> +               int flags() const;
> [...]
> +               bool isExcludeFieldList() const;

This looks a bit too "raw"... what I would do is something like:
  enum FormResetType { ResetAll, ResetIncluded, ResetExcluded }
and then use only this enum in both constructor and method. Maybe 
ResetAll is not needed, stating in the apidox that you should check for 
the emptiness of the field list.

-- 
Pino Toscano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 190 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/poppler/attachments/20100615/7d25063a/attachment.pgp>