Bug Hunting Session
Bug 47434 - FILEOPEN pptx arrows are shifted to the left
Summary: FILEOPEN pptx arrows are shifted to the left
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Impress (show other bugs)
Version:
(earliest affected)
3.5.0 RC2
Hardware: All All
: medium normal
Assignee: Muthu
URL:
Whiteboard: bibisected35 target:3.7.0 target:3.5....
Keywords: regression
Depends on:
Blocks:
 
Reported: 2012-03-17 04:35 UTC by Korrawit Pruegsanusak
Modified: 2013-11-15 22:46 UTC (History)
2 users (show)

See Also:
Crash report or crash signature:


Attachments
two arrows in the left are incorrectly shifted to the left (39.62 KB, application/vnd.openxmlformats-officedocument.presentationml.presentation)
2012-03-17 04:35 UTC, Korrawit Pruegsanusak
Details
screenshot in powerpoint 2007 (10.92 KB, image/png)
2012-03-17 04:36 UTC, Korrawit Pruegsanusak
Details
screenshot in libo3.4.4, expected (16.60 KB, image/png)
2012-03-17 04:37 UTC, Korrawit Pruegsanusak
Details
screenshot in libo3.5.0 rc2 - regression (19.87 KB, image/png)
2012-03-17 04:38 UTC, Korrawit Pruegsanusak
Details
arrow name (only number shown) in ooxml structure (10.80 KB, image/png)
2012-05-12 22:33 UTC, Korrawit Pruegsanusak
Details
new case - manually edited ooxml (45.13 KB, application/zip)
2012-05-13 00:55 UTC, Korrawit Pruegsanusak
Details
Patch (595 bytes, patch)
2012-08-01 11:08 UTC, Muthu
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Korrawit Pruegsanusak 2012-03-17 04:35:40 UTC
Created attachment 58591 [details]
two arrows in the left are incorrectly shifted to the left

The attached file is created with PowerPoint. Some arrows are incorrectly shifted to the left, please see screenshots in next comments.

[REPRODUCIBLE] with 3.5.0 RC 2 / WinXP
[NOT REPRODUCIBLE] with 3.4.4 / WinXP
So regression.
Comment 1 Korrawit Pruegsanusak 2012-03-17 04:36:34 UTC
Created attachment 58592 [details]
screenshot in powerpoint 2007
Comment 2 Korrawit Pruegsanusak 2012-03-17 04:37:40 UTC
Created attachment 58593 [details]
screenshot in libo3.4.4, expected
Comment 3 Korrawit Pruegsanusak 2012-03-17 04:38:07 UTC
Created attachment 58594 [details]
screenshot in libo3.5.0 rc2 - regression
Comment 4 Korrawit Pruegsanusak 2012-03-17 04:45:15 UTC
bibisect log:

git bisect start
# bad: [4c30602f43475389f81b1d981ce8ee9a3410b9d9] source-hash-85c6244b85b29c1d2bb9d89b62e9512dd65378b5
git bisect bad 4c30602f43475389f81b1d981ce8ee9a3410b9d9
# good: [65fd30f5cb4cdd37995a33420ed8273c0a29bf00] source-hash-d6cde02dbce8c28c6af836e2dc1120f8a6ef9932
git bisect good 65fd30f5cb4cdd37995a33420ed8273c0a29bf00
# good: [2faf4bc12ab490370d2196dedbc8091f9b09d0a5] source-hash-418a35f4861e863feb39eec73f4a39a87fbcb1f3
git bisect good 2faf4bc12ab490370d2196dedbc8091f9b09d0a5
# good: [b6fca7e58854bc617c5fc9a75d1c1720b0d7e1a4] source-hash-ce60138d339a5eb2a174a5d27063249acf2cac42
git bisect good b6fca7e58854bc617c5fc9a75d1c1720b0d7e1a4
# good: [216e447cb0a457985f20d4db481fa9c73b0bb775] source-hash-55c5ea43a59e505297fb6fa20b77aaa28f7c67bc
git bisect good 216e447cb0a457985f20d4db481fa9c73b0bb775
# good: [569ab0259fc5fcc3190d9ae9c44e1e28570bc8bd] source-hash-817bf1d41bb07aeb3ed7649d25c2b44ee4acb1fe
git bisect good 569ab0259fc5fcc3190d9ae9c44e1e28570bc8bd
# good: [cfad378ec3c0de9f19525a2eef4cef9bc056e3f6] source-hash-10f977981d2cfb6ba0ccd0185ccb12e212010bc2
git bisect good cfad378ec3c0de9f19525a2eef4cef9bc056e3f6
# bad: [a4c1bd2fe1dccc0fa192cc5cf9ebd2967c6a9920] source-hash-558b5ea32a99654dcb63526f107726f7aec4747f
git bisect bad a4c1bd2fe1dccc0fa192cc5cf9ebd2967c6a9920
Comment 5 Korrawit Pruegsanusak 2012-03-17 04:47:12 UTC
And the commit range to look is

http://cgit.freedesktop.org/libreoffice/core/log/?qt=range&q=10f977981d..558b5ea32

IIUC :)
Comment 6 Korrawit Pruegsanusak 2012-03-25 07:20:56 UTC
There is only one commit in the range mentioned in comment 5 which touch oox/
http://cgit.freedesktop.org/libreoffice/core/commit/?id=30b052e7b65434d2a78ab36d8f4475abe0c86a2a

Muthu, could you please have a look? Thanks. Sorry if I am incorrect.
Comment 7 Muthu 2012-03-26 01:37:42 UTC
thank you.
seems like a problem...will look at it...
Comment 8 Korrawit Pruegsanusak 2012-04-23 10:48:52 UTC
Correctly assign to Muthu :)
Comment 9 Muthu 2012-04-24 08:21:00 UTC
I guess, a duplicate of bug 43752 ?
Comment 10 Korrawit Pruegsanusak 2012-04-24 21:48:07 UTC
(In reply to comment #9)
> I guess, a duplicate of bug 43752 ?

Unfortunately, no. Bug 43752 is solved in 3.5.3 RC1, but this bug is not.
Comment 11 Korrawit Pruegsanusak 2012-05-12 22:30:05 UTC
Looking into OOXML structure, ppt/slides/slide1.xml, I found that the main difference between "Straight Arrow Connector 5" (correct arrow displayed) and "Straight Arrow Connector 7" (incorrect, arrow shifted to the left) is:

"Straight Arrow Connector 5" has attribute flipH="1" in element <a:xfrm flipH="1">, but "Straight Arrow Connector 7" doesn't have this attribute.

Following is the result of diff -u (pretty-printed, of course)

 <p:cxnSp>
        <p:nvCxnSpPr>
-               <p:cNvPr id="6" name="Straight Arrow Connector 5"/>
+               <p:cNvPr id="8" name="Straight Arrow Connector 7"/>
                <p:cNvCxnSpPr/>
                <p:nvPr/>
        </p:nvCxnSpPr>
        <p:spPr>
-               <a:xfrm flipH="1">
-                       <a:off x="4800600" y="2362200"/>
-                       <a:ext cx="2209800" cy="0"/>
+               <a:xfrm>
+                       <a:off x="1572490" y="2438400"/>
+                       <a:ext cx="2743200" cy="0"/>
                </a:xfrm>
                <a:prstGeom prst="straightConnector1">
                        <a:avLst/>


So, I tried adding flipH="1" to the "Straight Arrow Connector 7", and it displayed correctly. By "correctly" I mean it position is correct, and it points to the left as expected. IOW, it is same as in PowerPoint.

Hope this helps :)
Comment 12 Korrawit Pruegsanusak 2012-05-12 22:33:46 UTC
Created attachment 61541 [details]
arrow name (only number shown) in ooxml structure
Comment 13 Korrawit Pruegsanusak 2012-05-13 00:55:25 UTC
Created attachment 61542 [details]
new case - manually edited ooxml

I've created new files in PowerPoint 2007. It consists of two arrows, one horizontally flipped, one not. With some investigation, I found that:

* The arrow *without* flip attribute in <a:xfrm> will *not* show correctly if "cy" attribute in <a:ext> less than 180, else it will show correctly.

* The arrow with flip attribute <a:xfrm flipH="1"> will show correctly regardless of value of "cy".


In other words:
* <a:xfrm flipH="1"> - correct
* <a:xfrm> with <a:ext cx="??" cy="180"/> or larger - correct
* <a:xfrm> with <a:ext cx="??" cy="179"/> or lower - incorrect

The attachment contains 2 manually edited ooxml files. I edited ppt/slides/slide1.xml to have cy=179 and cy=180 in both arrows in the same file.


Speaking of the number 180, I am thinking of the angle / rotation stuff ... but I may be wrong

Note: If I understand correctly, "cy" should be the width of the shape. <http://www.schemacentral.com/sc/ooxml/e-a_ext-2.html>
Comment 14 Korrawit Pruegsanusak 2012-06-17 03:28:14 UTC
With sample file in attachment 61542 [details], I debugged it a bit and found that in function
    EnhancedCustomShapeEngine::render()
at http://opengrok.libreoffice.org/xref/core/svx/source/customshapes/EnhancedCustomShapeEngine.cxx#246

I tested the value of mxShape->getSize() and result was as follows:

* In buggy file (cy=179)   : width = 5555, height = -37926
* In non-bug file (cy=180) : width = 5555, height = 1

I'm not sure if I got through a right way or not. If yes, could you please give me some hints how should I continue? Thanks!
Comment 15 Muthu 2012-06-17 21:50:32 UTC
its really nice to see you get into the code...
It would be interesting to know why height is negative and if that is what is causing the problem...
Comment 16 Korrawit Pruegsanusak 2012-06-20 03:09:12 UTC
Thanks Muthu! A short update on this:

Line 551 in http://opengrok.libreoffice.org/xref/core/oox/source/drawingml/shape.cxx#551

> PropertySet( xSet ).setProperties( aShapeProps );

Makes mxShape's Height to be negative value, comparing before this line and also with non-buggy file.
Comment 17 Korrawit Pruegsanusak 2012-06-20 04:56:06 UTC
*If I understand correctly*, from line 551
> PropertySet( xSet ).setProperties( aShapeProps );

we're setting aShapeProps into xSet, which is properties of mxShape from line 459:
> Reference< XPropertySet > xSet( mxShape, UNO_QUERY );

And this is why mxShape's Height be negative after line 551.


Next, line 538
> aLineProperties.pushToPropMap( aShapeProps, rGraphicHelper, nLinePhClr );

push aLineProperties into aShapeProps. So, we have to see how aLineProperties is constructed.

Line 492 is where aLineProperties declared.
> LineProperties aLineProperties;

I'm now at this point.

PS. If I understood anything incorrectly, please feel free to tell :) Thanks!
Comment 18 Korrawit Pruegsanusak 2012-06-20 05:36:52 UTC
I'm currently lost now, wondered how aLineProperties affects mxShape.
Could you please help? Thanks very much! :)
Comment 19 Muthu 2012-06-21 03:45:47 UTC
aLineProperties are created while reading the xml.
Somewhere like
http://opengrok.libreoffice.org/xref/core/oox/source/drawingml/shape.cxx#242
http://opengrok.libreoffice.org/search?q=mpLinePropertiesPtr&project=core
and modified while the shape's properties are read. 


This might help you as well:
http://opengrok.libreoffice.org/search?q=mbFlipH&project=core
(and mpFlipV)
Comment 20 Korrawit Pruegsanusak 2012-06-30 21:03:02 UTC
Thanks and sorry for the late reply. I've been busy last week, so I could not do the full debugging session. Anyway, I tried this:

If it's an arrow and height < 0, set height = 1. I inserted this code after the line 551:
> PropertySet( xSet ).setProperties( aShapeProps );

And, yes!, it rendered correctly. So, I think next step is to find what cause the height to be negative.
Comment 21 Muthu 2012-07-06 07:21:54 UTC
@Korrawit: Cool! I think you should be able to solve this now! Do assign it to yourself, if required.
Comment 22 Korrawit Pruegsanusak 2012-07-10 14:14:05 UTC
Just a sorry note: I still don't have time to do a full debugging session.
Comment 23 Korrawit Pruegsanusak 2012-07-29 08:21:41 UTC
Sorry for the delay. Now I'm looking at this: (line number might change because my source is old)

#0  Rectangle::Rectangle (this=0xbfff89a0, rLT=..., rSize=...) at /home/tux/libo/solver/unxlngi6/inc/tools/gen.hxx:487
#1  0xab6051db in SdrObjCustomShape::TRSetBaseGeometry (this=0x8bc9938, rMatrix=...) at /home/tux/libo/svx/source/svdraw/svdoashp.cxx:3029
#2  0xab78fab3 in SvxShape::setPropertyValueImpl (this=0x89643c0, pProperty=0x8b26a20, rValue=
    uno::Any {Line1 = {Column1 = 5556.2888888888892, Column2 = 0, Column3 = 5357.7611111111109}, Line2 = {Column1 = 0, Column2 = 0.49722222222222223, Column3 = 4365.5888888888894}, Line3 = {Column1 = 0, Column2 = 0, Column3 = 1}})
    at /home/tux/libo/svx/source/unodraw/unoshape.cxx:2227
#3  0xab797a44 in SvxShapeText::setPropertyValueImpl (this=0x89643c0, rName="Transformation", pProperty=0x8b26a20, rValue=
    uno::Any {Line1 = {Column1 = 5556.2888888888892, Column2 = 0, Column3 = 5357.7611111111109}, Line2 = {Column1 = 0, Column2 = 0.49722222222222223, Column3 = 4365.5888888888894}, Line3 = {Column1 = 0, Column2 = 0, Column3 = 1}})
    at /home/tux/libo/svx/source/unodraw/unoshape.cxx:4198
#4  0xab78deee in SvxShape::_setPropertyValue (this=0x89643c0, rPropertyName="Transformation", rVal=
    uno::Any {Line1 = {Column1 = 5556.2888888888892, Column2 = 0, Column3 = 5357.7611111111109}, Line2 = {Column1 = 0, Column2 = 0.49722222222222223, Column3 = 4365.5888888888894}, Line3 = {Column1 = 0, Column2 = 0, Column3 = 1}})
    at /home/tux/libo/svx/source/unodraw/unoshape.cxx:1710
#5  0xaa3b151f in SdXShape::setPropertyValue (this=0x8bc8f28, aPropertyName="Transformation", aValue=
    uno::Any {Line1 = {Column1 = 5556.2888888888892, Column2 = 0, Column3 = 5357.7611111111109}, Line2 = {Column1 = 0, Column2 = 0.49722222222222223, Column3 = 4365.5888888888894}, Line3 = {Column1 = 0, Column2 = 0, Column3 = 1}})
    at /home/tux/libo/sd/source/ui/unoidl/unoobj.cxx:701
#6  0xab78dce8 in SvxShape::setPropertyValue (this=0x89643c0, rPropertyName="Transformation", rVal=
    uno::Any {Line1 = {Column1 = 5556.2888888888892, Column2 = 0, Column3 = 5357.7611111111109}, Line2 = {Column1 = 0, Column2 = 0.49722222222222223, Column3 = 4365.5888888888894}, Line3 = {Column1 = 0, Column2 = 0, Column3 = 1}})
    at /home/tux/libo/svx/source/unodraw/unoshape.cxx:1680
#7  0xab779d05 in SvxCustomShape::setPropertyValue (this=0x89643c0, aPropertyName="Transformation", aValue=
    uno::Any {Line1 = {Column1 = 5556.2888888888892, Column2 = 0, Column3 = 5357.7611111111109}, Line2 = {Column1 = 0, Column2 = 0.49722222222222223, Column3 = 4365.5888888888894}, Line3 = {Column1 = 0, Column2 = 0, Column3 = 1}})
    at /home/tux/libo/svx/source/unodraw/unoshap2.cxx:1914
#8  0xab78e9ac in SvxShape::setPropertyValues (this=0x89643c0, aPropertyNames=uno::Sequence of length 17 = {...}, 
    aValues=uno::Sequence of length 17 = {...}) at /home/tux/libo/svx/source/unodraw/unoshape.cxx:1899
#9  0xac4068b0 in oox::PropertySet::setProperties (this=0xbfff98c0, rPropNames=uno::Sequence of length 17 = {...}, 
    rValues=uno::Sequence of length 17 = {...}) at /home/tux/libo/oox/source/helper/propertyset.cxx:96
#10 0xac406aab in oox::PropertySet::setProperties (this=0xbfff98c0, rPropertyMap=...)
    at /home/tux/libo/oox/source/helper/propertyset.cxx:125
#11 0xac317ec3 in oox::drawingml::Shape::createAndInsert (this=0x8bd3588, rFilterBase=..., 
    rServiceName="com.sun.star.drawing.CustomShape", pTheme=0x89a9340, rxShapes=

Note that the line:
> PropertySet( xSet ).setProperties( aShapeProps );
mentioned in comment 20 is at frame #11.

The problem is the number 0.49722222222222223 returned when calling aScale.getY().
And FRound(0.49722222222222223) returns 0, not 1. So,

> Size aSize(FRound(aScale.getX()), FRound(aScale.getY()));

gets aSize = {5556, 0}. Then, calling

> Rectangle aBaseRect(aPoint, aSize);

which in turn calls

> inline Rectangle::Rectangle( const Point& rLT, const Size& rSize )
> {
>     nLeft   = rLT.X();
>     nTop    = rLT.Y();
>     nRight  = rSize.Width()  ? nLeft+rSize.Width()-1 : RECT_EMPTY;
>     nBottom = rSize.Height() ? nTop+rSize.Height()-1 : RECT_EMPTY;
> }

get nBottom = RECT_EMPTY. And RECT_EMPTY = -32767.
Comment 24 Korrawit Pruegsanusak 2012-07-29 08:24:30 UTC
Comparing with the correct-rendering file, its aScale.getY() returns 0.5, which FRound(0.5) = 1.

I'm not sure if this is a floating-point problem or not, I'll look a bit more where 0.49722222222222223 come from.
Comment 25 Korrawit Pruegsanusak 2012-07-29 10:04:50 UTC
OK, 179/360 = 0.49722222222222223.

It comes from line 386 in oox/source/drawingml/shape.cxx :
> aTransformation.scale(1/360.0, 1/360.0);

Before this line, aTransformation.get(1,1) = 179 = maSize.Height = cy in ooxml (see comment 13 and 14)
and aTransformation.get(0,0) = 2000264 = maSize.Width = cx in ooxml

After this line, we divide them with 360, so we get 0.497222...

And this explains: when cy >= 180, it rendered correctly. This is because cy/360 >= 0.5, and FRound gives >= 1.
But when cy < 180, cy/360 < 0.5, FRound gives 0, which triggers comment 23.

Using git annotate, this line of code come from http://cgit.freedesktop.org/libreoffice/core/commit/?id=9fb67c85eca1fdbe56c9dd24c3358bac833fd6a0

@Muthu, Now the problem is: I have no idea why we have to divide 360. Could you please help? Thanks very much!
Comment 26 Muthu 2012-07-30 08:56:32 UTC
@Korrawit: Cool! That's nice progress!

I am confused with this 179 :( Since Radek seems to be familiar with this code, I am CC'ing him - he should be able to guide you.

@Radek: Thanks!
Comment 27 Muthu 2012-08-01 10:25:13 UTC
FYI: I have started looking at this in depth.
Comment 28 Muthu 2012-08-01 11:08:26 UTC
Created attachment 65028 [details]
Patch
Comment 29 Muthu 2012-08-01 11:12:21 UTC
@Korrawit: After a quick debugging, I felt checking and setting the width should be fine. This is because a valid rectangle is required for the transformation and zero isn't intended.
Thank you so much for all the hard work...

@Radek: A review here would help, please...Thanks!
Comment 30 Muthu 2012-08-14 10:47:26 UTC
Marking this fixed.
Comment 31 Not Assigned 2012-08-14 10:48:03 UTC
Muthu Subramanian committed a patch related to this issue.
It has been pushed to "master":

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

fdo#47434: Zero rect. size causing wrong line positions.
Comment 32 Not Assigned 2012-08-20 16:11:12 UTC
Muthu Subramanian committed a patch related to this issue.
It has been pushed to "libreoffice-3-5":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=6cbb9602e15794b4d39d2482a40c22d3fd0328f9&g=libreoffice-3-5

fdo#47434: Zero rect. size causing wrong line positions.


It will be available in LibreOffice 3.5.7.
Comment 33 Not Assigned 2012-08-20 16:11:33 UTC
Muthu Subramanian committed a patch related to this issue.
It has been pushed to "libreoffice-3-6":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=9b1f4aac63e0f1e5453485f8b4c36c9020bbea03&g=libreoffice-3-6

fdo#47434: Zero rect. size causing wrong line positions.


It will be available in LibreOffice 3.6.2.
Comment 34 Not Assigned 2012-08-21 10:31:09 UTC
Muthu Subramanian committed a patch related to this issue.
It has been pushed to "libreoffice-3-6-1":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=350d9f972e00a05ff61140eafc078188252cc2de&g=libreoffice-3-6-1

fdo#47434: Zero rect. size causing wrong line positions.


It will be available already in LibreOffice 3.6.1.
Comment 35 Not Assigned 2012-08-21 14:41:02 UTC
Korrawit Pruegsanusak committed a patch related to this issue.
It has been pushed to "master":

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

fdo#47434 testcase