Bug 58190

Summary:	[PATCH] The current picture handling uses raw integers for types and index, replace with enum and reference
Product:	POI	Reporter:	mark.o <Mark.Olesen>
Component:	XSLF	Assignee:	POI Developers List <dev>
Status:	RESOLVED FIXED
Severity:	enhancement	Keywords:	PatchAvailable
Priority:	P2
Version:	3.13-dev
Target Milestone:	---
Hardware:	PC
OS:	All
Bug Depends on:
Bug Blocks:	59836
Attachments:	patch as basic coding example Patch for X/HSLF which uses the (H/XSLF)PictureData element addPicture() with File and InputStream parameters

Description mark.o 2015-07-29 11:42:32 UTC

It is much too easy to make errors when dealing with images.
Introduce enums (for image types and relations).
Provide thin wrapper to reference the document image number.

Patched code provided as a discussion and/or implementation basis.
Need to decide how much compatibilty to retain.

Comment 1 mark.o 2015-07-29 11:59:07 UTC

Created attachment 32942 [details]
patch as basic coding example

Comment 2 Andreas Beeker 2015-08-02 17:43:34 UTC

Created attachment 32952 [details]
Patch for X/HSLF which uses the (H/XSLF)PictureData element

Comment 3 Andreas Beeker 2015-08-02 20:24:22 UTC

Thank you for bringing this up.
I've changed a few things - I hope the new handling is ok for you.
Applied with r1693825

Comment 4 mark.o 2015-08-03 07:23:54 UTC

No problems with the handling - the main thing is that the integers index is gone, everything else should be encapsulated enough that nobody is bothered.

BTW: the PictureType enum has an "ooxml-id". I don't know that this is necessary at all. I believe it was only the integer value into the previous RELATIONS[] lookup, but doesn't have any intrinsic meaning ... it certainly isn't used anywhere in the code.

Comment 5 mark.o 2015-08-04 07:05:21 UTC

(In reply to Andreas Beeker from comment #3)
> Thank you for bringing this up.
> I've changed a few things - I hope the new handling is ok for you.
> Applied with r1693825

I've reopened with a patch for improved consistency/convenience.
I assume that I'll like many users and my image source is file-based. In this case it makes sense to support this directly (as is already done in HSLFSlideShow). I personally find this is much clearer.

Comment 6 mark.o 2015-08-04 07:06:46 UTC

Created attachment 32954 [details]
addPicture() with File and InputStream parameters

Comment 7 Dominik Stadler 2016-05-22 16:09:55 UTC

Applied via r1745073, thanks for the work on the patches. It would be good if you can add some unit-tests which cover these new methods so we can close this bug for good!

Comment 8 Javen O'Neal 2016-07-17 13:25:55 UTC

(In reply to Dominik Stadler from comment #7)
> Applied via r1745073, thanks for the work on the patches. It would be good
> if you can add some unit-tests which cover these new methods so we can close
> this bug for good!

Added some unit tests for SlideShow#findPicture and SlideShow.addPicture in r1753065 and r1753073. Is there anything else that needs a unit test in order to close this bug?