summaryrefslogtreecommitdiff
path: root/sc
diff options
context:
space:
mode:
authorMike Kaganski <mike.kaganski@collabora.com>2021-08-18 23:13:05 +0300
committerMike Kaganski <mike.kaganski@collabora.com>2021-08-20 07:47:08 +0200
commit0d1540f6007e0839199cab72bdff77077f25ef65 (patch)
tree4cded1b709cc987fd3a023048f5f48a5ea2c93ec /sc
parenteaa73b86648e2c6017ef66868c6ce2fe5302390d (diff)
tdf#142264: make sure to load potentially unloaded objects when saving
Commit 574eec9036c5f185b3572ba1e0ca9d111eb361dc happened to reveal a pre-existing problem that XLSX export only saved those OLE objects that were kept loaded in the OLE object cache, subject to thevalue of org.openoffice.Office.Common/Cache/DrawingEngine/OLE_Objects. Before that change, the imported charts were marked modified on load, and that prevented them from unloading in OLEObjCache::UnloadCheckHdl, because SdrOle2Obj::CanUnloadRunningObj returned false. After the mentioned change, the charts started to load without the wrong "modified" state, which allowed them to be properly managed by the cache, and the export filter implementation error surfaced. It's likely that commit 692878e3bb83c0fc104c5cca946c25ccf2d84ab2 tried to workaround the same underlying problem for charts that for some reason / at some point in time didn't get marked modified on load, and that commit converted an error shown in Excel into silently missing charts. This change makes sure that whenever a reference to chart document is requested from XclExpChartObj, it is actually loaded and ready for reading data. Possibly something could be done on the level of old reference that becomes non-functional (although valid) as the result of unloading, so that it would automatically reload on following use. That would make operating on the references robust. I didn't find an obvious way to do that. It is interesting to investigate, it the heizenbug related to images disappearing from documents, as users keep reporting without robust reproducers, might possibly be caused by a similar problem. Change-Id: I45fcdc98254157d805c7519340b5265526f27166 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/120688 Tested-by: Jenkins Reviewed-by: Mike Kaganski <mike.kaganski@collabora.com> Reviewed-on: https://gerrit.libreoffice.org/c/core/+/120695 Reviewed-by: Michael Stahl <michael.stahl@allotropia.de> (cherry picked from commit b8b3abc864e81c6ea76043a65ce234ada8651341) Reviewed-on: https://gerrit.libreoffice.org/c/core/+/120712 Tested-by: Jenkins CollaboraOffice <jenkinscollaboraoffice@gmail.com>
Diffstat (limited to 'sc')
-rw-r--r--sc/qa/unit/data/ods/many_charts.odsbin0 -> 192672 bytes
-rw-r--r--sc/qa/unit/subsequent_export-test.cxx89
-rw-r--r--sc/source/filter/excel/xeescher.cxx16
-rw-r--r--sc/source/filter/inc/xeescher.hxx3
4 files changed, 99 insertions, 9 deletions
diff --git a/sc/qa/unit/data/ods/many_charts.ods b/sc/qa/unit/data/ods/many_charts.ods
new file mode 100644
index 000000000000..31acdf66e1ed
--- /dev/null
+++ b/sc/qa/unit/data/ods/many_charts.ods
Binary files differ
diff --git a/sc/qa/unit/subsequent_export-test.cxx b/sc/qa/unit/subsequent_export-test.cxx
index 8b18ed93033f..c5f64eadafa3 100644
--- a/sc/qa/unit/subsequent_export-test.cxx
+++ b/sc/qa/unit/subsequent_export-test.cxx
@@ -69,6 +69,12 @@
#include <svl/zformat.hxx>
#include <test/xmltesttools.hxx>
+#include <com/sun/star/chart2/XChartDocument.hpp>
+#include <com/sun/star/chart2/XChartTypeContainer.hpp>
+#include <com/sun/star/chart2/XCoordinateSystemContainer.hpp>
+#include <com/sun/star/drawing/XDrawPage.hpp>
+#include <com/sun/star/drawing/XDrawPages.hpp>
+#include <com/sun/star/drawing/XDrawPagesSupplier.hpp>
#include <com/sun/star/drawing/XDrawPageSupplier.hpp>
#include <com/sun/star/awt/XBitmap.hpp>
#include <com/sun/star/frame/Desktop.hpp>
@@ -285,6 +291,7 @@ public:
void testButtonFormControlXlsxExport();
void testInvalidNamedRange();
void testTdf140431();
+ void testTdf142264ManyChartsToXLSX();
CPPUNIT_TEST_SUITE(ScExportTest);
@@ -469,6 +476,7 @@ public:
CPPUNIT_TEST(testButtonFormControlXlsxExport);
CPPUNIT_TEST(testInvalidNamedRange);
CPPUNIT_TEST(testTdf140431);
+ CPPUNIT_TEST(testTdf142264ManyChartsToXLSX);
CPPUNIT_TEST_SUITE_END();
@@ -5939,6 +5947,87 @@ void ScExportTest::testTdf140431()
xDocSh->DoClose();
}
+void ScExportTest::testTdf142264ManyChartsToXLSX()
+{
+ // The cache size for the test should be small enough, to make sure that some charts get
+ // unloaded in the process, and then loaded on demand properly (default is currently 20)
+ CPPUNIT_ASSERT_LESS(sal_Int32(40),
+ officecfg::Office::Common::Cache::DrawingEngine::OLE_Objects::get());
+
+ ScDocShellRef xDocSh = loadDoc(u"many_charts.", FORMAT_ODS);
+ CPPUNIT_ASSERT(xDocSh.is());
+ xDocSh = saveAndReload(xDocSh.get(), FORMAT_XLSX);
+ CPPUNIT_ASSERT(xDocSh.is());
+
+ auto xModel = xDocSh->GetModel();
+ css::uno::Reference<css::drawing::XDrawPagesSupplier> xSupplier(xModel,
+ css::uno::UNO_QUERY_THROW);
+ auto xDrawPages = xSupplier->getDrawPages();
+
+ // No charts (or other objects) on the first sheet, and resp. first draw page
+ css::uno::Reference<css::drawing::XDrawPage> xPage(xDrawPages->getByIndex(0),
+ css::uno::UNO_QUERY_THROW);
+ CPPUNIT_ASSERT_EQUAL(sal_Int32(0), xPage->getCount());
+
+ // 20 charts on the second sheet, and resp. second draw page
+ xPage.set(xDrawPages->getByIndex(1), css::uno::UNO_QUERY_THROW);
+ // Without the fix in place, this test would have failed with
+ // - Expected: 20
+ // - Actual : 0
+ // Because only the last 20 charts would get exported, all on the third sheet
+ CPPUNIT_ASSERT_EQUAL(sal_Int32(20), xPage->getCount());
+ for (sal_Int32 i = 0; i < xPage->getCount(); ++i)
+ {
+ css::uno::Reference<css::beans::XPropertySet> xProps(xPage->getByIndex(i),
+ css::uno::UNO_QUERY_THROW);
+ css::uno::Reference<css::chart2::XChartDocument> xChart(xProps->getPropertyValue("Model"),
+ css::uno::UNO_QUERY_THROW);
+ const auto xDiagram = xChart->getFirstDiagram();
+ CPPUNIT_ASSERT(xDiagram);
+
+ css::uno::Reference<css::chart2::XCoordinateSystemContainer> xCooSysContainer(
+ xDiagram, uno::UNO_QUERY_THROW);
+
+ const auto xCooSysSeq = xCooSysContainer->getCoordinateSystems();
+ for (const auto& rCooSys : xCooSysSeq)
+ {
+ css::uno::Reference<css::chart2::XChartTypeContainer> xChartTypeCont(
+ rCooSys, uno::UNO_QUERY_THROW);
+ uno::Sequence<uno::Reference<chart2::XChartType>> xChartTypeSeq
+ = xChartTypeCont->getChartTypes();
+ CPPUNIT_ASSERT(xChartTypeSeq.hasElements());
+ }
+ }
+
+ // 20 charts on the third sheet, and resp. third draw page
+ xPage.set(xDrawPages->getByIndex(2), css::uno::UNO_QUERY_THROW);
+ CPPUNIT_ASSERT_EQUAL(sal_Int32(20), xPage->getCount());
+ for (sal_Int32 i = 0; i < xPage->getCount(); ++i)
+ {
+ css::uno::Reference<css::beans::XPropertySet> xProps(xPage->getByIndex(i),
+ css::uno::UNO_QUERY_THROW);
+ css::uno::Reference<css::chart2::XChartDocument> xChart(xProps->getPropertyValue("Model"),
+ css::uno::UNO_QUERY_THROW);
+ const auto xDiagram = xChart->getFirstDiagram();
+ CPPUNIT_ASSERT(xDiagram);
+
+ css::uno::Reference<css::chart2::XCoordinateSystemContainer> xCooSysContainer(
+ xDiagram, uno::UNO_QUERY_THROW);
+
+ const auto xCooSysSeq = xCooSysContainer->getCoordinateSystems();
+ for (const auto& rCooSys : xCooSysSeq)
+ {
+ css::uno::Reference<css::chart2::XChartTypeContainer> xChartTypeCont(
+ rCooSys, uno::UNO_QUERY_THROW);
+ uno::Sequence<uno::Reference<chart2::XChartType>> xChartTypeSeq
+ = xChartTypeCont->getChartTypes();
+ CPPUNIT_ASSERT(xChartTypeSeq.hasElements());
+ }
+ }
+
+ xDocSh->DoClose();
+}
+
CPPUNIT_TEST_SUITE_REGISTRATION(ScExportTest);
CPPUNIT_PLUGIN_IMPLEMENT();
diff --git a/sc/source/filter/excel/xeescher.cxx b/sc/source/filter/excel/xeescher.cxx
index c053e2b643ed..8a9977ee443b 100644
--- a/sc/source/filter/excel/xeescher.cxx
+++ b/sc/source/filter/excel/xeescher.cxx
@@ -1561,13 +1561,10 @@ XclExpChartObj::XclExpChartObj( XclExpObjectManager& rObjMgr, Reference< XShape
// create the chart substream object
ScfPropertySet aShapeProp( xShape );
- Reference< XModel > xModel;
- aShapeProp.GetProperty( xModel, "Model" );
- mxChartDoc.set( xModel,UNO_QUERY );
css::awt::Rectangle aBoundRect;
aShapeProp.GetProperty( aBoundRect, "BoundRect" );
tools::Rectangle aChartRect( Point( aBoundRect.X, aBoundRect.Y ), Size( aBoundRect.Width, aBoundRect.Height ) );
- mxChart = std::make_shared<XclExpChart>( GetRoot(), xModel, aChartRect );
+ mxChart = std::make_shared<XclExpChart>(GetRoot(), GetChartDoc(), aChartRect);
}
XclExpChartObj::~XclExpChartObj()
@@ -1593,7 +1590,7 @@ void XclExpChartObj::SaveXml( XclExpXmlStream& rStrm )
if (xPropSet.is())
{
XclObjAny::WriteFromTo( rStrm, mxShape, GetTab() );
- ChartExport aChartExport(XML_xdr, pDrawing, mxChartDoc, &rStrm, drawingml::DOCUMENT_XLSX);
+ ChartExport aChartExport(XML_xdr, pDrawing, GetChartDoc(), &rStrm, drawingml::DOCUMENT_XLSX);
auto pURLTransformer = std::make_shared<ScURLTransformer>(*mpDoc);
aChartExport.SetURLTranslator(pURLTransformer);
static sal_Int32 nChartCount = 0;
@@ -1610,9 +1607,14 @@ void XclExpChartObj::SaveXml( XclExpXmlStream& rStrm )
pDrawing->endElement( FSNS( XML_xdr, XML_twoCellAnchor ) );
}
-const css::uno::Reference<css::chart::XChartDocument>& XclExpChartObj::GetChartDoc() const
+css::uno::Reference<css::chart::XChartDocument> XclExpChartObj::GetChartDoc() const
{
- return mxChartDoc;
+ SdrObject* pObj = SdrObject::getSdrObjectFromXShape(mxShape);
+ if (!pObj || pObj->GetObjIdentifier() != OBJ_OLE2)
+ return {};
+ // May load here - makes sure that we are working with actually loaded OLE object
+ return css::uno::Reference<css::chart::XChartDocument>(
+ static_cast<SdrOle2Obj*>(pObj)->getXModel(), css::uno::UNO_QUERY);
}
XclExpNote::XclExpNote(const XclExpRoot& rRoot, const ScAddress& rScPos,
diff --git a/sc/source/filter/inc/xeescher.hxx b/sc/source/filter/inc/xeescher.hxx
index da0c929a9714..3c57da157c57 100644
--- a/sc/source/filter/inc/xeescher.hxx
+++ b/sc/source/filter/inc/xeescher.hxx
@@ -322,13 +322,12 @@ public:
virtual void Save( XclExpStream& rStrm ) override;
virtual void SaveXml( XclExpXmlStream& rStrm ) override;
- const css::uno::Reference<css::chart::XChartDocument>& GetChartDoc() const;
+ css::uno::Reference<css::chart::XChartDocument> GetChartDoc() const;
private:
typedef std::shared_ptr< XclExpChart > XclExpChartRef;
XclExpChartRef mxChart; /// The chart itself (BOF/EOF substream data).
css::uno::Reference< css::drawing::XShape > mxShape;
- css::uno::Reference< css::chart::XChartDocument > mxChartDoc;
ScDocument* mpDoc;
};