[Poppler-bugs] [Bug 64821] [TAGGEDPDF] Expose the structure tree and attributes in poppler-glib

[bugzilla-daemon at freedesktop.org]

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

--- Comment #58 from Carlos Garcia Campos <carlosgc at gnome.org> ---
Comment on attachment 94018
  --> https://bugs.freedesktop.org/attachment.cgi?id=94018
[PATCH v16 8/9] glib: Implement accessors for element attributes

Review of attachment 94018:
-----------------------------------------------------------------

I have a lot of doubts about this API.

::: glib/poppler-structure-element.cc
@@ +212,5 @@
> +  const gchar *name;
> +  EnumType     value;
> +
> +  static const EnumNameValue<EnumType> values[];
> +  static const EnumType null = static_cast<EnumType> (-1);

I don't understand this null value, all attributed have a default value, so we
should always return the default value when the attribute is not present.

@@ +371,5 @@
> +static EnumType
> +name_to_enum (Object  *name_value,
> +              EnumType default_value = EnumNameValue<EnumType>::null)
> +{
> +  if (!name_value)

Can this really be called with NULL name_value?

@@ +378,5 @@
> +  for (const EnumNameValue<EnumType> *item = EnumNameValue<EnumType>::values ; item->name; item++)
> +    if (name_value->isName (item->name))
> +      return item->value;
> +
> +  return default_value;

Can this happen? in the frontend we assume that invalid attributes are ignored
by the core.

@@ +386,5 @@
> +template <typename EnumType>
> +static bool
> +name_to_enum (Object *name_value, EnumType& result)
> +{
> +  return (result = name_to_enum<EnumType> (name_value)) != EnumNameValue<EnumType>::null;

name_to_enum should never return null I think

@@ +410,5 @@
> +              g_variant_builder_add (&builder, "d", item.getNum ());
> +            }
> +          else
> +            {
> +              /* Free the resources in-use by the builder */

Aren't these values already checked in the core?

@@ +450,5 @@
> +              g_free (utf8_string);
> +            }
> +          else
> +            {
> +              /* Free the resources in-use by the builder */

Ditto.

@@ +468,5 @@
> +{
> +  Object *value = Attribute::getDefaultValue (attribute_type);
> +  const Attribute *attr;
> +
> +  if ((attr = poppler_structure_element->elem->findAttribute (attribute_type, inherit)))

Are we using inherit with any attribute? Shouldn't findAttribute use the
inherit internally only for attributes that are inheritable?

@@ +469,5 @@
> +  Object *value = Attribute::getDefaultValue (attribute_type);
> +  const Attribute *attr;
> +
> +  if ((attr = poppler_structure_element->elem->findAttribute (attribute_type, inherit)))
> +    value = attr->getValue ();

Here we could return attr->getValue (); directly, and call
Attribute::getDefaultValue (attribute_type) after this when the attr was not
found.

@@ +485,5 @@
> +
> +  EnumType result = name_to_enum<EnumType> ((attr == NULL)
> +                                            ? Attribute::getDefaultValue (attribute_type)
> +                                            : attr->getValue ());
> +  if (result_ptr != NULL)

Can this be called with a NULL result_ptr?

@@ +488,5 @@
> +                                            : attr->getValue ());
> +  if (result_ptr != NULL)
> +    *result_ptr = result;
> +
> +  return (result == EnumNameValue<EnumType>::null);

How can this be null if we are using the default value when the attribute is
not present?

@@ +1192,5 @@
> + */
> +gboolean
> +poppler_structure_element_get_placement (PopplerStructureElement    *poppler_structure_element,
> +                                         PopplerStructurePlacement *value,
> +                                         gboolean                   inherit)

hmm, I don't understand this generic way of getting the attributes. Since, all
attributes have a default value, we should always return the default value when
the attribute is not present. If we really need API to let the user know
whether an attribute is explicitly present we could add has_foo() methods
instead. Also, not all attributes are inheritable, why do we allow the user
decide? I think that inheritable attributes should be always looked up  in
their ancestors automatically, it should be transparent for the user. We could
add in the documentation what the default value is in case of the attr is not
found and which once are inheriable. So, I think this method should be
something like:

PopplerStructurePlacement 
poppler_structure_element_get_placement (PopplerStructureElement   
*poppler_structure_element);

It's not inheritable, and when not present inline will be returned. And the
same approach for all other methods.

@@ +1255,5 @@
> +                                                       POPPLER_STRUCTURE_BORDER_STYLE_NONE);
> +        }
> +    }
> +  else
> +    return FALSE;

This should never fail, invalid objects should be ignored by the core and never
passed to the forntends.

@@ +1286,5 @@
> +      if (value != NULL)
> +        value[0] = value[1] = value[2] = value[3] = object->getNum ();
> +    }
> +  else
> +    return FALSE;

Ditto.

@@ +1304,5 @@
> +        *value = object->getNum ();
> +      return TRUE;
> +    }
> +  else
> +    return FALSE;

Ditto.

@@ +1311,5 @@
> +static inline gboolean
> +convert_doubles_array (Object *object, gdouble **values, guint *n_values)
> +{
> +  if (!object->isArray ())
> +    return FALSE;

Ditto.

@@ +1353,5 @@
> +        *value = (guint) object->getNum ();
> +      return TRUE;
> +    }
> +  else
> +    return FALSE;

Ditto.

@@ +1360,5 @@
> +static gboolean
> +convert_color (Object *object, PopplerColor *color)
> +{
> +  if (!object->isArray () || object->arrayGetLength () != 3)
> +    return FALSE;

Ditto, and same in all  other cases.

@@ +2885,5 @@
> + *    on the attribute requested, as specified in the table. If the
> + *    attribute is not defined, <code>NULL</code> is returned.
> + */
> +GVariant *
> +poppler_structure_element_get_attribute (PopplerStructureElement  *poppler_structure_element,

Since we have convenient methods to get every property, I would not add this
generic method for now.

::: glib/poppler-structure-element.h
@@ +85,5 @@
>  /**
> + * PopplerStructureAttribute:
> + */
> +typedef enum {
> +  POPPLER_STRUCTURE_ATTRIBUTE_UNKNOWN,

Do we use this?

-- 
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/20140223/8ce4a992/attachment.html>