[poppler] Patch go Get PDF ID

[Rogério Brito]

Hi there.

On 07/02/2010 01:38 AM, srinivas adicherla wrote:
> +static void
> +get_id(char* encodedid,char *pdfid) {
> +  sprintf(pdfid,"%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x", encodedid[0] & 0xff,encodedid[1] & 0xff,

It would be nice if you put spaces after commas. Also, if you wrapped the line
before the arguments that get sprintf'ed, the lines would fit in fewer columns.

> +  char tmpid[33];

Why not give a name to magic constants?

>  typedef struct _PopplerDocument            PopplerDocument;
> +typedef struct _PopplerDocumentId	   PopplerDocumentId;
>  typedef struct _PopplerIndexIter           PopplerIndexIter;
(...)
>  	PROP_PERMISSIONS,
> -	PROP_METADATA
> +	PROP_METADATA,
> +        PROP_PERMANENT_ID,
> +	PROP_UPDATE_ID
>  };

There seems to be some kind of consistency problems between your indentation
style and that of poppler. While I would favor something like the Linux kernel
coding conventions, I would stick to a project's coding conventions for
contributed code.

>  	  }
>  	}
>        break;
> +    case PROP_PERMANENT_ID:
> +      {
> +        GooString permanent_id,update_id;
> +	if (document->doc->getID(&permanent_id,&update_id))
> +	{
> +	  g_value_set_string(value,permanent_id.getCString());
> +	}
> +	break;
> +      }

The indentation here is messy. Also, is weird to have the case body inside
braces. As C++ allows you to mix declarations and statements, the braces are not
necessary for the declaration of the variables (which you are, BTW, not
separating with a comma).

> +    case PROP_UPDATE_ID:
> +      {
> +        GooString permanent_id,update_id;
> +	if (document->doc->getID(&permanent_id,&update_id))
> +	{
> +	  g_value_set_string(value,update_id.getCString());
> +	}
> +	break;
> +      }

Now, something of a higher level: why not opt to use some functions
getPermanentID and getUpdateID, and implement getID as a wrapper around them?

> +   g_object_class_install_property
> +	  (G_OBJECT_CLASS (klass),
> +	   PROP_PERMANENT_ID,
> +	   g_param_spec_string ("permanent-id",

Any reason for the space before an open parenthesis?

> +struct _PopplerDocumentId
> +{
> +  gchar permanent_id[33];
> +  gchar update_id[33];
> +};

Some magic constants again.


Regards.