Re: svn commit: r743351 - /xmlgraphics/fop/branches/Temp_AreaTreeNewDesign/src/java/org/apache/fop/render/pcl/PCLDocumentHandler.java

[Vincent Hennebert]

And that goes on...

> Author: jeremias
> Date: Wed Feb 11 14:54:26 2009
> New Revision: 743351
> 
> URL: http://svn.apache.org/viewvc?rev=743351&view=rev
> Log:
> Added missing default font setup.
>  
> +    /** {...@inheritdoc} */
> +    public void setDefaultFontInfo(FontInfo fontInfo) {
> +        FontInfo fi = Java2DUtil.buildDefaultJava2DBasedFontInfo(fontInfo, 
> getUserAgent());
> +        setFontInfo(fi);
> +    }

The setDefaultFontInfo method is defined 5 times in the
AbstractBinaryWritingIFDocumentHandler hierarchy:

- one in ABWIFDocumentHandler itself, here’s the code:
        FontManager fontManager = getUserAgent().getFactory().getFontManager();
        FontCollection[] fontCollections = new FontCollection[] {
                new Base14FontCollection(fontManager.isBase14KerningEnabled())
        };

        FontInfo fi = (fontInfo != null ? fontInfo : new FontInfo());
        fi.setEventListener(new
FontEventAdapter(getUserAgent().getEventBroadcaster()));
        fontManager.setup(fi, fontCollections);
        setFontInfo(fi);

- one in the AFPDocumentHandler sub-class:
        FontManager fontManager = getUserAgent().getFactory().getFontManager();
        FontCollection[] fontCollections = new FontCollection[] {
            new AFPFontCollection(getUserAgent().getEventBroadcaster(), null)
        };

        FontInfo fi = (fontInfo != null ? fontInfo : new FontInfo());
        fi.setEventListener(new
FontEventAdapter(getUserAgent().getEventBroadcaster()));
        fontManager.setup(fi, fontCollections);
        setFontInfo(fi);

- one in PCLDocumentHandler:
        FontInfo fi = Java2DUtil.buildDefaultJava2DBasedFontInfo(fontInfo,
getUserAgent());
        setFontInfo(fi);

- one in TIFFDocumentHandler:
        FontInfo fi = Java2DUtil.buildDefaultJava2DBasedFontInfo(fontInfo,
getUserAgent());
        setFontInfo(fi);


One could think: it’s not too bad, the code is duplicated only twice, in
the last two cases it’s a one-line call to another method. Yes, but
guess what is the code of that method:
        Graphics2D graphics2D = Java2DFontMetrics.createFontMetricsGraphics2D();

        FontManager fontManager = userAgent.getFactory().getFontManager();
        FontCollection[] fontCollections = new FontCollection[] {
                new 
org.apache.fop.render.java2d.Base14FontCollection(graphics2D),
                new InstalledFontCollection(graphics2D)
        };

        FontInfo fi = (fontInfo != null ? fontInfo : new FontInfo());
        fi.setEventListener(new
FontEventAdapter(userAgent.getEventBroadcaster()));
        fontManager.setup(fi, fontCollections);
        return fi;


Looking further into the ABWIFDocumentHandler hierarchy, we can see how
the startDocument method is implemented:
- in AFPDocumentHandler:
        try {
            if (getUserAgent() == null) {
                throw new IllegalStateException(
                        "User agent must be set before starting PostScript
generation");
            }
            if (this.outputStream == null) {
                throw new IllegalStateException("OutputStream hasn't been set
through setResult()");
            }
            [...]
        } catch (IOException e) {
            throw new IFException("I/O error in startDocument()", e);
        }

- in PCLDocumentHandler:
        try {
            if (getUserAgent() == null) {
                throw new IllegalStateException(
                        "User agent must be set before starting PDF 
generation");
            }
            if (this.outputStream == null) {
                throw new IllegalStateException("OutputStream hasn't been set
through setResult()");
            }
            [...]
        } catch (IOException e) {
            throw new IFException("I/O error in startDocument()", e);
        }

- in PDFDocumentHandler:
        try {
            if (getUserAgent() == null) {
                throw new IllegalStateException(
                        "User agent must be set before starting PDF 
generation");
            }
            if (this.outputStream == null) {
                throw new IllegalStateException("OutputStream hasn't been set
through setResult()");
            }
            [...]
        } catch (IOException e) {
            throw new IFException("I/O error in startDocument()", e);
        }

- in PSDocumentHandler:
        try {
            if (getUserAgent() == null) {
                throw new IllegalStateException(
                        "User agent must be set before starting PostScript
generation");
            }
            if (this.outputStream == null) {
                throw new IllegalStateException("OutputStream hasn't been set
through setResult()");
            }
            [...]
        } catch (IOException e) {
            throw new IFException("I/O error in startDocument()", e);
        }

- in TIFFDocumentHandler:
        try {
            if (getUserAgent() == null) {
                throw new IllegalStateException(
                        "User agent must be set before starting PDF 
generation");
            }
            if (this.outputStream == null) {
                throw new IllegalStateException("OutputStream hasn't been set
through setResult()");
            }
            [...]
        } catch (IOException e) {
            throw new IFException("I/O error in startDocument()", e);
        }

Note the copy/paste errors, which means that only 2 out of the 5 error
messages are correct.
There’s no TODO statement anywhere, so I assume that this code is
considered to be finalized. IIC this is new code: no legacy here, no
inherited burden.

Now I have just one question: why? What benefit does this development
method bring that I’ve missed, and that overcomes all of the well-known
drawbacks?

Vincent