fdo#73001: Simplify the selection function logic & calculate correct results.

Fixing a bug and cleaning up the code all at the same time.  And don't forget
to write test for it as well.

Change-Id: Ia0322c4bebd4c5debcbfa4bb0902afbe581208b2

7 files changed, 218 insertions, 133 deletions

diff --git a/sc/inc/column.hxx b/sc/inc/column.hxx
index 6ec33360b2c3..8b09db3ef7ff 100644
--- a/sc/inc/column.hxx
+++ b/sc/inc/column.hxx
@@ -237,13 +237,8 @@ public:
 
     ScAttrIterator* CreateAttrIterator( SCROW nStartRow, SCROW nEndRow ) const;
 
-                //     UpdateSelectionFunction: multi-select
     void UpdateSelectionFunction(
-        const ScMarkData& rMark, ScFunctionData& rData, ScFlatBoolRowSegments& rHiddenRows,
-        bool bDoExclude, SCROW nExStartRow, SCROW nExEndRow );
-
-    void UpdateAreaFunction(
-        ScFunctionData& rData, ScFlatBoolRowSegments& rHiddenRows, SCROW nStartRow, SCROW nEndRow);
+        const ScMarkData& rMark, ScFunctionData& rData, ScFlatBoolRowSegments& rHiddenRows );
 
     void CopyToColumn(
         sc::CopyToDocContext& rCxt, SCROW nRow1, SCROW nRow2, sal_uInt16 nFlags, bool bMarked,
diff --git a/sc/inc/table.hxx b/sc/inc/table.hxx
index 09d541b304d6..68a17912fe6a 100644
--- a/sc/inc/table.hxx
+++ b/sc/inc/table.hxx
@@ -542,9 +542,7 @@ public:
                         double nStepValue, double nMaxValue, ScProgress* pProgress);
     OUString    GetAutoFillPreview( const ScRange& rSource, SCCOL nEndX, SCROW nEndY );
 
-    void        UpdateSelectionFunction( ScFunctionData& rData,
-                        SCCOL nStartCol, SCROW nStartRow, SCCOL nEndCol, SCROW nEndRow,
-                        const ScMarkData& rMark );
+    void UpdateSelectionFunction( ScFunctionData& rData, const ScMarkData& rMark );
 
     void        AutoFormat( SCCOL nStartCol, SCROW nStartRow, SCCOL nEndCol, SCROW nEndRow,
                                     sal_uInt16 nFormatNo );
diff --git a/sc/qa/unit/ucalc.cxx b/sc/qa/unit/ucalc.cxx
index cd0a9f3cfdcc..5196c98d5528 100644
--- a/sc/qa/unit/ucalc.cxx
+++ b/sc/qa/unit/ucalc.cxx
@@ -676,6 +676,145 @@ void Test::testDataEntries()
     m_pDoc->DeleteTab(0);
 }
 
+void Test::testSelectionFunction()
+{
+    m_pDoc->InsertTab(0, "Test");
+
+    // Insert values into B2:B4.
+    m_pDoc->SetString(ScAddress(1,1,0), "=1"); // formula
+    m_pDoc->SetValue(ScAddress(1,2,0), 2.0);
+    m_pDoc->SetValue(ScAddress(1,3,0), 3.0);
+
+    // Insert strings into B5:B8.
+    m_pDoc->SetString(ScAddress(1,4,0), "A");
+    m_pDoc->SetString(ScAddress(1,5,0), "B");
+    m_pDoc->SetString(ScAddress(1,6,0), "=\"C\""); // formula
+    m_pDoc->SetString(ScAddress(1,7,0), "D");
+
+    // Insert values into D2:D4.
+    m_pDoc->SetValue(ScAddress(3,1,0), 4.0);
+    m_pDoc->SetValue(ScAddress(3,2,0), 5.0);
+    m_pDoc->SetValue(ScAddress(3,3,0), 6.0);
+
+    // Insert edit text into D5.
+    ScFieldEditEngine& rEE = m_pDoc->GetEditEngine();
+    rEE.SetText("Rich Text");
+    m_pDoc->SetEditText(ScAddress(3,4,0), rEE.CreateTextObject());
+
+    // Insert Another string into D6.
+    m_pDoc->SetString(ScAddress(3,5,0), "E");
+
+    // Select B2:B8 & D2:D8 disjoint region.
+    ScRangeList aRanges;
+    aRanges.Append(ScRange(1,1,0,1,7,0)); // B2:B8
+    aRanges.Append(ScRange(3,1,0,3,7,0)); // D2:D8
+    ScMarkData aMark;
+    aMark.MarkFromRangeList(aRanges, true);
+
+    struct Check
+    {
+        ScSubTotalFunc meFunc;
+        double mfExpected;
+    };
+
+    {
+        Check aChecks[] =
+        {
+            { SUBTOTAL_FUNC_AVE,              3.5 },
+            { SUBTOTAL_FUNC_CNT2,            12.0 },
+            { SUBTOTAL_FUNC_CNT,              6.0 },
+            { SUBTOTAL_FUNC_MAX,              6.0 },
+            { SUBTOTAL_FUNC_MIN,              1.0 },
+            { SUBTOTAL_FUNC_SUM,             21.0 },
+            { SUBTOTAL_FUNC_SELECTION_COUNT, 14.0 }
+        };
+
+        for (size_t i = 0; i < SAL_N_ELEMENTS(aChecks); ++i)
+        {
+            double fRes = 0.0;
+            bool bRes = m_pDoc->GetSelectionFunction(aChecks[i].meFunc, ScAddress(), aMark, fRes);
+            CPPUNIT_ASSERT_MESSAGE("Failed to fetch selection function result.", bRes);
+            CPPUNIT_ASSERT_EQUAL(aChecks[i].mfExpected, fRes);
+        }
+    }
+
+    // Hide rows 4 and 6 and check the results again.
+
+    m_pDoc->SetRowHidden(3, 3, 0, true);
+    m_pDoc->SetRowHidden(5, 5, 0, true);
+    CPPUNIT_ASSERT_MESSAGE("This row should be hidden.", m_pDoc->RowHidden(3, 0));
+    CPPUNIT_ASSERT_MESSAGE("This row should be hidden.", m_pDoc->RowHidden(5, 0));
+
+    {
+        Check aChecks[] =
+        {
+            { SUBTOTAL_FUNC_AVE,              3.0 },
+            { SUBTOTAL_FUNC_CNT2,             8.0 },
+            { SUBTOTAL_FUNC_CNT,              4.0 },
+            { SUBTOTAL_FUNC_MAX,              5.0 },
+            { SUBTOTAL_FUNC_MIN,              1.0 },
+            { SUBTOTAL_FUNC_SUM,             12.0 },
+            { SUBTOTAL_FUNC_SELECTION_COUNT, 10.0 }
+        };
+
+        for (size_t i = 0; i < SAL_N_ELEMENTS(aChecks); ++i)
+        {
+            double fRes = 0.0;
+            bool bRes = m_pDoc->GetSelectionFunction(aChecks[i].meFunc, ScAddress(), aMark, fRes);
+            CPPUNIT_ASSERT_MESSAGE("Failed to fetch selection function result.", bRes);
+            CPPUNIT_ASSERT_EQUAL(aChecks[i].mfExpected, fRes);
+        }
+    }
+
+    // Make sure that when no selection is present, use the current cursor position.
+    ScMarkData aEmpty;
+
+    {
+        // D3 (numeric cell containing 5.)
+        ScAddress aPos(3, 2, 0);
+
+        Check aChecks[] =
+        {
+            { SUBTOTAL_FUNC_AVE,             5.0 },
+            { SUBTOTAL_FUNC_CNT2,            1.0 },
+            { SUBTOTAL_FUNC_CNT,             1.0 },
+            { SUBTOTAL_FUNC_MAX,             5.0 },
+            { SUBTOTAL_FUNC_MIN,             5.0 },
+            { SUBTOTAL_FUNC_SUM,             5.0 },
+            { SUBTOTAL_FUNC_SELECTION_COUNT, 1.0 }
+        };
+
+        for (size_t i = 0; i < SAL_N_ELEMENTS(aChecks); ++i)
+        {
+            double fRes = 0.0;
+            bool bRes = m_pDoc->GetSelectionFunction(aChecks[i].meFunc, aPos, aEmpty, fRes);
+            CPPUNIT_ASSERT_MESSAGE("Failed to fetch selection function result.", bRes);
+            CPPUNIT_ASSERT_EQUAL(aChecks[i].mfExpected, fRes);
+        }
+    }
+
+    {
+        // B7 (string formula cell containing ="C".)
+        ScAddress aPos(1, 6, 0);
+
+        Check aChecks[] =
+        {
+            { SUBTOTAL_FUNC_CNT2,            1.0 },
+            { SUBTOTAL_FUNC_SELECTION_COUNT, 1.0 }
+        };
+
+        for (size_t i = 0; i < SAL_N_ELEMENTS(aChecks); ++i)
+        {
+            double fRes = 0.0;
+            bool bRes = m_pDoc->GetSelectionFunction(aChecks[i].meFunc, aPos, aEmpty, fRes);
+            CPPUNIT_ASSERT_MESSAGE("Failed to fetch selection function result.", bRes);
+            CPPUNIT_ASSERT_EQUAL(aChecks[i].mfExpected, fRes);
+        }
+    }
+
+    m_pDoc->DeleteTab(0);
+}
+
 void Test::testCopyAttributes()
 {
     CPPUNIT_ASSERT_MESSAGE ("mashed up attributes", !(IDF_ATTRIB & IDF_CONTENTS));
diff --git a/sc/qa/unit/ucalc.hxx b/sc/qa/unit/ucalc.hxx
index 6bbc86a9d7f4..98528f6d6734 100644
--- a/sc/qa/unit/ucalc.hxx
+++ b/sc/qa/unit/ucalc.hxx
@@ -90,6 +90,12 @@ public:
      */
     void testDataEntries();
 
+    /**
+     * Selection function is responsible for displaying quick calculation
+     * results in the status bar.
+     */
+    void testSelectionFunction();
+
     void testFormulaCreateStringFromTokens();
     void testFormulaParseReference();
     void testFetchVectorRefArray();
@@ -306,6 +312,7 @@ public:
     CPPUNIT_TEST(testRangeList);
     CPPUNIT_TEST(testInput);
     CPPUNIT_TEST(testDataEntries);
+    CPPUNIT_TEST(testSelectionFunction);
     CPPUNIT_TEST(testFormulaCreateStringFromTokens);
     CPPUNIT_TEST(testFormulaParseReference);
     CPPUNIT_TEST(testFetchVectorRefArray);
diff --git a/sc/source/core/data/column2.cxx b/sc/source/core/data/column2.cxx
index 60b7b42ab0f7..4d7b4bf44004 100644
--- a/sc/source/core/data/column2.cxx
+++ b/sc/source/core/data/column2.cxx
@@ -3254,7 +3254,6 @@ namespace {
 class UpdateSubTotalHandler
 {
     ScFunctionData& mrData;
-    ScFlatBoolRowSegments& mrHiddenRows;
 
     void update(double fVal, bool bVal)
     {
@@ -3311,22 +3310,25 @@ class UpdateSubTotalHandler
     }
 
 public:
-    UpdateSubTotalHandler(ScFunctionData& rData, ScFlatBoolRowSegments& rHiddenRows) :
-        mrData(rData), mrHiddenRows(rHiddenRows) {}
+    UpdateSubTotalHandler(ScFunctionData& rData) : mrData(rData) {}
 
-    void operator() (size_t nRow, double fVal)
+    void operator() (size_t /*nRow*/, double fVal)
     {
-        if (mrHiddenRows.getValue(nRow))
-            return;
-
         update(fVal, true);
     }
 
-    void operator() (size_t nRow, ScFormulaCell* pCell)
+    void operator() (size_t /*nRow*/, const svl::SharedString&)
     {
-        if (mrHiddenRows.getValue(nRow))
-            return;
+        update(0.0, false);
+    }
 
+    void operator() (size_t /*nRow*/, const EditTextObject*)
+    {
+        update(0.0, false);
+    }
+
+    void operator() (size_t /*nRow*/, ScFormulaCell* pCell)
+    {
         double fVal = 0.0;
         bool bVal = false;
         if (mrData.eFunc != SUBTOTAL_FUNC_CNT2) // it doesn't interest us
@@ -3353,99 +3355,63 @@ public:
 
 //  multiple selections:
 void ScColumn::UpdateSelectionFunction(
-    const ScMarkData& rMark, ScFunctionData& rData, ScFlatBoolRowSegments& rHiddenRows,
-    bool bDoExclude, SCROW nExStartRow, SCROW nExEndRow)
+    const ScMarkData& rMark, ScFunctionData& rData, ScFlatBoolRowSegments& rHiddenRows )
 {
-    if ( rData.eFunc != SUBTOTAL_FUNC_SELECTION_COUNT )
+    sc::SingleColumnSpanSet aSpanSet;
+    aSpanSet.scan(rMark, nTab, nCol); // mark all selected rows.
+
+    // Exclude all hidden rows.
+    ScFlatBoolRowSegments::RangeData aRange;
+    SCROW nRow = 0;
+    while (nRow <= MAXROW)
     {
-        sc::SingleColumnSpanSet aSpanSet;
-        aSpanSet.scan(rMark, nTab, nCol);
-        if (bDoExclude)
-        {
-            aSpanSet.set(0, nExStartRow, false);
-            aSpanSet.set(nExEndRow+1, MAXROWCOUNT, false);
-        }
+        if (!rHiddenRows.getRangeData(nRow, aRange))
+            break;
 
-        sc::SingleColumnSpanSet::SpansType aSpans;
-        aSpanSet.getSpans(aSpans);
-        UpdateSubTotalHandler aFunc(rData, rHiddenRows);
-        sc::CellStoreType::iterator itCellPos = maCells.begin();
-        sc::SingleColumnSpanSet::SpansType::const_iterator it = aSpans.begin(), itEnd = aSpans.end();
-        for (; it != itEnd; ++it)
-        {
-            itCellPos = sc::ProcessFormulaNumeric(
-                itCellPos, maCells, it->mnRow1, it->mnRow2, aFunc);
-        }
+        if (aRange.mbValue)
+            // Hidden range detected.
+            aSpanSet.set(nRow, aRange.mnRow2, false);
+
+        nRow = aRange.mnRow2 + 1;
     }
-    else
-    {
-        SCROW nTop, nBottom;
 
-        // ScMarkData::GetArray() returns a valid array only if
-        // 'rMark.IsMultiMarked()' returns true.
-        // Since ScTable::UpdateSelectionFunction() already checked that first
-        // before calling this method it does not need to be repeated here.
+    sc::SingleColumnSpanSet::SpansType aSpans;
+    aSpanSet.getSpans(aSpans);
 
-        ScMarkArrayIter aIter(rMark.GetArray() + nCol);
-        ScFlatBoolRowSegments::RangeData aData;
+    sc::SingleColumnSpanSet::SpansType::const_iterator it = aSpans.begin(), itEnd = aSpans.end();
 
-        while (aIter.Next( nTop, nBottom ))
+    switch (rData.eFunc)
+    {
+        case SUBTOTAL_FUNC_SELECTION_COUNT:
         {
-            sal_Int32 nCellCount = 0;    // to get the count of selected visible cells
-            SCROW nRow = nTop;
-
-            while ( nRow <= nBottom )
+            // Simply count selected rows regardless of cell contents.
+            for (; it != itEnd; ++it)
+                rData.nCount += it->mnRow2 - it->mnRow1 + 1;
+        }
+        break;
+        case SUBTOTAL_FUNC_CNT2:
+        {
+            // We need to parse all non-empty cells.
+            sc::CellStoreType::const_iterator itCellPos = maCells.begin();
+            UpdateSubTotalHandler aFunc(rData);
+            for (; it != itEnd; ++it)
             {
-                if (!rHiddenRows.getRangeData(nRow, aData))     // failed to get range data
-                    break;
-
-                if (aData.mnRow2 > nBottom)
-                    aData.mnRow2 = nBottom;
-
-                if (!aData.mbValue)
-                {
-                    nCellCount += aData.mnRow2 - nRow + 1;
-
-                    // Till this point, nCellCount also includes count of those cells which are excluded
-                    // So, they should be decremented now.
-
-                    if ( bDoExclude && nExStartRow >= nRow && nExEndRow <= aData.mnRow2 )
-                        nCellCount -= nExEndRow - nExStartRow + 1;
-                }
-                nRow = aData.mnRow2 + 1;
+                itCellPos = sc::ParseAllNonEmpty(
+                    itCellPos, maCells, it->mnRow1, it->mnRow2, aFunc);
             }
-            rData.nCount += nCellCount;
         }
-    }
-}
-
-//  with bNoMarked ignore the multiple selections
-void ScColumn::UpdateAreaFunction(
-    ScFunctionData& rData, ScFlatBoolRowSegments& rHiddenRows, SCROW nStartRow, SCROW nEndRow)
-{
-    if ( rData.eFunc != SUBTOTAL_FUNC_SELECTION_COUNT )
-    {
-        UpdateSubTotalHandler aFunc(rData, rHiddenRows);
-        sc::ProcessFormulaNumeric(
-            maCells.begin(), maCells, nStartRow, nEndRow, aFunc);
-    }
-    else
-    {
-        sal_Int32 nCellCount = 0;    // to get the count of selected visible cells
-        SCROW nRow = nStartRow;
-        ScFlatBoolRowSegments::RangeData aData;
-
-        while (nRow <= nEndRow)
+        break;
+        default:
         {
-            if (!rHiddenRows.getRangeData(nRow, aData))
-               break;
-
-            if (!aData.mbValue)
-                nCellCount += (aData.mnRow2 <= nEndRow ? aData.mnRow2 : nEndRow) - nRow + 1;
-
-            nRow = aData.mnRow2 + 1;
+            // We need to parse only numeric values.
+            sc::CellStoreType::const_iterator itCellPos = maCells.begin();
+            UpdateSubTotalHandler aFunc(rData);
+            for (; it != itEnd; ++it)
+            {
+                itCellPos = sc::ParseFormulaNumeric(
+                    itCellPos, maCells, it->mnRow1, it->mnRow2, aFunc);
+            }
         }
-        rData.nCount += nCellCount;
     }
 }
 
diff --git a/sc/source/core/data/documen4.cxx b/sc/source/core/data/documen4.cxx
index 0aa6383f1211..ed496961c38b 100644
--- a/sc/source/core/data/documen4.cxx
+++ b/sc/source/core/data/documen4.cxx
@@ -613,22 +613,17 @@ bool ScDocument::GetSelectionFunction( ScSubTotalFunc eFunc,
 {
     ScFunctionData aData(eFunc);
 
-    ScRange aSingle( rCursor );
-    if ( rMark.IsMarked() )
-        rMark.GetMarkArea(aSingle);
-
-    SCCOL nStartCol = aSingle.aStart.Col();
-    SCROW nStartRow = aSingle.aStart.Row();
-    SCCOL nEndCol = aSingle.aEnd.Col();
-    SCROW nEndRow = aSingle.aEnd.Row();
+    ScMarkData aMark(rMark);
+    aMark.MarkToMulti();
+    if (!aMark.IsMultiMarked())
+        aMark.SetMarkArea(rCursor);
 
     SCTAB nMax = static_cast<SCTAB>(maTabs.size());
-    ScMarkData::const_iterator itr = rMark.begin(), itrEnd = rMark.end();
+    ScMarkData::const_iterator itr = aMark.begin(), itrEnd = aMark.end();
 
     for (; itr != itrEnd && *itr < nMax && !aData.bError; ++itr)
         if (maTabs[*itr])
-            maTabs[*itr]->UpdateSelectionFunction( aData,
-                            nStartCol, nStartRow, nEndCol, nEndRow, rMark );
+            maTabs[*itr]->UpdateSelectionFunction(aData, aMark);
 
             //! rMark an UpdateSelectionFunction uebergeben !!!!!
 
diff --git a/sc/source/core/data/table3.cxx b/sc/source/core/data/table3.cxx
index a92748568d2f..16ed68dd7de6 100644
--- a/sc/source/core/data/table3.cxx
+++ b/sc/source/core/data/table3.cxx
@@ -2308,30 +2308,15 @@ xub_StrLen ScTable::GetMaxNumberStringLen(
         return 0;
 }
 
-void ScTable::UpdateSelectionFunction( ScFunctionData& rData,
-                        SCCOL nStartCol, SCROW nStartRow, SCCOL nEndCol, SCROW nEndRow,
-                        const ScMarkData& rMark )
+void ScTable::UpdateSelectionFunction( ScFunctionData& rData, const ScMarkData& rMark )
 {
-    //  Cursor neben einer Markierung nicht beruecksichtigen:
-    //! nur noch MarkData uebergeben, Cursorposition ggf. hineinselektieren!!!
-    bool bSingle = ( rMark.IsMarked() || !rMark.IsMultiMarked() );
-
-    // Mehrfachselektion:
-
-    SCCOL nCol;
-    if ( rMark.IsMultiMarked() )
-        for (nCol=0; nCol<=MAXCOL && !rData.bError; nCol++)
-            if ( !pColFlags || !ColHidden(nCol) )
-                aCol[nCol].UpdateSelectionFunction( rMark, rData, *mpHiddenRows,
-                                                    bSingle && ( nCol >= nStartCol && nCol <= nEndCol ),
-                                                    nStartRow, nEndRow );
-
-    //  Einfachselektion (oder Cursor) nur wenn nicht negativ (und s.o.):
-
-    if ( bSingle && !rMark.IsMarkNegative() )
-        for (nCol=nStartCol; nCol<=nEndCol && !rData.bError; nCol++)
-            if ( !pColFlags || !ColHidden(nCol) )
-                aCol[nCol].UpdateAreaFunction( rData, *mpHiddenRows, nStartRow, nEndRow );
+    for (SCCOL nCol = 0; nCol <= MAXCOL && !rData.bError; ++nCol)
+    {
+        if (pColFlags && ColHidden(nCol))
+            continue;
+
+        aCol[nCol].UpdateSelectionFunction(rMark, rData, *mpHiddenRows);
+    }
 }
 
 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */