sc/inc/pivot/PivotTableFormatOutput.hxx | 13 sc/qa/unit/PivotTableFormatsImportExport.cxx | 141 ++++++- sc/qa/unit/data/xlsx/pivot-table/PivotTableCellFormatsTest_2_DataFieldInRow_ColumnLabelColor.xlsx |binary sc/qa/unit/data/xlsx/pivot-table/PivotTableCellFormatsTest_8_DataFieldInRow_DataColor.xlsx |binary sc/source/core/data/PivotTableFormatOutput.cxx | 199 ++++++---- 5 files changed, 273 insertions(+), 80 deletions(-)
New commits: commit 12eba0d6115b05792b302d4a5ad141d9eb528db4 Author: Tomaž Vajngerl <tomaz.vajng...@collabora.co.uk> AuthorDate: Mon Apr 15 11:28:38 2024 +0900 Commit: Tomaž Vajngerl <qui...@gmail.com> CommitDate: Mon Apr 15 17:48:47 2024 +0200 pivot: improve format matching, add empty cell checking to tests The patch improves the pivot table cell format matching with the output data, so we check if there is a complete match or a partial match. A partial match is when we found one match for a field, but other fields are not set, and if this partial match is found in exactly 1 line of output data, then we treat it as a complete match. This also refactors the code so there is code no duplication for column and row matching code - they work in the same way, just in a different orientation. This also adds checking of empty cells to the tests, so we can make sure there is no other cell in the pivot table that got the pivot table cell format applied to. A new test case was also added to the test class. Change-Id: I102ec8e33bc7a3f26bc898568ee0e33abe08bd27 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/166086 Tested-by: Tomaž Vajngerl <qui...@gmail.com> Reviewed-by: Tomaž Vajngerl <qui...@gmail.com> diff --git a/sc/inc/pivot/PivotTableFormatOutput.hxx b/sc/inc/pivot/PivotTableFormatOutput.hxx index 59cad62c013c..0aeb492cc34a 100644 --- a/sc/inc/pivot/PivotTableFormatOutput.hxx +++ b/sc/inc/pivot/PivotTableFormatOutput.hxx @@ -28,12 +28,14 @@ struct ScDPOutLevelData; namespace sc { +/// Direction or orientation of the results enum class FormatResultDirection { ROW, COLUMN }; +/// A field data for a format output, struct FormatOutputField { tools::Long nDimension = -2; @@ -42,6 +44,7 @@ struct FormatOutputField bool bSet = false; }; +/// Data for a entry of a format output, which matches a one format entry struct FormatOutputEntry { FormatType eType = FormatType::None; @@ -52,6 +55,7 @@ struct FormatOutputEntry std::vector<FormatOutputField> aColumnOutputFields; }; +/// data of one "field" that is part of a line struct FieldData { tools::Long mnDimension = -2; @@ -64,15 +68,19 @@ struct FieldData bool bContinue = false; }; +/// data for a "line" of one row/column axis struct LineData { + /// primary axis depending if we are row or column oriented (when selecting a column or row we get a "line") std::optional<SCCOLROW> oLine = std::nullopt; + /// secondary axis depending if we are row or column oriented (position in a line) std::optional<SCCOLROW> oPosition = std::nullopt; + /// fields of a line std::vector<FieldData> maFields; }; -class NameResolver; - +/** Format output class, which is responsible to check if any of the formats and matches the + * pivot table output, and applies the cell attributes when the match is found. */ class FormatOutput { private: @@ -93,7 +101,6 @@ public: void setFormats(sc::PivotTableFormats const& rPivotTableFormats) { mpFormats.reset(new sc::PivotTableFormats(rPivotTableFormats)); - maFormatOutputEntries.resize(mpFormats->size()); } void insertFieldMember(size_t nFieldIndex, const ScDPOutLevelData& rField, diff --git a/sc/qa/unit/PivotTableFormatsImportExport.cxx b/sc/qa/unit/PivotTableFormatsImportExport.cxx index 401e190b34f5..1c20cdc3b5ac 100644 --- a/sc/qa/unit/PivotTableFormatsImportExport.cxx +++ b/sc/qa/unit/PivotTableFormatsImportExport.cxx @@ -12,19 +12,11 @@ #include "helper/qahelper.hxx" #include <patattr.hxx> -#include <scitems.hxx> #include <document.hxx> -#include <generalfunction.hxx> -#include <dpcache.hxx> #include <dpobject.hxx> -#include <dpsave.hxx> -#include <dputil.hxx> #include <attrib.hxx> -#include <dpshttab.hxx> #include <globstr.hrc> #include <scresid.hxx> -#include <queryentry.hxx> -#include <queryparam.hxx> #include <rtl/string.hxx> #include <editeng/brushitem.hxx> #include <editeng/colritem.hxx> @@ -61,6 +53,24 @@ Color getFontColor(ScDocument& rDoc, OUString const& rAddressString) const SvxColorItem& rItem = pPattern->GetItem(ATTR_FONT_COLOR); return rItem.getColor(); } + +template <typename T> OUString checkNonEmptyAddresses(ScDocument& rDoc, T const& rArrayOfAddresses) +{ + OUString aString; + for (auto const& rAddressString : rArrayOfAddresses) + { + ScAddress aAddress; + aAddress.Parse(rAddressString, rDoc); + const ScPatternAttr* pPattern = rDoc.GetPattern(aAddress); + if (pPattern->GetItem(ATTR_FONT_COLOR).getColor() != COL_BLACK + || pPattern->GetItem(ATTR_BACKGROUND).GetColor() != COL_TRANSPARENT) + { + aString += rAddressString + " "; + } + } + return aString; +} + } // end anonymous namespace CPPUNIT_TEST_FIXTURE(ScPivotTableFormatsImportExport, @@ -69,6 +79,24 @@ CPPUNIT_TEST_FIXTURE(ScPivotTableFormatsImportExport, auto assertDocument = [](ScDocument& rDoc) { CPPUNIT_ASSERT_EQUAL(COL_YELLOW, getBackgroundColor(rDoc, u"G6"_ustr)); CPPUNIT_ASSERT_EQUAL(COL_LIGHTRED, getFontColor(rDoc, u"G7"_ustr)); + + // Make sure the other cells have the font color or background set to default + auto aEmptyAddresses = std::to_array<OUString>({ + u"G5"_ustr, + u"H5"_ustr, + u"I5"_ustr, + u"J5"_ustr, + u"K5"_ustr, + u"H6"_ustr, + u"I6"_ustr, + u"J6"_ustr, + u"K6"_ustr, + u"H7"_ustr, + u"I7"_ustr, + u"J7"_ustr, + u"K7"_ustr, + }); + CPPUNIT_ASSERT_EQUAL(OUString(), checkNonEmptyAddresses(rDoc, aEmptyAddresses)); }; createScDoc("xlsx/pivot-table/PivotTableCellFormatsTest_1_DataFieldInRow_RowLabelColor.xlsx"); @@ -82,6 +110,25 @@ CPPUNIT_TEST_FIXTURE(ScPivotTableFormatsImportExport, { auto assertDocument = [](ScDocument& rDoc) { CPPUNIT_ASSERT_EQUAL(Color(0x00B050), getBackgroundColor(rDoc, u"H5"_ustr)); + + // Make sure the other cells have the font color or background set to default + auto aEmptyAddresses = std::to_array<OUString>({ + u"G5"_ustr, + u"I5"_ustr, + u"J5"_ustr, + u"K5"_ustr, + u"G6"_ustr, + u"H6"_ustr, + u"I6"_ustr, + u"J6"_ustr, + u"K6"_ustr, + u"G7"_ustr, + u"H7"_ustr, + u"I7"_ustr, + u"J7"_ustr, + u"K7"_ustr, + }); + CPPUNIT_ASSERT_EQUAL(OUString(), checkNonEmptyAddresses(rDoc, aEmptyAddresses)); }; createScDoc( @@ -97,6 +144,24 @@ CPPUNIT_TEST_FIXTURE(ScPivotTableFormatsImportExport, auto assertDocument = [](ScDocument& rDoc) { CPPUNIT_ASSERT_EQUAL(COL_LIGHTRED, getFontColor(rDoc, u"H5"_ustr)); CPPUNIT_ASSERT_EQUAL(Color(0x92D050), getBackgroundColor(rDoc, u"I5"_ustr)); + + // Make sure the other cells have the font color or background set to default + auto aEmptyAddresses = std::to_array<OUString>({ + u"G5"_ustr, + u"G6"_ustr, + u"H6"_ustr, + u"I6"_ustr, + u"G7"_ustr, + u"H7"_ustr, + u"I7"_ustr, + u"G8"_ustr, + u"H8"_ustr, + u"I8"_ustr, + u"G9"_ustr, + u"H9"_ustr, + u"I9"_ustr, + }); + CPPUNIT_ASSERT_EQUAL(OUString(), checkNonEmptyAddresses(rDoc, aEmptyAddresses)); }; createScDoc( @@ -112,6 +177,23 @@ CPPUNIT_TEST_FIXTURE(ScPivotTableFormatsImportExport, auto assertDocument = [](ScDocument& rDoc) { CPPUNIT_ASSERT_EQUAL(COL_LIGHTRED, getFontColor(rDoc, u"H7"_ustr)); CPPUNIT_ASSERT_EQUAL(Color(0x92D050), getBackgroundColor(rDoc, u"I9"_ustr)); + + auto aEmptyAddresses = std::to_array<OUString>({ + u"G5"_ustr, + u"H5"_ustr, + u"I5"_ustr, + u"G6"_ustr, + u"H6"_ustr, + u"I6"_ustr, + u"G7"_ustr, + u"I7"_ustr, + u"G8"_ustr, + u"H8"_ustr, + u"I8"_ustr, + u"G9"_ustr, + u"H9"_ustr, + }); + CPPUNIT_ASSERT_EQUAL(OUString(), checkNonEmptyAddresses(rDoc, aEmptyAddresses)); }; createScDoc("xlsx/pivot-table/PivotTableCellFormatsTest_4_DataFieldInColumn_DataColor.xlsx"); @@ -127,6 +209,17 @@ CPPUNIT_TEST_FIXTURE(ScPivotTableFormatsImportExport, CPPUNIT_ASSERT_EQUAL(COL_YELLOW, getBackgroundColor(rDoc, u"I8"_ustr)); CPPUNIT_ASSERT_EQUAL(COL_LIGHTRED, getBackgroundColor(rDoc, u"I11"_ustr)); CPPUNIT_ASSERT_EQUAL(Color(0x0070C0), getBackgroundColor(rDoc, u"J13"_ustr)); + + auto aEmptyAddresses = std::to_array<OUString>({ + u"G5"_ustr, u"H5"_ustr, u"I5"_ustr, u"J5"_ustr, u"G6"_ustr, u"H6"_ustr, + u"I6"_ustr, u"J6"_ustr, u"G7"_ustr, u"H7"_ustr, u"I7"_ustr, u"J7"_ustr, + u"G8"_ustr, u"H8"_ustr, u"J8"_ustr, u"G9"_ustr, u"H9"_ustr, u"I9"_ustr, + u"J9"_ustr, u"G10"_ustr, u"H10"_ustr, u"I10"_ustr, u"J10"_ustr, u"G11"_ustr, + u"H11"_ustr, u"J11"_ustr, u"G12"_ustr, u"H12"_ustr, u"I12"_ustr, u"J12"_ustr, + u"G13"_ustr, u"H13"_ustr, u"I13"_ustr, u"G14"_ustr, u"H14"_ustr, u"I14"_ustr, + u"J14"_ustr, + }); + CPPUNIT_ASSERT_EQUAL(OUString(), checkNonEmptyAddresses(rDoc, aEmptyAddresses)); }; createScDoc("xlsx/pivot-table//" @@ -170,6 +263,38 @@ CPPUNIT_TEST_FIXTURE(ScPivotTableFormatsImportExport, assertDocument(*getScDoc()); } +CPPUNIT_TEST_FIXTURE(ScPivotTableFormatsImportExport, + PivotTableCellFormatsTest_8_DataFieldInRow_DataColor) +{ + auto assertDocument = [](ScDocument& rDoc) { + CPPUNIT_ASSERT_EQUAL(Color(0x00B0F0), getBackgroundColor(rDoc, u"I6"_ustr)); + CPPUNIT_ASSERT_EQUAL(COL_YELLOW, getBackgroundColor(rDoc, u"K7"_ustr)); + + // Make sure the other cells have the font color or background set to default + auto aEmptyAddresses = std::to_array<OUString>({ + u"G5"_ustr, + u"H5"_ustr, + u"I5"_ustr, + u"J5"_ustr, + u"K5"_ustr, + u"G6"_ustr, + u"H6"_ustr, + u"J6"_ustr, + u"K6"_ustr, + u"G7"_ustr, + u"H7"_ustr, + u"I7"_ustr, + u"J7"_ustr, + }); + CPPUNIT_ASSERT_EQUAL(OUString(), checkNonEmptyAddresses(rDoc, aEmptyAddresses)); + }; + + createScDoc("xlsx/pivot-table/PivotTableCellFormatsTest_8_DataFieldInRow_DataColor.xlsx"); + assertDocument(*getScDoc()); + saveAndReload("Calc Office Open XML"); + assertDocument(*getScDoc()); +} + CPPUNIT_PLUGIN_IMPLEMENT(); /* vim:set shiftwidth=4 softtabstop=4 expandtab: */ diff --git a/sc/qa/unit/data/xlsx/pivot-table/PivotTableCellFormatsTest_2_DataFieldInRow_ColumnLabelColor.xlsx b/sc/qa/unit/data/xlsx/pivot-table/PivotTableCellFormatsTest_2_DataFieldInRow_ColumnLabelColor.xlsx index e156cad5e041..1c8245751bfc 100644 Binary files a/sc/qa/unit/data/xlsx/pivot-table/PivotTableCellFormatsTest_2_DataFieldInRow_ColumnLabelColor.xlsx and b/sc/qa/unit/data/xlsx/pivot-table/PivotTableCellFormatsTest_2_DataFieldInRow_ColumnLabelColor.xlsx differ diff --git a/sc/qa/unit/data/xlsx/pivot-table/PivotTableCellFormatsTest_8_DataFieldInRow_DataColor.xlsx b/sc/qa/unit/data/xlsx/pivot-table/PivotTableCellFormatsTest_8_DataFieldInRow_DataColor.xlsx new file mode 100644 index 000000000000..48964032868b Binary files /dev/null and b/sc/qa/unit/data/xlsx/pivot-table/PivotTableCellFormatsTest_8_DataFieldInRow_DataColor.xlsx differ diff --git a/sc/source/core/data/PivotTableFormatOutput.cxx b/sc/source/core/data/PivotTableFormatOutput.cxx index c3732a888ae7..9f6dcf2355b7 100644 --- a/sc/source/core/data/PivotTableFormatOutput.cxx +++ b/sc/source/core/data/PivotTableFormatOutput.cxx @@ -20,6 +20,8 @@ namespace sc { +namespace +{ class NameResolver { private: @@ -67,8 +69,6 @@ public: } }; -namespace -{ void initLines(std::vector<LineData>& rLines, std::vector<ScDPOutLevelData> const& rFields) { for (size_t i = 0; i < rFields.size(); i++) @@ -126,6 +126,7 @@ void FormatOutput::prepare(SCTAB nTab, std::vector<ScDPOutLevelData> const& rCol if (!mpFormats) return; + // initialize row and column lines, so the number of fields matches the pivot table output initLines(maRowLines, rRowFields); if (rColumnFields.size() == 0 && bColumnFieldIsDataOnly) @@ -138,6 +139,7 @@ void FormatOutput::prepare(SCTAB nTab, std::vector<ScDPOutLevelData> const& rCol initLines(maColumnLines, rColumnFields); } + // check the table data exists auto* pTableData = mrObject.GetTableData(); if (!pTableData) return; @@ -147,6 +149,11 @@ void FormatOutput::prepare(SCTAB nTab, std::vector<ScDPOutLevelData> const& rCol NameResolver aNameResolver(*pTableData, rCache); + // Initialize format output entries (FormatOutputEntry) and set the data already available from output fields + // (rColumnFields and rRowFields) and the pivot table format list (PivotTableFormat). + + maFormatOutputEntries.resize(mpFormats->size()); + size_t nFormatIndex = 0; for (PivotTableFormat const& rFormat : mpFormats->getVector()) { @@ -157,8 +164,10 @@ void FormatOutput::prepare(SCTAB nTab, std::vector<ScDPOutLevelData> const& rCol initFormatOutputField(rEntry.aRowOutputFields, rRowFields, rFormat, aNameResolver); + // If column fields list is empty, but there is a data field in columns that is not part of column fields if (rColumnFields.size() == 0 && bColumnFieldIsDataOnly) { + // Initialize column output fields to have 1 data output field rEntry.aColumnOutputFields.resize(1); FormatOutputField& rOutputField = rEntry.aColumnOutputFields[0]; @@ -247,6 +256,112 @@ void FormatOutput::insertFieldMember(size_t nFieldIndex, ScDPOutLevelData const& fillLineAndFieldData(maColumnLines, nFieldIndex, rField, nMemberIndex, rMember, nColPos, nRowPos); } +namespace +{ +void checkForMatchingLines(std::vector<LineData> const& rLines, + std::vector<FormatOutputField> const& rFormatOutputField, + std::vector<std::reference_wrapper<const LineData>>& rMatches, + std::vector<std::reference_wrapper<const LineData>>& rMaybeMatches) +{ + for (LineData const& rLineData : rLines) + { + size_t nMatch = 0; + size_t nMaybeMatch = 0; + size_t nNoOfFields = rLineData.maFields.size(); + + for (size_t nIndex = 0; nIndex < nNoOfFields; nIndex++) + { + FieldData const& rFieldData = rLineData.maFields[nIndex]; + FormatOutputField const& rFormatEntry = rFormatOutputField[nIndex]; + bool bFieldMatch = false; + bool bFieldMaybeMatch = false; + + tools::Long nDimension = rFieldData.mnDimension; + if (nDimension == rFormatEntry.nDimension) + { + if (rFormatEntry.bSet) + { + if (nDimension == -2 && rFieldData.nIndex == rFormatEntry.nIndex) + bFieldMatch = true; + else if (nDimension != -2 && rFieldData.aName == rFormatEntry.aName) + bFieldMatch = true; + } + else if (!rFormatEntry.bSet && !rFieldData.bIsMember && !rFieldData.bContinue) + { + bFieldMatch = true; + } + else + { + bFieldMaybeMatch = true; + } + } + + if (!bFieldMatch && !bFieldMaybeMatch) + break; + + if (bFieldMatch) + nMatch++; + + if (bFieldMaybeMatch) + nMaybeMatch++; + } + + if (nMatch == nNoOfFields) + { + rMatches.push_back(std::cref(rLineData)); + break; + } + else if (nMatch + nMaybeMatch == nNoOfFields && nMatch != 0) + { + rMaybeMatches.push_back(std::cref(rLineData)); + } + } +} + +/** Check the lines in matches and maybe matches and output */ +void evaluateMatches(ScDocument& rDocument, + std::vector<std::reference_wrapper<const LineData>> const& rMatches, + std::vector<std::reference_wrapper<const LineData>> const& rMaybeMatches, + std::optional<SCCOLROW>& oRow, std::optional<SCCOLROW>& oColumn, + FormatOutputEntry const& rOutputEntry, FormatResultDirection eResultDirection) +{ + // We expect that tab and pattern to be set or this method shouldn't be called at all + assert(rOutputEntry.onTab); + assert(rOutputEntry.pPattern); + + // We can output only if there is exactly 1 match or 1 maybe match + if (rMatches.size() != 1 && rMaybeMatches.size() != 1) + return; + + LineData const& rLineData = !rMatches.empty() ? rMatches.back() : rMaybeMatches.back(); + + // Can't continue if we don't have complete row/column data + if (!rLineData.oLine || !rLineData.oPosition) + return; + + if (rOutputEntry.eType == FormatType::Label) + { + // Primary axis is set to column (line) then row (position) + SCCOLROW nColumn = *rLineData.oLine; + SCCOLROW nRow = *rLineData.oPosition; + + // In row orientation, the primary axis is row, then column, so we need to swap + if (eResultDirection == FormatResultDirection::ROW) + std::swap(nRow, nColumn); + + // Set the pattern to the sheet + rDocument.ApplyPattern(nColumn, nRow, *rOutputEntry.onTab, *rOutputEntry.pPattern); + } + else if (rOutputEntry.eType == FormatType::Data) + { + if (eResultDirection == FormatResultDirection::ROW) + oRow = rLineData.oLine; + else if (eResultDirection == FormatResultDirection::COLUMN) + oColumn = rLineData.oLine; + } +} + +} // end anonymous namespace void FormatOutput::apply(ScDocument& rDocument) { @@ -260,80 +375,26 @@ void FormatOutput::apply(ScDocument& rDocument) std::optional<SCCOLROW> oRow; std::optional<SCCOLROW> oColumn; - - for (LineData const& rLineData : maRowLines) { - bool bMatchesAll = true; + std::vector<std::reference_wrapper<const LineData>> rMatches; + std::vector<std::reference_wrapper<const LineData>> rMaybeMatches; - for (size_t nIndex = 0; nIndex < rLineData.maFields.size(); nIndex++) - { - FieldData const& rFieldData = rLineData.maFields[nIndex]; - FormatOutputField const& rFormatEntry = rOutputEntry.aRowOutputFields[nIndex]; - - tools::Long nDimension = rFieldData.mnDimension; - if (nDimension == rFormatEntry.nDimension) - { - if (rFormatEntry.bSet) - { - if (nDimension == -2 && rFieldData.nIndex != rFormatEntry.nIndex) - bMatchesAll = false; - else if (nDimension != -2 && rFieldData.aName != rFormatEntry.aName) - bMatchesAll = false; - } - } - else - { - bMatchesAll = false; - } - } - - if (bMatchesAll) - { - if (rLineData.oLine && rLineData.oPosition - && rOutputEntry.eType == FormatType::Label) - rDocument.ApplyPattern(*rLineData.oPosition, *rLineData.oLine, - *rOutputEntry.onTab, *rOutputEntry.pPattern); - else if (rOutputEntry.eType == FormatType::Data) - oRow = rLineData.oLine; - break; - } + checkForMatchingLines(maRowLines, rOutputEntry.aRowOutputFields, rMatches, + rMaybeMatches); + evaluateMatches(rDocument, rMatches, rMaybeMatches, oRow, oColumn, rOutputEntry, + FormatResultDirection::ROW); } - for (LineData const& rLineData : maColumnLines) { - bool bMatchesAll = true; - for (size_t nIndex = 0; nIndex < rLineData.maFields.size(); nIndex++) - { - FieldData const& rFieldData = rLineData.maFields[nIndex]; - FormatOutputField const& rFormatEntry = rOutputEntry.aColumnOutputFields[nIndex]; + std::vector<std::reference_wrapper<const LineData>> rMatches; + std::vector<std::reference_wrapper<const LineData>> rMaybeMatches; - tools::Long nDimension = rFieldData.mnDimension; - if (nDimension == rFormatEntry.nDimension) - { - if (rFormatEntry.bSet) - { - if (nDimension == -2 && rFieldData.nIndex != rFormatEntry.nIndex) - bMatchesAll = false; - else if (nDimension != -2 && rFieldData.aName != rFormatEntry.aName) - bMatchesAll = false; - } - } - else - { - bMatchesAll = false; - } - } - if (bMatchesAll) - { - if (rLineData.oLine && rLineData.oPosition - && rOutputEntry.eType == FormatType::Label) - rDocument.ApplyPattern(*rLineData.oLine, *rLineData.oPosition, - *rOutputEntry.onTab, *rOutputEntry.pPattern); - else if (rOutputEntry.eType == FormatType::Data) - oColumn = rLineData.oLine; - break; - } + checkForMatchingLines(maColumnLines, rOutputEntry.aColumnOutputFields, rMatches, + rMaybeMatches); + evaluateMatches(rDocument, rMatches, rMaybeMatches, oRow, oColumn, rOutputEntry, + FormatResultDirection::COLUMN); } + if (oColumn && oRow && rOutputEntry.eType == FormatType::Data) { rDocument.ApplyPattern(*oColumn, *oRow, *rOutputEntry.onTab, *rOutputEntry.pPattern);