tdf#130104 - FILESAVE XLSX: cell indent increased on each save

In OOXML, 1 indent = 3 space char width.

-----
The Old Method:
-----

XLSX Import:

As per the line:
sal_Int32 nIndent = getUnitConverter().scaleToMm100( 3.0 * maModel.mnIndent, Unit::Space );
assume the width of space char is 88, then:
If the OOXML indent is 1, then nIndent would be 264.5, and casted to 264.
If the OOXML indent is 2, then nIndent would be 528.5, and casted to 528.
If the OOXML indent is 3, then nIndent would be 792.5, and casted to 792.
...

Also, as Mike Kaganski has pointed out, we use twips in sc indent internally, thus it is wrong to convert to Mm100 unit here.

XLSX Export:

As per the line:
nTmpIndent = (nTmpIndent + 100) / 200;
Assume we did not edit the document upon open, and simply save it. Now:
If our indent is 264, then the calculated OOXML indent would be 1.82, and then casted to 1, while the expected value is 1.
If our indent is 528, then the calculated OOXML indent would be 3.14, and then casted to 3, while the expected value is 2.
If our indent is 792, then the calculated OOXML indent would be 4.46, and then casted to 4, while the expected value is 3.
...

Then if you reopen the saved xlsx file with Calc, the increament of indent continues on each save which causes serious
format loss.

Most importantly, if you change the indent of cells using the Calc toolbar indent icon, one-click would be 10pt = 200 twips,
see defined macro SC_INDENT_STEP. This causes a mess when you change the indent in an xlsx document.

-----
The New Method
-----

In this patch, I have changed the XLSX import to convert the excel indent unit to 3-spaces-width *in twips*.
Then, per code advice from Mike Kaganski, as a mirror operation, I have changed the XLSX export logic to detect the width
of the space char (which *should* be the same as the one detected at the time of xlsx import), and use this width to convert
the indent in twips unit to excel unit. This way, the indent will remain the same on xlsx export.

-----
TODO:
-----

1. On xlsx import of the file tdf130104_indent.xlsx, the default font (i.e. font for the "Normal" style) is "Times New Roman".
However, when the UI locale is set to Simplified Chinese and "Asian" option is enabled in Tools->Options->Language Settigns->
Languages->"Default Languages for Documents", upon resave as xlsx, the default font for the document is changed to "Noto Sans CJK SC"
on my system, which causes the space-width detected to be different from the width detected on xlsx import. This seems to be another
bug, see tdf#131349. (Luckily the unit test in this patch passes, this is because the change of space width resulted from the change
in default font is very small thus the conversion is not impacted.)

2. The UI part need to be improved, so that after xlsx import, if the user hit the "Increase Indent" or "Decrease Indent" toolar
icon to change the indent, Calc should be able to detect that we are operating in an xlsx file, thus the "increment" should
be 3 * width of space char, rather than the current SC_INDENT_STEP. Also, the if the user changes the default font of the xlsx
document, the Calc should recalculate the indent for each cell to reflect the possible change in width of space char.

Change-Id: I5f7a4ecbcd93079d1c19db3b0b641dda949f6fbf
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/123111
Tested-by: Mike Kaganski <mike.kaganski@collabora.com>
Reviewed-by: Mike Kaganski <mike.kaganski@collabora.com>
(cherry picked from commit 6b93ee72df1aa42d1a3482ffc396bd0c23134f8b)
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/123872
Tested-by: Jenkins

8 files changed, 119 insertions, 13 deletions

diff --git a/sc/qa/unit/data/xlsx/tdf130104_indent.xlsx b/sc/qa/unit/data/xlsx/tdf130104_indent.xlsx
new file mode 100644
index 000000000000..9cb1e78e4587
--- /dev/null
+++ b/sc/qa/unit/data/xlsx/tdf130104_indent.xlsx
diff --git a/sc/qa/unit/subsequent_export-test2.cxx b/sc/qa/unit/subsequent_export-test2.cxx
index cc266e5a2bc0..361ab683a7f3 100644
--- a/sc/qa/unit/subsequent_export-test2.cxx
+++ b/sc/qa/unit/subsequent_export-test2.cxx
@@ -202,6 +202,7 @@ public:
     void testTdf142264ManyChartsToXLSX();
     void testTdf143929MultiColumnToODS();
     void testTdf142578();
+    void testTdf130104_XLSXIndent();
 
     CPPUNIT_TEST_SUITE(ScExportTest2);
 
@@ -305,6 +306,7 @@ public:
     CPPUNIT_TEST(testTdf142264ManyChartsToXLSX);
     CPPUNIT_TEST(testTdf143929MultiColumnToODS);
     CPPUNIT_TEST(testTdf142578);
+    CPPUNIT_TEST(testTdf130104_XLSXIndent);
 
     CPPUNIT_TEST_SUITE_END();
 
@@ -2666,6 +2668,95 @@ void ScExportTest2::testTdf142578()
     xDocSh->DoClose();
 }
 
+void ScExportTest2::testTdf130104_XLSXIndent()
+{
+    ScDocShellRef xDocSh = loadDoc(u"tdf130104_indent.", FORMAT_XLSX);
+    CPPUNIT_ASSERT(xDocSh);
+
+    // Resave the xlsx file without any modification.
+    std::shared_ptr<utl::TempFile> pXPathFile
+        = ScBootstrapFixture::exportTo(&(*xDocSh), FORMAT_XLSX);
+    xmlDocUniquePtr pSheet
+        = XPathHelper::parseExport(pXPathFile, m_xSFactory, "xl/worksheets/sheet1.xml");
+    CPPUNIT_ASSERT(pSheet);
+    xmlDocUniquePtr pStyle = XPathHelper::parseExport(pXPathFile, m_xSFactory, "xl/styles.xml");
+    CPPUNIT_ASSERT(pStyle);
+
+    // Check to see whether the indents remain the same as the original ones:
+
+    // Get the style index number for cell A1
+    sal_Int32 nCellA1StyleIndex
+        = getXPath(pSheet, "/x:worksheet/x:sheetData/x:row[1]/x:c[1]", "s").toInt32() + 1;
+    // The indent for cell A1 should be 0
+    OString sStyleA1XPath
+        = "/x:styleSheet/x:cellXfs/x:xf[" + OString::number(nCellA1StyleIndex) + "]/x:alignment";
+    // (if this assertion fails, you should first check whether there is no style index set for this cell)
+    assertXPath(pStyle, sStyleA1XPath, "indent", "0");
+
+    sal_Int32 nCellA3StyleIndex
+        = getXPath(pSheet, "/x:worksheet/x:sheetData/x:row[3]/x:c[1]", "s").toInt32() + 1;
+    // The indent for cell A3 should be 1
+    OString sStyleA3XPath
+        = "/x:styleSheet/x:cellXfs/x:xf[" + OString::number(nCellA3StyleIndex) + "]/x:alignment";
+    assertXPath(pStyle, sStyleA3XPath, "indent", "1");
+
+    sal_Int32 nCellA6StyleIndex
+        = getXPath(pSheet, "/x:worksheet/x:sheetData/x:row[6]/x:c[1]", "s").toInt32() + 1;
+    OString sStyleA6XPath
+        = "/x:styleSheet/x:cellXfs/x:xf[" + OString::number(nCellA6StyleIndex) + "]/x:alignment";
+    assertXPath(pStyle, sStyleA6XPath, "indent", "2");
+
+    sal_Int32 nCellA9StyleIndex
+        = getXPath(pSheet, "/x:worksheet/x:sheetData/x:row[9]/x:c[1]", "s").toInt32() + 1;
+    OString sStyleA9XPath
+        = "/x:styleSheet/x:cellXfs/x:xf[" + OString::number(nCellA9StyleIndex) + "]/x:alignment";
+    assertXPath(pStyle, sStyleA9XPath, "indent", "3");
+
+    sal_Int32 nCellA12StyleIndex
+        = getXPath(pSheet, "/x:worksheet/x:sheetData/x:row[12]/x:c[1]", "s").toInt32() + 1;
+    OString sStyleA12XPath
+        = "/x:styleSheet/x:cellXfs/x:xf[" + OString::number(nCellA12StyleIndex) + "]/x:alignment";
+    assertXPath(pStyle, sStyleA12XPath, "indent", "4");
+
+    sal_Int32 nCellA15StyleIndex
+        = getXPath(pSheet, "/x:worksheet/x:sheetData/x:row[15]/x:c[1]", "s").toInt32() + 1;
+    OString sStyleA15XPath
+        = "/x:styleSheet/x:cellXfs/x:xf[" + OString::number(nCellA15StyleIndex) + "]/x:alignment";
+    assertXPath(pStyle, sStyleA15XPath, "indent", "5");
+
+    sal_Int32 nCellA18StyleIndex
+        = getXPath(pSheet, "/x:worksheet/x:sheetData/x:row[18]/x:c[1]", "s").toInt32() + 1;
+    OString sStyleA18XPath
+        = "/x:styleSheet/x:cellXfs/x:xf[" + OString::number(nCellA18StyleIndex) + "]/x:alignment";
+    assertXPath(pStyle, sStyleA18XPath, "indent", "6");
+
+    sal_Int32 nCellA21StyleIndex
+        = getXPath(pSheet, "/x:worksheet/x:sheetData/x:row[21]/x:c[1]", "s").toInt32() + 1;
+    OString sStyleA21XPath
+        = "/x:styleSheet/x:cellXfs/x:xf[" + OString::number(nCellA21StyleIndex) + "]/x:alignment";
+    assertXPath(pStyle, sStyleA21XPath, "indent", "7");
+
+    sal_Int32 nCellA24StyleIndex
+        = getXPath(pSheet, "/x:worksheet/x:sheetData/x:row[24]/x:c[1]", "s").toInt32() + 1;
+    OString sStyleA24XPath
+        = "/x:styleSheet/x:cellXfs/x:xf[" + OString::number(nCellA24StyleIndex) + "]/x:alignment";
+    assertXPath(pStyle, sStyleA24XPath, "indent", "8");
+
+    sal_Int32 nCellA27StyleIndex
+        = getXPath(pSheet, "/x:worksheet/x:sheetData/x:row[27]/x:c[1]", "s").toInt32() + 1;
+    OString sStyleA27XPath
+        = "/x:styleSheet/x:cellXfs/x:xf[" + OString::number(nCellA27StyleIndex) + "]/x:alignment";
+    assertXPath(pStyle, sStyleA27XPath, "indent", "9");
+
+    sal_Int32 nCellA30StyleIndex
+        = getXPath(pSheet, "/x:worksheet/x:sheetData/x:row[30]/x:c[1]", "s").toInt32() + 1;
+    OString sStyleA30XPath
+        = "/x:styleSheet/x:cellXfs/x:xf[" + OString::number(nCellA30StyleIndex) + "]/x:alignment";
+    assertXPath(pStyle, sStyleA30XPath, "indent", "10");
+
+    xDocSh->DoClose();
+}
+
 CPPUNIT_TEST_SUITE_REGISTRATION(ScExportTest2);
 
 CPPUNIT_PLUGIN_IMPLEMENT();
diff --git a/sc/source/filter/excel/xeextlst.cxx b/sc/source/filter/excel/xeextlst.cxx
index 1f0d8fcc942d..f556c47ecdca 100644
--- a/sc/source/filter/excel/xeextlst.cxx
+++ b/sc/source/filter/excel/xeextlst.cxx
@@ -211,7 +211,7 @@ void XclExpExtCF::SaveXml( XclExpXmlStream& rStrm )
         pBorder.reset();
 
     std::unique_ptr<XclExpCellAlign> pAlign(new XclExpCellAlign);
-    if (!pAlign->FillFromItemSet( rSet, false, GetBiff()))
+    if (!pAlign->FillFromItemSet(*this, rSet, false, GetBiff()))
         pAlign.reset();
 
     std::unique_ptr<XclExpCellProt> pCellProt(new XclExpCellProt);
diff --git a/sc/source/filter/excel/xestyle.cxx b/sc/source/filter/excel/xestyle.cxx
index 243560cf494b..9e576ca283ed 100644
--- a/sc/source/filter/excel/xestyle.cxx
+++ b/sc/source/filter/excel/xestyle.cxx
@@ -1445,8 +1445,8 @@ void XclExpCellProt::SaveXml( XclExpXmlStream& rStrm ) const
             XML_hidden,     ToPsz( mbHidden ) );
 }
 
-bool XclExpCellAlign::FillFromItemSet(
-        const SfxItemSet& rItemSet, bool bForceLineBreak, XclBiff eBiff, bool bStyle )
+bool XclExpCellAlign::FillFromItemSet(const XclRoot& rRoot, const SfxItemSet& rItemSet,
+                                      bool bForceLineBreak, XclBiff eBiff, bool bStyle)
 {
     bool bUsed = false;
     SvxCellHorJustify eHorAlign = rItemSet.Get( ATTR_HOR_JUSTIFY ).GetValue();
@@ -1457,9 +1457,10 @@ bool XclExpCellAlign::FillFromItemSet(
         case EXC_BIFF8: // attributes new in BIFF8
         {
             // text indent
-            tools::Long nTmpIndent = rItemSet.Get( ATTR_INDENT ).GetValue();
-            nTmpIndent = (nTmpIndent + 100) / 200; // 1 Excel unit == 10 pt == 200 twips
-            mnIndent = limit_cast< sal_uInt8 >( nTmpIndent, 0, 15 );
+            tools::Long nTmpIndent = rItemSet.Get( ATTR_INDENT ).GetValue();    // already in twips
+            tools::Long nSpaceWidth = rRoot.GetSpaceWidth();
+            sal_Int32 nIndent = static_cast<double>(nTmpIndent) / (3.0 * nSpaceWidth) + 0.5;
+            mnIndent = limit_cast< sal_uInt8 >( nIndent, 0, 15 );
             bUsed |= ScfTools::CheckItem( rItemSet, ATTR_INDENT, bStyle );
 
             // shrink to fit
@@ -2141,7 +2142,7 @@ void XclExpXF::Init( const SfxItemSet& rItemSet, sal_Int16 nScript,
     mbFmtUsed = ScfTools::CheckItem( rItemSet, ATTR_VALUE_FORMAT, IsStyleXF() );
 
     // alignment
-    mbAlignUsed = maAlignment.FillFromItemSet( rItemSet, bForceLineBreak, GetBiff(), IsStyleXF() );
+    mbAlignUsed = maAlignment.FillFromItemSet(*this, rItemSet, bForceLineBreak, GetBiff(), IsStyleXF());
 
     // cell border
     mbBorderUsed = maBorder.FillFromItemSet( rItemSet, GetPalette(), GetBiff(), IsStyleXF() );
@@ -3135,7 +3136,7 @@ XclExpDxfs::XclExpDxfs( const XclExpRoot& rRoot )
                         }
 
                         std::unique_ptr<XclExpCellAlign> pAlign(new XclExpCellAlign);
-                        if (!pAlign->FillFromItemSet( rSet, false, GetBiff()))
+                        if (!pAlign->FillFromItemSet(rRoot, rSet, false, GetBiff()))
                         {
                             pAlign.reset();
                         }
diff --git a/sc/source/filter/excel/xlroot.cxx b/sc/source/filter/excel/xlroot.cxx
index c232b6082b2f..190f85d74023 100644
--- a/sc/source/filter/excel/xlroot.cxx
+++ b/sc/source/filter/excel/xlroot.cxx
@@ -100,6 +100,7 @@ XclRootData::XclRootData( XclBiff eBiff, SfxMedium& rMedium,
     mfScreenPixelX( 50.0 ),
     mfScreenPixelY( 50.0 ),
     mnCharWidth( 110 ),
+    mnSpaceWidth(45),
     mnScTab( 0 ),
     mbExport( bExport )
 {
@@ -217,6 +218,9 @@ void XclRoot::SetCharWidth( const XclFontData& rFontData )
         // UnitConverter::finalizeImport()
         for (sal_Unicode cChar = '0'; cChar <= '9'; ++cChar)
             mrData.mnCharWidth = std::max( pPrinter->GetTextWidth( OUString(cChar)), mrData.mnCharWidth);
+
+        // Set the width of space ' ' character.
+        mrData.mnSpaceWidth = pPrinter->GetTextWidth(OUString(' '));
     }
     if( mrData.mnCharWidth <= 0 )
     {
@@ -224,6 +228,11 @@ void XclRoot::SetCharWidth( const XclFontData& rFontData )
         SAL_WARN( "sc", "XclRoot::SetCharWidth - invalid character width (no printer?)" );
         mrData.mnCharWidth = 11 * rFontData.mnHeight / 20;
     }
+    if (mrData.mnSpaceWidth <= 0)
+    {
+        SAL_WARN( "sc", "XclRoot::SetCharWidth - invalid character width (no printer?)" );
+        mrData.mnSpaceWidth = 45;
+    }
 }
 
 sal_Int32 XclRoot::GetHmmFromPixelX( double fPixelX ) const
diff --git a/sc/source/filter/inc/xestyle.hxx b/sc/source/filter/inc/xestyle.hxx
index 52afbd9c5673..173c9b6e6f02 100644
--- a/sc/source/filter/inc/xestyle.hxx
+++ b/sc/source/filter/inc/xestyle.hxx
@@ -325,7 +325,7 @@ struct XclExpCellAlign : public XclCellAlign
         @descr  Fills only the attributes exported in the passed BIFF version.
         @param bForceLineBreak  true = Set line break flag unconditionally.
         @return  true = At least one alignment item is set. */
-    bool                FillFromItemSet( const SfxItemSet& rItemSet,
+    bool                FillFromItemSet(const XclRoot& rRoot, const SfxItemSet& rItemSet,
                             bool bForceLineBreak, XclBiff eBiff, bool bStyle = false );
 
     /** Fills the data to the passed fields of a BIFF5/BIFF7 XF record. */
diff --git a/sc/source/filter/inc/xlroot.hxx b/sc/source/filter/inc/xlroot.hxx
index ded9a1ece30d..3e6db3701fd3 100644
--- a/sc/source/filter/inc/xlroot.hxx
+++ b/sc/source/filter/inc/xlroot.hxx
@@ -103,7 +103,8 @@ struct XclRootData
 
     double              mfScreenPixelX;     /// Width of a screen pixel (1/100 mm).
     double              mfScreenPixelY;     /// Height of a screen pixel (1/100 mm).
-    tools::Long                mnCharWidth;        /// Width of '0' in default font (twips).
+    tools::Long         mnCharWidth;        /// Width of '0' in default font (twips).
+    tools::Long         mnSpaceWidth;       /// Width of space char ' ' using default font.
     SCTAB               mnScTab;            /// Current Calc sheet index.
     const bool          mbExport;           /// false = Import, true = Export.
 
@@ -153,7 +154,8 @@ public:
     /** Returns the default script type, e.g. for blank cells. */
     sal_Int16    GetDefApiScript() const { return mrData.mnDefApiScript; }
     /** Returns the width of the '0' character (default font) for the current printer (twips). */
-    tools::Long         GetCharWidth() const { return mrData.mnCharWidth; }
+    tools::Long  GetCharWidth() const { return mrData.mnCharWidth; }
+    tools::Long  GetSpaceWidth() const { return mrData.mnSpaceWidth; }
     /** Returns the current Calc sheet index. */
     bool         IsInGlobals() const { return mrData.mnScTab == SCTAB_GLOBAL; }
     /** Returns the current Calc sheet index. */
@@ -251,7 +253,8 @@ public:
     void         SetUILanguage( LanguageType eLang ) { mrData.meUILang = eLang; }
     /** Sets the text encoding to import/export byte strings. */
     void                SetTextEncoding( rtl_TextEncoding eTextEnc );
-    /** Sets the width of the '0' character (default font) for the current printer (twips).
+    /** Sets the width of the '0' - '9' digit character as well as the ' ' space char
+        (using the default font) for the current printer (twips).
         @param rFontData  The font used for the '0' character. */
     void                SetCharWidth( const XclFontData& rFontData );
     /** Sets the current Calc sheet index. */
diff --git a/sc/source/filter/oox/stylesbuffer.cxx b/sc/source/filter/oox/stylesbuffer.cxx
index 7b02c2a36dcf..d40fcdc3d395 100644
--- a/sc/source/filter/oox/stylesbuffer.cxx
+++ b/sc/source/filter/oox/stylesbuffer.cxx
@@ -1183,7 +1183,9 @@ void Alignment::finalizeImport()
 
     /*  indentation: expressed as number of blocks of 3 space characters in
         OOXML. */
-    sal_Int32 nIndent = getUnitConverter().scaleToMm100( 3.0 * maModel.mnIndent, Unit::Space );
+    UnitConverter& rUnitConverter = getUnitConverter();
+    // Note: indents are stored in twips
+    sal_Int32 nIndent = rUnitConverter.scaleValue( 3.0 * maModel.mnIndent, Unit::Space, Unit::Twip);
     if( (0 <= nIndent) && (nIndent <= SAL_MAX_INT16) )
         maApiData.mnIndent = static_cast< sal_Int16 >( nIndent );