<html> <head> <base href="https://bugs.freedesktop.org/" /> </head> <body> <div> <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#c58">Comment # 58</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 <a class="email" href="mailto:carlosgc@gnome.org" title="Carlos Garcia Campos <carlosgc@gnome.org>"> Carlos Garcia Campos</a> <pre>Comment on <a href="attachment.cgi?id=94018" name="attach_94018" title="[PATCH v16 8/9] glib: Implement accessors for element attributes">attachment 94018</a> <a href="attachment.cgi?id=94018&action=edit" title="[PATCH v16 8/9] glib: Implement accessors for element attributes">[details]</a> <a href='page.cgi?id=splinter.html&bug=64821&attachment=94018'>[review]</a> [PATCH v16 8/9] glib: Implement accessors for element attributes Review of <a href="attachment.cgi?id=94018" name="attach_94018" title="[PATCH v16 8/9] glib: Implement accessors for element attributes">attachment 94018</a> <a href="attachment.cgi?id=94018&action=edit" title="[PATCH v16 8/9] glib: Implement accessors for element attributes">[details]</a> <a href='page.cgi?id=splinter.html&bug=64821&attachment=94018'>[review]</a>: ----------------------------------------------------------------- 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?</pre> </div> <hr> You are receiving this mail because: <ul> <li>You are on the CC list for the bug.</li> </ul> </body> </html>