[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 #75 from Carlos Garcia Campos <carlosgc at gnome.org> ---
Comment on attachment 94991
  --> https://bugs.freedesktop.org/attachment.cgi?id=94991
[PATCH v18 3/5] glib: Accessors for form fields of structure elements

Review of attachment 94991:
-----------------------------------------------------------------

This patch doesn't address or replies to my comments of the previous review.

::: glib/poppler-page.cc
@@ +1385,5 @@
> + * @structure_element: a #PopplerStructureElement.
> + *
> + * Obtains the #PopplerFormFieldMapping for a single form field associated
> + * with a structure element of type %POPPLER_STRUCTURE_ELEMENT_FORM. The
> + * returned value must be freed with g_object_unref().

PopplerFormFieldMapping is not a GObject, but a boxed type, that should be
freed with poppler_form_field_mapping_free().

::: glib/poppler-page.h
@@ +89,4 @@
>  cairo_surface_t       *poppler_page_get_image            (PopplerPage        *page,
>  							  gint                image_id);
>  GList              *poppler_page_get_form_field_mapping  (PopplerPage        *page);
> +PopplerFormFieldMapping*

PopplerFormFieldMapping* -> PopplerFormFieldMapping *

::: glib/poppler-structure-element.cc
@@ +783,5 @@
> +  g_return_val_if_fail (POPPLER_IS_STRUCTURE_ELEMENT (poppler_structure_element), NULL);
> +  g_return_val_if_fail (poppler_structure_element->elem != NULL, NULL);
> +
> +  if (poppler_structure_element->elem->getType () != StructElement::Form)
> +    return NULL;

As I said in the previous review, I think this is a programmer error, it should
use a g_return macro.

@@ +804,5 @@
> +    }
> +  else
> +    {
> +      // Element contains the form field ID as the MCID attribute.
> +      field_id = child->getMCID ();

Please, see my previous review.

::: glib/reference/poppler-sections.txt
@@ +611,4 @@
>  poppler_structure_element_get_alt_text
>  poppler_structure_element_get_actual_text
>  poppler_structure_element_get_text_spans
> +poppler_structure_element_form_get_field

You should add PopplerPage new api to the docs too.

-- 
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/20140303/c01a5d68/attachment.html>