Bug 88624 - Reportbuilder always stores absolute paths for graphics
Summary: Reportbuilder always stores absolute paths for graphics
Status: VERIFIED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Base (show other bugs)
Version:
(earliest affected)
4.3.5.2 release
Hardware: All All
: medium normal
Assignee: Not Assigned
URL:
Whiteboard: target:4.5.0 target:4.4.2 target:4.3.7
Keywords:
Depends on:
Blocks:
 
Reported: 2015-01-20 11:38 UTC by Gerhard Schaber
Modified: 2015-02-16 08:58 UTC (History)
6 users (show)

See Also:
Crash report or crash signature:


Attachments
tentative patch (821 bytes, patch)
2015-02-09 22:48 UTC, Lionel Elie Mamane
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Gerhard Schaber 2015-01-20 11:38:10 UTC
Problem description: 
When adding an image to a report, always the absolute path of the image is stored, independent of the setting Tools > Options > Load/Save > General > Save URLs relative to file system. This happens with 4.3.5.2. With 4.4, the report builder, and in turn LibreOffice as well, just freezes and never returns.

Steps to reproduce:
1. Insert graphics via file in a report
2. Store the report and the database file
3. Check the odb file with a text editor and search of the file name

Current behavior:
LibreOffice always stores the absolute path of the graphics and so the database file including the image files are not portable anymore.

Expected behavior:
Depending on the "Save URLs relative to file system", the relative path should be stored, quite like OpenOffice 4.1.1 does it with Oracle Report Builder 1.2.1. Well, one can deselect the report builder during the LibreOffice installation, and install the Oracle Report Builder as extension as workaround, but this is not maintained anymore.
Comment 1 Gerhard Schaber 2015-01-20 12:12:38 UTC
Correction, deselecting the report builder during installation is no workaround. If the internal report builder is not installed, the reports cannot be opened, no matter if the Oracle Report Builder 1.2.1 extension has been added manually or not.
Comment 2 Gerhard Schaber 2015-01-20 12:32:58 UTC
See also https://issues.apache.org/ooo/show_bug.cgi?id=96013.
Comment 3 raal 2015-01-20 12:36:25 UTC
I can confirm with LO 4.3.5, win7
Comment 4 Gerhard Schaber 2015-01-30 10:46:04 UTC
The LibreOffice 4.4 freeze is covered with this ticket: https://bugs.documentfoundation.org/show_bug.cgi?id=88824
Comment 5 Alex Thurgood 2015-01-30 13:54:41 UTC
(In reply to schaber from comment #0)
> Problem description: 


> Depending on the "Save URLs relative to file system", the relative path
> should be stored, quite like OpenOffice 4.1.1 does it with Oracle Report
> Builder 1.2.1. Well, one can deselect the report builder during the
> LibreOffice installation, and install the Oracle Report Builder as extension
> as workaround, but this is not maintained anymore.


Unfortunately, even this process isn't possible on Mac OSX without breaking the digital signature of the packet, and removing the extension from the app bundle by hand.
Comment 6 Gerhard Schaber 2015-02-03 10:50:33 UTC
Is there any workaround (other than using OpenOffice)?
Comment 7 Gerhard Schaber 2015-02-09 10:59:41 UTC
I know that time estimations are not always possible, but in which time frame do you think can this be resolved, or is there any way to use the Oracle Report builder extension instead? Or can anyone point me in the right direction in the source code?
Comment 8 Lionel Elie Mamane 2015-02-09 15:51:10 UTC
(In reply to schaber from comment #7)
> (...) can anyone point me in the right direction in the source code?

Sure. At some level, the problem seems to be that in file reportdesign/source/core/api/ReportDefinition.cxx in function OReportDefinition::storeToStorage around line 1394, the "aDescriptor.getUnpackedValueOrDefault(utl::MediaDescriptor::PROP_DOCUMENTBASEURL(),OUString())"
returns empty string, because aDescriptor contains empty string for "DocumentBaseURL.

If I change that to the full URL of the current .odb, then the absolute filepath is still SHOWN, but the relative path is STORED. So this seems to be what we need to concentrate on.

This itself comes from argument _aMediaDescriptor (line 1359)
which comes from file embeddedobj/source/commonembedding/persistence.cxx function OCommonEmbeddedObject::StoreDocToStorage_Impl line 787:

xDoc->storeToStorage( xStorage, aArgs );

aArgs is constructed just above:

780             aArgs[2].Name = "DocumentBaseURL";
781             aArgs[2].Value <<= aBaseURL;

which comes from a bit above:

765         OUString aBaseURL = GetBaseURLFrom_Impl(rMediaArgs, rObjArgs);

rMediaArgs and rObjArgs are empty sequences and are arguments. So we go up on the call stack:

OCommonEmbeddedObject::storeOwn in embeddedobj/source/commonembedding/persistence.cxx line 1631

1631            StoreDocToStorage_Impl( m_xObjectStorage, aEmpty, aEmpty, nStorageFormat, m_aEntryName, true );


So that's where we need to replace one, or both, of the "aEmpty" by something that makes sense...
Comment 9 Lionel Elie Mamane 2015-02-09 15:58:01 UTC
Indeed, looking at the output of

git log --grep=96013 -p

to see what change fixed this in OO.org back in 2009, I see in particular:

--- a/reportdesign/source/core/api/ReportDefinition.cxx
+++ b/reportdesign/source/core/api/ReportDefinition.cxx
@@ -1396,14 +1395,25 @@ void SAL_CALL OReportDefinition::storeToStorage( const uno::Reference< embed::XS
     /** property map for export info set */
     comphelper::PropertyMapEntry aExportInfoMap[] =
     {
-        { MAP_LEN( "UsePrettyPrinting" ), 0, &::getCppuType((sal_Bool*)0), beans::PropertyAttribute::MAYBEVOID, 0},
-        { MAP_LEN( "StreamName"), 0,&::getCppuType( (::rtl::OUString *)0 ),beans::PropertyAttribute::MAYBEVOID, 0 },
+        { MAP_LEN( "UsePrettyPrinting" ), 0, &::getCppuType((sal_Bool*)0),          beans::PropertyAttribute::MAYBEVOID, 0 }
+        { MAP_LEN( "StreamName")        , 0,&::getCppuType( (::rtl::OUString *)0 ), beans::PropertyAttribute::MAYBEVOID, 0 }
+        { MAP_LEN( "StreamRelPath")     , 0,&::getCppuType( (::rtl::OUString *)0 ), beans::PropertyAttribute::MAYBEVOID, 0 }
+        { MAP_LEN( "BaseURI")           , 0,&::getCppuType( (::rtl::OUString *)0 ), beans::PropertyAttribute::MAYBEVOID, 0 }
         { NULL, 0, 0, NULL, 0, 0 }
     };
     uno::Reference< beans::XPropertySet > xInfoSet( comphelper::GenericPropertySet_CreateInstance( new comphelper::PropertyS
 
     SvtSaveOptions aSaveOpt;
     xInfoSet->setPropertyValue(rtl::OUString(RTL_CONSTASCII_USTRINGPARAM("UsePrettyPrinting")), uno::makeAny(aSaveOpt.IsPret
+    if ( aSaveOpt.IsSaveRelFSys() )
+    {
+        const ::rtl::OUString sVal( aDescriptor.getUnpackedValueOrDefault(aDescriptor.PROP_DOCUMENTBASEURL(),::rtl::OUString
+        xInfoSet->setPropertyValue(rtl::OUString(RTL_CONSTASCII_USTRINGPARAM("BaseURI")), uno::makeAny(sVal));
+    } // if ( aSaveOpt.IsSaveRelFSys() )
+    const ::rtl::OUString sHierarchicalDocumentName( aDescriptor.getUnpackedValueOrDefault(rtl::OUString(RTL_CONSTASCII_USTR
+    xInfoSet->setPropertyValue(rtl::OUString(RTL_CONSTASCII_USTRINGPARAM("StreamRelPath")), uno::makeAny(sHierarchicalDocume
+
+



Which is exactly the code I started from in my previous comment. So this means (probably) that back in the time, aDescriptor (and thus _aMediaDescriptor) did contain a sensible value (the full path to the .odb file) for "Document Base URL". Need to find why/how this disappeared.
Comment 10 Gerhard Schaber 2015-02-09 21:40:30 UTC
Apparently changed to empty with changeset
2014-06-19 fdo#71076, fdo#71767: Preserve number formats when charts are copied.
Change-Id: If5ae8852152012483237e7602e56a0c46ea8748a
Comment 11 Gerhard Schaber 2015-02-09 21:54:38 UTC
Hi Kohei,

any idea why
  StoreDocToStorage_Impl( m_xObjectStorage, nStorageFormat, GetBaseURL_Impl(), 
was changed to
  StoreDocToStorage_Impl( m_xObjectStorage, aEmpty, aEmpty, nStorageFormat, m_aEntryName, true );

http://cgit.freedesktop.org/libreoffice/core/tree/embeddedobj/source/commonembedding/persistence.cxx?id=1d38cb365543924f9c50014e6b2227e77de1d0c9
@@ -1599,7 +1628,8 @@ void SAL_CALL OCommonEmbeddedObject::storeOwn()
Comment 12 Lionel Elie Mamane 2015-02-09 22:48:22 UTC
Created attachment 113268 [details]
tentative patch

This patch seems to work... Kohei? Any comment? Is this the right approach, or do you think it would be better to add back "aBaseURL" to the signature (argument list) of StoreDocToStorage_Impl, so that each caller can decide like they did before?
Comment 13 Gerhard Schaber 2015-02-09 22:54:09 UTC
Should m_aEntryName also be set as in the original implementation?

I am asking myself, whether the previous change from GetBaseURL_Impl(), m_aEntryName to aEmpty, aEmpty has an effect on other parts of the software.
Comment 14 Lionel Elie Mamane 2015-02-09 23:02:40 UTC
(In reply to schaber from comment #13)
> Should m_aEntryName also be set as in the original implementation?

m_aEntryName is still passed exactly as before.
Comment 15 Gerhard Schaber 2015-02-09 23:07:14 UTC
Right. I was confused by the other aEmpty.
Comment 16 Kohei Yoshida 2015-02-14 14:12:35 UTC
Lionel, your patch looks reasonable to me.  If someone can verify that that change doesn't break tdf#71767 and tdf#71076, then it should be okay.
Comment 17 Kohei Yoshida 2015-02-14 14:14:45 UTC
My memory is a bit hazy, but I believe the MediaArg is for the parent document, whereas the ObjectArg is for the embedded document.
Comment 18 Lionel Elie Mamane 2015-02-14 18:54:35 UTC
(In reply to Kohei Yoshida (inactive) from comment #16)
> Lionel, your patch looks reasonable to me.  If someone can verify that that
> change doesn't break tdf#71767 and tdf#71076, then it should be okay.

If does not break 71767, but I'm not sure I understood the reproduction instructions of bug 71076; it says to paste-as-GDI the *data* but then to look at the chart for the bug, this does not make sense to me.

I'm pushing my patch.
Comment 19 Commit Notification 2015-02-14 18:55:29 UTC
Lionel Elie Mamane committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=5b81128f77426b7105530a99b1eba2c580a8fa37

tdf#88624 set DocumentBaseURL when saving report

It will be available in 4.5.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds
Affected users are encouraged to test the fix and report feedback.
Comment 20 Gerhard Schaber 2015-02-16 08:11:05 UTC
Many thanks. I can confirm that the current issue is resolved with 4.5.
Comment 22 Gerhard Schaber 2015-02-16 08:58:38 UTC
About the fix for 71767 (Copy-Paste problem from Calc to Impress when there are dates in charts) that caused this issue. I can reproduce 71767 with 4.2.5.1, but neither with 4.2.8, nor with 4.5.0 from 2015-02-15 00:50:33 (Win-x86@62-TDF). So, the original fix still seems to work.