[poppler] Patch to Get/Set PDF ID in the trailer dictionary, get page labels .

[Pino Toscano]

Hi,

I'm not a poppler-glib developer, but there are few notes below.

Alle giovedì 10 giugno 2010, srinivas adicherla ha scritto:
> +/* following three functions has added to get and set pdf ID stored
> in Trailer Dictionary*/
> +static void
> +get_id(char* id,char *buff) {
> +
> +       int i;
> +       for( i = 0; i < 16; i++ ) {
> +
> +               char *buf = (char*)malloc(3*sizeof(char));
> +               sprintf(buf,"%02x",id[i] & 0xff);
> +
> +               if(i == 0) strcpy(buff,buf);
> +               else strcat(buff,buf);
> +       
> +               free (buf);
> +       }
> +}

This is pretty inefficient: you are allocating on the heap 3 bytes 16 
times (!). It would be much better if you could just use an array of 3 
chars on stack, and reuse it at every iteration.
(Minor: a better naming than 'buf' and 'buff' would help a lot to 
distinguish them.)

> +
> +gboolean
> +poppler_document_get_trailer_id(char *uri,char *password,char
> *id[2]) {
> [...]
> +       for(int i=0; i<2; i++) {
> +               id[i] = (char*)malloc(32*sizeof(char));
> +       }

Excluding the fact that such for loop is useless (just manually do the 
two calls it does), why do you dynamically malloc the two result strings 
if you already know they will be 32 chars long? Then, instead of a out 
parameter of a two-strings array, just use two out parameters of 32 
bytes.

> +void *
> +poppler_document_generate_and_set_id(char *uri,char *password) {

If this function returns nothing, then declare it as void, not as void*.

> +char*
> +poppler_document_get_page_label_by_index(PopplerDocument *document,
> +                                       int index)
> +{
> +       GooString *string = new GooString();
> +       Catalog *catalog  = document->doc->getCatalog ();
> +
> +       catalog->indexToLabel(index,string);
> +
> +       char *label = string->getCString();
> +       return label;
> +}

'string' is leaked here, just put it on stack.

> +char**          
> +poppler_document_get_page_labels(PopplerDocument *document)
> +{
> +
> +       char **labels;
> +       Catalog *catalog= document->doc->getCatalog ();
> +       int npages      = poppler_document_get_n_pages(document);
> +
> +       labels          = (char**)malloc(sizeof(char*)*npages);
> +
> +       int i;
> +       for(i = 0; i < npages; i++) {
> +       
> +               GooString *label = new GooString();
> +               catalog->indexToLabel(i,label);
> +               labels[i] = label->getCString();
> +       }
> +       return labels;
> +}

This is tricker: the 'label' allocated at each loop is leaked, but you 
use its internal storage as return value. As before, put the GooString 
on stack, but strdup its storage.

> +/* Labels */
> +char*
> +poppler_document_get_page_label_by_index(PopplerDocument *document,
> +                                       int index);
> +char**          
> +poppler_document_get_page_labels(PopplerDocument *document);

A label is specific to every page, so I guess these two functions would 
quite more suited as part of the PopplerPage API.
Also, they miss they are not documented with gtk-doc, just like the rest 
of the glib API.

> +/*Get the PDF ID from the trailer dictionary. Pdf Id contains two
> strings of length 16 bytes each, represented in Hex*/
> +gboolean        poppler_document_get_trailer_id(char *uri,char
> *password,char *id[2]);
> +
> +/*Generate the Pdf Id and set it in the Pdf file*/
> +void   *poppler_document_generate_and_set_id(char *uri,char
> *password);

Most probably they should not be just "free functions", but be part of 
the PopplerDocument API, so you first open a document as usual, then get 
its trailer and then free the document.
Also, the uri and password parameters should be 'const', to indicate 
they won't be changed in the functions. And as above, they miss gtk-doc 
API documentation.

-- 
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/20100610/36cd404b/attachment.pgp>