Review Request: read XML in the rigth way. ie. <a> <b>\n<c> has 5 nodes, not 3
Jan Hambrecht
jaham at gmx.net
Wed Aug 17 11:17:58 BST 2011
> On Aug. 16, 2011, 5:45 p.m., Jan Hambrecht wrote:
> > Instead of passing a new bool parameter to all these methods of KoXmlReader, I would prefer having a single function setWhitespaceStrippingEnabled. This sets an internal flag which can be used at all the places necessary.
>
> Jaime Torres Amate wrote:
> There are involved 1 class (KoXmlDocument) and 2 namespaces (KoXml -public- and anonymous -private-) in the patch.
> I do not like the idea of having one global variable in each of the namespaces and in the class (or a module global variable) that are modified by one method in the class (or in the KoXml namespace).
> I do not like the idea of a single function (outside of the class) changing the behavior of a class.
> I do not like the idea of a class method changing the behavior of a namespace.
>
> I would have done it if all the methods were in one class or in children classes or in private namespaces.
>
Sorry I was not clear in what I wrote. What I meant is that all the setContent methods in KoXmlDocument and the setDocument function in the KoXml namespace. So if you put a method into KoXmlDocument to set an internal flag, you can remove the stripWhitespace parameter from these setContent methods. And as setDocument gets a KoXmlDocument passed, you can check that member in that function too, using a getter from KoXmlDocument.
While thinking about that, it would also be possible to pass the stripWhitespace parameter to the constructor of KoXmlDocument. Then you you only need a additional getter to check the value of the flag from within KoXml::setDocument.
The other functions all belong to the implementation, so i think those can keep the additional parameter. But I think having a single place within the public interface to set that parameter results in a much cleaner interface.
I hope you can agree to that.
- Jan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/101857/#review5747
-----------------------------------------------------------
On Aug. 15, 2011, 5:56 p.m., Jaime Torres Amate wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101857/
> -----------------------------------------------------------
>
> (Updated Aug. 15, 2011, 5:56 p.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 modified 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/20110817/36b92ff6/attachment.htm>
More information about the calligra-devel
mailing list