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.

Change-Id: I35b6599cc47fa51b8754fd921c61a3b31a283547
Reviewed-on: https://gerrit.libreoffice.org/60946
Reviewed-by: Miklos Vajna <vmiklos@collabora.co.uk>
Tested-by: Jenkins

5 files changed, 64 insertions, 31 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 fedfb7fbd4ce..d26a0f886e8c 100644
--- a/sw/qa/extras/rtfimport/rtfimport.cxx
+++ b/sw/qa/extras/rtfimport/rtfimport.cxx
@@ -1148,6 +1148,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 b5a96b401e89..f45e9b484cfd 100644
--- a/writerfilter/source/rtftok/rtfdocumentimpl.cxx
+++ b/writerfilter/source/rtftok/rtfdocumentimpl.cxx
@@ -487,12 +487,10 @@ 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)
-        {
-            RTFReferenceProperties& rProps
-                = *static_cast<RTFReferenceProperties*>(it->second.get());
-            lcl_copyFlatten(rProps, aStyleAttributes, aStyleSprms);
-        }
+        // Take paragraph style into account for character properties as well,
+        // as paragraph style may contain character properties.
+        RTFReferenceProperties& rProps = *static_cast<RTFReferenceProperties*>(it->second.get());
+        lcl_copyFlatten(rProps, aStyleAttributes, aStyleSprms);
 
         if (itChar != m_aStyleTableEntries.end())
         {
@@ -506,8 +504,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 new RTFReferenceProperties(attributes, sprms);
     }
 
diff --git a/writerfilter/source/rtftok/rtfsprm.cxx b/writerfilter/source/rtftok/rtfsprm.cxx
index c6cc55d1c655..dd80af103367 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 new RTFValue(0);
+        switch (id)
+        {
+            case NS_ooxml::LN_EG_RPrBase_b:
+                return new 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 new 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, new 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 65d4eda68d05..e46a7b68881f 100644
--- a/writerfilter/source/rtftok/rtfsprm.hxx
+++ b/writerfilter/source/rtftok/rtfsprm.hxx
@@ -61,7 +61,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.