On 6 July 2015 at 12:42, Peter Kelly <pmke...@apache.org> wrote: > Hi Ian, looks decent so far. Just a few comments: > > ODFLenses.h is missing from the CMakeLists.txt file. This won’t affect > compilation at all, but it does mean that it will now show up in the file > list in IDEs like Xcode and Visual Studio. > > I got a number of “control reaches end of non-void function” warnings, > mostly in functions marked TBD, like ODFPut and ODFCreate. I’d recommend > setting a “not implemented” error here using DFErrorFormat, and then > returning 0. > +1, I always get suspicious when I get that warning.
Should we move the code to trunk ? I suggest to do it. rgds jan i. > > There were also some warnings about incorrect use of format strings in > ODFStyles.c. There’s some places here where color macros like CYAN and > RESET are used where in one instance they’re part of the format string > itself, and in others they’re passed as string parameters, like the > following: > > printf(CYAN "Create CSS Properties for %s\n", styleName, RESET); > > This gives me a warning that not all the format parameters are used. > > ODFContainerGet and ODFConainerPut: > > These functions are only really relevant for "container" elements that can > have a variable set of children - that is, where the children can be added, > removed, and rearranged. Where this is not the case, it is possible > (better? i'm not sure) to do this without the container logic. > > For example, <office:body> has exactly one child - either a <office:text>, > <office:spreadsheet>, or <office:presentation>. This determines the type of > document. The code that's currently there calls ODFContainerGet with the > ODFTextLens function table as a parameter, and that in turn calls > ODFTextGet, which calls ODFContainerGet with an ODFTextLevelLens. Finally, > this results in ODFTextLevelGet being called. A more direct approach would > be to simply call ODFTextGet from ODFBodyGet, since we can deduce that's > what will be called in all cases. > > A case where you *would* need to use the generic container functions is > for ODFTextLevelLens (i.e. a containing element which can have multiple > block-level elements such as paragraphs and tables), and similarly for any > element that can contain inline elements, like <text:h>, <text:p>, and > <text:span>. > > On the topic of the latter, this should be handled recursively. Currently, > ODFParagraphContentGet only handlines child nodes that are text nodes. But > if you have a line with multiple, different formatting in it, e.g. bold and > italic, then each of these will be a <text:span>. These can be arbitrarily > nested, unlike in Word, but like in HTML. > > — > Dr Peter M. Kelly > pmke...@apache.org > > PGP key: http://www.kellypmk.net/pgp-key <http://www.kellypmk.net/pgp-key> > (fingerprint 5435 6718 59F0 DD1F BFA0 5E46 2523 BAA1 44AE 2966) > > > On 4 Jul 2015, at 11:40 am, Ian C <i...@amham.net> wrote: > > > > FYI > > > > I just pushed a new branch ODFLenses so the world can see what I am > doing. > > Currently it only works in one direction. ODF -> HTML. > > > > Working on the return path now. > > > > -- > > Cheers, > > > > Ian C > >
