Review Request 111773: Add api for writing ODF that is generated from the ODF RNG file

[Friedrich W. H. Kossebau]

Am Sonntag, 4. August 2013, 18:48:17 schrieb Jos van den Oever:
> > On Aug. 4, 2013, 5:57 p.m., Friedrich W. H. Kossebau wrote:
> > > Meh, too late, committed while I was giving it finally also a look.
> 
> > > Please still pick up the following comments and do a fix-up commit:
> Thanks for the thorough look. You caught one bug and quite some style
> issues.
> 
> wrt to the product remark, you seem more knowledgable on the subject and i
> dont want to learn more cmake folklore.
> 
> Pushing as a fixup.
> 
> > On Aug. 4, 2013, 5:57 p.m., Friedrich W. H. Kossebau wrote:
> > > devtools/rng2cpp/rng2cpp.cpp, line 959
> > > <http://git.reviewboard.kde.org/r/111773/diff/5/?file=176045#file176045l
> > > ine959>> > 
> > >     isEmpty()
> 
> A constant may be "".
> 
> > On Aug. 4, 2013, 5:57 p.m., Friedrich W. H. Kossebau wrote:
> > > devtools/rng2cpp/rng2cpp.cpp, line 962
> > > <http://git.reviewboard.kde.org/r/111773/diff/5/?file=176045#file176045l
> > > ine962>> > 
> > >     better isEmpty() than isNull(), unless that difference is important
> > >     here
> 
> why is it better?

Depends on the usecase of course. Normally an empty string has the same 
meaning like a null string. But people might not be aware and just pass a "", 
which will fail the test on isNull(). So isEmpty() catches all of that.

But if here isNull() (as in: no string at all) has a separate meaning, because 
a "" is valid option, than of course this "is better" does not apply at all.
Was not obvious to me here that "" is a valid value :)

> > On Aug. 4, 2013, 5:57 p.m., Friedrich W. H. Kossebau wrote:
> > > devtools/CMakeLists.txt, line 10
> > > <http://git.reviewboard.kde.org/r/111773/diff/5/?file=176043#file176043l
> > > ine10>> > 
> > >     Please turn rng2cpp into a product.
> > >     Means you add a product definition for this tool and wrap the call
> > >     add_subdirectory(rng2cpp) into the proper conditions.
> If you think this is important, you can do so, but I do not see the benefit.
> The dependencies are set correctly.

These are not dependencies as in make terms, but in "what to enable for build" 
terms. So if somebody just wants to build Krita & Words, it is possible to 
just name those two in that custom productset, and still all internal required 
dependencies and nice-to-have addons are automatically added to be build as 
well, but not more.

Cared for that, okay.

> > On Aug. 4, 2013, 5:57 p.m., Friedrich W. H. Kossebau wrote:
> > > libs/odf/writeodf/odfwriter.h, lines 67-70
> > > <http://git.reviewboard.kde.org/r/111773/diff/5/?file=176058#file176058l
> > > ine67>> > 
> > >     This method can result in data-loss, no?
> > >     
> > >     Passing quint64 value to QString::number(long) will not work on
> > >     32-bit systems, as long is only 32bit there. And possibly on Win64
> > >     as well IIRC. (Also long is signed).
> > >     
> > >     So the API of this method promises more then it can.
> > >     
> > >     Please fix!
> 
> on 32-bit qlonglong would be automatically used.

ah, ignore me here indeed, somehow I missed the other integer-like overloads 
of QString::number(x) :/

> > On Aug. 4, 2013, 5:57 p.m., Friedrich W. H. Kossebau wrote:
> > > libs/odf/writeodf/odfwriter.h, line 96
> > > <http://git.reviewboard.kde.org/r/111773/diff/5/?file=176058#file176058l
> > > ine96>> > 
> > >     so T is always just passed by value? what kind of Ts are expected
> > >     here?
> 
> T is a pointer for both uses.
>     void addCompleteElement(const char* cstr);
>     void addCompleteElement(QIODevice* dev);

Ah, right, is indirectly limited by what overloads there are with 
KoXMLWriter::addCompleteElement(...), okay. Just surprised to see this solved 
by a templated method, instead of two implemented methods then.

Cheers
Friedrich