[Libreoffice] [REVIEW] Fix incorrect import of multi-range data source in chart

I'd like to backport the attached patch to the 3-4 branch.

Without this patch, import of charts in Excel documents fail when the
data series consist of multiple cell ranges, which is common in complex
chart objects.  The failure happens only in locales where the function
and range separator is ',' (Tools - Options - Calc - Formula -
Separators - Function), which is default in the English locales and
others.

I've attached a very small example document to demonstrate the failed
chart import, which is amazingly smaller than the patch itself when
compressed. ;-)

Anyway, review and sign-off very much appreciated.

Kohei

-- 
Kohei Yoshida, LibreOffice hacker, Calc
<kohei.yoshida@suse.com>

From 0e83658f97696e498ebbc616056fe1a4089ce321 Mon Sep 17 00:00:00 2001
From: Kohei Yoshida <kohei.yoshida@suse.com>
Date: Tue, 9 Aug 2011 15:49:15 -0400
Subject: [PATCH] Correctly import from Excel charts with multiple ranges.

This was "fixed" in i#107275, but the fix only worked in locales
where the range/argument separator was ';' (e.g. German locale).
This is the correct fix that works in all locales regardless of
the separator.
---
 sc/inc/reftokenhelper.hxx                     |    2 +-
 sc/source/core/tool/reftokenhelper.cxx        |    4 +-
 sc/source/filter/xml/XMLTableShapeResizer.cxx |    5 ++-
 sc/source/ui/unoobj/chart2uno.cxx             |   38 +++++++++++++++++--------
 4 files changed, 33 insertions(+), 16 deletions(-)

diff --git a/sc/inc/reftokenhelper.hxx b/sc/inc/reftokenhelper.hxx
index 8451a16..3566fd7 100644
--- a/sc/inc/reftokenhelper.hxx
+++ b/sc/inc/reftokenhelper.hxx
@@ -55,7 +55,7 @@ public:
      */
     static void compileRangeRepresentation(
         ::std::vector<ScTokenRef>& rRefTokens, const ::rtl::OUString& rRangeStr, ScDocument* pDoc,
-        ::formula::FormulaGrammar::Grammar eGrammar = ::formula::FormulaGrammar::GRAM_ENGLISH);
+        const sal_Unicode cSep, ::formula::FormulaGrammar::Grammar eGrammar);
 
     static bool getRangeFromToken(ScRange& rRange, const ScTokenRef& pToken, bool bExternal = 
false);
 
diff --git a/sc/source/core/tool/reftokenhelper.cxx b/sc/source/core/tool/reftokenhelper.cxx
index e77f130..fb54280 100644
--- a/sc/source/core/tool/reftokenhelper.cxx
+++ b/sc/source/core/tool/reftokenhelper.cxx
@@ -47,9 +47,9 @@ using ::std::auto_ptr;
 using ::rtl::OUString;
 
 void ScRefTokenHelper::compileRangeRepresentation(
-    vector<ScTokenRef>& rRefTokens, const OUString& rRangeStr, ScDocument* pDoc, 
FormulaGrammar::Grammar eGrammar)
+    vector<ScTokenRef>& rRefTokens, const OUString& rRangeStr, ScDocument* pDoc,
+    const sal_Unicode cSep, FormulaGrammar::Grammar eGrammar)
 {
-    const sal_Unicode cSep = ScCompiler::GetNativeSymbol(ocSep).GetChar(0);
     const sal_Unicode cQuote = '\'';
 
     // #i107275# ignore parentheses
diff --git a/sc/source/filter/xml/XMLTableShapeResizer.cxx 
b/sc/source/filter/xml/XMLTableShapeResizer.cxx
index e0425c0..6d61b80 100644
--- a/sc/source/filter/xml/XMLTableShapeResizer.cxx
+++ b/sc/source/filter/xml/XMLTableShapeResizer.cxx
@@ -35,6 +35,7 @@
 #include "chartlis.hxx"
 #include "XMLConverter.hxx"
 #include "rangeutl.hxx"
+#include "compiler.hxx"
 #include "reftokenhelper.hxx"
 #include <tools/debug.hxx>
 #include <com/sun/star/sheet/XSpreadsheetDocument.hpp>
@@ -94,7 +95,9 @@ void ScMyOLEFixer::CreateChartListener(ScDocument* pDoc,
         return;
 
     auto_ptr< vector<ScTokenRef> > pRefTokens(new vector<ScTokenRef>);
-    ScRefTokenHelper::compileRangeRepresentation(*pRefTokens, aRangeStr, pDoc);
+    const sal_Unicode cSep = ScCompiler::GetNativeSymbol(ocSep).GetChar(0);
+    ScRefTokenHelper::compileRangeRepresentation(
+        *pRefTokens, aRangeStr, pDoc, cSep, formula::FormulaGrammar::GRAM_ENGLISH);
     if (!pRefTokens->empty())
     {
         ScChartListener* pCL(new ScChartListener(rName, pDoc, pRefTokens.release()));
diff --git a/sc/source/ui/unoobj/chart2uno.cxx b/sc/source/ui/unoobj/chart2uno.cxx
index 32fc54a..8c43501 100644
--- a/sc/source/ui/unoobj/chart2uno.cxx
+++ b/sc/source/ui/unoobj/chart2uno.cxx
@@ -1043,7 +1043,9 @@ void ScChart2DataProvider::Notify( SfxBroadcaster& /*rBC*/, const SfxHint& 
rHint
     }
 
     vector<ScTokenRef> aTokens;
-    ScRefTokenHelper::compileRangeRepresentation(aTokens, aRangeRepresentation, m_pDocument, 
m_pDocument->GetGrammar());
+    const sal_Unicode cSep = ScCompiler::GetNativeSymbol(ocSep).GetChar(0);
+    ScRefTokenHelper::compileRangeRepresentation(
+        aTokens, aRangeRepresentation, m_pDocument, cSep, m_pDocument->GetGrammar());
     return !aTokens.empty();
 }
 
@@ -1419,7 +1421,9 @@ ScChart2DataProvider::createDataSource(
     }
 
     vector<ScTokenRef> aRefTokens;
-    ScRefTokenHelper::compileRangeRepresentation(aRefTokens, aRangeRepresentation, m_pDocument, 
m_pDocument->GetGrammar());
+    const sal_Unicode cSep = ScCompiler::GetNativeSymbol(ocSep).GetChar(0);
+    ScRefTokenHelper::compileRangeRepresentation(
+        aRefTokens, aRangeRepresentation, m_pDocument, cSep, m_pDocument->GetGrammar());
     if (aRefTokens.empty())
         // Invalid range representation.  Bail out.
         throw lang::IllegalArgumentException();
@@ -1743,7 +1747,9 @@ uno::Sequence< beans::PropertyValue > SAL_CALL 
ScChart2DataProvider::detectArgum
                 {
                     bFirstCellAsLabel = true;
                     vector<ScTokenRef> aTokens;
-                    ScRefTokenHelper::compileRangeRepresentation( aTokens, 
xLabel->getSourceRangeRepresentation(), m_pDocument, m_pDocument->GetGrammar() );
+                    const sal_Unicode cSep = ScCompiler::GetNativeSymbol(ocSep).GetChar(0);
+                    ScRefTokenHelper::compileRangeRepresentation(
+                        aTokens, xLabel->getSourceRangeRepresentation(), m_pDocument, cSep, 
m_pDocument->GetGrammar());
                     aLabel.initRangeAnalyzer(aTokens);
                     vector<ScTokenRef>::const_iterator itr = aTokens.begin(), itrEnd = 
aTokens.end();
                     for (; itr != itrEnd; ++itr)
@@ -1760,7 +1766,9 @@ uno::Sequence< beans::PropertyValue > SAL_CALL 
ScChart2DataProvider::detectArgum
                 if( xValues.is())
                 {
                     vector<ScTokenRef> aTokens;
-                    ScRefTokenHelper::compileRangeRepresentation( aTokens, 
xValues->getSourceRangeRepresentation(), m_pDocument, m_pDocument->GetGrammar() );
+                    const sal_Unicode cSep = ScCompiler::GetNativeSymbol(ocSep).GetChar(0);
+                    ScRefTokenHelper::compileRangeRepresentation(
+                        aTokens, xValues->getSourceRangeRepresentation(), m_pDocument, cSep, 
m_pDocument->GetGrammar());
                     aValues.initRangeAnalyzer(aTokens);
                     vector<ScTokenRef>::const_iterator itr = aTokens.begin(), itrEnd = 
aTokens.end();
                     for (; itr != itrEnd; ++itr)
@@ -1967,7 +1975,9 @@ uno::Sequence< beans::PropertyValue > SAL_CALL 
ScChart2DataProvider::detectArgum
         return false;
 
     vector<ScTokenRef> aTokens;
-    ScRefTokenHelper::compileRangeRepresentation(aTokens, aRangeRepresentation, m_pDocument, 
m_pDocument->GetGrammar());
+    const sal_Unicode cSep = ScCompiler::GetNativeSymbol(ocSep).GetChar(0);
+    ScRefTokenHelper::compileRangeRepresentation(
+        aTokens, aRangeRepresentation, m_pDocument, cSep, m_pDocument->GetGrammar());
     return !aTokens.empty();
 }
 
@@ -1984,14 +1994,16 @@ uno::Reference< chart2::data::XDataSequence > SAL_CALL
     if(!m_pDocument || (aRangeRepresentation.getLength() == 0))
         return xResult;
 
-    // Note: the range representation must be in Calc A1 format.  The import 
-    // filters use this method to pass data ranges, and they have no idea what
-    // the current formula syntax is.  In the future we should add another
-    // method to allow the client code to directly pass tokens representing
-    // ranges.
+    // Note: the range representation must be in Calc A1 format, with English
+    // function names and ';' as the union operator in case of multiple
+    // ranges. The import filters use this method to pass data ranges, and
+    // they have no idea what the current formula syntax is. In the future we
+    // should add another method to allow the client code to directly pass
+    // tokens representing ranges.
 
     vector<ScTokenRef> aRefTokens;
-    ScRefTokenHelper::compileRangeRepresentation(aRefTokens, aRangeRepresentation, m_pDocument);
+    ScRefTokenHelper::compileRangeRepresentation(
+        aRefTokens, aRangeRepresentation, m_pDocument, ';', FormulaGrammar::GRAM_ENGLISH);
     if (aRefTokens.empty())
         return xResult;
 
@@ -2029,7 +2041,9 @@ rtl::OUString SAL_CALL ScChart2DataProvider::convertRangeToXML( const 
rtl::OUStr
         return aRet;
 
     vector<ScTokenRef> aRefTokens;
-    ScRefTokenHelper::compileRangeRepresentation(aRefTokens, sRangeRepresentation, m_pDocument, 
m_pDocument->GetGrammar());
+    const sal_Unicode cSep = ScCompiler::GetNativeSymbol(ocSep).GetChar(0);
+    ScRefTokenHelper::compileRangeRepresentation(
+        aRefTokens, sRangeRepresentation, m_pDocument, cSep, m_pDocument->GetGrammar());
     if (aRefTokens.empty())
         throw lang::IllegalArgumentException();
 
-- 
1.7.3.4

Attachment: chart-series-disjoint.xls.gz
Description: GNU Zip compressed data