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

Review of attachment 94907:
-----------------------------------------------------------------

You should also add the new methods to the sections.txt file.

::: glib/poppler-structure-element.cc
@@ +765,5 @@
>  
>  /**
> + * poppler_structure_element_get_form_field:
> + * @poppler_structure_element: A #PopplerStructureElement
> + *

We need a brief explanation here, and comment that this should be called only
for FORM elements

@@ +775,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)

I think it's a programmer error to call this for a different element type, so I
would make this a g_return

@@ +781,5 @@
> +
> +  // TODO Handle elements which have a Role attribute (used sometimes for
> +  //      non-editable widgets, to describe their appearance). Editable
> +  //      fields have only a single child, with the field identifier.
> +  if (poppler_structure_element->elem->getNumElements () != 1)

I think I'm going to rename the getElements api as getChildren, this confused
me

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

Where is this documented? According to the spec, if the role is not present the
element should have a single child that is an object reference.

"(Form) A widget annotation representing an interactive form field (see 12.7,
“Interactive Forms”). If the element contains a Role attribute, it may contain
content
items that represent the value of the (non-interactive) form field. If the
element
omits a Role attribute (see Table 348), it shall have only one child: an object
reference (see 14.7.4.3, “PDF Objects as Content Items”) identifying the widget
annotation. The annotations’ appearance stream (see 12.5.5, “Appearance
Streams”) shall describe the appearance of the form element."

So, no MCID allowed here. I still think this should be done by the core.

@@ +814,5 @@
> + * Return value: (transfer full): A #PopplerFormFieldMapping, or %NULL if
> + *    the element is not a %POPPLER_STRUCTURE_ELEMENT_FORM
> + */
> +PopplerFormFieldMapping *
> +poppler_structure_element_get_form_field_mapping (PopplerStructureElement *poppler_structure_element)

I insist I'm not sure about this API. The mapping is something associated to a
page, it's the coordinates of the form field in a page. I think this could be
moved to poppler page as something like:

poppler_page_get_form_field_mapping_for_element (page, element);

This would be more unconvenient to use because the user has to create a
PopplerPage, but I think it's less confusing.

-- 
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/20140301/a6e13d9c/attachment.html>