Jeremias Maerki wrote:
On 24.11.2008 13:14:35 Adrian Cumiskey wrote:
Jeremias,

Jeremias Maerki wrote:
Adrian,

I'm unhappy with this change. I know you're constantly trying to
improve things by refactoring but you also need to be careful about
preserving compatibility. In this case, you broke my external PDF-in-PDF
plug-in by changing the interface from "Class getSupportedImageClass()"
to "Class[] getSupportedImageClasses()". Furthermore, chosing
"ImageHandler" as the interface name will result in a name clash when
the AFP branch is merged back into Trunk and I'll try to merge those
changes into the IF branch. After all, in this branch the whole image
handling is already properly abstracted for all renderers, not just PDF
and AFP. So I think we need to revisit that before we consider a merge
of the AFP GOCA branch into Trunk.

I think, we have the following options:
1. Remove ImageHandler in the GOCA branch, restore plug-in compatibility
for PDFImageHandler and have a separate service registry class for AFP
and PDF. (My preffered option)
I think this option would involve the most amount of work and regression testing. Furthermore, this is work that will probably be thrown away later. It does not need to be done now and can be revisited later when the Temp_AreaTreeNewDesign branch is ready to be merged back into trunk.

2. We can selectively merge parts of the IF branch (the ImageHandler
abstraction) into trunk before merging the rest (I could do that for you,
if nobody objects the partial merge-back). From there you could make use
of it in the GOCA branch and restore the PDFImageHandler. You could then
use the same image handlers for the current code as well as the
implementation for the new IF.
This sounds a bit messy, its better we do the merge in one go, a halfway situation is never a good one and I would expect quite a bit of management overhead created by this.

3. I have to adjust my PDF plug-in and do another release knowing that
this is only for restoring compatibility.
I would favour this option. This represents the least amount of change to FOP and the least amount of work/regression testing etc. This is a very small change that should only effect a small number of users who upgrade their FOP installation to trunk and forget to upgrade their pdf images plugin.

Cheap excuse to ignore backwards-compatibility. What if there were other
people who wrote plug-ins of their own?

I'm sure if such a person existed and they wrote their own FOP plug-in they could very easily fix a very small compatibility issue such as this.

I have included the simple 2 line patch file for src/java/org/apache/fop/render/pdf/pdfbox/PDFBoxPDFImageHandler.java. Apologies if this creates a little more work for you.

Your patch has one problem: it kills backwards-compatibility for FOP
0.95. I won't delete the old method so this whole thing will still work
with 0.95.

I don't see how this would at all affect 0.95 compatibility.  You already 
provide a cersion 1.2
(http://www.jeremias-maerki.ch/download/fop/pdf-images) which will work just 
fine for 0.95
installations - so no change there. And you also already provide a version 1.1a (from rev 611768 or later), so all that needs to be done once the Temp_AFPGOCAResources branch is merged back into trunk is release a new trunk version with the 2 line patch applied.

The necessary changes in my plug-in are negligible. What isn't is the
time I'll spend documenting which version of the plug-in the users have
to select for which FOP version. Add to that time I spend on answering
the questions of the people who don't RTFM.

I sympathize, users can be demanding even when you give of your time freely :).

I wonder if you've spent one second thinking about
backwards-compatibility before changing the PDFImageHandler interface.

Quite simply I wasn't aware that your external pdf images package touched upon this interface. Its not part of the FOP code base. I will continue to always want what is best for the health of the project, I find your comment both unnecessarily rude and disrespectful.

Adrian.

Reply via email to