[Libreoffice-commits] core.git: Branch 'libreoffice-6-3' - sw/qa writerfilter/source

Miklos Vajna (via logerrit) logerrit at kemper.freedesktop.org
Tue Nov 5 14:13:35 UTC 2019


 sw/qa/extras/ooxmlexport/data/tdf125038.docx      |binary
 sw/qa/extras/ooxmlexport/data/tdf125038b.docx     |binary
 sw/qa/extras/ooxmlexport/data/tdf125038c.docx     |binary
 sw/qa/extras/ooxmlexport/ooxmlexport14.cxx        |   42 +++++
 writerfilter/source/dmapper/DomainMapper_Impl.cxx |  165 ++++++++++++++++++++--
 writerfilter/source/dmapper/DomainMapper_Impl.hxx |   13 +
 6 files changed, 205 insertions(+), 15 deletions(-)

New commits:
commit 58d798ea44b9e2a57681e990a8acc747bc287c0b
Author:     Miklos Vajna <vmiklos at collabora.com>
AuthorDate: Mon Oct 28 16:58:41 2019 +0100
Commit:     Tamás Zolnai <tamas.zolnai at collabora.com>
CommitDate: Tue Nov 5 15:12:44 2019 +0100

    tdf#125038 DOCX import: fix various issues with MERGEFIELD inside IF fields
    
    This is a combination of 4 commits.
    
    This is the 1st commit:
    
    Related: tdf#125038 DOCX import: fix unexpected MERGEFIELD result inside IF
    
    The problem is that DOCX supports nesting MERGEFIELD fields inside IF
    fields, while SwHiddenTextField only supports a single string as a
    condition.
    
    This means in case there are MERGEFIELD fields inside the IF field,
    those fields will be inserted to the doc model before the IF field,
    exposing their value, while Word only uses their value during the
    evaluation of the IF expression.
    
    Fix the problem by inspecting the parent field command before setting
    the MERGEFIELD result.
    
    (cherry picked from commit 7b0534cb70e96028c8525285c42a71415704cede)
    
    Conflicts:
            writerfilter/source/dmapper/DomainMapper_Impl.hxx
    
    This is commit #2:
    
    Related: tdf#125038 DOCX import: fix unexpected linebreak inside IF condition
    
    Writer body text is expected to only contain the result of the field. So
    in case both the field command and the field result contains a
    linebreak, we need to make sure that linebreaks are ignored in the field
    command for field types where the Writer field implementation expects a
    single string.
    
    With this, the number of paragraphs in the bugdoc is now correct.
    
    (cherry picked from commit 97f9af714ea1c46e498fa99f7ca34fc1708d38a6)
    
    This is commit #3:
    
    tdf#125038 DOCX import: fix lost MERGEFIELD result inside an IF field
    
    The problem here was that the IF field result didn't have a plain text
    string, rather it had a MERGEFIELD in it. Writer's conditional text
    field expects a plain text string, so just use the result of the
    MERGEFIELD for an IF parent. Do this in a generic way, it's likely that
    other parent-child field combinations want to do the same in the future.
    
    With this, all lost strings are fixed from the original bugdoc + all
    unexpected content is hidden in Writer as well.
    
    (cherry picked from commit d09336fbdceaafd9320466b660a2b32a07dcc16a)
    
    This is commit #4:
    
    tdf#125038 DOCX import: better support for linebreaks in IF fields
    
    IF fields can't contain linebreaks, so instead of just calling
    finishParagraph() and hoping it does something sane, explicitly handle
    them: remember the properties and perform the call only once the field
    is closed.
    
    (cherry picked from commit d40c2be38aaf56116f4dc7be9e78f4e9695407fc)
    
    Change-Id: I676aa2c83f12cb600829177a0eb25558822b1d94
    Reviewed-on: https://gerrit.libreoffice.org/81982
    Tested-by: Jenkins
    Reviewed-by: Tamás Zolnai <tamas.zolnai at collabora.com>

diff --git a/sw/qa/extras/ooxmlexport/data/tdf125038.docx b/sw/qa/extras/ooxmlexport/data/tdf125038.docx
new file mode 100644
index 000000000000..b4dd622f95e0
Binary files /dev/null and b/sw/qa/extras/ooxmlexport/data/tdf125038.docx differ
diff --git a/sw/qa/extras/ooxmlexport/data/tdf125038b.docx b/sw/qa/extras/ooxmlexport/data/tdf125038b.docx
new file mode 100644
index 000000000000..3aa189daded8
Binary files /dev/null and b/sw/qa/extras/ooxmlexport/data/tdf125038b.docx differ
diff --git a/sw/qa/extras/ooxmlexport/data/tdf125038c.docx b/sw/qa/extras/ooxmlexport/data/tdf125038c.docx
new file mode 100644
index 000000000000..10234b864627
Binary files /dev/null and b/sw/qa/extras/ooxmlexport/data/tdf125038c.docx differ
diff --git a/sw/qa/extras/ooxmlexport/ooxmlexport14.cxx b/sw/qa/extras/ooxmlexport/ooxmlexport14.cxx
index b06b342c7071..27317edd1615 100644
--- a/sw/qa/extras/ooxmlexport/ooxmlexport14.cxx
+++ b/sw/qa/extras/ooxmlexport/ooxmlexport14.cxx
@@ -39,6 +39,48 @@ DECLARE_OOXMLEXPORT_TEST(testTdf108350_noFontdefaults, "tdf108350_noFontdefaults
     //CPPUNIT_ASSERT_EQUAL_MESSAGE("Font size", 10.f, getProperty<float>(xStyleProps, "CharHeight"));
 }
 
+DECLARE_OOXMLIMPORT_TEST(testTdf125038, "tdf125038.docx")
+{
+    OUString aActual = getParagraph(1)->getString();
+    // Without the accompanying fix in place, this test would have failed with:
+    // - Expected: phone:...
+    // - Actual  : result1result2phone:...
+    // i.e. the result if the inner MERGEFIELD fields ended up in the body text.
+    CPPUNIT_ASSERT_EQUAL(OUString("phone: \t1234567890"), aActual);
+}
+
+DECLARE_OOXMLIMPORT_TEST(testTdf125038b, "tdf125038b.docx")
+{
+    // Load a document with an IF field, where the IF field command contains a paragraph break.
+    uno::Reference<text::XTextDocument> xTextDocument(mxComponent, uno::UNO_QUERY);
+    uno::Reference<container::XEnumerationAccess> xParagraphAccess(xTextDocument->getText(), uno::UNO_QUERY);
+    uno::Reference<container::XEnumeration> xParagraphs = xParagraphAccess->createEnumeration();
+    CPPUNIT_ASSERT(xParagraphs->hasMoreElements());
+    uno::Reference<text::XTextRange> xParagraph(xParagraphs->nextElement(), uno::UNO_QUERY);
+
+    // Without the accompanying fix in place, this test would have failed with:
+    // - Expected: phone: 1234
+    // - Actual  :
+    // i.e. the the first paragraph was empty and the second paragraph had the content.
+    CPPUNIT_ASSERT_EQUAL(OUString("phone: 1234"), xParagraph->getString());
+    CPPUNIT_ASSERT(xParagraphs->hasMoreElements());
+    xParagraphs->nextElement();
+
+    // Without the accompanying fix in place, this test would have failed with:
+    // - Expression: !xParagraphs->hasMoreElements()
+    // i.e. the document had 3 paragraphs, while only 2 was expected.
+    CPPUNIT_ASSERT(!xParagraphs->hasMoreElements());
+}
+
+DECLARE_OOXMLIMPORT_TEST(testTdf125038c, "tdf125038c.docx")
+{
+    OUString aActual = getParagraph(1)->getString();
+    // Without the accompanying fix in place, this test would have failed with:
+    // - Expected: email: test at test.test
+    // - Actual  : email:
+    // I.e. the result of the MERGEFIELD field inside an IF field was lost.
+    CPPUNIT_ASSERT_EQUAL(OUString("email: test at test.test"), aActual);
+}
 
 CPPUNIT_PLUGIN_IMPLEMENT();
 
diff --git a/writerfilter/source/dmapper/DomainMapper_Impl.cxx b/writerfilter/source/dmapper/DomainMapper_Impl.cxx
index a32991c6a4cc..f6086a249fe0 100644
--- a/writerfilter/source/dmapper/DomainMapper_Impl.cxx
+++ b/writerfilter/source/dmapper/DomainMapper_Impl.cxx
@@ -171,6 +171,52 @@ struct FieldConversion
 
 typedef std::unordered_map<OUString, FieldConversion> FieldConversionMap_t;
 
+/// Gives access to the parent field contenxt of the topmost one, if there is any.
+static FieldContextPtr GetParentFieldContext(const std::deque<FieldContextPtr>& rFieldStack)
+{
+    if (rFieldStack.size() < 2)
+    {
+        return nullptr;
+    }
+
+    return rFieldStack[rFieldStack.size() - 2];
+}
+
+/// Decides if the pInner field inside pOuter is allowed in Writer core, depending on their type.
+static bool IsFieldNestingAllowed(const FieldContextPtr& pOuter, const FieldContextPtr& pInner)
+{
+    if (!pOuter->GetFieldId())
+    {
+        return true;
+    }
+
+    if (!pInner->GetFieldId())
+    {
+        return true;
+    }
+
+    switch (pOuter->GetFieldId().get())
+    {
+        case FIELD_IF:
+        {
+            switch (pInner->GetFieldId().get())
+            {
+                case FIELD_MERGEFIELD:
+                {
+                    return false;
+                }
+                default:
+                    break;
+            }
+            break;
+        }
+        default:
+            break;
+    }
+
+    return true;
+}
+
 uno::Any FloatingTableInfo::getPropertyValue(const OUString &propertyName)
 {
     for( beans::PropertyValue const & propVal : m_aFrameProperties )
@@ -627,7 +673,7 @@ uno::Reference< text::XTextAppend > const &  DomainMapper_Impl::GetTopTextAppend
 FieldContextPtr const &  DomainMapper_Impl::GetTopFieldContext()
 {
     SAL_WARN_IF(m_aFieldStack.empty(), "writerfilter.dmapper", "Field stack is empty");
-    return m_aFieldStack.top();
+    return m_aFieldStack.back();
 }
 
 void DomainMapper_Impl::InitTabStopFromStyle( const uno::Sequence< style::TabStop >& rInitTabStops )
@@ -1178,6 +1224,32 @@ void DomainMapper_Impl::finishParagraph( const PropertyMapPtr& pPropertyMap, con
 {
     if (m_bDiscardHeaderFooter)
         return;
+
+    if (!m_aFieldStack.empty())
+    {
+        FieldContextPtr pFieldContext = m_aFieldStack.back();
+        if (pFieldContext && !pFieldContext->IsCommandCompleted())
+        {
+            std::vector<OUString> aCommandParts = pFieldContext->GetCommandParts();
+            if (!aCommandParts.empty() && aCommandParts[0] == "IF")
+            {
+                // Conditional text field conditions don't support linebreaks in Writer.
+                return;
+            }
+        }
+
+        if (pFieldContext && pFieldContext->IsCommandCompleted())
+        {
+            if (pFieldContext->GetFieldId() == FIELD_IF)
+            {
+                // Conditional text fields can't contain newlines, finish the paragraph later.
+                FieldParagraph aFinish{pPropertyMap, bRemove};
+                pFieldContext->GetParagraphsToFinish().push_back(aFinish);
+                return;
+            }
+        }
+    }
+
 #ifdef DBG_UTIL
     TagLogger::getInstance().startElement("finishParagraph");
 #endif
@@ -3157,14 +3229,14 @@ void DomainMapper_Impl::PushFieldContext()
     uno::Reference< text::XTextRange > xStart;
     if (xCrsr.is())
         xStart = xCrsr->getStart();
-    m_aFieldStack.push( new FieldContext( xStart ) );
+    m_aFieldStack.push_back(new FieldContext(xStart));
 }
 /*-------------------------------------------------------------------------
 //the current field context waits for the completion of the command
   -----------------------------------------------------------------------*/
 bool DomainMapper_Impl::IsOpenFieldCommand() const
 {
-    return !m_aFieldStack.empty() && !m_aFieldStack.top()->IsCommandCompleted();
+    return !m_aFieldStack.empty() && !m_aFieldStack.back()->IsCommandCompleted();
 }
 /*-------------------------------------------------------------------------
 //the current field context waits for the completion of the command
@@ -3178,7 +3250,7 @@ bool DomainMapper_Impl::IsOpenField() const
 void DomainMapper_Impl::SetFieldLocked()
 {
     if (IsOpenField())
-        m_aFieldStack.top()->SetFieldLocked();
+        m_aFieldStack.back()->SetFieldLocked();
 }
 
 HeaderFooterContext::HeaderFooterContext(bool bTextInserted)
@@ -3272,7 +3344,7 @@ void DomainMapper_Impl::AppendFieldCommand(OUString const & rPartOfCommand)
     TagLogger::getInstance().endElement();
 #endif
 
-    FieldContextPtr pContext = m_aFieldStack.top();
+    FieldContextPtr pContext = m_aFieldStack.back();
     OSL_ENSURE( pContext.get(), "no field context available");
     if( pContext.get() )
     {
@@ -4155,7 +4227,7 @@ void DomainMapper_Impl::CloseFieldCommand()
 
     FieldContextPtr pContext;
     if(!m_aFieldStack.empty())
-        pContext = m_aFieldStack.top();
+        pContext = m_aFieldStack.back();
     OSL_ENSURE( pContext.get(), "no field context available");
     if( pContext.get() )
     {
@@ -4215,8 +4287,20 @@ void DomainMapper_Impl::CloseFieldCommand()
                     break;
                 }
                 default:
+                {
+                    FieldContextPtr pOuter = GetParentFieldContext(m_aFieldStack);
+                    if (pOuter)
+                    {
+                        if (!IsFieldNestingAllowed(pOuter, m_aFieldStack.back()))
+                        {
+                            // Parent field can't host this child field: don't create a child field
+                            // in this case.
+                            bCreateField = false;
+                        }
+                    }
                     break;
                 }
+                }
                 if (m_bStartTOC && (aIt->second.eFieldId == FIELD_PAGEREF) )
                 {
                     bCreateField = false;
@@ -4825,7 +4909,8 @@ void DomainMapper_Impl::CloseFieldCommand()
                         }
                         uno::Reference< text::XTextContent > xToInsert( xTC, uno::UNO_QUERY );
 
-                        uno::Sequence<beans::PropertyValue> aValues = m_aFieldStack.top()->getProperties()->GetPropertyValues();
+                        uno::Sequence<beans::PropertyValue> aValues
+                            = m_aFieldStack.back()->getProperties()->GetPropertyValues();
                         appendTextContent(xToInsert, aValues);
                         m_bSetCitation = true;
                     }
@@ -4919,22 +5004,46 @@ bool DomainMapper_Impl::IsFieldResultAsString()
 {
     bool bRet = false;
     OSL_ENSURE( !m_aFieldStack.empty(), "field stack empty?");
-    FieldContextPtr pContext = m_aFieldStack.top();
+    FieldContextPtr pContext = m_aFieldStack.back();
     OSL_ENSURE( pContext.get(), "no field context available");
     if( pContext.get() )
     {
         bRet = pContext->GetTextField().is() || pContext->GetFieldId() == FIELD_FORMDROPDOWN;
     }
+
+    if (!bRet)
+    {
+        FieldContextPtr pOuter = GetParentFieldContext(m_aFieldStack);
+        if (pOuter)
+        {
+            if (!IsFieldNestingAllowed(pOuter, m_aFieldStack.back()))
+            {
+                // Child field has no backing SwField, but the parent has: append is still possible.
+                bRet = pOuter->GetTextField().is();
+            }
+        }
+    }
     return bRet;
 }
 
 void DomainMapper_Impl::AppendFieldResult(OUString const& rString)
 {
     assert(!m_aFieldStack.empty());
-    FieldContextPtr pContext = m_aFieldStack.top();
+    FieldContextPtr pContext = m_aFieldStack.back();
     SAL_WARN_IF(!pContext.get(), "writerfilter.dmapper", "no field context");
     if (pContext.get())
     {
+        FieldContextPtr pOuter = GetParentFieldContext(m_aFieldStack);
+        if (pOuter)
+        {
+            if (!IsFieldNestingAllowed(pOuter, pContext))
+            {
+                // Child can't host the field result, forward to parent.
+                pOuter->AppendResult(rString);
+                return;
+            }
+        }
+
         pContext->AppendResult(rString);
     }
 }
@@ -4969,8 +5078,24 @@ void DomainMapper_Impl::SetFieldResult(OUString const& rResult)
     TagLogger::getInstance().chars(rResult);
 #endif
 
-    FieldContextPtr pContext = m_aFieldStack.top();
+    FieldContextPtr pContext = m_aFieldStack.back();
     OSL_ENSURE( pContext.get(), "no field context available");
+
+    if (m_aFieldStack.size() > 1)
+    {
+        // This is a nested field. See if the parent supports nesting on the Writer side.
+        FieldContextPtr pParentContext = m_aFieldStack[m_aFieldStack.size() - 2];
+        if (pParentContext)
+        {
+            std::vector<OUString> aParentParts = pParentContext->GetCommandParts();
+            // Conditional text fields don't support nesting in Writer.
+            if (!aParentParts.empty() && aParentParts[0] == "IF")
+            {
+                return;
+            }
+        }
+    }
+
     if( pContext.get() )
     {
         uno::Reference<text::XTextField> xTextField = pContext->GetTextField();
@@ -5092,7 +5217,7 @@ void DomainMapper_Impl::SetFieldFFData(const FFDataHandler::Pointer_t& pFFDataHa
 
     if (!m_aFieldStack.empty())
     {
-        FieldContextPtr pContext = m_aFieldStack.top();
+        FieldContextPtr pContext = m_aFieldStack.back();
         if (pContext.get())
         {
             pContext->setFFDataHandler(pFFDataHandler);
@@ -5115,7 +5240,7 @@ void DomainMapper_Impl::PopFieldContext()
     if (m_aFieldStack.empty())
         return;
 
-    FieldContextPtr pContext = m_aFieldStack.top();
+    FieldContextPtr pContext = m_aFieldStack.back();
     OSL_ENSURE( pContext.get(), "no field context available");
     if( pContext.get() )
     {
@@ -5191,7 +5316,7 @@ void DomainMapper_Impl::PopFieldContext()
                         // e.g. SdtEndBefore.
                         if (m_pLastCharacterContext.get())
                             aMap.InsertProps(m_pLastCharacterContext);
-                        aMap.InsertProps(m_aFieldStack.top()->getProperties());
+                        aMap.InsertProps(m_aFieldStack.back()->getProperties());
                         appendTextContent(xToInsert, aMap.GetPropertyValues());
                         CheckRedline( xToInsert->getAnchor( ) );
                     }
@@ -5271,10 +5396,22 @@ void DomainMapper_Impl::PopFieldContext()
         }
 
         //TOCs have to include all the imported content
+    }
 
+    std::vector<FieldParagraph> aParagraphsToFinish;
+    if (pContext)
+    {
+        aParagraphsToFinish = pContext->GetParagraphsToFinish();
     }
+
     //remove the field context
-    m_aFieldStack.pop();
+    m_aFieldStack.pop_back();
+
+    // Finish the paragraph(s) now that the field is closed.
+    for (const auto& rFinish : aParagraphsToFinish)
+    {
+        finishParagraph(rFinish.m_pPropertyMap, rFinish.m_bRemove);
+    }
 }
 
 
diff --git a/writerfilter/source/dmapper/DomainMapper_Impl.hxx b/writerfilter/source/dmapper/DomainMapper_Impl.hxx
index 3e8a2236da92..17728959e9f7 100644
--- a/writerfilter/source/dmapper/DomainMapper_Impl.hxx
+++ b/writerfilter/source/dmapper/DomainMapper_Impl.hxx
@@ -130,6 +130,13 @@ public:
     bool getTextInserted();
 };
 
+/// Information about a paragraph to be finished after a field end.
+struct FieldParagraph
+{
+    PropertyMapPtr m_pPropertyMap;
+    bool m_bRemove = false;
+};
+
 /// field stack element
 class FieldContext : public virtual SvRefBase
 {
@@ -156,6 +163,8 @@ class FieldContext : public virtual SvRefBase
     /// (Character) properties of the field itself.
     PropertyMapPtr m_pProperties;
 
+    std::vector<FieldParagraph> m_aParagraphsToFinish;
+
 public:
     explicit FieldContext(css::uno::Reference<css::text::XTextRange> const& xStart);
     ~FieldContext() override;
@@ -203,6 +212,8 @@ public:
     const PropertyMapPtr& getProperties() { return m_pProperties; }
 
     ::std::vector<OUString> GetCommandParts() const;
+
+    std::vector<FieldParagraph>& GetParagraphsToFinish() { return m_aParagraphsToFinish; }
 };
 
 struct TextAppendContext
@@ -417,7 +428,7 @@ private:
     std::stack<TextAppendContext>                                                   m_aTextAppendStack;
     std::stack<AnchoredContext>                                                     m_aAnchoredStack;
     std::stack<HeaderFooterContext>                                                 m_aHeaderFooterStack;
-    std::stack<FieldContextPtr>                                                     m_aFieldStack;
+    std::deque<FieldContextPtr>                                                     m_aFieldStack;
     bool                                                                            m_bSetUserFieldContent;
     bool                                                                            m_bSetCitation;
     bool                                                                            m_bSetDateValue;


More information about the Libreoffice-commits mailing list