include/svl/stylepool.hxx | 6 +- svl/CppunitTest_svl_items.mk | 1 svl/qa/unit/items/stylepool.cxx | 91 ++++++++++++++++++++++++++++++++++ svl/source/items/stylepool.cxx | 57 +++++++++++++++++---- sw/inc/istyleaccess.hxx | 3 - sw/source/core/doc/swstylemanager.cxx | 8 +- sw/source/core/txtnode/ndtxt.cxx | 2 7 files changed, 152 insertions(+), 16 deletions(-)
New commits: commit b2a5a44003203487c555ca640800b4e8f82cf9f1 Author: Miklos Vajna <[email protected]> AuthorDate: Wed Mar 20 14:49:15 2019 +0100 Commit: Miklos Vajna <[email protected]> CommitDate: Thu Mar 21 17:11:13 2019 +0100 sw: make ODT export of paragraph auto-styles deterministic If a complex enough document is loaded into Writer and saved as ODT, then the content.xml's automatic paragraph styles (P<num>) are re-ordered on each save, which leads to unnecessary noise. The actual random order is created during import by the time we convert direct formatting (e.g. from HTML import) to autostyles, as StylePoolImpl::maRoot stores autostyles in a map that orders autostyle parents based on their pointer address. This has benefits like automatic ordering of item sets and fast comparison, so don't change that, but extend the svl API to also track the name of those parents. This way by the time StylePool::createIterator() would iterate over those autostyles, it can order the parents by their name, so two import->export runs will result in the same autostyle ordering. (This appears to be the only indeterminism in content.xml for a test HTML input, while meta.xml and settings.xml still changes all the time.) (cherry picked from commit eb128a7d6bbc27b4dbbf9461c81c90e40203b114) Conflicts: svl/source/items/stylepool.cxx Change-Id: I1cfcae2c664a5c5c3dee48be733046979c1593ed diff --git a/include/svl/stylepool.hxx b/include/svl/stylepool.hxx index fc8a6802d10f..04dcdaa8be17 100644 --- a/include/svl/stylepool.hxx +++ b/include/svl/stylepool.hxx @@ -40,9 +40,13 @@ public: @param SfxItemSet the SfxItemSet to insert + @param pParentName + Name of the parent of rSet. If set, createIterator() can be more deterministic by iterating + over item sets ordered by parent names. + @return a shared pointer to the SfxItemSet */ - std::shared_ptr<SfxItemSet> insertItemSet( const SfxItemSet& rSet ); + std::shared_ptr<SfxItemSet> insertItemSet( const SfxItemSet& rSet, const OUString* pParentName = nullptr ); /** Create an iterator diff --git a/svl/CppunitTest_svl_items.mk b/svl/CppunitTest_svl_items.mk index c8e5e0699635..28e8772d71ad 100644 --- a/svl/CppunitTest_svl_items.mk +++ b/svl/CppunitTest_svl_items.mk @@ -15,6 +15,7 @@ $(eval $(call gb_CppunitTest_use_sdk_api,svl_items)) $(eval $(call gb_CppunitTest_add_exception_objects,svl_items, \ svl/qa/unit/items/test_IndexedStyleSheets \ + svl/qa/unit/items/stylepool \ )) $(eval $(call gb_CppunitTest_use_libraries,svl_items, \ diff --git a/svl/qa/unit/items/stylepool.cxx b/svl/qa/unit/items/stylepool.cxx new file mode 100644 index 000000000000..bf89fd30cb83 --- /dev/null +++ b/svl/qa/unit/items/stylepool.cxx @@ -0,0 +1,91 @@ +/* -*- 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 <unotest/bootstrapfixturebase.hxx> + +#include <svl/itempool.hxx> +#include <svl/itemset.hxx> +#include <svl/stylepool.hxx> +#include <svl/stritem.hxx> +#include <sal/log.hxx> + +namespace +{ +/// Tests svl StylePool. +class StylePoolTest : public CppUnit::TestFixture +{ + void testIterationOrder(); + + CPPUNIT_TEST_SUITE(StylePoolTest); + CPPUNIT_TEST(testIterationOrder); + CPPUNIT_TEST_SUITE_END(); +}; + +void StylePoolTest::testIterationOrder() +{ + // Set up a style pool with multiple parents. + SfxStringItem aDefault1(1); + std::vector<SfxPoolItem*> aDefaults{ &aDefault1 }; + SfxItemInfo const aItems[] = { { 1, false } }; + + SfxItemPool* pPool = new SfxItemPool("test", 1, 1, aItems); + pPool->SetDefaults(&aDefaults); + { + // Set up parents in mixed order to make sure we do not sort by pointer address. + SfxItemSet aParent1(*pPool, 1, 1); + SfxItemSet aChild1(*pPool, 1, 1); + aChild1.SetParent(&aParent1); + SfxStringItem aItem1(1, "Item1"); + aChild1.Put(aItem1); + + SfxItemSet aParent3(*pPool, 1, 1); + SfxItemSet aChild3(*pPool, 1, 1); + aChild3.SetParent(&aParent3); + SfxStringItem aItem3(1, "Item3"); + aChild3.Put(aItem3); + + SfxItemSet aParent2(*pPool, 1, 1); + SfxItemSet aChild2(*pPool, 1, 1); + aChild2.SetParent(&aParent2); + SfxStringItem aItem2(1, "Item2"); + aChild2.Put(aItem2); + + // Insert item sets in alphabetical order. + StylePool aStylePool; + OUString aChild1Name("Child1"); + aStylePool.insertItemSet(aChild1, &aChild1Name); + OUString aChild3Name("Child3"); + aStylePool.insertItemSet(aChild3, &aChild3Name); + OUString aChild2Name("Child2"); + aStylePool.insertItemSet(aChild2, &aChild2Name); + std::unique_ptr<IStylePoolIteratorAccess> pIter(aStylePool.createIterator()); + std::shared_ptr<SfxItemSet> pStyle1 = pIter->getNext(); + CPPUNIT_ASSERT(pStyle1.get()); + const SfxStringItem* pItem1 = static_cast<const SfxStringItem*>(pStyle1->GetItem(1)); + CPPUNIT_ASSERT_EQUAL(OUString("Item1"), pItem1->GetValue()); + std::shared_ptr<SfxItemSet> pStyle2 = pIter->getNext(); + CPPUNIT_ASSERT(pStyle2.get()); + const SfxStringItem* pItem2 = static_cast<const SfxStringItem*>(pStyle2->GetItem(1)); + // Without the accompanying fix in place, this test would have failed with 'Expected: Item2; + // Actual: Item3'. The iteration order depended on the pointer address on the pointer + // address of the parents. + CPPUNIT_ASSERT_EQUAL(OUString("Item2"), pItem2->GetValue()); + std::shared_ptr<SfxItemSet> pStyle3 = pIter->getNext(); + CPPUNIT_ASSERT(pStyle3.get()); + const SfxStringItem* pItem3 = static_cast<const SfxStringItem*>(pStyle3->GetItem(1)); + CPPUNIT_ASSERT_EQUAL(OUString("Item3"), pItem3->GetValue()); + CPPUNIT_ASSERT(!pIter->getNext()); + } + SfxItemPool::Free(pPool); +} +} + +CPPUNIT_TEST_SUITE_REGISTRATION(StylePoolTest); + +/* vim:set shiftwidth=4 softtabstop=4 expandtab: */ diff --git a/svl/source/items/stylepool.cxx b/svl/source/items/stylepool.cxx index cfc61ab4ed0e..ff631fc7ac5c 100644 --- a/svl/source/items/stylepool.cxx +++ b/svl/source/items/stylepool.cxx @@ -279,29 +279,62 @@ namespace { Node* mpNode; const bool mbSkipUnusedItemSets; const bool mbSkipIgnorable; + /// List of item set parents, ordered by their name. + std::vector<const SfxItemSet*> maParents; + /// The iterator's current position. + std::vector<const SfxItemSet*>::iterator mpCurrParent; public: // #i86923# Iterator( std::map< const SfxItemSet*, Node >& rR, const bool bSkipUnusedItemSets, - const bool bSkipIgnorable ) + const bool bSkipIgnorable, + const std::map< const SfxItemSet*, OUString>& rParentNames ) : mrRoot( rR ), - mpCurrNode( rR.begin() ), mpNode(nullptr), mbSkipUnusedItemSets( bSkipUnusedItemSets ), mbSkipIgnorable( bSkipIgnorable ) - {} + { + // Collect the parent pointers into a vector we can sort. + for (const auto& rParent : mrRoot) + maParents.push_back(rParent.first); + + // Sort the parents using their name, if they have one. + if (!rParentNames.empty()) + { + std::sort(maParents.begin(), maParents.end(), + [&rParentNames](const SfxItemSet* pA, const SfxItemSet* pB) { + OUString aA; + OUString aB; + auto it = rParentNames.find(pA); + if (it != rParentNames.end()) + aA = it->second; + it = rParentNames.find(pB); + if (it != rParentNames.end()) + aB = it->second; + return aA < aB; + }); + } + + // Start the iteration. + mpCurrParent = maParents.begin(); + if (mpCurrParent != maParents.end()) + mpCurrNode = mrRoot.find(*mpCurrParent); + } virtual std::shared_ptr<SfxItemSet> getNext() override; }; std::shared_ptr<SfxItemSet> Iterator::getNext() { std::shared_ptr<SfxItemSet> pReturn; - while( mpNode || mpCurrNode != mrRoot.end() ) + while( mpNode || mpCurrParent != maParents.end() ) { if( !mpNode ) { mpNode = &mpCurrNode->second; - ++mpCurrNode; + // Perform the actual increment. + ++mpCurrParent; + if (mpCurrParent != maParents.end()) + mpCurrNode = mrRoot.find(*mpCurrParent); // #i86923# if ( mpNode->hasItemSet( mbSkipUnusedItemSets ) ) { @@ -346,6 +379,8 @@ class StylePoolImpl { private: std::map< const SfxItemSet*, Node > maRoot; + /// Names of maRoot keys. + std::map< const SfxItemSet*, OUString> maParentNames; // #i86923# std::unique_ptr<SfxItemSet> mpIgnorableItems; public: @@ -362,17 +397,19 @@ public: "<StylePoolImpl::StylePoolImpl(..)> - <SfxItemSet::Clone( sal_False )> does not work as excepted - <mpIgnorableItems> is not empty." ); } - std::shared_ptr<SfxItemSet> insertItemSet( const SfxItemSet& rSet ); + std::shared_ptr<SfxItemSet> insertItemSet( const SfxItemSet& rSet, const OUString* pParentName = nullptr ); // #i86923# IStylePoolIteratorAccess* createIterator( bool bSkipUnusedItemSets, bool bSkipIgnorableItems ); }; -std::shared_ptr<SfxItemSet> StylePoolImpl::insertItemSet( const SfxItemSet& rSet ) +std::shared_ptr<SfxItemSet> StylePoolImpl::insertItemSet( const SfxItemSet& rSet, const OUString* pParentName ) { bool bNonPoolable = false; Node* pCurNode = &maRoot[ rSet.GetParent() ]; + if (pParentName) + maParentNames[ rSet.GetParent() ] = *pParentName; SfxItemIter aIter( rSet ); const SfxPoolItem* pItem = aIter.GetCurItem(); // Every SfxPoolItem in the SfxItemSet causes a step deeper into the tree, @@ -439,7 +476,7 @@ std::shared_ptr<SfxItemSet> StylePoolImpl::insertItemSet( const SfxItemSet& rSet IStylePoolIteratorAccess* StylePoolImpl::createIterator( bool bSkipUnusedItemSets, bool bSkipIgnorableItems ) { - return new Iterator( maRoot, bSkipUnusedItemSets, bSkipIgnorableItems ); + return new Iterator( maRoot, bSkipUnusedItemSets, bSkipIgnorableItems, maParentNames ); } // Ctor, Dtor and redirected methods of class StylePool, nearly inline ;-) @@ -448,8 +485,8 @@ StylePool::StylePool( SfxItemSet* pIgnorableItems ) : pImpl( new StylePoolImpl( pIgnorableItems ) ) {} -std::shared_ptr<SfxItemSet> StylePool::insertItemSet( const SfxItemSet& rSet ) -{ return pImpl->insertItemSet( rSet ); } +std::shared_ptr<SfxItemSet> StylePool::insertItemSet( const SfxItemSet& rSet, const OUString* pParentName ) +{ return pImpl->insertItemSet( rSet, pParentName ); } // #i86923# IStylePoolIteratorAccess* StylePool::createIterator( const bool bSkipUnusedItemSets, diff --git a/sw/inc/istyleaccess.hxx b/sw/inc/istyleaccess.hxx index 1f47ee8ecb82..32d6efc47eab 100644 --- a/sw/inc/istyleaccess.hxx +++ b/sw/inc/istyleaccess.hxx @@ -37,7 +37,8 @@ public: virtual ~IStyleAccess() {} virtual std::shared_ptr<SfxItemSet> getAutomaticStyle( const SfxItemSet& rSet, - SwAutoStyleFamily eFamily ) = 0; + SwAutoStyleFamily eFamily, + const OUString* pParentName = nullptr ) = 0; virtual void getAllStyles( std::vector<std::shared_ptr<SfxItemSet>> &rStyles, SwAutoStyleFamily eFamily ) = 0; /** It's slow to iterate through a stylepool looking for a special name, but if diff --git a/sw/source/core/doc/swstylemanager.cxx b/sw/source/core/doc/swstylemanager.cxx index 483ca3885d3d..628d8fc0f3e6 100644 --- a/sw/source/core/doc/swstylemanager.cxx +++ b/sw/source/core/doc/swstylemanager.cxx @@ -71,7 +71,8 @@ public: {} virtual ~SwStyleManager() override; virtual std::shared_ptr<SfxItemSet> getAutomaticStyle( const SfxItemSet& rSet, - IStyleAccess::SwAutoStyleFamily eFamily ) override; + IStyleAccess::SwAutoStyleFamily eFamily, + const OUString* pParentName = nullptr ) override; virtual std::shared_ptr<SfxItemSet> getByName( const OUString& rName, IStyleAccess::SwAutoStyleFamily eFamily ) override; virtual void getAllStyles( std::vector<std::shared_ptr<SfxItemSet>> &rStyles, @@ -101,10 +102,11 @@ void SwStyleManager::clearCaches() } std::shared_ptr<SfxItemSet> SwStyleManager::getAutomaticStyle( const SfxItemSet& rSet, - IStyleAccess::SwAutoStyleFamily eFamily ) + IStyleAccess::SwAutoStyleFamily eFamily, + const OUString* pParentName ) { StylePool& rAutoPool = eFamily == IStyleAccess::AUTO_STYLE_CHAR ? aAutoCharPool : aAutoParaPool; - return rAutoPool.insertItemSet( rSet ); + return rAutoPool.insertItemSet( rSet, pParentName ); } std::shared_ptr<SfxItemSet> SwStyleManager::cacheAutomaticStyle( const SfxItemSet& rSet, diff --git a/sw/source/core/txtnode/ndtxt.cxx b/sw/source/core/txtnode/ndtxt.cxx index 6f9092e9295d..e0b6f0a3edee 100644 --- a/sw/source/core/txtnode/ndtxt.cxx +++ b/sw/source/core/txtnode/ndtxt.cxx @@ -835,7 +835,7 @@ void SwTextNode::NewAttrSet( SwAttrPool& rPool ) aNewAttrSet.Put( aFormatColl ); aNewAttrSet.SetParent( &pAnyFormatColl->GetAttrSet() ); - mpAttrSet = GetDoc()->GetIStyleAccess().getAutomaticStyle( aNewAttrSet, IStyleAccess::AUTO_STYLE_PARA ); + mpAttrSet = GetDoc()->GetIStyleAccess().getAutomaticStyle( aNewAttrSet, IStyleAccess::AUTO_STYLE_PARA, &sVal ); } // override SwIndexReg::Update => text hints do not need SwIndex for start/end! _______________________________________________ Libreoffice-commits mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/libreoffice-commits
