From 3022e2759c0194c10a335e4dcf6a67e5c0770e15 Mon Sep 17 00:00:00 2001 From: Dennis Francis Date: Fri, 19 Apr 2019 23:15:53 +0530 Subject: tdf#122590: follow-up : import x14:cfRule priorities If there are x:cfRule's and x14:cfRule's with matching range-list, then insert the conditional-fmt entries into the document in the order of the priorities. That is don't just append the x14:cfRule entries to the document after the x:cfRule entries are inserted. There was also a off-by-one bug in the priority export of x14:cfRule entries. This caused the priority numbers to be duplicated. This is also fixed. Reviewed-on: https://gerrit.libreoffice.org/71311 Tested-by: Jenkins Reviewed-by: Andras Timar (cherry picked from commit c2f1c68ffb6dfa1ce7de09dcc428d6c53549e88d) Change-Id: I5b0d11c4456b2966b808f6ee589075a870f43768 Reviewed-on: https://gerrit.libreoffice.org/72003 Reviewed-by: Andras Timar Tested-by: Andras Timar Reviewed-on: https://gerrit.libreoffice.org/79221 --- .../data/xlsx/conditional_fmt_checkpriority.xlsx | Bin 0 -> 10682 bytes sc/qa/unit/subsequent_export-test.cxx | 48 +++++++++++ sc/source/filter/excel/xeextlst.cxx | 2 +- sc/source/filter/inc/condformatbuffer.hxx | 13 ++- sc/source/filter/inc/extlstcontext.hxx | 4 +- sc/source/filter/oox/condformatbuffer.cxx | 93 +++++++++++++++++++-- sc/source/filter/oox/extlstcontext.cxx | 15 +++- 7 files changed, 163 insertions(+), 12 deletions(-) create mode 100644 sc/qa/unit/data/xlsx/conditional_fmt_checkpriority.xlsx diff --git a/sc/qa/unit/data/xlsx/conditional_fmt_checkpriority.xlsx b/sc/qa/unit/data/xlsx/conditional_fmt_checkpriority.xlsx new file mode 100644 index 000000000000..e9af11d00615 Binary files /dev/null and b/sc/qa/unit/data/xlsx/conditional_fmt_checkpriority.xlsx differ diff --git a/sc/qa/unit/subsequent_export-test.cxx b/sc/qa/unit/subsequent_export-test.cxx index f3f0bd291f4a..29849f8531e7 100644 --- a/sc/qa/unit/subsequent_export-test.cxx +++ b/sc/qa/unit/subsequent_export-test.cxx @@ -112,6 +112,7 @@ public: void testDataBarExportXLSX(); void testConditionalFormatRangeListXLSX(); void testConditionalFormatContainsTextXLSX(); + void testConditionalFormatPriorityCheckXLSX(); void testMiscRowHeightExport(); void testNamedRangeBugfdo62729(); void testBuiltinRangesXLSX(); @@ -242,6 +243,7 @@ public: CPPUNIT_TEST(testDataBarExportXLSX); CPPUNIT_TEST(testConditionalFormatRangeListXLSX); CPPUNIT_TEST(testConditionalFormatContainsTextXLSX); + CPPUNIT_TEST(testConditionalFormatPriorityCheckXLSX); CPPUNIT_TEST(testMiscRowHeightExport); CPPUNIT_TEST(testNamedRangeBugfdo62729); CPPUNIT_TEST(testBuiltinRangesXLSX); @@ -373,6 +375,8 @@ void ScExportTest::registerNamespaces(xmlXPathContextPtr& pXmlXPathCtx) { BAD_CAST("r"), BAD_CAST("http://schemas.openxmlformats.org/package/2006/relationships") }, { BAD_CAST("number"), BAD_CAST("urn:oasis:names:tc:opendocument:xmlns:datastyle:1.0") }, { BAD_CAST("loext"), BAD_CAST("urn:org:documentfoundation:names:experimental:office:xmlns:loext:1.0") }, + { BAD_CAST("x14"), BAD_CAST("http://schemas.microsoft.com/office/spreadsheetml/2009/9/main") }, + { BAD_CAST("xm"), BAD_CAST("http://schemas.microsoft.com/office/excel/2006/main") }, }; for(size_t i = 0; i < SAL_N_ELEMENTS(aNamespaces); ++i) { @@ -3998,6 +4002,50 @@ void ScExportTest::testConditionalFormatContainsTextXLSX() assertXPathContent(pDoc, "//x:conditionalFormatting/x:cfRule/x:formula", "NOT(ISERROR(SEARCH(\"test\",A1)))"); } +void ScExportTest::testConditionalFormatPriorityCheckXLSX() +{ + ScDocShellRef xDocSh = loadDoc("conditional_fmt_checkpriority.", FORMAT_XLSX); + CPPUNIT_ASSERT(xDocSh.is()); + + xmlDocPtr pDoc = XPathHelper::parseExport2(*this, *xDocSh, m_xSFactory, "xl/worksheets/sheet1.xml", FORMAT_XLSX); + CPPUNIT_ASSERT(pDoc); + + constexpr bool bHighPriorityExtensionA1 = true; // Should A1's extension cfRule has higher priority than normal cfRule ? + constexpr bool bHighPriorityExtensionA3 = false; // Should A3's extension cfRule has higher priority than normal cfRule ? + + size_t nA1NormalPriority = 0; + size_t nA1ExtPriority = 0; + size_t nA3NormalPriority = 0; + size_t nA3ExtPriority = 0; + + for (size_t nIdx = 1; nIdx <= 2; ++nIdx) + { + OString aIdx = OString::number(nIdx); + OUString aCellAddr = getXPath(pDoc, "//x:conditionalFormatting[" + aIdx + "]", "sqref"); + OUString aPriority = getXPath(pDoc, "//x:conditionalFormatting[" + aIdx + "]/x:cfRule", "priority");; + + CPPUNIT_ASSERT_MESSAGE("conditionalFormatting sqref must be either A1 or A3", aCellAddr == "A1" || aCellAddr == "A3"); + + if (aCellAddr == "A1") + nA1NormalPriority = aPriority.toUInt32(); + else + nA3NormalPriority = aPriority.toUInt32(); + + aCellAddr = getXPathContent(pDoc, "//x:extLst/x:ext[1]/x14:conditionalFormattings/x14:conditionalFormatting[" + aIdx + "]/xm:sqref"); + aPriority = getXPath(pDoc, "//x:extLst/x:ext[1]/x14:conditionalFormattings/x14:conditionalFormatting[" + aIdx + "]/x14:cfRule", "priority"); + + CPPUNIT_ASSERT_MESSAGE("x14:conditionalFormatting sqref must be either A1 or A3", aCellAddr == "A1" || aCellAddr == "A3"); + + if (aCellAddr == "A1") + nA1ExtPriority = aPriority.toUInt32(); + else + nA3ExtPriority = aPriority.toUInt32(); + } + + CPPUNIT_ASSERT_EQUAL_MESSAGE("Wrong priorities for A1", bHighPriorityExtensionA1, nA1ExtPriority < nA1NormalPriority); + CPPUNIT_ASSERT_EQUAL_MESSAGE("Wrong priorities for A3", bHighPriorityExtensionA3, nA3ExtPriority < nA3NormalPriority); +} + void ScExportTest::testEscapeCharInNumberFormatXLSX() { ScDocShellRef xDocSh = loadDoc("tdf81939.", FORMAT_XLSX); diff --git a/sc/source/filter/excel/xeextlst.cxx b/sc/source/filter/excel/xeextlst.cxx index dc7902065df1..78103231e096 100644 --- a/sc/source/filter/excel/xeextlst.cxx +++ b/sc/source/filter/excel/xeextlst.cxx @@ -408,7 +408,7 @@ void XclExpExtCfRule::SaveXml( XclExpXmlStream& rStrm ) sax_fastparser::FSHelperPtr& rWorksheet = rStrm.GetCurrentStream(); rWorksheet->startElementNS( XML_x14, XML_cfRule, XML_type, pType, - XML_priority, mnPriority == -1 ? nullptr : OString::number(mnPriority).getStr(), + XML_priority, mnPriority == -1 ? nullptr : OString::number(mnPriority + 1).getStr(), XML_operator, mOperator, XML_id, maId.getStr(), FSEND ); diff --git a/sc/source/filter/inc/condformatbuffer.hxx b/sc/source/filter/inc/condformatbuffer.hxx index d67f648e8086..b59db9654be6 100644 --- a/sc/source/filter/inc/condformatbuffer.hxx +++ b/sc/source/filter/inc/condformatbuffer.hxx @@ -160,6 +160,9 @@ public: /** Imports rule settings from a CFRULE record. */ void importCfRule( SequenceInputStream& rStrm ); + /** Directly set a ScFormatEntry with a priority ready for finalizeImport(). */ + void setFormatEntry(sal_Int32 nPriority, ScFormatEntry* pEntry); + /** Creates a conditional formatting rule in the Calc document. */ void finalizeImport(); @@ -174,6 +177,7 @@ private: const CondFormat& mrCondFormat; CondFormatRuleModel maModel; ScConditionalFormat* mpFormat; + ScFormatEntry* mpFormatEntry; std::unique_ptr mpColor; std::unique_ptr mpDataBar; std::unique_ptr mpIconSet; @@ -190,9 +194,12 @@ struct CondFormatModel explicit CondFormatModel(); }; +class CondFormatBuffer; + /** Represents a conditional formatting object with a list of affected cell ranges. */ class CondFormat : public WorksheetHelper { +friend class CondFormatBuffer; public: explicit CondFormat( const WorksheetHelper& rHelper ); @@ -268,15 +275,18 @@ public: class ExtCfCondFormat { public: - ExtCfCondFormat(const ScRangeList& aRange, std::vector< std::unique_ptr >& rEntries); + ExtCfCondFormat(const ScRangeList& aRange, std::vector< std::unique_ptr >& rEntries, + std::vector* pPriorities = nullptr); ~ExtCfCondFormat(); const ScRangeList& getRange(); const std::vector< std::unique_ptr >& getEntries(); + const std::vector& getPriorities() const { return maPriorities; } private: std::vector< std::unique_ptr > maEntries; ScRangeList const maRange; + std::vector maPriorities; }; typedef std::shared_ptr< CondFormat > CondFormatRef; @@ -307,6 +317,7 @@ private: CondFormatVec maCondFormats; /// All conditional formatting in a sheet. ExtCfDataBarRuleVec maCfRules; /// All external conditional formatting rules in a sheet. std::vector< std::unique_ptr > maExtCondFormats; + sal_Int32 mnNonPrioritizedRuleNextPriority = 1048576; }; } // namespace xls diff --git a/sc/source/filter/inc/extlstcontext.hxx b/sc/source/filter/inc/extlstcontext.hxx index 85aafd1604d1..31754d7bda98 100644 --- a/sc/source/filter/inc/extlstcontext.hxx +++ b/sc/source/filter/inc/extlstcontext.hxx @@ -57,11 +57,13 @@ public: private: OUString aChars; // Characters of between xml elements. OUString rStyle; // Style of the corresponding condition + sal_Int32 nPriority; // Priority of last cfRule element. ScConditionMode eOperator; // Used only when cfRule type is "cellIs" bool isPreviousElementF; // Used to distinguish alone from and std::vector > maEntries; - std::unique_ptr mpCurrentRule; std::vector< OUString > rFormulas; // It holds formulas for a range, there can be more formula for same range. + std::unique_ptr mpCurrentRule; + std::vector maPriorities; }; /** diff --git a/sc/source/filter/oox/condformatbuffer.cxx b/sc/source/filter/oox/condformatbuffer.cxx index 088e33a7c061..0962cc51a1f0 100644 --- a/sc/source/filter/oox/condformatbuffer.cxx +++ b/sc/source/filter/oox/condformatbuffer.cxx @@ -18,6 +18,8 @@ */ #include +#include +#include #include #include @@ -432,7 +434,8 @@ void CondFormatRuleModel::setBiff12TextType( sal_Int32 nOperator ) CondFormatRule::CondFormatRule( const CondFormat& rCondFormat, ScConditionalFormat* pFormat ) : WorksheetHelper( rCondFormat ), mrCondFormat( rCondFormat ), - mpFormat(pFormat) + mpFormat(pFormat), + mpFormatEntry(nullptr) { } @@ -706,8 +709,20 @@ void CondFormatRule::importCfRule( SequenceInputStream& rStrm ) } } +void CondFormatRule::setFormatEntry(sal_Int32 nPriority, ScFormatEntry* pEntry) +{ + maModel.mnPriority = nPriority; + mpFormatEntry = pEntry; +} + void CondFormatRule::finalizeImport() { + if (mpFormatEntry) + { + mpFormat->AddEntry(mpFormatEntry); + return; + } + ScConditionMode eOperator = ScConditionMode::NONE; /* Replacement formula for unsupported rule types (text comparison rules, @@ -1099,10 +1114,63 @@ ScConditionalFormat* findFormatByRange(const ScRangeList& rRange, const ScDocume return nullptr; } +class ScRangeListHasher +{ +public: + size_t operator() (ScRangeList const& rRanges) const + { + size_t nHash = 0; + for (size_t nIdx = 0; nIdx < rRanges.size(); ++nIdx) + nHash += rRanges[nIdx].hashArea(); + return nHash; + } +}; + } void CondFormatBuffer::finalizeImport() { + std::unordered_set aDoneExtCFs; + typedef std::unordered_map RangeMap; + typedef std::vector> FormatEntries; + RangeMap aRangeMap; + for (auto& rxCondFormat : maCondFormats) + { + if (aRangeMap.find(rxCondFormat->getRanges()) != aRangeMap.end()) + continue; + aRangeMap[rxCondFormat->getRanges()] = rxCondFormat.get(); + } + + size_t nExtCFIndex = 0; + for (const auto& rxExtCondFormat : maExtCondFormats) + { + ScDocument* pDoc = &getScDocument(); + const ScRangeList& rRange = rxExtCondFormat->getRange(); + RangeMap::iterator it = aRangeMap.find(rRange); + if (it != aRangeMap.end()) + { + CondFormat& rCondFormat = *it->second; + const FormatEntries& rEntries = rxExtCondFormat->getEntries(); + const std::vector& rPriorities = rxExtCondFormat->getPriorities(); + size_t nEntryIdx = 0; + for (const auto& rxEntry : rEntries) + { + CondFormatRuleRef xRule = rCondFormat.createRule(); + ScFormatEntry* pNewEntry = rxEntry->Clone(pDoc); + sal_Int32 nPriority = rPriorities[nEntryIdx]; + if (nPriority == -1) + nPriority = mnNonPrioritizedRuleNextPriority++; + xRule->setFormatEntry(nPriority, pNewEntry); + rCondFormat.insertRule(xRule); + ++nEntryIdx; + } + + aDoneExtCFs.insert(nExtCFIndex); + } + + ++nExtCFIndex; + } + CondFormatVec::iterator it = maCondFormats.begin(); CondFormatVec::iterator it_end = maCondFormats.end(); for( ; it != it_end; ++it ) @@ -1118,11 +1186,17 @@ void CondFormatBuffer::finalizeImport() (*ext_it).get()->finalizeImport(); } - for (auto itr = maExtCondFormats.begin(); itr != maExtCondFormats.end(); ++itr) + nExtCFIndex = 0; + for (const auto& rxExtCondFormat : maExtCondFormats) { - ScDocument* pDoc = &getScDocument(); + if (aDoneExtCFs.count(nExtCFIndex)) + { + ++nExtCFIndex; + continue; + } - const ScRangeList& rRange = (*itr)->getRange(); + ScDocument* pDoc = &getScDocument(); + const ScRangeList& rRange = rxExtCondFormat->getRange(); SCTAB nTab = rRange.front().aStart.Tab(); ScConditionalFormat* pFormat = findFormatByRange(rRange, pDoc, nTab); if (!pFormat) @@ -1134,11 +1208,13 @@ void CondFormatBuffer::finalizeImport() pDoc->AddCondFormatData(rRange, nTab, nKey); } - const std::vector< std::unique_ptr >& rEntries = (*itr)->getEntries(); + const std::vector< std::unique_ptr >& rEntries = rxExtCondFormat->getEntries(); for (auto i = rEntries.begin(); i != rEntries.end(); ++i) { pFormat->AddEntry((*i)->Clone(pDoc)); } + + ++nExtCFIndex; } rStyleIdx = 0; // Resets style index. @@ -1305,10 +1381,15 @@ void ExtCfDataBarRule::importCfvo( const AttributeList& rAttribs ) maModel.maColorScaleType = rAttribs.getString( XML_type, OUString() ); } -ExtCfCondFormat::ExtCfCondFormat(const ScRangeList& rRange, std::vector< std::unique_ptr >& rEntries): +ExtCfCondFormat::ExtCfCondFormat(const ScRangeList& rRange, std::vector< std::unique_ptr >& rEntries, + std::vector* pPriorities): maRange(rRange) { maEntries.swap(rEntries); + if (pPriorities) + maPriorities = *pPriorities; + else + maPriorities.resize(maEntries.size(), -1); } ExtCfCondFormat::~ExtCfCondFormat() diff --git a/sc/source/filter/oox/extlstcontext.cxx b/sc/source/filter/oox/extlstcontext.cxx index f30e01a0ad08..533f672ac95c 100644 --- a/sc/source/filter/oox/extlstcontext.cxx +++ b/sc/source/filter/oox/extlstcontext.cxx @@ -83,6 +83,7 @@ void ExtCfRuleContext::onStartElement( const AttributeList& rAttribs ) ExtConditionalFormattingContext::ExtConditionalFormattingContext(WorksheetContextBase& rFragment): WorksheetContextBase(rFragment) { + nPriority = -1; isPreviousElementF = false; } @@ -103,6 +104,7 @@ ContextHandlerRef ExtConditionalFormattingContext::onCreateContext(sal_Int32 nEl { OUString aType = rAttribs.getString(XML_type, OUString()); OUString aId = rAttribs.getString(XML_id, OUString()); + nPriority = rAttribs.getInteger( XML_priority, -1 ); if (aType == "dataBar") { @@ -178,6 +180,7 @@ void ExtConditionalFormattingContext::onEndElement() case XM_TOKEN(f): { rFormulas.push_back(aChars); + maPriorities.push_back(nPriority); } break; case XLS14_TOKEN( cfRule ): @@ -200,9 +203,9 @@ void ExtConditionalFormattingContext::onEndElement() aRange[i].aEnd.SetTab(nTab); } - if(isPreviousElementF) // sqref can be alone in some cases. + if (isPreviousElementF) // sqref can be alone in some cases. { - for(const OUString& rFormula : rFormulas) + for (const OUString& rFormula : rFormulas) { ScAddress rPos = aRange.GetTopLeftCorner(); rStyle = getStyles().createExtDxfStyle(rStyleIdx); @@ -214,11 +217,17 @@ void ExtConditionalFormattingContext::onEndElement() maEntries.push_back(std::unique_ptr(pEntry)); rStyleIdx++; } + + assert(rFormulas.size() == maPriorities.size()); rFormulas.clear(); } std::vector< std::unique_ptr >& rExtFormats = getCondFormats().importExtCondFormat(); - rExtFormats.push_back(o3tl::make_unique(aRange, maEntries)); + rExtFormats.push_back(o3tl::make_unique(aRange, maEntries, &maPriorities)); + + if (isPreviousElementF) + maPriorities.clear(); + isPreviousElementF = false; } break; -- cgit