<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#c60">Comment # 60</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:aperez@igalia.com" title="Adrian Perez de Castro <aperez@igalia.com>"> Adrian Perez de Castro</a> <pre>(In reply to <a href="show_bug.cgi?id=64821#c58">comment #58</a>) > 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> [review] > [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> [review]: > ----------------------------------------------------------------- > > 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. Only some attributes have default values (check the attribute maps in poppler/StructElement.cc, line 302). The value EnumType<T>::null is returned during attribute lookup if: - The attribute does not have a default value, AND - the attribute is not defined, AND - the value for the attribute is not inherited from a parent element. BTW, in the attributes map the items with a default value are those that should have one according to the PDF spec. > @@ +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? Yes, for example in line 501 there is: EnumType result = name_to_enum<EnumType> ((attr == NULL) ? Attribute::getDefaultValue (attribute_type) : attr->getValue ()); In this particular case, “NULL” is passed as “name_value” to “name_to_enum()” when the attribute is not defined (“attr == NULL”) for a structure element, and “Attribute::getDefault()” returns “NULL” (that is, the attribute does not have a default 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. You are right with this one: if “name_value” is not “NULL”, then it must be one of the values of the enum because the invalid attributes are indeed discarded by the core. I am adding “g_assert_not_reached()” before the “return” statement. Unfortunately, I have to keep the “return” to make the compiler happy and avoid it to complain about the function ending without returning qa value. > @@ +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 For the reason mentioned above: the “null” value is returned when the attribute is not defined and it has no default value. Checking for “null” is what makes it possible for the API methods to return “FALSE” when the attribute lookup failed and there is no default. Funny thing: I never got to use this overloaded “name_to_enum()” version, so I am going to remove it — the actual check is done in “attr_to_enum()”. > @@ +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? Correct. This check is redundant, I will remove it. > @@ +450,5 @@ > > + g_free (utf8_string); > > + } > > + else > > + { > > + /* Free the resources in-use by the builder */ > > Ditto. Re: 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? Yes, it makes sense to hide the fact that some attributes can be inherited in the frontend. If some use-case arises in the future that needs to know exactly where an attribute is defined, we can add something like poppler_structure_element_has_attribute() later on. > @@ +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. You are right, it is better to avoid doing the call to “Attribute::getDefault()” when the attribute has a value. I think it will look nice like this: const Attribute *attr = poppler_structure_element->elem->findAttribute (attribute_type, inherit); return (attr != NULL) ? attr->getValue () : Attribute::getDefaultValue (attribute_type); > @@ +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? Nope. It is possible, but I remember that we talked at some point on changing the API documentation and GI-annotations to disallow passing “NULL”. I am going to remove this check here, and in methods that use the function I will be adding non-NULL checks with “g_return_val_if_fail()”. > @@ +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); As discussed today in IRC, the API is going to be simplified so the frontend returns reasonable default values for attributes which do not have a default, or they will signal the non-existence of the attribute by other means (like for example returning a NULL pointer and so on.) > 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. True. > @@ +1286,5 @@ > > + if (value != NULL) > > + value[0] = value[1] = value[2] = value[3] = object->getNum (); > > + } > > + else > > + return FALSE; > > Ditto. Yep. > @@ +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. Certainly.</pre> </div> <hr> You are receiving this mail because: <ul> <li>You are on the CC list for the bug.</li> </ul> </body> </html>