<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#c8">Comment # 8</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=80732" name="attach_80732" title="[PATCH v4 5/6] Tagged-PDF: Expose the structure tree in poppler-glib">attachment 80732</a> <a href="attachment.cgi?id=80732&action=edit" title="[PATCH v4 5/6] Tagged-PDF: Expose the structure tree in poppler-glib">[details]</a> <a href='page.cgi?id=splinter.html&bug=64821&attachment=80732'>[review]</a> [PATCH v4 5/6] Tagged-PDF: Expose the structure tree in poppler-glib Review of <a href="attachment.cgi?id=80732" name="attach_80732" title="[PATCH v4 5/6] Tagged-PDF: Expose the structure tree in poppler-glib">attachment 80732</a> <a href="attachment.cgi?id=80732&action=edit" title="[PATCH v4 5/6] Tagged-PDF: Expose the structure tree in poppler-glib">[details]</a> <a href='page.cgi?id=splinter.html&bug=64821&attachment=80732'>[review]</a>: ----------------------------------------------------------------- We are very later in the release cycle, and I have a lot of doubts about the API, so I think it'll be better to wait for the next cycle to add this API. We can try to merge the logical structure and tagged pdf support in the core for this cycle though. For contents extracted from the document like the logical structure we usually use an iterator based API, see fonts, index and layer for example. I wonder if we should do the same for document structure, I'm not sure. I would split this patch too. ::: glib/poppler-document.cc @@ +1220,5 @@ > return retval; > } > > +/** > + * poppler_document_get_structure: What about using poppler_document_logical_structure? It's a bit longer but makes clear this returns information about the logical structure of the PDF document. @@ +1224,5 @@ > + * poppler_document_get_structure: > + * @document: A #PopplerDocument > + * > + * Returns the #PopplerStructure of the document. This object is owned by > + * the called. You should explain a briefly here what the document logical structure is @@ +1226,5 @@ > + * > + * Returns the #PopplerStructure of the document. This object is owned by > + * the called. > + * > + * Return value: (transfer full): The #PopplerStructure of the document. a #PopplerStructure object representing the logical structure of the document, or %NULL @@ +1236,5 @@ > + > + g_return_val_if_fail (POPPLER_IS_DOCUMENT (document), NULL); > + > + tree_root = document->doc->getStructTreeRoot (); > + if (!tree_root) return NULL; Use two lines for this please. ::: glib/poppler-private.h @@ +112,5 @@ > + StructElement *elem; > + gchar *id; > + gchar *title; > + gchar *text; > + gchar *text_r; what does r stand for? please don't use abbreviations ::: glib/poppler-structure-element.cc @@ +599,5 @@ > +} > + > + > +/** > + * SECTION:poppler-structure-element Move this to the beginning. @@ +617,5 @@ > +{ > + GObjectClass parent_class; > +}; > + > +G_DEFINE_TYPE (PopplerStructureElement, poppler_structure_element, G_TYPE_OBJECT); Trailing ; is not needed. @@ +628,5 @@ > + > + g_assert (structure); > + g_assert (element); > + > + poppler_structure_element = (PopplerStructureElement *) g_object_new (POPPLER_TYPE_STRUCTURE_ELEMENT, NULL, NULL); What's the second NULL? I you are not providing constructor properties, a single NULL is enough. @@ +631,5 @@ > + > + poppler_structure_element = (PopplerStructureElement *) g_object_new (POPPLER_TYPE_STRUCTURE_ELEMENT, NULL, NULL); > + poppler_structure_element->text = NULL; > + poppler_structure_element->text_r = NULL; > + poppler_structure_element->children = NULL; GObjects are 0 filled when allocated, you can avoid all NULL/0 initializations. @@ +636,5 @@ > + poppler_structure_element->structure = structure; > + poppler_structure_element->elem = element; > + > + if (element->getNumElements ()) > + poppler_structure_element->children = (PopplerStructureElement**) g_new0 (PopplerStructureElement*, GPtrArray could be more convenient to use. You can use g_ptr_array_new_full (element->getNumElements (), g_object_unref); @@ +698,5 @@ > +/** > + * poppler_structure_element_get_page: > + * @poppler_structure_element: A #PopplerStructureElement > + * > + * Return value: Number of the page that contains the element, of of -> or @@ +702,5 @@ > + * Return value: Number of the page that contains the element, of > + * <code>-1</code> if not defined. > + */ > +gint > +poppler_structure_element_get_page (PopplerStructureElement *poppler_structure_element) Use just element or structure_element to make it a bit shorter. @@ +720,5 @@ > +/** > + * poppler_structure_element_is_content: > + * @poppler_structure_element: A #PopplerStructureElement > + * > + * Checks whether an element is actual document content. You should explain here what document comments means here. @@ +737,5 @@ > +/** > + * poppler_structure_element_is_inline: > + * @poppler_structure_element: A #PopplerStructureElement > + * > + * Checks whether an element is an inline element. Ditto, what's an inline element? @@ +754,5 @@ > +/** > + * poppler_structure_element_is_block: > + * @poppler_structure_element: A #PopplerStructureElement > + * > + * Checks whether an element is a block element. Ditto. @@ +771,5 @@ > +/** > + * poppler_structure_element_get_n_children: > + * @poppler_structure_element: A #PopplerStructureElement > + * > + * Gets the number of children of @structure_element. argument is @poppler_structure_element not @structure_element. I'm fine with the shorter name, but use it consistently. @@ +791,5 @@ > + * @index: Index of the children element to obtain. > + * > + * Return value: (transfer none): A #PopplerStructureElement. > + */ > +PopplerStructureElement* PopplerStructureElement * @@ +796,5 @@ > +poppler_structure_element_get_child (PopplerStructureElement *poppler_structure_element, > + guint index) > +{ > + g_return_val_if_fail (POPPLER_IS_STRUCTURE_ELEMENT (poppler_structure_element), NULL); > + g_assert (poppler_structure_element->elem); This should not be needed, it's already checked when the object is created. @@ +797,5 @@ > + guint index) > +{ > + g_return_val_if_fail (POPPLER_IS_STRUCTURE_ELEMENT (poppler_structure_element), NULL); > + g_assert (poppler_structure_element->elem); > + g_assert (poppler_structure_element->elem->getNumElements () >= 0); Use g_return_val_if_fail @@ +816,5 @@ > + * > + * Return value: (transfer none): The identifier of the element (if > + * defined), or %NULL. > + */ > +const gchar* const gchar * @@ +1136,5 @@ > + * @attribute: A #PopperStructureAttribute value. > + * @value (out): A #GValue in which to return the value of the attribute. > + * @inherit: Whether to look up for inheritable attribute values in the > + * ancestors of the element, if the attribute is not defined in the > + * element. Is this something the API user should decided? or are some attributes that are inherited by definition in the spec? @@ +1276,5 @@ > + */ > +GVariant* > +poppler_structure_element_get_attribute (PopplerStructureElement *poppler_structure_element, > + PopplerStructureAttribute attribute, > + gboolean inherit) This kind of API where a single method does everything is not convenient to use in the end, the use always have to deal with a GVariant. I prefer have different methods, get_attribute_int, get_attribute_bool, get_attribute_color, etc. @@ +1450,5 @@ > + * > + * Return value: Whether the element is a reference to another object. > + */ > +gboolean > +poppler_structure_element_is_reference (PopplerStructureElement *poppler_structure_element) Is this really needed? Object reference are something internal to the PDF structure that I'm not sure we want to expose in the API. Maybe we can merge the content and reference type and have a get_content_type() or something similar where content type can be link, annot, form field, contents (not sure contents is the best name to expose marked content, though), etc. @@ +1495,5 @@ > + return reftype; > +} > + > +/** > + * poppler_structure_element_get_reference_link: If I understand this correctly, the struct element corresponds to a link in the document, right? Shouldn't it be poppler_structure_element_get_referenced_link, since the links is referenced by the struct element? @@ +1502,5 @@ > + * Return value: (transfer full): The #PopplerAnnot pointed by the object > + * reference, or %NULL of the element is not a reference pointing to > + * a #PopplerLink. > + */ > +PopplerLinkMapping* Why do we return a mapping? Struct tree elements are supposed to be the document structure independently from where they are physically in the document, no? To get the link mapping we have API for that. There's, however, a bounding box attribute defined for tagged PDFs for illustration elements and tables, but I guess it should be get as an attribute of the struct element. @@ +1584,5 @@ > + * Return value: (transfer full): A #PopplerLinkMapping, or %NULL if the > + * link cannot be found. > + */ > +PopplerLinkMapping* > +poppler_structure_element_find_link (PopplerStructureElement *poppler_structure_element) I think this is something that can be done with poppler_structure_element_get_child and poppler_structure_element_get_reference_link. I would avoid this kind of convenient methods for now, if we see this is commonly duplicated by API users we can always add it later. ::: glib/poppler-structure-element.h @@ +29,5 @@ > +#define POPPLER_STRUCTURE_ELEMENT(obj) (G_TYPE_CHECK_INSTANCE_CAST ((obj), POPPLER_TYPE_STRUCTURE_ELEMENT, PopplerStructureElement)) > +#define POPPLER_IS_STRUCTURE_ELEMENT(obj) (G_TYPE_CHECK_INSTANCE_TYPE ((obj), POPPLER_TYPE_STRUCTURE_ELEMENT)) > + > +/** > + * PopplerStructureElementKind: Use Type instead of Kind for consistency with other enums like PopplerActionType, PopplerDestType, PopplerAnnotType, etc. ::: glib/poppler-structure.cc @@ +131,5 @@ > + > + > + > +/** > + * SECTION:poppler-structure Move this section description to the beginning, after the includes. @@ +164,5 @@ > +{ > + GObjectClass parent_class; > +}; > + > +G_DEFINE_TYPE (PopplerStructure, poppler_structure, G_TYPE_OBJECT); The trailing ; is not needed. @@ +172,5 @@ > +poppler_structure_init (PopplerStructure *poppler_structure) > +{ > +} > + > + Leve only one empty line between functions @@ +200,5 @@ > + gobject_class->finalize = poppler_structure_finalize; > +} > + > + > +PopplerStructure* PopplerStructure * @@ +207,5 @@ > +{ > + PopplerStructure *poppler_structure; > + > + g_return_val_if_fail (POPPLER_IS_DOCUMENT (poppler_document), NULL); > + g_assert (root); why g_return_val_if_fail for poppler_document but g_assert for root? Use g_assert in both cases, since this is not public API method. @@ +212,5 @@ > + > + poppler_structure = (PopplerStructure*) g_object_new (POPPLER_TYPE_STRUCTURE, NULL, NULL); > + > + poppler_structure->document = (PopplerDocument*) g_object_ref (poppler_document); > + poppler_structure->root = root; Do not line up the = @@ +213,5 @@ > + poppler_structure = (PopplerStructure*) g_object_new (POPPLER_TYPE_STRUCTURE, NULL, NULL); > + > + poppler_structure->document = (PopplerDocument*) g_object_ref (poppler_document); > + poppler_structure->root = root; > + poppler_structure->children = NULL; GObjects are alwats 0 filled when allocated, this is already NULL. @@ +217,5 @@ > + poppler_structure->children = NULL; > + > + if (root->getNumElements ()) > + poppler_structure->children = (PopplerStructureElement**) g_new0 (PopplerStructureElement*, > + root->getNumElements ()); A GPtrArray could be more convenient to use. @@ +250,5 @@ > +poppler_structure_get_child (PopplerStructure *poppler_structure, > + guint index) > +{ > + g_return_val_if_fail (POPPLER_IS_STRUCTURE (poppler_structure), NULL); > + g_assert (poppler_structure->root); I don't think this is needed, PopplerStructure object is created by poppler internally with a StructTreeRoot and there's already an assert in _poppler_structure_new to make usre it's created with a valid pointer. @@ +251,5 @@ > + guint index) > +{ > + g_return_val_if_fail (POPPLER_IS_STRUCTURE (poppler_structure), NULL); > + g_assert (poppler_structure->root); > + g_assert (poppler_structure->root->getNumElements () >= 0); Use g_return_val_if_fail here too @@ +258,5 @@ > + if (!poppler_structure->children[index]) > + { > + poppler_structure->children[index] = _poppler_structure_element_new (poppler_structure, > + poppler_structure->root->getElement (index)); > + g_object_ref_sink (poppler_structure->children[index]); Why are you doing this? PopplerStructureElement is a GObject not a GInitiallyUnowned, so it's created with a real reference. You are leaking this element.</pre> </div> <hr> You are receiving this mail because: <ul> <li>You are on the CC list for the bug.</li> </ul> </body> </html>