Bug 58190 - [PATCH] The current picture handling uses raw integers for types and index, replace with enum and reference
Summary: [PATCH] The current picture handling uses raw integers for types and index, r...
Status: RESOLVED FIXED
Alias: None
Product: POI
Classification: Unclassified
Component: XSLF (show other bugs)
Version: 3.13-dev
Hardware: PC All
: P2 enhancement (vote)
Target Milestone: ---
Assignee: POI Developers List
URL:
Keywords: PatchAvailable
Depends on:
Blocks: 59836
  Show dependency tree
 
Reported: 2015-07-29 11:42 UTC by mark.o
Modified: 2016-08-22 19:15 UTC (History)
0 users



Attachments
patch as basic coding example (23.58 KB, patch)
2015-07-29 11:59 UTC, mark.o
Details | Diff
Patch for X/HSLF which uses the (H/XSLF)PictureData element (16.34 KB, application/zip)
2015-08-02 17:43 UTC, Andreas Beeker
Details
addPicture() with File and InputStream parameters (7.65 KB, patch)
2015-08-04 07:06 UTC, mark.o
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
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?