<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#c24">Comment # 24</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=81009" name="attach_81009" title="[PATCH v5 02/10] Tagged-PDF: Implement parsing of StructTreeRoot">attachment 81009</a> <a href="attachment.cgi?id=81009&action=edit" title="[PATCH v5 02/10] Tagged-PDF: Implement parsing of StructTreeRoot">[details]</a></span> <a href='page.cgi?id=splinter.html&bug=64815&attachment=81009'>[review]</a>
[PATCH v5 02/10] Tagged-PDF: Implement parsing of StructTreeRoot
Review of <span class=""><a href="attachment.cgi?id=81009" name="attach_81009" title="[PATCH v5 02/10] Tagged-PDF: Implement parsing of StructTreeRoot">attachment 81009</a> <a href="attachment.cgi?id=81009&action=edit" title="[PATCH v5 02/10] Tagged-PDF: Implement parsing of StructTreeRoot">[details]</a></span> <a href='page.cgi?id=splinter.html&bug=64815&attachment=81009'>[review]</a>:
-----------------------------------------------------------------
Looks good in general, but you should make sure it doesn't break the build. I
have a few other comments.
::: poppler/Catalog.cc
@@ +854,5 @@
<span class="quote">> + return NULL;
> + }
> +
> + if (catalog.dictLookup("StructTreeRoot", &root)->isDict("StructTreeRoot")) {
> + structTreeRoot = new StructTreeRoot(doc, root.getDict(), getMarkInfo() & markInfoMarked);</span >
What should we do in case of suspects? I don't like bool parameters, maybe we
could just pass the mark info flags and let the StructTreeRoot decide what to
do depending on the flags. You don't even need to pass this as a parameter,
since the StructTreeRoot keeps a pointer to the PDFDoc, you can get the mark
inflo flags from the doc directly:
doc->getCatalog()->getMarkInfo()
::: poppler/Catalog.h
@@ +124,4 @@
<span class="quote">> GooString *readMetadata();
>
> // Return the structure tree root object.
> + StructTreeRoot *getStructTreeRoot();</span >
As I said, this breaks the build. It's very important that every patch builds,
since it might affect any future bisection, for example.
::: poppler/StructTreeRoot.cc
@@ +110,5 @@
<span class="quote">> + // Parse the children StructElements
> + Object kids;
> + if (root->lookup("K", &kids)->isArray()) {
> + if (marked && kids.arrayGetLength() > 1) {
> + error(errSyntaxWarning, -1, "K in StructTreeRoot has more than one children in a tagged PDF");</span >
I would leave this for patches to add tagged pdf support, adding first just the
generic logical structure parsing.
@@ +118,5 @@
<span class="quote">> + kids.arrayGetNF(i, &ref);
> + if (kids.arrayGet(i, &obj)->isDict()) {
> + StructElement *child = new StructElement(obj.getDict(), this);
> + if (child->isOk()) {
> + if (marked && !(child->getType() == StructElement::Document ||</span >
Ditto.
@@ +125,5 @@
<span class="quote">> + child->getType() == StructElement::Div)) {
> + error(errSyntaxWarning, -1, "StructTreeRoot element of tagged PDF is wrong type ({0:s})", child->getTypeName());
> + }
> + appendElement(child);
> + if (ref.isRef()) parentTreeAdd(ref.getRef(), child);</span >
Move the if body to its own line, please.
::: poppler/StructTreeRoot.h
@@ +36,5 @@
<span class="quote">> + unsigned getNumElements() const { return elements.size(); }
> + const StructElement *getElement(int i) const { return elements.at(i); }
> + StructElement *getElement(int i) { return elements.at(i); }
> + void appendElement(StructElement *element)
> + { if (element && element->isOk()) elements.push_back(element); }</span >
We normally indent the body when it's in a new line.
@@ +39,5 @@
<span class="quote">> + void appendElement(StructElement *element)
> + { if (element && element->isOk()) elements.push_back(element); }
> +
> + const StructElement *findParentElement(unsigned index) const
> + { return (index < parentTree.size() && parentTree[index].size() == 1) ? parentTree[index][0].element : NULL; }</span >
Ditto. I wonder if this would be easier to read in the .cc file, though.</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>