[Poppler-bugs] [Bug 64815] [TAGGEDPDF] Parse the Tagged-PDF document structure tree when present

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=64815

--- Comment #49 from Carlos Garcia Campos <carlosgc at gnome.org> ---
Comment on attachment 86677
  --> https://bugs.freedesktop.org/attachment.cgi?id=86677
[PATCH v8 04/15] Tagged-PDF: Parsing of StructElem standard types and

Review of attachment 86677:
-----------------------------------------------------------------

::: poppler/Makefile.am
@@ +299,2 @@
>  	StructElement.cc	\
> +	StructTreeRoot.cc	\

This change looks unrelated.

::: poppler/StructElement.cc
@@ +260,5 @@
> +  Object Auto;
> +  Object Start;
> +  Object None;
> +  Object Before;
> +  Object Nat1;

It's weird that struct members start with a capital letter, but I guess it
looks better in the  macros below.

@@ +282,5 @@
> +static const AttributeDefaults attributeDefaults;
> +
> +
> +#define ATTR_LIST_END      { Attribute::Unknown, NULL, NULL, gFalse, NULL }
> +#define ATTR_D(x, i, c, v) { Attribute::x, #x, &attributeDefaults.v, i, c }

What D stand for? Default?

@@ +283,5 @@
> +
> +
> +#define ATTR_LIST_END      { Attribute::Unknown, NULL, NULL, gFalse, NULL }
> +#define ATTR_D(x, i, c, v) { Attribute::x, #x, &attributeDefaults.v, i, c }
> +#define ATTR_N(x, i, c)    { Attribute::x, #x, NULL, i, c }

And N? Normal? NotDefault? What about using ATTR and ATTR_WITH_DEFAULT? Also I
would use words instead of abbreviations to improve the readability type,
inheritable, check, defaultValue.

@@ +288,5 @@
> +
> +static const AttributeMapEntry attributeMapCommonShared[] =
> +{
> +  ATTR_D(Placement,       gFalse, isPlacementName, Inline),
> +  ATTR_D(WritingMode,     gFalse, isWritingModeName, LrTb),

WritingMode is inheritable

@@ +295,5 @@
> +  ATTR_D(BorderStyle,     gFalse, isBorderStyle, None),
> +  ATTR_N(BorderThickness, gTrue,  isPositiveOrOptionalArray4),
> +  ATTR_D(Padding,         gFalse, isPositiveOrArray4, Zero),
> +  ATTR_N(Color,           gTrue,  isRGBColor),
> +  ATTR_LIST_END

Do we really need this being the array statically allocated?

@@ +341,5 @@
> +  ATTR_LIST_END
> +};
> +
> +static const AttributeMapEntry attributeMapCommonList[] = {
> +  ATTR_D(ListNumbering, gFalse, isListNumberingName, None),

This is inheritable

@@ +740,5 @@
> +      formatted->Set(formattedA);
> +    else
> +      formatted = new GooString(formattedA);
> +  } else {
> +    delete formatted;

You should set formatted to NULL here

@@ +744,5 @@
> +    delete formatted;
> +  }
> +}
> +
> +GBool Attribute::typeCheck(StructElement *element)

checkType sounds a bit better to me

@@ +747,5 @@
> +
> +GBool Attribute::typeCheck(StructElement *element)
> +{
> +  // If an element is passed, tighther type-checking can be done.
> +  if (element) {

You could use an early return here, to save an indentation level.

@@ +765,5 @@
> +
> +  return gTrue;
> +}
> +
> +Attribute::Type Attribute::typeForName(const char *name, StructElement *element)

You are using get for all other methods, use getTypeForName here for
consistency.

@@ +936,5 @@
> +{
> +  if (isContent())
> +    return parent->findAttribute(attributeType, inherit, attributeOwner);
> +
> +  if (attributeType != Attribute::Unknown && attributeType != Attribute::UserProperty) {

I would return early here.

@@ +1132,5 @@
> +            for (unsigned j = attrIndex; j < getNumAttributes(); j++)
> +              getAttribute(j)->setRevision(revision);
> +          } else {
> +            error(errSyntaxWarning, -1, "C item is wrong type ({0:s})", iobj.getTypeName());
> +          }

iobj.free()

@@ +1133,5 @@
> +              getAttribute(j)->setRevision(revision);
> +          } else {
> +            error(errSyntaxWarning, -1, "C item is wrong type ({0:s})", iobj.getTypeName());
> +          }
> +        }

attr.free()

::: poppler/StructElement.h
@@ +27,2 @@
>  class StructTreeRoot;
> +class TextWordList;

What do you need this for?

@@ +116,5 @@
> +  static Type typeForName(const char *name, StructElement *element = NULL);
> +  static Attribute *parseUserProperty(Dict *property);
> +
> +  friend class StructElement;
> +};

The Attribute class implementation is huge, you might consider moving it to its
own file

-- 
You are receiving this mail because:
You are on the CC list for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/poppler-bugs/attachments/20131004/d527e294/attachment.html>