Bug 84394 - FILESAVE: picture in imported XLS file is not saved correctly
Summary: FILESAVE: picture in imported XLS file is not saved correctly
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Calc (show other bugs)
Version:
(earliest affected)
4.4.0.0.alpha0+ Master
Hardware: Other All
: medium normal
Assignee: Not Assigned
URL:
Whiteboard: target:5.2.0
Keywords: regression
Depends on:
Blocks:
 
Reported: 2014-09-27 14:30 UTC by Giuseppe Bilotta
Modified: 2016-10-25 19:02 UTC (History)
4 users (show)

See Also:
Crash report or crash signature:


Attachments
XLS file containing a picture that is not saved correctly on export to ODS (43.00 KB, application/vnd.ms-excel)
2014-09-27 14:30 UTC, Giuseppe Bilotta
Details
Change anchorage to other cell than A1 (50.00 KB, application/vnd.ms-excel)
2014-09-27 16:23 UTC, Jacques Guilleron
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Giuseppe Bilotta 2014-09-27 14:30:22 UTC
Created attachment 106960 [details]
XLS file containing a picture that is not saved correctly on export to ODS

Open the attached .xls file in Calc. Save it in ODS format. Exit Calc, then open the .ods file.

Expected result: the single picture in the first sheet of the XLS file is still there.

Actual result: the picture disappears (apparently reduced to a very small square in the upper left corner)

Bug found in:

LibreOffice 4.3.1.2 430m0(Build:2) (packaged in Debian unstable)

LibreOfficeDev 4.4.0.0.alpha0 92f92db220fe9393b9dd10914b804767649f9b99 (compiled from sources)
Comment 1 Jacques Guilleron 2014-09-27 16:23:28 UTC
Created attachment 106963 [details]
Change anchorage to other cell than A1
Comment 2 Jacques Guilleron 2014-09-27 16:26:20 UTC
Hi Giuseppe,

I just change the anchorage to be in another place than into A1 and this this seems to work. Once saCan you confirm?
Comment 3 Jacques Guilleron 2014-09-27 16:35:15 UTC
Sorry. My e-amil is gone away a little quickly.

Once saved to ods format and reopened, the group is visible like before. Do you confirm?

Regards,

Jacques
Comment 4 Giuseppe Bilotta 2014-09-29 16:10:19 UTC
Hello Jacques, I can indeed confirm that the issue doesn't present itself when the image is anchored on a different cell.

Even more interesting, if I open the ODS file produced when the anchorage is not in A1, and then move it to A1, save, close Calc, reopen the file, the anchorage moves to B2. Looks like some off-by-one error?
Comment 5 tommy27 2016-04-16 07:23:28 UTC Comment hidden (obsolete)
Comment 6 Giuseppe Bilotta 2016-04-20 14:07:49 UTC
Hello,

the bug is still present in libreoffice 5.1.2 (as backaged in debian unstable). It is NOT preset in 3.3 or in 3.6, so it's a regression. As soon as I have some more free time I'll check again with the current master branch, as well as with older versions trying to bisect the breaking point.
Comment 7 Giuseppe Bilotta 2016-04-20 21:10:30 UTC
After a rather boring bisecting session, I've verified that the bug has been introduced between 4.0.0.3 and 4.0.1.2: with the 4.0.0.3 build, the XLS gets converted to ODS correctly, with 4.0.1.2 the picture disappears.
Comment 8 Giuseppe Bilotta 2016-04-20 21:39:01 UTC
A very cursory look at the commits between 4.0.0 and 4.0.1 hints that 795796da79d5dd815fb2d31e8d4a244bf8d54328 might be the culprit, although I don't have time to verify this now.
Comment 9 Giuseppe Bilotta 2016-04-21 09:26:46 UTC
That commit seems to be the issue indeed. Or more specifically, the call to SetCellAnchoredFromPosition if bPageAnchor is false.

In the specific case, the flags read from the xls have a value of 0x02 (not sure about the meaning), so bPageAnchor is set to fallse, and the call to SetCellAnchoredFromPosition gives completely bogus results:

setting cell anchor from position
anchor: maStart (0, 0, 0)
	maEnd (0, 0, 0)
	maStartOffset (0, 0)
	maEndOffset (-32767, -32767)
	meType 3
	maLastRect (0, 0, -32767, -32767)
anchor: maStart (0, 0, 0)
	maEnd (0, 0, 0)
	maStartOffset (0, 0)
	maEndOffset (-32767, -32767)
	meType 3
	maLastRect (0, 0, -32767, -32767)

hence the object disappearance. Further looking into this, it seems that the logic and snap rectangles for the object have values (0, 0, -32767, -32767), which I assume indicate the rectangle is invalid.

Since the object actually displays correctly when the XLS is loaded in Calc, I assume that what happens is that the anchoring is being done too early, before the object rectangle is correctly determined. I've tried moving the SetCellAnchoredFromPosition call to after PreProcessSdrObject within XclImpDffConverted::ProcessObj, but this doesn't fix it. I'm afraid I don't know enough about the internals of LO to see where the call should be moved to ensure its correctness though.
Comment 10 Giuseppe Bilotta 2016-04-21 11:28:09 UTC
Ok, after some more investigation, the problem is that the picture in question is a group, so its bounding rectangle cannot be computed until all the nested objects have been loaded, which only happens _after_ the group is processed. This is what causes the issues.
Comment 11 raal 2016-05-02 16:29:52 UTC
(In reply to Giuseppe Bilotta from comment #8)
> A very cursory look at the commits between 4.0.0 and 4.0.1 hints that
> 795796da79d5dd815fb2d31e8d4a244bf8d54328 might be the culprit, although I
> don't have time to verify this now.

Noel, could you look? Thank you

author	Noel Power <noel.power@suse.com>	2013-01-24 18:08:41 (GMT)
committer	Eike Rathke <erack@redhat.com>	2013-02-07 13:13:13 (GMT)
commit	795796da79d5dd815fb2d31e8d4a244bf8d54328 (patch)
tree	62b9fa63ece01a8255b61a039ce7c911bf328e31
parent	a5acd33bc54cbc00f2194c923641fbd0ab45d801 (diff)
export/import anchoring for xls(x) drawing ( & ole ) objects fdo#58360
Comment 12 Giuseppe Bilotta 2016-05-02 20:24:23 UTC
For what it's worth, I proposed a partial fix in https://gerrit.libreoffice.org/#/c/24292/ 

The fix is only partial because it fixes the XLS-specific issue of importing nested groups, but not the anchoring issue (which seems to be more general, and affects other formats as well).
Comment 13 Commit Notification 2016-05-07 22:36:33 UTC
Giuseppe Bilotta committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=1ca277fad69d8a53788a5d457a2dd99bd50b6910

tdf#84394: xls load: delay cell-anchoring of object groups

It will be available in 5.2.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 14 Commit Notification 2016-05-07 22:36:37 UTC
Giuseppe Bilotta committed a patch related to this issue.
It has been pushed to "master":

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

tdf#84394: add testcase and unit test

It will be available in 5.2.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 15 pietro.pangallo 2016-06-16 09:58:26 UTC
Verified fixed in:
Version: 5.3.0.0.alpha0+
Build ID: a8bd44573b75d1399257d6f5d052611439607189
CPU Threads: 2; OS Version: Linux 4.1; UI Render: default; 
TinderBox: Linux-rpm_deb-x86_64@70-TDF, Branch:master, Time: 2016-06-13_23:46:49
Locale: it-IT (it_IT.UTF-8)
OS: openSUSE Leap 42.1 (x86_64)

Status NEW -> RESOLVED FIXED