Re: [libgsf] property write support

[Manuel Mausz <manuel mausz fabasoft com>]

Hi Jody,

sorry for the late reply but I'm very busy at the moment. I'll work on
your improvements as soon as I have a few minutes (hopefully next week).
Anyway, here are my comments.

> Please merge these two routines into
> something that does the lookup and returns a
>       GsfMSOleMetaDataPropMap const *
> Then add convenience wrappers if necessary.
Ok :)

> I don't see this used in your patch.  Why do we need it ?
This was used to assign types like boolean to 32bit (like msdn says).
However it works without this alignment, so I'll will delete this
routine.

> 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.
Already done on my computer.

> It seems simpler to map the name to id, then compare those rather
> than this.
Will do this as soon i have merged the two routines.

> Beware of non-C89 variable declarations,  some compilers are not as
> permissive as gcc.
Renamed to props_vector.

> 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.
Renamed to props_count.

> To ease readability please merge the VT_I4 and default cases.
-----
> Can we get this into #define
-----
> I'd rather see these with fixed size of 4 to correspond to the on
> disk format rather than tying it to the compiler.
Already done.

> Seems like we need to use the 'doc_not_component' kludge here to
> filter which of the properties to export.
The doc_not_component is used to to write the correct section format
identifier.
My code above splits the "known"/default properties (which are declared
in document_props, component_props and common_props) and the user
defined properties. This is needed to calculate the offsets before
writing and generating the dictionary.

> Why are we duplicating all this code ?
It's not really duplicated code. The first section contains the "known"
properties, the second section the user defined properties and the
dictionary. Thus there are different data and offsets.
Perhaps I can merge some code but I don't think this will improve the
readability.

> 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 ?
The stream will only be truncated if gsf_output_write() fails, unknown
properties will be (hopefully) left out. Thus I think stop writing and
returning an error is a good idea.

Regards,
Manuel

On Fri, 2005-04-08 at 05:23, Jody Goldberg wrote:
> On Tue, Mar 29, 2005 at 11:57:29PM -0500, Jody Goldberg wrote:
> > 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.
>
> Thank you for the contribution.  How would you prefer to proceed from here ?
> As outlined there are a few improvements I'd like to discuss before
> we can get this code into production.  Will you have the resources
> to make them ?
>
> If it's simpler for you I can be reached by voice
> (905) 707-6579 from 9-8 (UTC-5)
>
> Once again, thanks
>     Jody