[calligra] libs/kotext/styles: Fix saving line-height to ODF. Just because the property exists doesn't mean it's valid.

[Pierre]

On Tuesday 12 July 2011 14:17:12 Sebastian Sauer wrote:
> Pierre wrote:
> > On Friday 08 July 2011 18:51:17 Sebastian Sauer wrote:
> >> Git commit 9f017cf21d343c1408214499bd878b987d8daf6f by Sebastian Sauer.
> >> Committed on 08/07/2011 at 18:43.
> >> Pushed by sebsauer into branch 'master'.
> >> 
> >> Fix saving line-height to ODF. Just because the property exists doesn't
> >> mean it's valid.
> >> 
> >> M  +3    -3    libs/kotext/styles/KoParagraphStyle.cpp
> >> 
> >> http://commits.kde.org/calligra/9f017cf21d343c1408214499bd878b987d8daf6f
> 
> [...]
> 
> > I don't disagree with the idea of this change, but this is not really a
> > bug in calligra itself.
> > 
> > I will fix the unit test you broke, by adding a specific exception for
> > this property. But the following code in OpenDocument relax ng is wrong :
> > 
> > <attribute name="fo:line-height">
> > <choice>
> > <value>normal</value>
> > <ref name="nonNegativeLength"/>
> > <ref name="percent"/>
> > </choice>
> > </attribute>
> > 
> > Since nonNegativeLength can be zero, it means that 0 is a valid
> > line-height... Still, the specification doesn't tell us what it means.
> 
> First the fix above makes sure we do NOT save ALWAYS as percent. Second it
> makes sure all out code is consistent. Layouting and Words ALL do expect
> !=0 (please grep yourself).
> 
> That the specs are using nonNegativeLength does NOT mean that zero is
> included. The specs are NOT that clever. The only way to check what ranges
> are allowed and/or processed, what happens with invalid ranges, what are
> the default values, etc. pp. is trying MSO and OO.org and looking at there
> source- code (OO since MSO doesn't have that possibility). That's what I
> did. The allowed minimum value is 0.02" what is btw EXACTLY what I wrote
> down last year as comment and which was ignored by those who broke that
> logic again. Please see in KoParagraphStyle.cpp line 1247 which says;
> 
> // fixed value is between 0.0201in and 3.9402in
> 
> You will not find that in the specs or in the schema. The specs and the
> schema do NOT handle ranges, default values, exceptions, corrections, etc.
> pp. You can also try yourself loading ODT's with different values into
> OO.org (I btw did that last year and added the comment at line 1247 for
> that and already fixed it before but seems someone keeps to break it).
> 
> Now I like to know what unittest I broke?
Hi

Your mail sound angry, I don't see how I could offend you, so if I did offend 
you, I'm really sorry and be sure it wasn't meant to.

When I said the spec is buggy, it *really* is. It is not smart enough to include 
ranges, that's one issue, but it is smart enough to say that a length of zero is 
allowed or not, and in line-height case, it is allowed since nonNegativeLength 
means >= 0, that's the difference with positiveLength which means > 0.

The unit test that broke is the automatic OpenDocument test 
(TestOpenDocumentStyle).
You didn't see the breakage since there are many other broken cases to fix, you 
would have to run the test manually to see it. I'll work around by faking the 
references and using positiveLength instead of nonNegativeLength here, but this 
really should be fixed in the specification, and I'll report that.

 Pierre
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20110712/39bc63dd/attachment.sig>