tdf#119599 RTF import: fix missing deduplication of font size

Deciding when to and when not to deduplicate repeated direct formatting
of paragraph / character properties is stricky, this bug is about a case
when deduplication should happen and did not, since commit
1970a686273c5d4fc1eeb4430283e37085d9f647 (tdf#113408 RTF import style
dedup: separate paragraph and character handling, 2017-10-31).
Especially that deduplication works in both directions: it should remove
properties which are duplicated and also should insert explicit default
values for not repeated properties.

Fix the problem by making the getDefaultSPRM() aware of the context
(which style type it deals with), and then by making sure that only
default properties relevant for the given style type are inserted.

(cherry picked from commit 49614a9ea971ff7f370f863ce8a2735aab973cee)

Conflicts:
	writerfilter/source/rtftok/rtfdocumentimpl.cxx
	writerfilter/source/rtftok/rtfsprm.cxx

Change-Id: I35b6599cc47fa51b8754fd921c61a3b31a283547
Reviewed-on: https://gerrit.libreoffice.org/61237
Tested-by: Jenkins
Tested-by: Xisco Faulí <xiscofauli@libreoffice.org>
Reviewed-by: Caolán McNamara <caolanm@redhat.com>
Tested-by: Caolán McNamara <caolanm@redhat.com>

5 files changed, 63 insertions, 27 deletions

diff --git a/sw/qa/extras/rtfimport/data/tdf119599.rtf b/sw/qa/extras/rtfimport/data/tdf119599.rtf
new file mode 100644
index 000000000000..5a5d4654a43b
--- /dev/null
+++ b/sw/qa/extras/rtfimport/data/tdf119599.rtf
@@ -0,0 +1,9 @@
+{\rtf1\ansi
+{\stylesheet
+{\s0 Normal;}
+{\s146\fs32 para style;}
+}
+\paperh15840\paperw12240\margl1800\margr1800\margt1440\margb1440
+\pard\plain \s146\fs32
+hello.
+\par }
diff --git a/sw/qa/extras/rtfimport/rtfimport.cxx b/sw/qa/extras/rtfimport/rtfimport.cxx
index 659b422f5f1a..87fbbb51989b 100644
--- a/sw/qa/extras/rtfimport/rtfimport.cxx
+++ b/sw/qa/extras/rtfimport/rtfimport.cxx
@@ -1140,6 +1140,14 @@ DECLARE_RTFIMPORT_TEST(testTdf90260Par, "hello.rtf")
     CPPUNIT_ASSERT_EQUAL(2, getParagraphs());
 }
 
+DECLARE_RTFIMPORT_TEST(testTdf119599, "tdf119599.rtf")
+{
+    uno::Reference<beans::XPropertyState> xRun(getRun(getParagraph(1), 1), uno::UNO_QUERY);
+    // This was beans::PropertyState_DIRECT_VALUE, changing the font size in
+    // the style had no effect on the rendering result.
+    CPPUNIT_ASSERT_EQUAL(beans::PropertyState_DEFAULT_VALUE, xRun->getPropertyState("CharHeight"));
+}
+
 DECLARE_RTFIMPORT_TEST(testTdf90315, "tdf90315.rtf")
 {
     uno::Reference<text::XTextSectionsSupplier> xTextSectionsSupplier(mxComponent, uno::UNO_QUERY);
diff --git a/writerfilter/source/rtftok/rtfdocumentimpl.cxx b/writerfilter/source/rtftok/rtfdocumentimpl.cxx
index d48df600c34f..4094dc97dacf 100644
--- a/writerfilter/source/rtftok/rtfdocumentimpl.cxx
+++ b/writerfilter/source/rtftok/rtfdocumentimpl.cxx
@@ -489,8 +489,9 @@ RTFDocumentImpl::getProperties(RTFSprms& rAttributes, RTFSprms const& rSprms, Id
         RTFSprms aStyleSprms;
         RTFSprms aStyleAttributes;
         // Ensure the paragraph style is a flat list.
-        if (!nStyleType || nStyleType == NS_ooxml::LN_Value_ST_StyleType_paragraph)
-            lcl_copyFlatten(rProps, aStyleAttributes, aStyleSprms);
+        // Take paragraph style into account for character properties as well,
+        // as paragraph style may contain character properties.
+        lcl_copyFlatten(rProps, aStyleAttributes, aStyleSprms);
 
         if (itChar != m_aStyleTableEntries.end())
         {
@@ -502,8 +503,8 @@ RTFDocumentImpl::getProperties(RTFSprms& rAttributes, RTFSprms const& rSprms, Id
         }
 
         // Get rid of direct formatting what is already in the style.
-        RTFSprms const sprms(aSprms.cloneAndDeduplicate(aStyleSprms));
-        RTFSprms const attributes(rAttributes.cloneAndDeduplicate(aStyleAttributes));
+        RTFSprms const sprms(aSprms.cloneAndDeduplicate(aStyleSprms, nStyleType));
+        RTFSprms const attributes(rAttributes.cloneAndDeduplicate(aStyleAttributes, nStyleType));
         return std::make_shared<RTFReferenceProperties>(attributes, sprms);
     }
 
diff --git a/writerfilter/source/rtftok/rtfsprm.cxx b/writerfilter/source/rtftok/rtfsprm.cxx
index f768785a5cba..d63eaef70694 100644
--- a/writerfilter/source/rtftok/rtfsprm.cxx
+++ b/writerfilter/source/rtftok/rtfsprm.cxx
@@ -137,21 +137,36 @@ void RTFSprms::eraseLast(Id nKeyword)
     }
 }
 
-static RTFValue::Pointer_t getDefaultSPRM(Id const id)
+static RTFValue::Pointer_t getDefaultSPRM(Id const id, Id nStyleType)
 {
-    switch (id)
+    if (!nStyleType || nStyleType == NS_ooxml::LN_Value_ST_StyleType_character)
     {
-        case NS_ooxml::LN_CT_Spacing_before:
-        case NS_ooxml::LN_CT_Spacing_after:
-        case NS_ooxml::LN_EG_RPrBase_b:
-        case NS_ooxml::LN_CT_Ind_left:
-        case NS_ooxml::LN_CT_Ind_right:
-        case NS_ooxml::LN_CT_Ind_firstLine:
-            return std::make_shared<RTFValue>(0);
+        switch (id)
+        {
+            case NS_ooxml::LN_EG_RPrBase_b:
+                return std::make_shared<RTFValue>(0);
+            default:
+                break;
+        }
+    }
 
-        default:
-            return RTFValue::Pointer_t();
+    if (!nStyleType || nStyleType == NS_ooxml::LN_Value_ST_StyleType_paragraph)
+    {
+        switch (id)
+        {
+            case NS_ooxml::LN_CT_Spacing_before:
+            case NS_ooxml::LN_CT_Spacing_after:
+            case NS_ooxml::LN_CT_Ind_left:
+            case NS_ooxml::LN_CT_Ind_right:
+            case NS_ooxml::LN_CT_Ind_firstLine:
+                return std::make_shared<RTFValue>(0);
+
+            default:
+                break;
+        }
     }
+
+    return RTFValue::Pointer_t();
 }
 
 /// Is it problematic to deduplicate this SPRM?
@@ -199,7 +214,8 @@ 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)
+static void cloneAndDeduplicateSprm(std::pair<Id, RTFValue::Pointer_t> const& rSprm, RTFSprms& ret,
+                                    Id nStyleType)
 {
     RTFValue::Pointer_t const pValue(ret.find(rSprm.first));
     if (pValue)
@@ -211,9 +227,10 @@ static void cloneAndDeduplicateSprm(std::pair<Id, RTFValue::Pointer_t> const& rS
         }
         else if (!rSprm.second->getSprms().empty() || !rSprm.second->getAttributes().empty())
         {
-            RTFSprms const sprms(pValue->getSprms().cloneAndDeduplicate(rSprm.second->getSprms()));
-            RTFSprms const attributes(
-                pValue->getAttributes().cloneAndDeduplicate(rSprm.second->getAttributes()));
+            RTFSprms const sprms(
+                pValue->getSprms().cloneAndDeduplicate(rSprm.second->getSprms(), nStyleType));
+            RTFSprms const attributes(pValue->getAttributes().cloneAndDeduplicate(
+                rSprm.second->getAttributes(), nStyleType));
             // 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,
@@ -223,16 +240,17 @@ static void cloneAndDeduplicateSprm(std::pair<Id, RTFValue::Pointer_t> const& rS
     else
     {
         // not found - try to override style with default
-        RTFValue::Pointer_t const pDefault(getDefaultSPRM(rSprm.first));
+        RTFValue::Pointer_t const pDefault(getDefaultSPRM(rSprm.first, nStyleType));
         if (pDefault)
         {
             ret.set(rSprm.first, pDefault);
         }
         else if (!rSprm.second->getSprms().empty() || !rSprm.second->getAttributes().empty())
         {
-            RTFSprms const sprms(RTFSprms().cloneAndDeduplicate(rSprm.second->getSprms()));
+            RTFSprms const sprms(
+                RTFSprms().cloneAndDeduplicate(rSprm.second->getSprms(), nStyleType));
             RTFSprms const attributes(
-                RTFSprms().cloneAndDeduplicate(rSprm.second->getAttributes()));
+                RTFSprms().cloneAndDeduplicate(rSprm.second->getAttributes(), nStyleType));
             if (!sprms.empty() || !attributes.empty())
             {
                 ret.set(rSprm.first, std::make_shared<RTFValue>(attributes, sprms));
@@ -314,14 +332,14 @@ void RTFSprms::duplicateList(const RTFValue::Pointer_t& pAbstract)
                     = getNestedAttribute(*this, NS_ooxml::LN_CT_PPrBase_ind, rListLevelPair.first);
                 if (!pParagraphValue)
                     putNestedAttribute(*this, NS_ooxml::LN_CT_PPrBase_ind, rListLevelPair.first,
-                                       getDefaultSPRM(rListLevelPair.first));
+                                       getDefaultSPRM(rListLevelPair.first, 0));
 
                 break;
         }
     }
 }
 
-RTFSprms RTFSprms::cloneAndDeduplicate(RTFSprms& rReference) const
+RTFSprms RTFSprms::cloneAndDeduplicate(RTFSprms& rReference, Id nStyleType) const
 {
     RTFSprms ret(*this);
     ret.ensureCopyBeforeWrite();
@@ -337,10 +355,10 @@ RTFSprms RTFSprms::cloneAndDeduplicate(RTFSprms& rReference) const
         if (rSprm.first == NS_ooxml::LN_CT_Style_pPr)
         {
             for (auto& i : rSprm.second->getSprms())
-                cloneAndDeduplicateSprm(i, ret);
+                cloneAndDeduplicateSprm(i, ret, nStyleType);
         }
         else
-            cloneAndDeduplicateSprm(rSprm, ret);
+            cloneAndDeduplicateSprm(rSprm, ret, nStyleType);
     }
     return ret;
 }
diff --git a/writerfilter/source/rtftok/rtfsprm.hxx b/writerfilter/source/rtftok/rtfsprm.hxx
index 2c4247925f8b..e18fa0e5e955 100644
--- a/writerfilter/source/rtftok/rtfsprm.hxx
+++ b/writerfilter/source/rtftok/rtfsprm.hxx
@@ -56,7 +56,7 @@ public:
     /// Removes elements which are already in the reference set.
     /// Also insert default values to override attributes of style
     /// (yes, really; that's what Word does).
-    RTFSprms cloneAndDeduplicate(RTFSprms& rReference) const;
+    RTFSprms cloneAndDeduplicate(RTFSprms& rReference, Id nStyleType) const;
     /// Inserts default values to override attributes of pAbstract.
     void duplicateList(const RTFValue::Pointer_t& pAbstract);
     /// Removes duplicated values based on in-list properties.