<html>
<head>
<base href="https://bugs.freedesktop.org/" />
</head>
<body>
<p>
<div>
<b><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#c40">Comment # 40</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 <span class="vcard"><a class="email" href="mailto:carlosgc@gnome.org" title="Carlos Garcia Campos <carlosgc@gnome.org>"> <span class="fn">Carlos Garcia Campos</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=89919" name="attach_89919" title="[PATCH v11 07/11] glib: Expose inline attributes of structure elements">attachment 89919</a> <a href="attachment.cgi?id=89919&action=edit" title="[PATCH v11 07/11] glib: Expose inline attributes of structure elements">[details]</a></span> <a href='page.cgi?id=splinter.html&bug=64821&attachment=89919'>[review]</a>
[PATCH v11 07/11] glib: Expose inline attributes of structure elements
Review of <span class=""><a href="attachment.cgi?id=89919" name="attach_89919" title="[PATCH v11 07/11] glib: Expose inline attributes of structure elements">attachment 89919</a> <a href="attachment.cgi?id=89919&action=edit" title="[PATCH v11 07/11] glib: Expose inline attributes of structure elements">[details]</a></span> <a href='page.cgi?id=splinter.html&bug=64821&attachment=89919'>[review]</a>:
-----------------------------------------------------------------
My main concern is that we already have both a way to provide font information
and text attributes, but I think none of them fit in this API. We have
PopplerFontInfo, but it's actually an iterator, and we have
PopplerTextAttributes, that contains the offsets of the text in the page text.
::: glib/poppler-structure-element.cc
@@ +718,5 @@
<span class="quote">> + * <informalexample><programlisting>
> + * GList *text_spans = poppler_structure_element_get_text_spans (element);
> + * /<!-- -->* Use the text spans *<!-- -->/
> + * g_list_free_full (text_spans,
> + * (GDestroyNotify) poppler_text_span_free);</span >
We usually provide a convenient method to do this, see
poppler_page_free_link_mapping, poppler_page_free_text_attributes, etc.
@@ +743,5 @@
<span class="quote">> + {
> + PopplerTextSpan *span = g_slice_new0 (PopplerTextSpan);
> + span->text = _poppler_goo_string_to_utf8 (i->getText ());
> +
> + GfxRGB rgb = i->getColor();</span >
Is this copying the struct? should it return a const reference instead or a
pointer?
@@ +761,5 @@
<span class="quote">> + span->flags |= POPPLER_TEXT_SPAN_SERIF_FONT;
> + if (i->getFont ()->isItalic ())
> + span->flags |= POPPLER_TEXT_SPAN_ITALIC;
> + if (i->getFont ()->isBold ())
> + span->flags |= POPPLER_TEXT_SPAN_BOLD;</span >
Why not doing this in the core too and here simply do i->getFlags()?
@@ +775,5 @@
<span class="quote">> + span->flags |= POPPLER_TEXT_SPAN_BOLD;
> + default:
> + break;
> + }
> + }</span >
I would move this block to a helper function that creates a PopplerTextSpan
from a given TextSpan
@@ +796,5 @@
<span class="quote">> + */
> +PopplerTextSpan *
> +poppler_text_span_copy (PopplerTextSpan *poppler_text_span)
> +{
> + PopplerTextSpan *new_span = g_slice_new0 (PopplerTextSpan);</span >
You can use g_slice_dup and then you only need to manually copy the heap
allocated members.
@@ +938,5 @@
<span class="quote">> + return poppler_text_span->font_name;
> +}
> +
> +/**
> + * poppler_text_span_get_link_target:</span >
What is a link target? a named destination?</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are on the CC list for the bug.</li>
</ul>
</body>
</html>