[Libreoffice-commits] core.git: Branch 'distro/collabora/cp-6.2' - sc/qa sc/source
Mike Kaganski (via logerrit)
logerrit at kemper.freedesktop.org
Wed Jun 19 10:17:24 UTC 2019
sc/qa/unit/subsequent_export-test.cxx | 104 ++++++++++++----------------------
sc/source/filter/excel/xetable.cxx | 43 ++++++++------
sc/source/filter/inc/xetable.hxx | 9 +-
3 files changed, 69 insertions(+), 87 deletions(-)
New commits:
commit 149fac648bd78578c2e6a0275bf832053721dcd3
Author: Mike Kaganski <mike.kaganski at collabora.com>
AuthorDate: Tue Apr 23 15:59:24 2019 +0300
Commit: Mike Kaganski <mike.kaganski at collabora.com>
CommitDate: Wed Jun 19 12:16:49 2019 +0200
tdf#124741: export default column width to XLSX
For some reason, we have never exported the default column width
to XLSX, although we have such export for XLS. This led to bugs
like tdf#100946. Workarounds applied to those bugs have made LO
to export columns which shouldn't have been exported, abusing
"customWidth" attribute in XclExpColinfo::XclExpColinfo depending
on if the column has size different from an app-global default;
after that, sheet-local default was determined, and then columns
which have mbCustomWidth set (i.e., different from app default),
as well as those different from sheet-local default, were stored.
Effectively, the hack had disabled the removal of defaults from
maColInfos in XclExpColinfoBuffer::Finalize. We even had unit
tests explicitly testing that we export those columns that Excel
skips. The effect of that is not only unnecessary data in the file;
the data was actually wrong (customWidth actually means that the
width was edited manually by user, even if equal to the default,
thus changing Excel handling of the column); and also Calc
initializes all columns to the right of last used column for such
a file. Only in case when app-global default happened to match
sheet-local one, columns would have properties allowing them to
be removed from maColInfos in the end of XclExpColinfoBuffer::Finalize,
which still resulted in problems similar to the workarounded one.
This patch implements proper export of the default column width
to XLSX, thus partially reverting changes made for tdf#100946 in
commit 40d892a2db4d750aaf0562c63004e693c028273c. Actually, our
export to XLSX does not depend on the 5-pixel correction (see
ECMA-376-1:2016 18.3.1.81), since the exported default depends
only on most-used column width. XclExpDefcolwidth implementation
was edited to only take the correction into account when exporting
to XLS (to keep status quo; it's clear that XLS widths export
and import implementation is incorrect: some empirical formula
is used in XclTools::GetXclDefColWidthCorrection, that was
introduced in commit 555d702903fb0857122024e1ab78a72d122d3f16 for
i#3006, that doesn't match any documentation, and changes widths
of columns in exported XLS - something to be fixed independently).
Change-Id: I227aca17e56247cbb839444717066a898987c4f1
Reviewed-on: https://gerrit.libreoffice.org/71132
Reviewed-by: Mike Kaganski <mike.kaganski at collabora.com>
Tested-by: Mike Kaganski <mike.kaganski at collabora.com>
Reviewed-on: https://gerrit.libreoffice.org/74348
diff --git a/sc/qa/unit/subsequent_export-test.cxx b/sc/qa/unit/subsequent_export-test.cxx
index e7656233f979..305fbced21e0 100644
--- a/sc/qa/unit/subsequent_export-test.cxx
+++ b/sc/qa/unit/subsequent_export-test.cxx
@@ -779,80 +779,60 @@ void ScExportTest::testCustomColumnWidthExportXLSX()
xmlDocPtr pSheet = XPathHelper::parseExport(pXPathFile, m_xSFactory, "xl/worksheets/sheet1.xml");
CPPUNIT_ASSERT(pSheet);
- // First column, has everything default (width in Calc: 1280)
+ // tdf#124741: check that we export default width, otherwise the skipped columns would have
+ // wrong width. Previously defaultColWidth attribute was missing
+ double nDefWidth
+ = getXPath(pSheet, "/x:worksheet/x:sheetFormatPr", "defaultColWidth").toDouble();
+ CPPUNIT_ASSERT_DOUBLES_EQUAL(11.53515625, nDefWidth, 0.01);
+
+ // First column, has everything default (width in Calc: 1280), skipped
+
+ // Second column, has custom width (width in Calc: 1225)
assertXPath(pSheet, "/x:worksheet/x:cols/x:col[1]", "hidden", "false");
assertXPath(pSheet, "/x:worksheet/x:cols/x:col[1]", "outlineLevel", "0");
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col[1]", "customWidth", "false");
+ assertXPath(pSheet, "/x:worksheet/x:cols/x:col[1]", "customWidth", "true");
assertXPath(pSheet, "/x:worksheet/x:cols/x:col[1]", "collapsed", "false");
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col[1]", "min", "1");
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col[1]", "max", "1");
+ assertXPath(pSheet, "/x:worksheet/x:cols/x:col[1]", "min", "2");
+ assertXPath(pSheet, "/x:worksheet/x:cols/x:col[1]", "max", "2");
- // Second column, has custom width (width in Calc: 1225)
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col[2]", "hidden", "false");
+ // Third column, has everything default (width in Calc: 1280), skipped
+
+ // Fourth column has custom width. Columns from 4 to 7 are hidden
+ assertXPath(pSheet, "/x:worksheet/x:cols/x:col[2]", "hidden", "true");
assertXPath(pSheet, "/x:worksheet/x:cols/x:col[2]", "outlineLevel", "0");
assertXPath(pSheet, "/x:worksheet/x:cols/x:col[2]", "customWidth", "true");
assertXPath(pSheet, "/x:worksheet/x:cols/x:col[2]", "collapsed", "false");
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col[2]", "min", "2");
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col[2]", "max", "2");
+ assertXPath(pSheet, "/x:worksheet/x:cols/x:col[2]", "min", "4");
+ assertXPath(pSheet, "/x:worksheet/x:cols/x:col[2]", "max", "4");
- // Third column, has everything default (width in Calc: 1280)
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col[3]", "hidden", "false");
+ // 5th column has custom width. Columns from 4 to 7 are hidden
+ assertXPath(pSheet, "/x:worksheet/x:cols/x:col[3]", "hidden", "true");
assertXPath(pSheet, "/x:worksheet/x:cols/x:col[3]", "outlineLevel", "0");
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col[3]", "customWidth", "false");
+ assertXPath(pSheet, "/x:worksheet/x:cols/x:col[3]", "customWidth", "true");
assertXPath(pSheet, "/x:worksheet/x:cols/x:col[3]", "collapsed", "false");
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col[3]", "min", "3");
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col[3]", "max", "3");
+ assertXPath(pSheet, "/x:worksheet/x:cols/x:col[3]", "min", "5");
+ assertXPath(pSheet, "/x:worksheet/x:cols/x:col[3]", "max", "5");
- // Fourth column has custom width. Columns from 4 to 7 are hidden
+ // 6th and 7th columns have default width and they are hidden
assertXPath(pSheet, "/x:worksheet/x:cols/x:col[4]", "hidden", "true");
assertXPath(pSheet, "/x:worksheet/x:cols/x:col[4]", "outlineLevel", "0");
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col[4]", "customWidth", "true");
+ assertXPath(pSheet, "/x:worksheet/x:cols/x:col[4]", "customWidth", "false");
assertXPath(pSheet, "/x:worksheet/x:cols/x:col[4]", "collapsed", "false");
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col[4]", "min", "4");
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col[4]", "max", "4");
+ assertXPath(pSheet, "/x:worksheet/x:cols/x:col[4]", "min", "6");
+ assertXPath(pSheet, "/x:worksheet/x:cols/x:col[4]", "max", "7");
- // 5th column has custom width. Columns from 4 to 7 are hidden
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col[5]", "hidden", "true");
+ // 8th column has everything default - skipped
+
+ // 9th column has custom width
+ assertXPath(pSheet, "/x:worksheet/x:cols/x:col[5]", "hidden", "false");
assertXPath(pSheet, "/x:worksheet/x:cols/x:col[5]", "outlineLevel", "0");
assertXPath(pSheet, "/x:worksheet/x:cols/x:col[5]", "customWidth", "true");
assertXPath(pSheet, "/x:worksheet/x:cols/x:col[5]", "collapsed", "false");
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col[5]", "min", "5");
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col[5]", "max", "5");
-
- // 6th and 7th columns has default width and it are hidden
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col[6]", "hidden", "true");
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col[6]", "outlineLevel", "0");
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col[6]", "customWidth", "false");
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col[6]", "collapsed", "false");
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col[6]", "min", "6");
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col[6]", "max", "7");
+ assertXPath(pSheet, "/x:worksheet/x:cols/x:col[5]", "min", "9");
+ assertXPath(pSheet, "/x:worksheet/x:cols/x:col[5]", "max", "9");
- // 8th column has everything default
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col[7]", "hidden", "false");
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col[7]", "outlineLevel", "0");
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col[7]", "customWidth", "false");
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col[7]", "collapsed", "false");
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col[7]", "min", "8");
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col[7]", "max", "8");
-
- // 9th column has custom width
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col[8]", "hidden", "false");
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col[8]", "outlineLevel", "0");
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col[8]", "customWidth", "true");
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col[8]", "collapsed", "false");
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col[8]", "min", "9");
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col[8]", "max", "9");
-
- // Rest of columns are default
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col[9]", "hidden", "false");
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col[9]", "outlineLevel", "0");
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col[9]", "customWidth", "false");
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col[9]", "collapsed", "false");
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col[9]", "min", "10");
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col[9]", "max", "1025");
-
- // We expected that exactly 9 unique Nodes will be produced
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col", 9);
+ // We expected that exactly 5 unique Nodes will be produced
+ assertXPath(pSheet, "/x:worksheet/x:cols/x:col", 5);
assertXPath(pSheet, "/x:worksheet/x:sheetData/x:row[1]", "hidden", "false");
assertXPath(pSheet, "/x:worksheet/x:sheetData/x:row[1]", "outlineLevel", "0");
@@ -917,10 +897,7 @@ void ScExportTest::testColumnWidthResaveXLSX()
assertXPath(pSheet, "/x:worksheet/x:cols/x:col[5]", "width", "250");
assertXPath(pSheet, "/x:worksheet/x:cols/x:col[5]", "customWidth", "true");
- // The last column [6] is not existing in Excel sheet, and it is added only by LibreOffice.
- // This column width is default and it is depended on operating system.
-
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col", 6);
+ assertXPath(pSheet, "/x:worksheet/x:cols/x:col", 5);
}
#if HAVE_MORE_FONTS
@@ -1080,13 +1057,8 @@ void ScExportTest::testOutlineExportXLSX()
assertXPath(pSheet, "/x:worksheet/x:cols/x:col[12]", "min", "25");
assertXPath(pSheet, "/x:worksheet/x:cols/x:col[12]", "max", "26");
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col[13]", "hidden", "false");
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col[13]", "outlineLevel", "0");
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col[13]", "collapsed", "false");
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col[13]", "min", "27");
-
- // We expected that exactly 13 unique Nodes will be produced
- assertXPath(pSheet, "/x:worksheet/x:cols/x:col", 13);
+ // We expected that exactly 12 unique Nodes will be produced
+ assertXPath(pSheet, "/x:worksheet/x:cols/x:col", 12);
// First row is empty and default so it is not written into XML file
// so we need to save 29 rows, as it provides information about outLineLevel
diff --git a/sc/source/filter/excel/xetable.cxx b/sc/source/filter/excel/xetable.cxx
index 7ab2bd2b7809..7185d2b67f4c 100644
--- a/sc/source/filter/excel/xetable.cxx
+++ b/sc/source/filter/excel/xetable.cxx
@@ -1543,38 +1543,48 @@ void XclExpDimensions::WriteBody( XclExpStream& rStrm )
namespace {
-double lclGetCorrectedColWidth( const XclExpRoot& rRoot, sal_uInt16 nXclColWidth )
+double lclGetCChCorrection(const XclExpRoot& rRoot)
{
+ // Convert the correction from 1/256ths of a character size to count of chars
+ // TODO: make to fit ECMA-376-1:2016 18.3.1.81 sheetFormatPr (Sheet Format Properties):
+ // 5 pixels are added to the base width: 2 for margin padding on each side, plus 1 for gridline
+ // So this should depend on rRoot.GetCharWidth(), not on font height
+
long nFontHt = rRoot.GetFontBuffer().GetAppFontData().mnHeight;
- return nXclColWidth - XclTools::GetXclDefColWidthCorrection( nFontHt );
+ return XclTools::GetXclDefColWidthCorrection(nFontHt) / 256.0;
}
} // namespace
XclExpDefcolwidth::XclExpDefcolwidth( const XclExpRoot& rRoot ) :
- XclExpUInt16Record( EXC_ID_DEFCOLWIDTH, EXC_DEFCOLWIDTH_DEF ),
+ XclExpDoubleRecord(EXC_ID_DEFCOLWIDTH, EXC_DEFCOLWIDTH_DEF + lclGetCChCorrection(rRoot)),
XclExpRoot( rRoot )
{
}
bool XclExpDefcolwidth::IsDefWidth( sal_uInt16 nXclColWidth ) const
{
- double fNewColWidth = lclGetCorrectedColWidth( GetRoot(), nXclColWidth );
// This formula is taking number of characters with GetValue()
- // and it is translating it into default column width. 0.5 means half character.
+ // and it is translating it into default column width.
// https://msdn.microsoft.com/en-us/library/documentformat.openxml.spreadsheet.column.aspx
- long defaultColumnWidth = static_cast< long >( 256.0 * ( GetValue() + 0.5 ) );
+ double defaultColumnWidth = 256.0 * GetValue();
// exactly matched, if difference is less than 1/16 of a character to the left or to the right
- return std::abs( defaultColumnWidth - fNewColWidth ) < 16;
+ return std::abs(defaultColumnWidth - nXclColWidth) < 256.0 * 1.0 / 16.0;
}
void XclExpDefcolwidth::SetDefWidth( sal_uInt16 nXclColWidth )
{
- double fNewColWidth = lclGetCorrectedColWidth( GetRoot(), nXclColWidth );
- // This function is taking width and translate it into number of characters
- // Next this number of characters are stored. 0.5 means half character.
- SetValue( limit_cast< sal_uInt16 >( fNewColWidth / 256.0 - 0.5 ) );
+ SetValue(nXclColWidth / 256.0);
+}
+
+void XclExpDefcolwidth::Save(XclExpStream& rStrm)
+{
+ double fCorrectedCCh = GetValue() - lclGetCChCorrection(GetRoot());
+ // Convert double to sal_uInt16
+ XclExpUInt16Record aUInt16Rec(GetRecId(),
+ static_cast<sal_uInt16>(std::round(fCorrectedCCh)));
+ aUInt16Rec.Save(rStrm);
}
XclExpColinfo::XclExpColinfo( const XclExpRoot& rRoot,
@@ -1604,10 +1614,6 @@ XclExpColinfo::XclExpColinfo( const XclExpRoot& rRoot,
// column flags
::set_flag( mnFlags, EXC_COLINFO_HIDDEN, rDoc.ColHidden(nScCol, nScTab) );
- XclExpDefcolwidth defColWidth = XclExpDefcolwidth( rRoot );
- mbCustomWidth = !defColWidth.IsDefWidth( mnWidth );
- set_flag(mnFlags, EXC_COLINFO_CUSTOMWIDTH, mbCustomWidth);
-
// outline data
rOutlineBfr.Update( nScCol );
::set_flag( mnFlags, EXC_COLINFO_COLLAPSED, rOutlineBfr.IsCollapsed() );
@@ -1620,12 +1626,13 @@ void XclExpColinfo::ConvertXFIndexes()
maXFId.ConvertXFIndex( GetRoot() );
}
-bool XclExpColinfo::IsDefault( const XclExpDefcolwidth& rDefColWidth ) const
+bool XclExpColinfo::IsDefault( const XclExpDefcolwidth& rDefColWidth )
{
+ mbCustomWidth = !rDefColWidth.IsDefWidth(mnWidth);
return (maXFId.mnXFIndex == EXC_XF_DEFAULTCELL) &&
(mnFlags == 0) &&
(mnOutlineLevel == 0) &&
- rDefColWidth.IsDefWidth( mnWidth );
+ !mbCustomWidth;
}
bool XclExpColinfo::TryMerge( const XclExpColinfo& rColInfo )
@@ -2693,7 +2700,7 @@ void XclExpCellTable::SaveXml( XclExpXmlStream& rStrm )
sax_fastparser::FSHelperPtr& rWorksheet = rStrm.GetCurrentStream();
rWorksheet->startElement( XML_sheetFormatPr,
// OOXTODO: XML_baseColWidth
- // OOXTODO: XML_defaultColWidth
+ XML_defaultColWidth, OString::number(maColInfoBfr.GetDefColWidth()),
// OOXTODO: XML_customHeight
// OOXTODO: XML_thickTop
// OOXTODO: XML_thickBottom
diff --git a/sc/source/filter/inc/xetable.hxx b/sc/source/filter/inc/xetable.hxx
index de398e614f89..fe22d555b80a 100644
--- a/sc/source/filter/inc/xetable.hxx
+++ b/sc/source/filter/inc/xetable.hxx
@@ -676,7 +676,7 @@ private:
default width. If the passed width is rounded up or down to get the default
width, the function returns false.
*/
-class XclExpDefcolwidth : public XclExpUInt16Record, protected XclExpRoot
+class XclExpDefcolwidth : public XclExpDoubleRecord, protected XclExpRoot
{
public:
explicit XclExpDefcolwidth( const XclExpRoot& rRoot );
@@ -686,6 +686,8 @@ public:
/** Sets the passed column width (in 1/256 character width) as default width. */
void SetDefWidth( sal_uInt16 nXclColWidth );
+
+ virtual void Save(XclExpStream& rStrm) override;
};
/** Contains the column settings for a range of columns.
@@ -717,8 +719,8 @@ public:
/** Returns the number of columns represented by this record. */
sal_uInt16 GetColCount() const { return mnLastXclCol - mnFirstXclCol + 1; }
- /** Returns true, if the column has default format and width. */
- bool IsDefault( const XclExpDefcolwidth& rDefColWidth ) const;
+ /** Returns true, if the column has default format and width. Also sets mbCustomWidth */
+ bool IsDefault( const XclExpDefcolwidth& rDefColWidth );
virtual void SaveXml( XclExpXmlStream& rStrm ) override;
@@ -760,6 +762,7 @@ public:
virtual void Save( XclExpStream& rStrm ) override;
virtual void SaveXml( XclExpXmlStream& rStrm ) override;
sal_uInt8 GetHighestOutlineLevel() { return mnHighestOutlineLevel; }
+ double GetDefColWidth() { return maDefcolwidth.GetValue(); }
private:
typedef XclExpRecordList< XclExpColinfo > XclExpColinfoList;
More information about the Libreoffice-commits
mailing list