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 _______________________________________________ gnumeric-list mailing list [email protected] http://mail.gnome.org/mailman/listinfo/gnumeric-list
