Review Request: read XML in the rigth way. ie. <a> <b>\n<c> has 5 nodes, not 3

[Jan Hambrecht]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/101857/#review5803
-----------------------------------------------------------


If you fix the things I commented on, I think it can go in.


libs/odf/KoXmlReader.h
<http://git.reviewboard.kde.org/r/101857/#comment5123>

    I am not in favour of passing the stripSpaces parameter to the copy constructor. It should copy the value from the passed KoXmlDocument, that is the semantic of a copy constructor. I one wants to change the strip whitespace behaviour, the setter setWhitespaceStripping can still be used.



libs/odf/KoXmlReader.h
<http://git.reviewboard.kde.org/r/101857/#comment5128>

    Please add some documentation for the setting here.



libs/odf/KoXmlReader.h
<http://git.reviewboard.kde.org/r/101857/#comment5127>

    You should not pass the paramter here. KoXmlNodeData should properly initialize its stripSpace member in the constuctor.
    And as this KoXmlDocument constructor is private, nobody could ever pass a different value here anyway.



libs/odf/KoXmlReader.cpp
<http://git.reviewboard.kde.org/r/101857/#comment5124>

    I don't think you have to pass the stripSpaces parameter here, it is already a member of that class. You can just access the member inside this function.



libs/odf/KoXmlReader.cpp
<http://git.reviewboard.kde.org/r/101857/#comment5126>

    You should initialize the stripSpaces member to true here.



libs/odf/KoXmlReader.cpp
<http://git.reviewboard.kde.org/r/101857/#comment5122>

    Please use 4 spaces for indentation.


- Jan


On Aug. 18, 2011, 12:04 a.m., Jaime Torres Amate wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101857/
> -----------------------------------------------------------
> 
> (Updated Aug. 18, 2011, 12:04 a.m.)
> 
> 
> Review request for Calligra.
> 
> 
> Summary
> -------
> 
> Quoting the w3 consortium:
> [Definition: All text that is not markup constitutes the character data of the document.]
> 
> And in section
> http://www.w3.org/TR/REC-xml/#sec-white-space
> 
> In editing XML documents, it is often convenient to use "white space" (spaces, tabs, and blank lines) to set apart the markup for greater readability. 
> Such white space is typically not intended for inclusion in the delivered version of the document. 
> On the other hand, "significant" white space that should be preserved in the delivered version is common, for example in poetry and source code.
> 
> An XML processor MUST always pass all characters in a document that are not markup through to the application. 
> A  validating XML processor MUST also inform the application which of these characters constitute white space appearing in element content.
> 
> [Definition: An element type has mixed content when elements of that type may contain character data, optionally interspersed with child elements.] 
> ------------------
> The attached patch modifies the xml parser to return the spaces between > and < as text elements.
> 
> I have added the TestXmlReaderWithoutSpaces to remove all the additional spaces between nodes.
> 
> The second version of this patch introduces a new method that is called only when setContent is called with the new default boolean parameter set to false.
> 
> 
> Diffs
> -----
> 
>   libs/odf/KoXmlReader.h 3f9ddf4 
>   libs/odf/KoXmlReader.cpp ad5e9d2 
>   libs/odf/tests/CMakeLists.txt ad632c8 
>   libs/odf/tests/TestXmlReaderWithoutSpaces.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/101857/diff
> 
> 
> Testing
> -------
> 
> The TestXmlReader test and the new TestXmlReaderWithoutSpaces are OK.
> 
> Today, only the following tests failed:
> 
>          19 - Plan-ScriptingTester (Failed) 
>          20 - KPlato-RCPSTester (Failed) (with a crash)
>          24 - kspread-Formula (Failed)
>          37 - kspread-InformationFunctions (Failed)
>          41 - kspread-TextFunctions (Failed)
>          43 - kspread-ValueFormatter (Failed)
>         172 - libs-widgets-KoResourceTagging_test (Failed)
>         193 - libs-kotext-styles-TestOpenDocumentStyle (Failed)
>         196 - kotext-odf-TestChangeTracking (Failed)
> 
> 
> Thanks,
> 
> Jaime Torres
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20110818/1aa60756/attachment.htm>