[Poppler-bugs] [Bug 64821] [TAGGEDPDF] Expose the structure tree and attributes in poppler-glib

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=64821

--- Comment #52 from Carlos Garcia Campos <carlosgc at gnome.org> ---
Comment on attachment 90264
  --> https://bugs.freedesktop.org/attachment.cgi?id=90264
[PATCH v12 07/11] glib: Expose inline attributes of structure elements

Review of attachment 90264:
-----------------------------------------------------------------

::: glib/poppler-structure-element.cc
@@ +705,5 @@
>  }
> +
> +
> +static PopplerTextSpan *
> +_poppler_convert_text_span (const TextSpan& span)

This could be called text_span_to_poppler_text_span for consistency with other
similar methods. Do not use _ prefix for private static methods.

@@ +708,5 @@
> +static PopplerTextSpan *
> +_poppler_convert_text_span (const TextSpan& span)
> +{
> +    PopplerTextSpan *new_span = g_slice_new0 (PopplerTextSpan);
> +    new_span->text = _poppler_goo_string_to_utf8 (span.getText ());

We should check that the returned text is not NULL.

@@ +766,5 @@
> + * guint n_spans;
> + * PopplerTextSpan **text_spans =
> + *    poppler_structure_element_get_text_spans (element, &n_spans);
> + * /<!-- -->* Use the text spans *<!-- -->/
> + * poppler_text_spans_free (text_spans, n_spans);

This is not correct, the free method doesn't return the size of the array.

@@ +769,5 @@
> + * /<!-- -->* Use the text spans *<!-- -->/
> + * poppler_text_spans_free (text_spans, n_spans);
> + * </programlisting></informalexample>
> + *
> + * Return value: (transfer full) (array length=n_text_spans):

You should also add the element type annot so that bindings know how to free
the contents.

@@ +770,5 @@
> + * poppler_text_spans_free (text_spans, n_spans);
> + * </programlisting></informalexample>
> + *
> + * Return value: (transfer full) (array length=n_text_spans):
> + *    Null-terminated array of #PopplerTextSpan structures.

I think we should either make the array null terminated and not return the size
or return the size making the out param mandatory and not adding the extra null
element

@@ +779,5 @@
> +poppler_structure_element_get_text_spans (PopplerStructureElement *poppler_structure_element,
> +                                          guint                   *n_text_spans)
> +{
> +  g_return_val_if_fail (POPPLER_IS_STRUCTURE_ELEMENT (poppler_structure_element), 0);
> +  g_assert (poppler_structure_element->elem);

Use g_return macros in public methods.

@@ +805,5 @@
> + * Frees the memory used by an array of #PopplerTextSpan elements, typically
> + * obtained using poppler_structure_element_get_text_spans().
> + */
> +void
> +poppler_structure_element_free_text_spans (PopplerTextSpan **poppler_text_spans)

Previous patch returned a GList for which a convenient free method makes more
sense. I think case, if we return the size of the array, I think we can avoid
this free method and let the user do it.

@@ +828,5 @@
> + * Since: 0.26
> + */
> +PopplerTextSpan *
> +poppler_text_span_copy (PopplerTextSpan *poppler_text_span)
> +{

Use g_return macros in all public methods

@@ +840,5 @@
> +/**
> + * poppler_text_span_free:
> + * @poppler_text_span: A #PopplerTextSpan
> + *
> + * Freed a text span.

Frees

@@ +911,5 @@
> + *
> + * Since: 0.26
> + */
> +PopplerColor *
> +poppler_text_span_get_color (PopplerTextSpan *poppler_text_span)

I think it would be better to return the color as an out parameter

::: glib/poppler-structure-element.h
@@ +82,4 @@
>    POPPLER_STRUCTURE_ELEMENT_FORM,
>  } PopplerStructureElementKind;
>  
> +typedef struct _PopplerTextSpan PopplerTextSpan;

This should be in poppler.h with all other forward declarations.

@@ +112,4 @@
>  gboolean                     poppler_structure_element_iter_next                  (PopplerStructureElementIter *iter);
>  void                         poppler_structure_element_iter_free                  (PopplerStructureElementIter *iter);
>  
> +PopplerTextSpan             *poppler_text_span_copy                               (PopplerTextSpan *poppler_text_span);

You should expose here the get_type method too

::: glib/reference/poppler-sections.txt
@@ +600,1 @@
>  poppler_structure_element_iter_get_type

poppler_text_span_get_type is missing here.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/poppler-bugs/attachments/20140209/f63048b5/attachment.html>