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 <noel.grandin@collabora.co.uk>

3 files changed, 60 insertions, 55 deletions

diff --git a/sc/qa/unit/ucalc.cxx b/sc/qa/unit/ucalc.cxx
index 78f41c2708ce..36810a4b3060 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