Re: [libgsf] property write support

[Jody Goldberg <jody gnome org>]

On Tue, Mar 29, 2005 at 10:42:55AM +0200, Karl Pitrich wrote:
> Hi Jody,
>
> we've added property-write support to libgsf and are contributing it
> under LGPL. We're hope that you review the changes and commit them to
> CVS. Please find a patch against last thursday's CVS-HEAD in the
> attached message.

Lovely.  I've been hoping someone would finish this off.
 
> The author of the patch is Manuel Mausz, thus please contact him
> directly for questions regarding the code.
The patch is nice work.  I've got some suggestions to improve the
maintainability.  Welcome to the fun of open-source and code-review.

> +msole_gsf_name_to_prop_id (char const *name)
> +{
> +     GsfMSOleMetaDataPropMap const *map = NULL;
> +     unsigned i = 0;
> +
> +     map = component_props;
> +     i = G_N_ELEMENTS (component_props);
> +     while (i-- > 0) {
> +             if (!strncmp(map[i].gsf_name, name, strlen(map[i].gsf_name)))
> +                     return map[i].id;
> +     }
> +
> +     map = document_props;
> +     i = G_N_ELEMENTS (document_props);
> +     while (i-- > 0) {
> +             if (!strncmp(map[i].gsf_name, name, strlen(map[i].gsf_name)))
> +                     return map[i].id;
> +     }
> +
> +     map = common_props;
> +     i = G_N_ELEMENTS (common_props);
> +     while (i-- > 0) {
> +             if (!strncmp(map[i].gsf_name, name, strlen(map[i].gsf_name)))
> +                     return map[i].id;
> +     }
This is wasteful.  Please create some hash tables.

> +static GsfMSOleVariantType
> +msole_gsf_name_to_prop_type (char const *name)
and this is duplicate code.  Please merge these two routines into
something that does the lookup and returns a
        GsfMSOleMetaDataPropMap const *
Then add convenience wrappers if necessary.

> +static guint32
> +gsf_align_to_32bit(gsf_off_t offset) {
I don't see this used in your patch.  Why do we need it ?
  
> +static void
> +add_props(gpointer name, gpointer value, gpointer user_data)
We use a naming convention of
    cb_<foo>
for callbacks.  It's also preferable to use the right types here in
the decl, and cast the function pointer down below in the foreach.

> +{
> +     if (memcmp(name, GSF_META_NAME_LANGUAGE, sizeof(GSF_META_NAME_LANGUAGE)) &&
> +         memcmp(name, GSF_META_NAME_DICTIONARY, sizeof(GSF_META_NAME_DICTIONARY))) {
It seems simpler to map the name to id, then compare those rather
than this.

> +
> +             gboolean error = FALSE;
> +             guint32 it = ((AddPropsStruct *)user_data)->it;
> +             GsfMSOleMetaDataProp_real *props = NULL;
> +             props = &((AddPropsStruct *)user_data)->props[it];
See comment re: function argument types above.

> +             gsf_off_t offset;
> +             GValue *tmp = NULL;
> +
> +             GsfDocPropVector *vector = NULL;
> +             guint vector_num_values = 1;
> +             guint i;
Beware of non-C89 variable declarations,  some compilers are not as
permissive as gcc.

> +             for (i=0; i < vector_num_values; i++) {
The majority case here will be non-vector.  Could we call this
something other than vector_num_values ?  That makes this code seem
vector specific.

> +                                     offset += 1;
> +                                     break;
> +
> +                             case G_TYPE_INT:
> +                                     switch(msole_gsf_name_to_prop_type((char *)name)) {
> +                                             case VT_I2:
> +                                                     props->type = VT_I2;
> +                                                     offset += 2;
> +                                                     break;
> +
> +                                             case VT_I4:
> +                                                     props->type = VT_I4;
> +                                                     offset += 4;
> +                                                     break;
To ease readability please merge the VT_I4 and default cases.
> +                             case G_TYPE_UINT:
> +                                     switch(msole_gsf_name_to_prop_type((char *)name)) {
As above.

> +                     case VT_FILETIME:
> +                             timestamp = (GsfTimestamp const *)g_value_get_boxed((GValue *)prop->value);
> +                             timet_value = (time_t)timestamp->timet;
> +
> +#ifdef _MSC_VER
> +                             timet_value += 11644473600i64;
> +#else
> +                             timet_value += 11644473600ULL;
> +#endif
Can we get this into #define

> +gboolean
> +gsf_msole_metadata_write_real (GsfOutput *out, GsfDocMetaData *meta_data,
> +     gboolean doc_not_component, GError **err)
> +{
> +     gboolean success = TRUE;
> +     gsf_off_t offset = 0;
> +     guint8 header[] = {
        static guint8 const header[] = {

> +     guint32 byte_count = 0;    /* count of bytes in a section */
> +     guint32 props_count = 0;   /* count of propertys global */

> +     section_offset += sizeof(byte_count);  /* size of section byte count */
> +     section_offset += sizeof(props_count); /* size of property count */
I'd rather see these with fixed size of 4 to correspond to the on
disk format rather than tying it to the compiler.

> +     if (success) {
> +             props_struct = g_new (AddPropsStruct, 1);
> +             props_struct->props = props;
> +             props_struct->udef_props = udef_props;
> +             props_struct->it = 0;
> +             props_struct->count = 0;
> +             props_struct->dict_count = 0;
> +             props_struct->offset = section_offset;
> +             props_struct->dict_offset = section_offset;
> +             gsf_doc_meta_data_foreach(meta_data, add_props, props_struct);
> +     }
Seems like we need to use the 'doc_not_component' kludge here to
filter which of the properties to export.

> +      */
> +     if (success) {
> +             if (prop_codepage != NULL) {
> +                     // TODO: GSF_META_NAME_LANGUAGE = VT_UI2 - msdn says VT_I2
Hmm.  The largest values I've seen for this are 10,000 we can't
prove it either way.  Let's trust msdn.

> +                     //GSF_LE_SET_GUINT32 (buf+0, msole_gsf_name_to_prop_type(GSF_META_NAME_LANGUAGE);
Watch out for C++ style comments.

> +     /*
> +      * Write 2. Section
> +      */
Why are we duplicating all this code ?

> +     if (prop_codepage != NULL)
> +             g_free(prop_codepage);
g_free tests for NULL, no need to add wrapper tests.

> +     return success;
I worry about situations where things partially fail and the
resulting output is broken.   Do you think it's feasible to get a
wrapper to this that would truncate the stream on failure ?