On 6 July 2015 at 15:04, Ian C <i...@amham.net> wrote: > Thanks for the feedback. > > Re warnings, I don't see any in the ODF code, I get a couple in minizip. > minizip is third party code that I work on replacing (when I dont work on apacheCON).
> > Am I missing some compiler/make switches I wonder. > > My compiler is gcc version 4.9.2 20150212 (Red Hat 4.9.2-6) (GCC) > > If I'd seen the warnings I would have addressed them. > If peter do not beat me to it, I will copy you the list I have in visual studio. rgds jan i. > > On Mon, Jul 6, 2015 at 7:42 PM, jan i <j...@apache.org> wrote: > > 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 > >> > >> > > > > -- > Cheers, > > Ian C >
