On 24.11.2008 15:50:40 Adrian Cumiskey wrote:
> 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.

Again: it's not the fix that is the problem. It is about
backwards-compatibility and that's an issue mostly for users and release
managers. Developers have an easy life. Imagine what would happen if
Linus Torvalds would constantly change the API for low-level drivers or
if Sun would simply change APIs in their Java class library without
respecting backwards compatibility.

> >> 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.

Yeah, and after your logic I'll continue to play that game indefinitely.
Each time we release a new FOP, I have to update my plug-in. That's not
my idea of software development.

> > 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.

It doesn't really matter if I have an external implementation of that
interface or not. The interface was designed as a service provider
interface with dynamic registration. That automatically turns it into an
external interface for FOP which needs to be treated with care.

The PDF renderer doesn't profit from the changes you made. It just
breaks the already existing plug-ins. You've had an idea for the image
handler plug-ins for the AFP Renderer and you wanted to reuse the
registry part. And that's what affects the PDF part. The name clash in
the IF branch is another story. That one is not even so important.

Change happens, yes, improvements happen, too, but the "after me the
Flood" mentality makes my blood boil.


Jeremias Maerki

Reply via email to