<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#c68">Comment # 68</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=94908" name="attach_94908" title="[PATCH v17 4/5] glib: Accessors for document structure references">attachment 94908</a> <a href="attachment.cgi?id=94908&action=edit" title="[PATCH v17 4/5] glib: Accessors for document structure references">[details]</a> <a href='page.cgi?id=splinter.html&bug=64821&attachment=94908'>[review]</a> [PATCH v17 4/5] glib: Accessors for document structure references Review of <a href="attachment.cgi?id=94908" name="attach_94908" title="[PATCH v17 4/5] glib: Accessors for document structure references">attachment 94908</a> <a href="attachment.cgi?id=94908&action=edit" title="[PATCH v17 4/5] glib: Accessors for document structure references">[details]</a> <a href='page.cgi?id=splinter.html&bug=64821&attachment=94908'>[review]</a>: ----------------------------------------------------------------- ::: glib/poppler-structure-element.cc @@ +931,5 @@ > + * > + * Return value: Whether the element is a reference to another object. > + */ > +gboolean > +poppler_structure_element_is_reference (PopplerStructureElement *poppler_structure_element) I would avoid this generic api, since we don't have a way to get the referenced object when it's not link, annot or form field @@ +947,5 @@ > + * Return value: The type of object pointed to by the reference, a value of > + * #PopplerStructureReference. > + */ > +PopplerStructureReference > +poppler_structure_element_get_reference_type (PopplerStructureElement *poppler_structure_element) Same here, we should use the referenced object only for structure elements that can contain children that are object references, but knowing what objects we are looking for. @@ +1002,5 @@ > + return NULL; > +} > + > +/** > + * poppler_structure_element_get_reference_link: The method name is link_action. I prefer to name this just link, though as this is what we do already in our api, poppler_get_link_mapping actually returns a list of LinkActions @@ +1015,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); > + > + AnnotLink *link = _poppler_structure_element_find_annot_link (poppler_structure_element); Maybe I'm missing something from the spec, but why do we have to find the link? Shouldn't the link structure element reference a link annotation object? @@ +1028,5 @@ > + * object, or %NULL if the element is not a reference pointing to > + * a link. > + */ > +PopplerLinkMapping * > +poppler_structure_element_get_reference_link_mapping (PopplerStructureElement *poppler_structure_element) I have the same concerns about this API as with the form field mapping ::: glib/poppler-structure-element.h @@ +92,5 @@ > +typedef enum { > + POPPLER_STRUCTURE_REFERENCE_UNKNOWN, > + POPPLER_STRUCTURE_REFERENCE_ANNOT, > + POPPLER_STRUCTURE_REFERENCE_LINK, > +} PopplerStructureReference; Why are we handling this differently than form fields? Annot and Link are also element types, so the same way I can get a form field for a FORM element, I should be able to get the link or annot for a LINK or ANNOT element, without any special api for references. @@ +95,5 @@ > + POPPLER_STRUCTURE_REFERENCE_LINK, > +} PopplerStructureReference; > + > + > +typedef struct _PopplerTextSpan PopplerTextSpan; This looks unrelated to this patch.</pre> </div> <hr> You are receiving this mail because: <ul> <li>You are on the CC list for the bug.</li> </ul> </body> </html>