Issue 81395 - Inconsistency in exporting ParaLineSpacing property in PPT
Summary: Inconsistency in exporting ParaLineSpacing property in PPT
Status: CLOSED FIXED
Alias: None
Product: Impress
Classification: Application
Component: save-export (show other issues)
Version: 680m222
Hardware: All Linux, all
: P3 Trivial (vote)
Target Milestone: OOo 2.4
Assignee: christian.guenther
QA Contact: issues@graphics
URL:
Keywords: ms_interoperability
Depends on:
Blocks:
 
Reported: 2007-09-08 18:09 UTC by hub
Modified: 2008-07-03 17:08 UTC (History)
1 user (show)

See Also:
Issue Type: PATCH
Latest Confirmation in: ---
Developer Difficulty: ---


Attachments
patch to fix the issue (1.09 KB, patch)
2007-09-08 18:10 UTC, hub
no flags Details | Diff
the sample document. (167.50 KB, application/vnd.ms-powerpoint)
2007-09-08 18:10 UTC, hub
no flags Details
new patch (2.95 KB, patch)
2007-10-10 05:52 UTC, hub
no flags Details | Diff
patch (replace all the others) (3.45 KB, patch)
2007-12-06 23:29 UTC, hub
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this issue.
Description hub 2007-09-08 18:09:40 UTC
There is an inconsistency in exporting ParaLineSpacing property in PPT binary
format cause issues with round-trip PPT conversion.

How to reproduce the problem:

1/ open the attached PPT presentation
2/ in the bulletted list, click on one of the lines and do Format -> Default
Formatting
You'll notice that the line spacing will be reduced
3/ save the presentation as PPT, and close it
4/ reopen the saved presentation
5/ repeat step 2/

If the presentation is saved in OpenDocument, the problem does not occur.

I tracked it down to this:
http://bonsai.go-oo.org/cvsblame.cgi?file=graphics/sd/source/filter/eppt/eppt.cxx&rev=1.58&root=/home/ooweb/cvsup

Around line 2418, the LineSpacing is modified. I don't understand why, because
on import there is nothing that is done to it. I suspect that the change a few
line below for revision 1.28 should have removed line 2422 to line 2424 added in
revision 1.27 of the file.

Attached to this bug is

1/ the document to reproduce
2/ the patch to fix the problem. 

Can some somebody (sj) with more expertise on the PPT binary format review the
change (and possibly commit it)?

Thanks a lot.
Comment 1 hub 2007-09-08 18:10:11 UTC
Created attachment 48067 [details]
patch to fix the issue
Comment 2 hub 2007-09-08 18:10:45 UTC
Created attachment 48068 [details]
the sample document.
Comment 3 hub 2007-09-08 18:11:06 UTC
the patch is to be applied to the 'sd' module.
Comment 4 hub 2007-09-25 17:17:28 UTC
Sven, since you wrote the initial code, I think you are the right person to
review this patch.

Thanks.
Comment 5 sven.jacobi 2007-09-26 10:49:57 UTC
sj->hub: Yes, you are right, we do have a round issue here, but your patch does
not solve this problem. If we would apply your patch, normally in SO created
documents would get a really different linespacing in PPT.

The reason for this is that StarOffice and MSOffice are using different
linespacings. In StarOffice the linespacing depends to the current font that is
used, test it, you will be able to place 44 lines with a single linespacing
having the fontsize 12 on a page when using the font "arial", but if you change
the font to "arial black" only 35 lines will fit the page. 

If you are doing the same in ppt you will have no problems, the linespacing is
equal, it doesn't matter which font is used.

For the import I fixed this by adding a new feature that allows to use fixed
linespacing in text objects (SdrTextFixedCellHeightItem).

The code you removed is necessary for the conversion of text objects that are
created by StarOffice (SdrTextFixedCellHeightItem==false). The master fix would
be to take care of the SdrTextFixedCellHeightItem even in the exporter.

I hope I could help so far.


Comment 6 hub 2007-10-10 05:52:48 UTC
Created attachment 48797 [details]
new patch
Comment 7 hub 2007-10-10 05:53:42 UTC
just attaching this patch likely do be incomplete as apparentyly the property
does not get propagated...
Comment 8 hub 2007-10-14 18:10:21 UTC
Sven,

Apparently this property is not set in the style sheets. so the above patch end
up being a no-op.

Any idea?

Thanks.
Comment 9 sven.jacobi 2007-10-16 14:22:02 UTC
sj->hub: sorry, I dont't have an Idea. I also will have to debug the problem to
be able to give you a furher advice.
Comment 10 hub 2007-10-31 19:27:51 UTC
All the problem lies with the fact that SdrTextFixedCellHeightItem is a property
for Shape. And the problem is located in the stylesheet export.

I don't see how that can be fixed. I tried to pass the current XShape down to
the stylesheet, but actually at that point there is no Shape created. Any idea?
Comment 11 sven.jacobi 2007-11-01 10:27:30 UTC
I also don|t have an idea, I would take a look into our odf export filter in
xmloff, there the fixedcellheight property is also written.
Comment 12 hub 2007-11-09 19:38:21 UTC
So I have looked again, starting over completely from scratch.

Here is what I found.

During Import:

In svx/source/svdraw/scfppt.cxx:
PPTParagraphObj::ApplyTo(), around line 6313, it fetch the read line spacing
calling GetAttrib( PPT_ParaAttr_LineFeed, ..).
http://lxr.go-oo.org/source/graphics/svx/source/svdraw/svdfppt.cxx#6313

Line 6336 it gets the Atom, but then inside the if, it is a no-op.
http://lxr.go-oo.org/source/graphics/svx/source/svdraw/svdfppt.cxx#6336

The property is not changed on import.

Now to the export.
In sd/source/filter/eppt/eppt.cxx.
In PPTExParaSheet::SetStyleSheet(), around line 2421, the nLineSpacing, which is
the property mentioned above is being modified 3 lines below.
http://lxr.go-oo.org/source/graphics/sd/source/filter/eppt/eppt.cxx#2424

What is imported does not get exported the same.

If the patch above is wrong, then it is in the importer where we should
recalculate the property in svdfppt.cxx:6336
http://lxr.go-oo.org/source/graphics/svx/source/svdraw/svdfppt.cxx#6336
And I have no clue what to do :-/
Comment 13 sven.jacobi 2007-11-13 11:17:04 UTC
In r1.110 of svdfppt.cxx the noop was:
nPropLineSpace = (sal_uInt8)((double)nVal2 * pAtom->fScaling + 0.5);
which is completely right. After introducing the SdrTextFixedCellHeightItem in
r1.111 fScaling has been done by the editengine, so it was also correct to
remove the fScaling in the import. (btw it would be good to remove the noop.)

To repeat my opinion, each text that is imported from a PPT document has to be
interpreted as if there is SdrTextFixedCellHeightItem set to true, but this is
not the fact when exporting text to PPT then the SdrTextFixedCellHeightItem
might be true and nothing special has to be done(if the text was previously been
imported from PPT) or it might be false if the text was newly been created in
OOo, then the line spacing must take care of the fluctuating font height -> the
export has to be fixed


Comment 14 hub 2007-12-06 23:28:38 UTC
So the bug is actually in both the import and the export, and in sd core.

On import, add a SdrTextFixedCellHeightItem(TRUE) to the ItemSet

In the core, make sure the empty itemset for stylesheet contain
SDRATTR_TEXT_USEFIXEDCELLHEIGHT in the range.

On export, query the property FontIndependentLineSpacing and act accordingly.

Attaching a patch that fix the issue.
Comment 15 hub 2007-12-06 23:29:34 UTC
Created attachment 50165 [details]
patch (replace all the others)
Comment 16 sven.jacobi 2007-12-07 15:19:47 UTC
The latest patch looks good, I will add it to cws[impress136] which will most
probably be integrated into OOo2.4.
Comment 17 sven.jacobi 2007-12-07 16:12:02 UTC
The patch has been added.
Comment 18 sven.jacobi 2008-01-14 12:15:44 UTC
This issue is solved now in impress136 and ready to be verified.
Comment 19 wolframgarten 2008-01-14 13:23:35 UTC
Ok under windows.
Comment 20 christian.guenther 2008-01-15 15:12:48 UTC
CGU: Verified in cws impress136
Comment 21 christian.guenther 2008-07-03 17:08:14 UTC
CGU: Integrated in dev300m22