It is only safe to remove a relation of a PictureShape if there are no more relations to that media on its slide. I created a small example pptx where I copy pasted an image on a slide and removed only one of them using poi. The current implementation removes the relation and the document gets corrupted because of the invalid link. The proposed fix is to iterate on the pictures of the slide and only remove the relation if that is the only one.
Created attachment 36155 [details] change in removePictureRelation function and testcase
Created attachment 36156 [details] example pptx
Comment on attachment 36155 [details] change in removePictureRelation function and testcase Would it be better if the new code threw an exception if it isn't safe to remove the picture relation? + if (numberOfRelations <= 1) { + removeRelation(pictureShape.getBlipId()); + } else { + throw new IllegalStateException("Cannot remove picture relation because there are other references to the picture"); + }
(In reply to PJ Fanning from comment #3) > Comment on attachment 36155 [details] > change in removePictureRelation function and testcase > > Would it be better if the new code threw an exception if it isn't safe to > remove the picture relation? > > + if (numberOfRelations <= 1) { > + removeRelation(pictureShape.getBlipId()); > + } else { > + throw new IllegalStateException("Cannot remove picture relation > because there are other references to the picture"); > + } As far as I understood, it wouldn't. It is a valid case to remove just one PictureShape, but leave the relation between the Slide and the media. The new testcase would also fail at TestXSLFBugs.bug62736(TestXSLFBugs.java:124) The reason why I used <= 1 that in POIXMLDocumentPart.java we just return false if there are no relations, and I wanted to keep the existing functionality in that case. RelationPart rp = relations.get(partId); if (rp == null) { // part is not related with this POIXMLDocumentPart return false; }
https://svn.apache.org/viewvc?view=revision&revision=1842055