sc/qa/unit/ucalc.cxx | 44 +++++++++++++------------- sc/source/core/data/drwlayer.cxx | 5 +- sc/source/core/data/table2.cxx | 66 +++++++++++++++++++++------------------ 3 files changed, 60 insertions(+), 55 deletions(-)
New commits: commit b039427ab35ba7a287388ee9ede428c5a5e51412 Author: Noel Grandin <[email protected]> AuthorDate: Sat Dec 21 14:54:28 2024 +0200 Commit: Xisco Fauli <[email protected]> CommitDate: Thu Dec 26 12:42:16 2024 +0100 Re-write ScTable::GetRowForHeight Cleanup up this method, making it more organised in how it iterates over spans, and adding asserts to verify assumptions. The asserts uncovered that we are sometimes fed bad data, so now we deal with that (and not sanitising the input is possibly why the original code was a little weird). Also, now that we are returning correct data, our caller does not need to correct the return value Change-Id: I094931bb2e93a3273b7a928fc7e83eed75e3db51 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/179040 Tested-by: Jenkins Reviewed-by: Noel Grandin <[email protected]> Signed-off-by: Xisco Fauli <[email protected]> Reviewed-on: https://gerrit.libreoffice.org/c/core/+/179204 diff --git a/sc/qa/unit/ucalc.cxx b/sc/qa/unit/ucalc.cxx index 9a8b9dd40d3f..21df5d6b3234 100644 --- a/sc/qa/unit/ucalc.cxx +++ b/sc/qa/unit/ucalc.cxx @@ -793,37 +793,37 @@ CPPUNIT_TEST_FIXTURE(Test, testRowForHeight) }; std::vector<Check> aChecks = { - { -2000, -1 }, - { -1000, -1 }, + { -2000, 0 }, + { -1000, 0 }, { -1, 0 }, { 0, 0 }, // row 0 begins - { 1, 1 }, - { 99, 1 }, // row 0 ends + { 1, 0 }, + { 99, 0 }, // row 0 ends { 100, 1 }, // row 1 begins - { 101, 2 }, - { 120, 2 }, - { 199, 2 }, // row 1 ends + { 101, 1 }, + { 120, 1 }, + { 199, 1 }, // row 1 ends { 200, 2 }, // row 2 begins - { 201, 6 }, - { 299, 6 }, // row 2 ends + { 201, 2 }, + { 299, 2 }, // row 2 ends { 300, 6 }, // row 6 begins, because 3-5 are hidden - { 330, 7 }, - { 399, 7 }, // row 6 ends + { 330, 6 }, + { 399, 6 }, // row 6 ends { 400, 7 }, // row 7 begins - { 401, 13 }, - { 420, 13 }, - { 499, 13 }, // row 7 ends + { 401, 7 }, + { 420, 7 }, + { 499, 7 }, // row 7 ends { 500, 13 }, // row 13 begins, because 8-12 are hidden - { 501, 14 }, - { 599, 14 }, - { 600, 14 }, - { 699, 14 }, // row 13 ends (row 13 is 200 pixels high) + { 501, 13 }, + { 599, 13 }, + { 600, 13 }, + { 699, 13 }, // row 13 ends (row 13 is 200 pixels high) { 700, 14 }, // row 14 begins - { 780, 15 }, - { 899, 15 }, // row 14 ends (row 14 is 200 pixels high) + { 780, 14 }, + { 899, 14 }, // row 14 ends (row 14 is 200 pixels high) { 900, 15 }, // row 15 begins - { 1860, 20 }, - { 4020, 28 }, + { 1860, 19 }, + { 4020, 27 }, }; for (const Check& rCheck : aChecks) diff --git a/sc/source/core/data/drwlayer.cxx b/sc/source/core/data/drwlayer.cxx index 102b4564df54..d41f21e86d71 100644 --- a/sc/source/core/data/drwlayer.cxx +++ b/sc/source/core/data/drwlayer.cxx @@ -1473,10 +1473,9 @@ bool ScDrawLayer::GetPrintArea( ScRange& rRange, bool bSetHor, bool bSetVer ) co nStartY = o3tl::toTwips(nStartY, o3tl::Length::mm100); nEndY = o3tl::toTwips(nEndY, o3tl::Length::mm100); SCROW nRow = pDoc->GetRowForHeight( nTab, nStartY); - rRange.aStart.SetRow( nRow>0 ? (nRow-1) : 0); + rRange.aStart.SetRow( nRow); nRow = pDoc->GetRowForHeight( nTab, nEndY); - rRange.aEnd.SetRow( nRow == pDoc->MaxRow() ? pDoc->MaxRow() : - (nRow>0 ? (nRow-1) : 0)); + rRange.aEnd.SetRow( nRow ); } } else diff --git a/sc/source/core/data/table2.cxx b/sc/source/core/data/table2.cxx index 388caa164806..907f08a24cec 100644 --- a/sc/source/core/data/table2.cxx +++ b/sc/source/core/data/table2.cxx @@ -4255,27 +4255,41 @@ tools::Long ScTable::GetRowOffset( SCROW nRow, bool bHiddenAsZero ) const SCROW ScTable::GetRowForHeight(tools::Long nHeight) const { - tools::Long nSum = 0; + // clamp bad data + if (nHeight < 0) + return 0; - ScFlatBoolRowSegments::RangeData aData; + // We are iterating over two data arrays here, each of which + // is a range/compressed view of the underlying data. + tools::Long nSum = 0; + ScFlatBoolRowSegments::RangeData aHiddenRange; + aHiddenRange.mnRow1 = -1; + aHiddenRange.mnRow2 = -1; + aHiddenRange.mbValue = false; // silence MSVC C4701 ScFlatUInt16RowSegments::RangeData aRowHeightRange; + aRowHeightRange.mnRow1 = -1; aRowHeightRange.mnRow2 = -1; aRowHeightRange.mnValue = 1; // silence MSVC C4701 for (SCROW nRow = 0; nRow <= rDocument.MaxRow(); ++nRow) { - if (!mpHiddenRows->getRangeData(nRow, aData)) - // Failed to fetch the range data for whatever reason. - break; + // fetch hidden data range if necessary + if (aHiddenRange.mnRow2 < nRow) + { + if (!mpHiddenRows->getRangeData(nRow, aHiddenRange)) + // Failed to fetch the range data for whatever reason. + break; + } - if (aData.mbValue) + if (aHiddenRange.mbValue) { // This row is hidden. Skip ahead all hidden rows. - nRow = aData.mnRow2; + nRow = aHiddenRange.mnRow2; continue; } + // fetch height data range if necessary if (aRowHeightRange.mnRow2 < nRow) { if (!mpRowHeights->getRangeData(nRow, aRowHeightRange)) @@ -4283,34 +4297,26 @@ SCROW ScTable::GetRowForHeight(tools::Long nHeight) const break; } - // find the last common row between hidden & height spans - SCROW nLastCommon = std::min(aData.mnRow2, aRowHeightRange.mnRow2); - assert (nLastCommon >= nRow); - SCROW nCommon = nLastCommon - nRow + 1; + assert(aHiddenRange.mnRow1 <= nRow && aHiddenRange.mnRow2 >= nRow && "the current hidden-row span should overlap the current row"); + assert(!aHiddenRange.mbValue && "the current hidden-row span should have visible==true"); + assert(aRowHeightRange.mnRow1 <= nRow && aRowHeightRange.mnRow2 >= nRow && "the current height span should overlap the current row"); - // how much further to go ? - tools::Long nPixelsLeft = nHeight - nSum; - tools::Long nCommonPixels = static_cast<tools::Long>(aRowHeightRange.mnValue) * nCommon; + // find the last common row between hidden & height spans + SCROW nLastCommon = std::min(aHiddenRange.mnRow2, aRowHeightRange.mnRow2); + SCROW nCommonRows = nLastCommon - nRow + 1; + // height of common span + tools::Long nCommonPixels = static_cast<tools::Long>(aRowHeightRange.mnValue) * nCommonRows; - // are we in the zone ? - if (nCommonPixels > nPixelsLeft) + // is the target height inside the common span ? + if (nSum + nCommonPixels > nHeight) { - nRow += (nPixelsLeft + aRowHeightRange.mnValue - 1) / aRowHeightRange.mnValue; - - // FIXME: finding this next row is far from elegant, - // we have a single caller, which subtracts one as well(!?) - if (nRow >= rDocument.MaxRow()) - return rDocument.MaxRow(); - - if (!mpHiddenRows->getRangeData(nRow, aData)) - // Failed to fetch the range data for whatever reason. - break; + // calculate how many rows to skip inside the common span + nRow += (nHeight - nSum) / aRowHeightRange.mnValue; - if (aData.mbValue) - // These rows are hidden. - nRow = aData.mnRow2 + 1; + assert(aHiddenRange.mnRow1 <= nRow && aHiddenRange.mnRow2 >= nRow && "the current hidden-row span should overlap the current row"); + assert(aRowHeightRange.mnRow1 <= nRow && aRowHeightRange.mnRow2 >= nRow && "the current height span should overlap the current row"); - return nRow <= rDocument.MaxRow() ? nRow : rDocument.MaxRow(); + return nRow; } // skip the range and keep hunting
