tdf#137180 RTF import: fix bad left margin with direct format/para style/num

This is a case when a left margin appears as direct formatting on a
paragraph, the paragraph has a style and the style has the same left
margin. But the paragraph has a numbering as well: so it's not valid to
deduplicate the left margin from the direct formatting, because then the
left margin from the numbering will be used, which can be a different
value.

(cherry picked from commit 3ee3ae85de3a29ebfb89e75960b65417bfd6ca55)

Conflicts:
writerfilter/source/rtftok/rtfsprm.cxx

Change-Id: I5e27502b8d505bdfddfdc910858f62e501db35a5
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/104224
Tested-by: Jenkins
Reviewed-by: Caolán McNamara <caolanm@redhat.com>

6 files changed, 130 insertions, 12 deletions

diff --git a/writerfilter/CppunitTest_writerfilter_rtftok.mk b/writerfilter/CppunitTest_writerfilter_rtftok.mk
index 4ca730b9d3fc..db038292ebdd 100644
--- a/writerfilter/CppunitTest_writerfilter_rtftok.mk
+++ b/writerfilter/CppunitTest_writerfilter_rtftok.mk
@@ -17,6 +17,7 @@ $(eval $(call gb_CppunitTest_use_externals,writerfilter_rtftok,\
 
 $(eval $(call gb_CppunitTest_add_exception_objects,writerfilter_rtftok, \
     writerfilter/qa/cppunittests/rtftok/rtfsdrimport \
+    writerfilter/qa/cppunittests/rtftok/rtfsprm \
 ))
 
 $(eval $(call gb_CppunitTest_use_libraries,writerfilter_rtftok, \
diff --git a/writerfilter/qa/cppunittests/rtftok/data/left-margin-dedup.rtf b/writerfilter/qa/cppunittests/rtftok/data/left-margin-dedup.rtf
new file mode 100644
index 000000000000..8536694a294b
--- /dev/null
+++ b/writerfilter/qa/cppunittests/rtftok/data/left-margin-dedup.rtf
@@ -0,0 +1,26 @@
+{\rtf1
+{\fonttbl
+{\f0 Times New Roman;}
+{\f3\froman\fcharset2\fprq2 Symbol;}
+}
+{\stylesheet
+{\ql Normal;}
+{\s44\ql\fi-288\li360 Table Text Bullet;}
+{\s59\ql\li720 List Paragraph;}
+}
+{\*\listtable
+{\list
+{\listlevel\levelnfc23\leveljc0\leveljcn0\levelfollow0
+\levelstartat1\levelspace0\levelindent0
+{\leveltext\'01\u-3913 ?;}
+{\levelnumbers;}
+\f3\fbias0 \fi-360\li1305 }
+{\listname ;}
+\listid845246918}
+}
+{\*\listoverridetable
+{\listoverride\listid845246918\listoverridecount0\ls27}
+}
+\pard\plain\s44\ql\fi-360\li720\ls27 P1\par
+\pard\plain\s59\ql\fi-360\li720\ls27 P2\par
+}
diff --git a/writerfilter/qa/cppunittests/rtftok/rtfsprm.cxx b/writerfilter/qa/cppunittests/rtftok/rtfsprm.cxx
new file mode 100644
index 000000000000..9f5ec63e8b3f
--- /dev/null
+++ b/writerfilter/qa/cppunittests/rtftok/rtfsprm.cxx
@@ -0,0 +1,83 @@
+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
+/*
+ * This file is part of the LibreOffice project.
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+
+#include <test/bootstrapfixture.hxx>
+#include <unotest/macros_test.hxx>
+
+#include <com/sun/star/beans/XPropertySet.hpp>
+#include <com/sun/star/beans/XPropertyState.hpp>
+#include <com/sun/star/frame/Desktop.hpp>
+#include <com/sun/star/text/XTextDocument.hpp>
+
+using namespace ::com::sun::star;
+
+namespace
+{
+/// Tests for writerfilter/source/rtftok/rtfsprm.cxx.
+class Test : public test::BootstrapFixture, public unotest::MacrosTest
+{
+private:
+    uno::Reference<lang::XComponent> mxComponent;
+
+public:
+    void setUp() override;
+    void tearDown() override;
+    uno::Reference<lang::XComponent>& getComponent() { return mxComponent; }
+};
+
+void Test::setUp()
+{
+    test::BootstrapFixture::setUp();
+
+    mxDesktop.set(frame::Desktop::create(mxComponentContext));
+}
+
+void Test::tearDown()
+{
+    if (mxComponent.is())
+        mxComponent->dispose();
+
+    test::BootstrapFixture::tearDown();
+}
+
+char const DATA_DIRECTORY[] = "/writerfilter/qa/cppunittests/rtftok/data/";
+
+CPPUNIT_TEST_FIXTURE(Test, testLeftMarginDedup)
+{
+    OUString aURL = m_directories.getURLFromSrc(DATA_DIRECTORY) + "left-margin-dedup.rtf";
+    getComponent() = loadFromDesktop(aURL);
+    uno::Reference<text::XTextDocument> xTextDocument(getComponent(), uno::UNO_QUERY);
+    uno::Reference<container::XEnumerationAccess> xText(xTextDocument->getText(), uno::UNO_QUERY);
+    uno::Reference<container::XEnumeration> xParagraphs = xText->createEnumeration();
+    uno::Reference<beans::XPropertySet> xParagraph(xParagraphs->nextElement(), uno::UNO_QUERY);
+    sal_Int32 nLeftMargin = 0;
+    xParagraph->getPropertyValue("ParaLeftMargin") >>= nLeftMargin;
+    CPPUNIT_ASSERT_EQUAL(static_cast<sal_Int32>(1270), nLeftMargin);
+
+    uno::Reference<beans::XPropertyState> xParagraphState(xParagraph, uno::UNO_QUERY);
+    CPPUNIT_ASSERT_EQUAL(beans::PropertyState_DIRECT_VALUE,
+                         xParagraphState->getPropertyState("ParaLeftMargin"));
+
+    xParagraph.set(xParagraphs->nextElement(), uno::UNO_QUERY);
+    nLeftMargin = 0;
+    xParagraph->getPropertyValue("ParaLeftMargin") >>= nLeftMargin;
+    CPPUNIT_ASSERT_EQUAL(static_cast<sal_Int32>(1270), nLeftMargin);
+
+    xParagraphState.set(xParagraph, uno::UNO_QUERY);
+    // Without the accompanying fix in place, this test would have failed with:
+    // - Expected: 0 (DIRECT_VALUE)
+    // - Actual  : 1 (DEFAULT_VALUE)
+    // i.e. the left margin was not a direct formatting, which means left margin from the numbering
+    // was used instead.
+    CPPUNIT_ASSERT_EQUAL(beans::PropertyState_DIRECT_VALUE,
+                         xParagraphState->getPropertyState("ParaLeftMargin"));
+}
+}
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/writerfilter/source/rtftok/rtfdocumentimpl.cxx b/writerfilter/source/rtftok/rtfdocumentimpl.cxx
index 97844308de0b..f470ac552fad 100644
--- a/writerfilter/source/rtftok/rtfdocumentimpl.cxx
+++ b/writerfilter/source/rtftok/rtfdocumentimpl.cxx
@@ -525,7 +525,7 @@ RTFDocumentImpl::getProperties(const RTFSprms& rAttributes, RTFSprms const& rSpr
         }
 
         // Get rid of direct formatting what is already in the style.
-        RTFSprms const sprms(aSprms.cloneAndDeduplicate(aStyleSprms, nStyleType, true));
+        RTFSprms const sprms(aSprms.cloneAndDeduplicate(aStyleSprms, nStyleType, true, &aSprms));
         RTFSprms const attributes(
             rAttributes.cloneAndDeduplicate(aStyleAttributes, nStyleType, true));
         return new RTFReferenceProperties(attributes, sprms);
diff --git a/writerfilter/source/rtftok/rtfsprm.cxx b/writerfilter/source/rtftok/rtfsprm.cxx
index fe6af8e5e4d8..5c2edbb1c65a 100644
--- a/writerfilter/source/rtftok/rtfsprm.cxx
+++ b/writerfilter/source/rtftok/rtfsprm.cxx
@@ -196,7 +196,7 @@ static RTFValue::Pointer_t getDefaultSPRM(Id const id, Id nStyleType)
 }
 
 /// Is it problematic to deduplicate this SPRM?
-static bool isSPRMDeduplicateBlacklist(Id nId)
+static bool isSPRMDeduplicateBlacklist(Id nId, RTFSprms* pDirect)
 {
     switch (nId)
     {
@@ -223,6 +223,11 @@ static bool isSPRMDeduplicateBlacklist(Id nId)
         case NS_ooxml::LN_CT_Border_themeTint:
         case NS_ooxml::LN_CT_Border_themeColor:
             return true;
+        // Removing \fi and \li if the style has the same value would mean taking these values from
+        // \ls, while deduplication would be done to take the values from the style.
+        case NS_ooxml::LN_CT_Ind_firstLine:
+        case NS_ooxml::LN_CT_Ind_left:
+            return pDirect && pDirect->find(NS_ooxml::LN_CT_PPrBase_numPr);
 
         default:
             return false;
@@ -252,22 +257,24 @@ static bool isSPRMChildrenExpected(Id nId)
 
 /// Does the clone / deduplication of a single sprm.
 static void cloneAndDeduplicateSprm(std::pair<Id, RTFValue::Pointer_t> const& rSprm, RTFSprms& ret,
-                                    Id nStyleType)
+                                    Id nStyleType, RTFSprms* pDirect = nullptr)
 {
     RTFValue::Pointer_t const pValue(ret.find(rSprm.first));
     if (pValue)
     {
         if (rSprm.second->equals(*pValue))
         {
-            if (!isSPRMDeduplicateBlacklist(rSprm.first))
+            if (!isSPRMDeduplicateBlacklist(rSprm.first, pDirect))
+            {
                 ret.erase(rSprm.first); // duplicate to style
+            }
         }
         else if (!rSprm.second->getSprms().empty() || !rSprm.second->getAttributes().empty())
         {
-            RTFSprms const sprms(
-                pValue->getSprms().cloneAndDeduplicate(rSprm.second->getSprms(), nStyleType));
+            RTFSprms const sprms(pValue->getSprms().cloneAndDeduplicate(
+                rSprm.second->getSprms(), nStyleType, /*bImplicitPPr =*/false, pDirect));
             RTFSprms const attributes(pValue->getAttributes().cloneAndDeduplicate(
-                rSprm.second->getAttributes(), nStyleType));
+                rSprm.second->getAttributes(), nStyleType, /*bImplicitPPr =*/false, pDirect));
             // Don't copy the sprm in case we expect it to have children but it doesn't have some.
             if (!isSPRMChildrenExpected(rSprm.first) || !sprms.empty() || !attributes.empty())
                 ret.set(rSprm.first,
@@ -377,7 +384,7 @@ void RTFSprms::duplicateList(const RTFValue::Pointer_t& pAbstract)
 }
 
 RTFSprms RTFSprms::cloneAndDeduplicate(RTFSprms& rReference, Id const nStyleType,
-                                       bool const bImplicitPPr) const
+                                       bool const bImplicitPPr, RTFSprms* pDirect) const
 {
     RTFSprms ret(*this);
     ret.ensureCopyBeforeWrite();
@@ -393,10 +400,10 @@ RTFSprms RTFSprms::cloneAndDeduplicate(RTFSprms& rReference, Id const nStyleType
         if (bImplicitPPr && rSprm.first == NS_ooxml::LN_CT_Style_pPr)
         {
             for (const auto& i : rSprm.second->getSprms())
-                cloneAndDeduplicateSprm(i, ret, nStyleType);
+                cloneAndDeduplicateSprm(i, ret, nStyleType, pDirect);
         }
         else
-            cloneAndDeduplicateSprm(rSprm, ret, nStyleType);
+            cloneAndDeduplicateSprm(rSprm, ret, nStyleType, pDirect);
     }
     return ret;
 }
diff --git a/writerfilter/source/rtftok/rtfsprm.hxx b/writerfilter/source/rtftok/rtfsprm.hxx
index dd3e05cb8723..bb4f7c790819 100644
--- a/writerfilter/source/rtftok/rtfsprm.hxx
+++ b/writerfilter/source/rtftok/rtfsprm.hxx
@@ -63,8 +63,9 @@ public:
     /// Also insert default values to override attributes of style
     /// (yes, really; that's what Word does).
     /// @param bImplicitPPr implicit dereference of top-level pPr SPRM
-    RTFSprms cloneAndDeduplicate(RTFSprms& rReference, Id nStyleType,
-                                 bool bImplicitPPr = false) const;
+    /// @param pDirect pointer to the root of the direct formatting SPRM tree, if any
+    RTFSprms cloneAndDeduplicate(RTFSprms& rReference, Id nStyleType, bool bImplicitPPr = false,
+                                 RTFSprms* pDirect = nullptr) const;
     /// Inserts default values to override attributes of pAbstract.
     void duplicateList(const RTFValue::Pointer_t& pAbstract);
     /// Removes duplicated values based on in-list properties.