<html>
<head>
<base href="https://bugs.freedesktop.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW --- - [TAGGEDPDF] Parse the Tagged-PDF document structure tree when present"
href="https://bugs.freedesktop.org/show_bug.cgi?id=64815#c25">Comment # 25</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW --- - [TAGGEDPDF] Parse the Tagged-PDF document structure tree when present"
href="https://bugs.freedesktop.org/show_bug.cgi?id=64815">bug 64815</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=81010" name="attach_81010" title="[PATCH v5 03/10] Tagged-PDF: Implement parsing of StructElem objects">attachment 81010</a> <a href="attachment.cgi?id=81010&action=edit" title="[PATCH v5 03/10] Tagged-PDF: Implement parsing of StructElem objects">[details]</a></span> <a href='page.cgi?id=splinter.html&bug=64815&attachment=81010'>[review]</a>
[PATCH v5 03/10] Tagged-PDF: Implement parsing of StructElem objects
Review of <span class=""><a href="attachment.cgi?id=81010" name="attach_81010" title="[PATCH v5 03/10] Tagged-PDF: Implement parsing of StructElem objects">attachment 81010</a> <a href="attachment.cgi?id=81010&action=edit" title="[PATCH v5 03/10] Tagged-PDF: Implement parsing of StructElem objects">[details]</a></span> <a href='page.cgi?id=splinter.html&bug=64815&attachment=81010'>[review]</a>:
-----------------------------------------------------------------
Please, try to make sure patches build before submitting them.
::: poppler/StructElement.cc
@@ +82,5 @@
<span class="quote">> + { StructElement::Figure, "Figure", elementTypeUndefined },
> + { StructElement::Formula, "Formula", elementTypeUndefined },
> + { StructElement::Form, "Form", elementTypeUndefined },
> + { StructElement::TOC, "TOC", elementTypeUndefined },
> + { StructElement::TOCI, "TOCI", elementTypeUndefined },</span >
Isn't all this also part of the standard attributes defined for tagged PDFs? If
we leave the tagged pdf support for a different patch, maybe you can merge this
and the StructTreeRoot into a single patch and you don't need to dummy impl of
StructElement in the previous patch.
@@ +237,5 @@
<span class="quote">> +
> +GooString* StructElement::getText(GooString *string, GBool recursive) const
> +{
> + // TODO: Dummy implementation, complete
> + return NULL;</span >
Do we really need this? or can it be added in a follow up patch with the proper
code instead?
@@ +338,5 @@
<span class="quote">> +
> + // Language (optional).
> + if (element->lookup("Lang", &obj)->isString()) {
> + s->language = obj.getString();
> + obj.initNull(); // The StructElement takes ownership of the GooString</span >
Same concern I had in previous review.
::: poppler/StructElement.h
@@ +74,5 @@
<span class="quote">> + const GooString *getID() const { return isContent() ? NULL : s->id; }
> + GooString *getID() { return isContent() ? NULL : s->id; }
> +
> + // Optional ISO language name, e.g. en_US
> + GooString *getLang()</span >
getLanguage() is not that long :-)
@@ +75,5 @@
<span class="quote">> + GooString *getID() { return isContent() ? NULL : s->id; }
> +
> + // Optional ISO language name, e.g. en_US
> + GooString *getLang()
> + { return (!isContent() && s->language) ? s->language : (parent ? parent->getLang() : NULL); }</span >
We normally indent this.
@@ +85,5 @@
<span class="quote">> + void setRevision(Guint revision) { if (isContent()) s->revision = revision; }
> +
> + // Optional element title, in human-readable form.
> + const GooString *getTitle() const { return isContent() ? NULL : s->title; }
> + GooString *getTitle() { return isContent() ? NULL : s->title; }</span >
Do we really need the two versions of every getter? I prefer to only add the
code that is actually used and add more later when needed.
@@ +98,5 @@
<span class="quote">> +
> + void appendElement(StructElement *element)
> + { if (!isContent() && element && element->isOk()) s->elements.push_back(element); }
> +
> +<<<<<<< HEAD</span >
Please, at least try to build the patches before submitting them.
@@ +133,5 @@
<span class="quote">> + //
> + // The text will be appended to the passed GooString. If NULL is passed,
> + // a new string is returned, and the ownership passed to the caller.
> + //
> + GooString* getText(GooString *string = NULL, GBool recursive = gTrue) const;</span >
Don't include this in this first patch if it's unused and unimplemented.</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>