<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#c52">Comment # 52</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=90264" name="attach_90264" title="[PATCH v12 07/11] glib: Expose inline attributes of structure elements">attachment 90264</a> <a href="attachment.cgi?id=90264&action=edit" title="[PATCH v12 07/11] glib: Expose inline attributes of structure elements">[details]</a> <a href='page.cgi?id=splinter.html&bug=64821&attachment=90264'>[review]</a> [PATCH v12 07/11] glib: Expose inline attributes of structure elements Review of <a href="attachment.cgi?id=90264" name="attach_90264" title="[PATCH v12 07/11] glib: Expose inline attributes of structure elements">attachment 90264</a> <a href="attachment.cgi?id=90264&action=edit" title="[PATCH v12 07/11] glib: Expose inline attributes of structure elements">[details]</a> <a href='page.cgi?id=splinter.html&bug=64821&attachment=90264'>[review]</a>: ----------------------------------------------------------------- ::: glib/poppler-structure-element.cc @@ +705,5 @@ > } > + > + > +static PopplerTextSpan * > +_poppler_convert_text_span (const TextSpan& span) This could be called text_span_to_poppler_text_span for consistency with other similar methods. Do not use _ prefix for private static methods. @@ +708,5 @@ > +static PopplerTextSpan * > +_poppler_convert_text_span (const TextSpan& span) > +{ > + PopplerTextSpan *new_span = g_slice_new0 (PopplerTextSpan); > + new_span->text = _poppler_goo_string_to_utf8 (span.getText ()); We should check that the returned text is not NULL. @@ +766,5 @@ > + * guint n_spans; > + * PopplerTextSpan **text_spans = > + * poppler_structure_element_get_text_spans (element, &n_spans); > + * /* Use the text spans */ > + * poppler_text_spans_free (text_spans, n_spans); This is not correct, the free method doesn't return the size of the array. @@ +769,5 @@ > + * /* Use the text spans */ > + * poppler_text_spans_free (text_spans, n_spans); > + * </programlisting></informalexample> > + * > + * Return value: (transfer full) (array length=n_text_spans): You should also add the element type annot so that bindings know how to free the contents. @@ +770,5 @@ > + * poppler_text_spans_free (text_spans, n_spans); > + * </programlisting></informalexample> > + * > + * Return value: (transfer full) (array length=n_text_spans): > + * Null-terminated array of #PopplerTextSpan structures. I think we should either make the array null terminated and not return the size or return the size making the out param mandatory and not adding the extra null element @@ +779,5 @@ > +poppler_structure_element_get_text_spans (PopplerStructureElement *poppler_structure_element, > + guint *n_text_spans) > +{ > + g_return_val_if_fail (POPPLER_IS_STRUCTURE_ELEMENT (poppler_structure_element), 0); > + g_assert (poppler_structure_element->elem); Use g_return macros in public methods. @@ +805,5 @@ > + * Frees the memory used by an array of #PopplerTextSpan elements, typically > + * obtained using poppler_structure_element_get_text_spans(). > + */ > +void > +poppler_structure_element_free_text_spans (PopplerTextSpan **poppler_text_spans) Previous patch returned a GList for which a convenient free method makes more sense. I think case, if we return the size of the array, I think we can avoid this free method and let the user do it. @@ +828,5 @@ > + * Since: 0.26 > + */ > +PopplerTextSpan * > +poppler_text_span_copy (PopplerTextSpan *poppler_text_span) > +{ Use g_return macros in all public methods @@ +840,5 @@ > +/** > + * poppler_text_span_free: > + * @poppler_text_span: A #PopplerTextSpan > + * > + * Freed a text span. Frees @@ +911,5 @@ > + * > + * Since: 0.26 > + */ > +PopplerColor * > +poppler_text_span_get_color (PopplerTextSpan *poppler_text_span) I think it would be better to return the color as an out parameter ::: glib/poppler-structure-element.h @@ +82,4 @@ > POPPLER_STRUCTURE_ELEMENT_FORM, > } PopplerStructureElementKind; > > +typedef struct _PopplerTextSpan PopplerTextSpan; This should be in poppler.h with all other forward declarations. @@ +112,4 @@ > gboolean poppler_structure_element_iter_next (PopplerStructureElementIter *iter); > void poppler_structure_element_iter_free (PopplerStructureElementIter *iter); > > +PopplerTextSpan *poppler_text_span_copy (PopplerTextSpan *poppler_text_span); You should expose here the get_type method too ::: glib/reference/poppler-sections.txt @@ +600,1 @@ > poppler_structure_element_iter_get_type poppler_text_span_get_type is missing here.</pre> </div> <hr> You are receiving this mail because: <ul> <li>You are on the CC list for the bug.</li> </ul> </body> </html>