Bug 71434 - FILEOPEN pptx: Master slide texts show with specific placeholder type
Summary: FILEOPEN pptx: Master slide texts show with specific placeholder type
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Impress (show other bugs)
Version:
(earliest affected)
unspecified
Hardware: All All
: medium normal
Assignee: Korrawit Pruegsanusak
URL:
Whiteboard: target:4.3.0 target:4.1.5 target:4.2....
Keywords:
Depends on:
Blocks:
 
Reported: 2013-11-09 18:53 UTC by Korrawit Pruegsanusak
Modified: 2013-12-08 09:17 UTC (History)
1 user (show)

See Also:
Crash report or crash signature:


Attachments
pptx file (151.95 KB, application/vnd.openxmlformats-officedocument.presentationml.presentation)
2013-11-09 18:53 UTC, Korrawit Pruegsanusak
Details
screenshot (76.56 KB, image/jpeg)
2013-11-09 18:54 UTC, Korrawit Pruegsanusak
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Korrawit Pruegsanusak 2013-11-09 18:53:00 UTC
Created attachment 88942 [details]
pptx file

Master slide texts show overlapping an image, when placeholder type="body".

Earlier bugs, bug 35372, was fixed when placeholder type="obj", but this bug deals with type="body".

The sample file was modified from bug 46594 to use public domain image.

Steps: Open the attachment and see

Expected: only image, no texts overlapping
Actual: texts overlap an image

Assign this bug for me because I'm working on this.
Comment 1 Korrawit Pruegsanusak 2013-11-09 18:54:27 UTC
Created attachment 88943 [details]
screenshot
Comment 2 Korrawit Pruegsanusak 2013-11-09 19:09:17 UTC
Hello Muthu,

Since you have fixed bug 35372 with this commit http://cgit.freedesktop.org/libreoffice/core/commit/?id=90352e5f1e50291960c944f9a1f44ab3e91d6503

Source code: http://opengrok.libreoffice.org/xref/core/oox/source/ppt/pptgraphicshapecontext.cxx#147

There are 2 choices for fixing this problem:

1. add a check for XML_body in this line (line 151)
> if( pPlaceholder->getSubType() == XML_obj )

2. do as your TODO said (line 150)
> TODO: Check if pPlaceholder->getSubType is none (i.e. none explicitly specified)
That is, should I add a guard for all possible types of placeholder?

Muthu, I'd like to ask that, which should I do?
Comment 3 Korrawit Pruegsanusak 2013-11-09 19:11:40 UTC
Oops, forgot to CC Muthu :\

Muthu, could you please read comment 2, thanks.
Comment 4 Korrawit Pruegsanusak 2013-11-26 11:42:46 UTC
Submitted patch for review at https://gerrit.libreoffice.org/6814

I selected the second choice because I've found pptx document that has an image with placeholder type of "title".
Comment 5 Muthu 2013-11-26 12:35:24 UTC
Just saw this one :)
So, the patch seems fine. Thank you!

(But, I am not too sure about that ToDo. Could be some older one?)
Comment 6 Commit Notification 2013-11-26 12:35:35 UTC
Korrawit Pruegsanusak committed a patch related to this issue.
It has been pushed to "master":

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

fdo#71434: don't show master text if PlaceHolder types defined



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 7 Muthu 2013-11-26 12:37:22 UTC
But, the ToDo seems to refer to: "what happens if SubType isn't set"...anyways...
Comment 8 Korrawit Pruegsanusak 2013-11-26 16:03:50 UTC
Hello Muthu,
Thanks for review & push :)

(In reply to comment #7)
> But, the ToDo seems to refer to: "what happens if SubType isn't
> set"...anyways...

Oh, so I misunderstood your TODO. But I don't think this will change an outcome, because bUseText still be true (since initialization) in both old and new code.

Anyway, I submitted another 2 patches for stable branches:
-4-2 at https://gerrit.libreoffice.org/6822
-4-1 at https://gerrit.libreoffice.org/6821

Could you please review & push again? Thanks :)
Comment 9 Muthu 2013-11-27 04:37:32 UTC
It would be nice if somebody else reviews that for the stable branches :)
Comment 10 Commit Notification 2013-12-03 09:44:43 UTC
Korrawit Pruegsanusak committed a patch related to this issue.
It has been pushed to "libreoffice-4-1":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=0f4a452be3342d0206d6724faf170fa2786a7777&h=libreoffice-4-1

fdo#71434: don't show master text if PlaceHolder types defined


It will be available in LibreOffice 4.1.5.

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 11 Commit Notification 2013-12-03 09:46:03 UTC
Korrawit Pruegsanusak committed a patch related to this issue.
It has been pushed to "libreoffice-4-2":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=a06bc4102934c47e1aebe53ea24415fafb7c506a&h=libreoffice-4-2

fdo#71434: don't show master text if PlaceHolder types defined


It will be available in LibreOffice 4.2.

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 12 Korrawit Pruegsanusak 2013-12-03 12:01:23 UTC
Thanks very much, resolving.

Anyway, while creating the unit test, I encountered another bug, reported at bug#72260. It would be great if you could take a look there. Thanks again! :)
Comment 13 Commit Notification 2013-12-08 09:17:08 UTC
Korrawit Pruegsanusak committed a patch related to this issue.
It has been pushed to "master":

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

unittest for fdo#71434



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.