Related: tdf#161652 sw, RTF paste: avoid duplicated numbering styles

Open the DOCX bugdoc from
<https://bugs.documentfoundation.org/show_bug.cgi?id=161652#c6>,
select-all, copy, open a second document, paste as RTF, paste as RTF
again, the number of char styles in the document change from 27 to 55,
then to 73. That is surprising, paragraph styles work by first creating
them then a 2nd paste just refers to them.

It seems what happens is that paragraph styles are handled in
StyleSheetTable::ApplyStyleSheetsImpl(), and we have an explicit "When
pasting, don't update existing styles" code there; on the other hand
ListDef::GetStyleName() solves the style name collision by appending
enough "a" characters at the end of the style name till there is no
collision.

Fix the inconsistency by adapting the list style behavior to match what
paragraph styles do: if we don't open an RTF file but paste into an
existing document, then try to use the existing style on collision.

Fixing this on the RTF producing side would be less effective, also one
could argue that in case a numbering uses e.g. 3 levels, then it still
makes sense to emit the definition for all levels to help the editor
once numbering levels are increased in the pasted content.

Change-Id: Ia211cb4300c3c41dae8c815ddfaf30cc2f0de8b5
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/169609
Reviewed-by: Miklos Vajna <vmiklos@collabora.com>
Tested-by: Jenkins

7 files changed, 160 insertions, 5 deletions

diff --git a/sw/CppunitTest_sw_writerfilter_dmapper.mk b/sw/CppunitTest_sw_writerfilter_dmapper.mk
index d865706c2b50..a56d02e4b71d 100644
--- a/sw/CppunitTest_sw_writerfilter_dmapper.mk
+++ b/sw/CppunitTest_sw_writerfilter_dmapper.mk
@@ -23,6 +23,7 @@ $(eval $(call gb_CppunitTest_add_exception_objects,sw_writerfilter_dmapper, \
     sw/qa/writerfilter/dmapper/DomainMapper \
     sw/qa/writerfilter/dmapper/DomainMapper_Impl \
     sw/qa/writerfilter/dmapper/GraphicImport \
+    sw/qa/writerfilter/dmapper/NumberingManager \
     sw/qa/writerfilter/dmapper/TableManager \
     sw/qa/writerfilter/dmapper/TextEffectsHandler \
     sw/qa/writerfilter/dmapper/PropertyMap \
diff --git a/sw/qa/writerfilter/dmapper/NumberingManager.cxx b/sw/qa/writerfilter/dmapper/NumberingManager.cxx
new file mode 100644
index 000000000000..8c60033a74d7
--- /dev/null
+++ b/sw/qa/writerfilter/dmapper/NumberingManager.cxx
@@ -0,0 +1,75 @@
+/* -*- 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/unoapi_test.hxx>
+
+#include <comphelper/propertyvalue.hxx>
+#include <unotools/streamwrap.hxx>
+
+#include <com/sun/star/container/XNameContainer.hpp>
+#include <com/sun/star/document/XImporter.hpp>
+#include <com/sun/star/document/XFilter.hpp>
+#include <com/sun/star/style/XStyleFamiliesSupplier.hpp>
+#include <com/sun/star/text/XTextDocument.hpp>
+
+using namespace com::sun::star;
+
+namespace
+{
+/// Tests for sw/source/writerfilter/dmapper/NumberingManager.cxx.
+class Test : public UnoApiTest
+{
+public:
+    Test()
+        : UnoApiTest(u"/sw/qa/writerfilter/dmapper/data/"_ustr)
+    {
+    }
+};
+
+CPPUNIT_TEST_FIXTURE(Test, testPasteBulletListStyleName)
+{
+    // Given a document with a WWNum1 list style:
+    mxComponent
+        = loadFromDesktop(u"private:factory/swriter"_ustr, u"com.sun.star.text.TextDocument"_ustr);
+    uno::Reference<style::XStyleFamiliesSupplier> xStyleFamiliesSupplier(mxComponent,
+                                                                         uno::UNO_QUERY);
+    uno::Reference<container::XNameAccess> xStyleFamilies
+        = xStyleFamiliesSupplier->getStyleFamilies();
+    uno::Reference<container::XNameContainer> xStyles;
+    xStyleFamilies->getByName(u"NumberingStyles"_ustr) >>= xStyles;
+    uno::Reference<lang::XMultiServiceFactory> xFactory(mxComponent, uno::UNO_QUERY);
+    uno::Reference<uno::XInterface> xStyle
+        = xFactory->createInstance("com.sun.star.style.NumberingStyle");
+    xStyles->insertByName(u"WWNum1"_ustr, uno::Any(xStyle));
+
+    // When pasting bullets to that document:
+    uno::Reference<text::XTextDocument> xTextDocument(mxComponent, uno::UNO_QUERY);
+    uno::Reference<text::XTextRange> xText = xTextDocument->getText();
+    uno::Reference<text::XTextRange> xBodyEnd = xText->getEnd();
+    uno::Reference<document::XFilter> xFilter(
+        m_xSFactory->createInstance(u"com.sun.star.comp.Writer.RtfFilter"_ustr), uno::UNO_QUERY);
+    uno::Reference<document::XImporter> xImporter(xFilter, uno::UNO_QUERY);
+    xImporter->setTargetDocument(mxComponent);
+    std::unique_ptr<SvStream> pStream(
+        new SvFileStream(createFileURL(u"clipboard-bullets.rtf"), StreamMode::READ));
+    uno::Reference<io::XStream> xStream(new utl::OStreamWrapper(std::move(pStream)));
+    uno::Sequence aDescriptor{ comphelper::makePropertyValue(u"InputStream"_ustr, xStream),
+                               comphelper::makePropertyValue(u"InsertMode"_ustr, true),
+                               comphelper::makePropertyValue(u"TextInsertModeRange"_ustr,
+                                                             xBodyEnd) };
+    CPPUNIT_ASSERT(xFilter->filter(aDescriptor));
+
+    // Then make sure we don't create new list styles, but reuse the existing ones:
+    // Without the accompanying fix in place, this test would have failed, new character styles were
+    // created again and again on each paste.
+    CPPUNIT_ASSERT(!xStyles->hasByName(u"WWNum1a"_ustr));
+}
+}
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/sw/qa/writerfilter/dmapper/data/clipboard-bullets.rtf b/sw/qa/writerfilter/dmapper/data/clipboard-bullets.rtf
new file mode 100644
index 000000000000..1e9202125ad8
--- /dev/null
+++ b/sw/qa/writerfilter/dmapper/data/clipboard-bullets.rtf
@@ -0,0 +1,48 @@
+{\rtf1\ansi\deff4\adeflang1025
+{\fonttbl
+{\f0\froman\fprq2\fcharset0 Times New Roman;}
+{\f1\froman\fprq2\fcharset2 Symbol;}
+{\f2\fswiss\fprq2\fcharset0 Arial;}
+{\f3\froman\fprq2\fcharset0 Liberation Serif
+{\*\falt Times New Roman}
+;}
+{\f4\froman\fprq2\fcharset0 Calibri;}
+}
+{\stylesheet
+{\s0\snext0\rtlch\afs22\alang1025 \ltrch\lang1033\langfe1033\hich\af4\loch\sl259\slmult1\ql\widctlpar\sb0\sa160\ltrpar\hyphpar0\cf0\f4\fs22\lang1033\kerning1\dbch\af10\langfe1033 Normal;}
+{\*\cs21\snext21 ListLabel 3;}
+{\*\cs22\snext22 ListLabel 2;}
+{\*\cs23\snext23 ListLabel 1;}
+{\s25\sbasedon0\snext25\loch\li720\lin720\sb0\sa160\contextualspace List Paragraph;}
+{\s26\sbasedon0\snext26\rtlch\af8 \ltrch\loch\noline Index;}
+{\s27\sbasedon0\snext27\rtlch\af8\afs24\ai \ltrch\loch\sb120\sa120\noline\fs24\i caption;}
+{\s28\sbasedon29\snext28\rtlch\af8 \ltrch List;}
+{\s29\sbasedon0\snext29\loch\sl276\slmult1\sb0\sa140 Body Text;}
+}
+{\*\listtable
+{\list\listtemplateid1
+{\listlevel\levelnfc23\leveljc0\levelstartat1\levelfollow0
+{\leveltext \'01\u61623 ?;}
+{\levelnumbers;}
+\fi-360\li720}
+{\listlevel\levelnfc23\leveljc0\levelstartat1\levelfollow0
+{\leveltext \'01\u111 ?;}
+{\levelnumbers;}
+\rtlch\af7 \ltrch\loch\fi-360\li1440}
+{\listlevel\levelnfc23\leveljc0\levelstartat1\levelfollow0
+{\leveltext \'01\u61607 ?;}
+{\levelnumbers;}
+\fi-360\li2160}
+\listid1}
+}
+{\listoverridetable
+{\listoverride\listid1\listoverridecount0\ls1}
+}
+\pard\plain before\par
+\pard\plain \s25\loch\li720\lin720\sb0\sa160\contextualspace
+\ilvl0\ls1 \fi-360\li1440\lin1440 A
+\par \pard\plain \s25\loch\li720\lin720\sb0\sa160\contextualspace
+\ilvl1\ls1 \fi-360\li2160\lin2160 B
+\par \pard\plain \s25\loch\li720\lin720\sb0\sa160\contextualspace
+\ilvl2\ls1 \fi-360\li2880\lin2880\sb0\sa160\contextualspace C
+}
diff --git a/sw/source/writerfilter/dmapper/DomainMapper.cxx b/sw/source/writerfilter/dmapper/DomainMapper.cxx
index adb29ba57c23..71760a6e3b56 100644
--- a/sw/source/writerfilter/dmapper/DomainMapper.cxx
+++ b/sw/source/writerfilter/dmapper/DomainMapper.cxx
@@ -5110,6 +5110,11 @@ OUString DomainMapper::GetUnusedCharacterStyleName()
     return m_pImpl->GetUnusedCharacterStyleName();
 }
 
+bool DomainMapper::IsNewDoc() const
+{
+    return m_pImpl->IsNewDoc();
+}
+
 } //namespace writerfilter
 
 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/sw/source/writerfilter/dmapper/DomainMapper.hxx b/sw/source/writerfilter/dmapper/DomainMapper.hxx
index ecd5c4459273..fb03a9a9fd8b 100644
--- a/sw/source/writerfilter/dmapper/DomainMapper.hxx
+++ b/sw/source/writerfilter/dmapper/DomainMapper.hxx
@@ -143,6 +143,7 @@ public:
 
     css::uno::Reference<css::container::XNameContainer> const & GetCharacterStyles();
     OUString GetUnusedCharacterStyleName();
+    bool IsNewDoc() const;
 
 private:
     // Stream
diff --git a/sw/source/writerfilter/dmapper/NumberingManager.cxx b/sw/source/writerfilter/dmapper/NumberingManager.cxx
index 6147af5e0228..3c4c89f156e5 100644
--- a/sw/source/writerfilter/dmapper/NumberingManager.cxx
+++ b/sw/source/writerfilter/dmapper/NumberingManager.cxx
@@ -421,7 +421,8 @@ ListDef::~ListDef( )
 }
 
 const OUString & ListDef::GetStyleName(sal_Int32 const nId,
-    uno::Reference<container::XNameContainer> const& xStyles)
+    uno::Reference<container::XNameContainer> const& xStyles,
+    DomainMapper& rDMapper)
 {
     if (xStyles.is())
     {
@@ -429,6 +430,12 @@ const OUString & ListDef::GetStyleName(sal_Int32 const nId,
 
         while (xStyles->hasByName(sStyleName)) // unique
         {
+            if (!rDMapper.IsNewDoc())
+            {
+                // When pasting, don't rename our style, just use the existing one.
+                break;
+            }
+
             sStyleName += "a";
         }
 
@@ -538,12 +545,24 @@ void ListDef::CreateNumberingRules( DomainMapper& rDMapper,
     try
     {
         // Create the numbering style
+        bool bUpdate = true;
         if (GetId() == nOutline)
             m_StyleName = "Outline"; //SwNumRule.GetOutlineRuleName()
         else
-            xStyles->insertByName(
-                GetStyleName(GetId(), xStyles),
-                css::uno::Any(uno::Reference<css::style::XStyle>(xTextDoc->createNumberingStyle())));
+        {
+            OUString aStyleName = GetStyleName(GetId(), xStyles, rDMapper);
+            if (xStyles->hasByName(aStyleName) && !rDMapper.IsNewDoc())
+            {
+                // When pasting, don't update existing styles.
+                bUpdate = false;
+            }
+            else
+            {
+                xStyles->insertByName(
+                    aStyleName,
+                    css::uno::Any(uno::Reference<css::style::XStyle>(xTextDoc->createNumberingStyle())));
+            }
+        }
 
         uno::Any oStyle = xStyles->getByName(GetStyleName());
         uno::Reference< beans::XPropertySet > xStyle( oStyle, uno::UNO_QUERY_THROW );
@@ -551,6 +570,12 @@ void ListDef::CreateNumberingRules( DomainMapper& rDMapper,
         // Get the default OOo Numbering style rules
         uno::Any aRules = xStyle->getPropertyValue( getPropertyName( PROP_NUMBERING_RULES ) );
         aRules >>= m_xNumRules;
+        if (!bUpdate)
+        {
+            // If it was requested that we don't update existing styles, we fetched the numbering
+            // rules, don't modify them.
+            return;
+        }
 
         uno::Sequence<uno::Sequence<beans::PropertyValue>> aProps = GetMergedPropertyValues();
 
diff --git a/sw/source/writerfilter/dmapper/NumberingManager.hxx b/sw/source/writerfilter/dmapper/NumberingManager.hxx
index 36571bc87ec4..767b6b5d60e9 100644
--- a/sw/source/writerfilter/dmapper/NumberingManager.hxx
+++ b/sw/source/writerfilter/dmapper/NumberingManager.hxx
@@ -189,7 +189,7 @@ public:
 
     // Mapping functions
     const OUString & GetStyleName() const { return m_StyleName; };
-    const OUString & GetStyleName(sal_Int32 nId, css::uno::Reference<css::container::XNameContainer> const& xStyles);
+    const OUString & GetStyleName(sal_Int32 nId, css::uno::Reference<css::container::XNameContainer> const& xStyles, DomainMapper& rDMapper);
 
     css::uno::Sequence< css::uno::Sequence<css::beans::PropertyValue> > GetMergedPropertyValues();