<html> <head> <base href="https://bugs.freedesktop.org/" /> </head> <body> <div> <a class="bz_bug_link bz_status_NEW " title="NEW --- - [TAGGEDPDF] Expose the structure tree and attributes in poppler-glib" href="https://bugs.freedesktop.org/show_bug.cgi?id=64821#c67">Comment # 67</a> on <a class="bz_bug_link bz_status_NEW " title="NEW --- - [TAGGEDPDF] Expose the structure tree and attributes in poppler-glib" href="https://bugs.freedesktop.org/show_bug.cgi?id=64821">bug 64821</a> from <a class="email" href="mailto:carlosgc@gnome.org" title="Carlos Garcia Campos <carlosgc@gnome.org>"> Carlos Garcia Campos</a> <pre>Comment on <a href="attachment.cgi?id=94907" name="attach_94907" title="[PATCH v17 3/5] glib: Accessors for form fields of structure elements">attachment 94907</a> <a href="attachment.cgi?id=94907&action=edit" title="[PATCH v17 3/5] glib: Accessors for form fields of structure elements">[details]</a> <a href='page.cgi?id=splinter.html&bug=64821&attachment=94907'>[review]</a> [PATCH v17 3/5] glib: Accessors for form fields of structure elements Review of <a href="attachment.cgi?id=94907" name="attach_94907" title="[PATCH v17 3/5] glib: Accessors for form fields of structure elements">attachment 94907</a> <a href="attachment.cgi?id=94907&action=edit" title="[PATCH v17 3/5] glib: Accessors for form fields of structure elements">[details]</a> <a href='page.cgi?id=splinter.html&bug=64821&attachment=94907'>[review]</a>: ----------------------------------------------------------------- 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.</pre> </div> <hr> You are receiving this mail because: <ul> <li>You are on the CC list for the bug.</li> </ul> </body> </html>