sdext.pdfimport Writer: Do not visit DrawElement twice in WriterXmlEmitter

Discovered when working on commit f6004e1c457ddab5e0c91e6159875d25130b108a.

To reproduce, you can print the content of aOutput2 above the line:
https://opengrok.libreoffice.org/xref/core/sdext/source/pdfimport/test/tests.cxx?r=f6004e1c#836,
and then you get the xml content as shown in:
https://bugs.documentfoundation.org/attachment.cgi?id=183217,
in which you can see that there are duplicated draw:frame(s) with identical draw:z-index.

To dig into the problem, you may take a look at the following code block:

1    for( const auto& rxChild : elem.Children )
2    {
3        PageElement* pPage = dynamic_cast<PageElement*>(rxChild.get());
4        if( pPage )
5        {
6            for( auto child_it = pPage->Children.begin(); child_it != pPage->Children.end(); ++child_it )
7            {
8                if( dynamic_cast<DrawElement*>(child_it->get()) != nullptr )
9                    (*child_it)->visitedBy( *this, child_it );
10           }
11       }
12   }
13
14   for( auto it = elem.Children.begin(); it != elem.Children.end(); ++it )
15   {
16       if( dynamic_cast<DrawElement*>(it->get()) != nullptr )
17           (*it)->visitedBy( *this, it );
18   }

In the for loop in lines 1:12:
* "elem" is a "DocumentElement" which is derived from "Element". It's childen are PageElement(s).
* "pPage" is a "PageElement" which is also derived from "Element". It's childen are DrawElement(s).
* "child_it", as in the for loop in lines 6:10, is a "DrawElement" which is derived from
"GraphicalElement", whereas "GraphicalElement" is derived from "Element".
In this block, the code goes through each of the pages and visit the DrawElements there.

The code in lines 14:18 seems to assume that, a DrawElement, in addition to be a child of
PageElement, may also be a child of DocumentElement. See the comment in souce code.
Yes, it may be. For such DrawElement(s), the visiting is done in lines 14:18 separately.

Note that The above code uses dynamic_cast to determine whether a node is a DrawElement or not.

The problem is that, in determining whether the node during the 2nd visiting is a DrawElement,
it accidently used "== nullptr". This may be an inadvertent error. If the dynamic_cast is a nullprt,
then it is not a DrawElement and the visiting should not be done there.

Resolve this by using "!= nullptr" in the second dynamic_cast checking.

Change-Id: I066100e98039d505d8b10c390954e014b78cff4f
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/141680
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>

1 files changed, 1 insertions, 1 deletions

diff --git a/sdext/source/pdfimport/tree/writertreevisiting.cxx b/sdext/source/pdfimport/tree/writertreevisiting.cxx
index 2ece5307bd53..deabf365088b 100644
--- a/sdext/source/pdfimport/tree/writertreevisiting.cxx
+++ b/sdext/source/pdfimport/tree/writertreevisiting.cxx
@@ -382,7 +382,7 @@ void WriterXmlEmitter::visit( DocumentElement& elem, const std::list< std::uniqu
     // only DrawElement types
     for( auto it = elem.Children.begin(); it != elem.Children.end(); ++it )
     {
-        if( dynamic_cast<DrawElement*>(it->get()) == nullptr )
+        if( dynamic_cast<DrawElement*>(it->get()) != nullptr )
             (*it)->visitedBy( *this, it );
     }