xmlgraphics-fop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jeremias Maerki <...@jeremias-maerki.ch>
Subject Re: svn commit: r743351 - /xmlgraphics/fop/branches/Temp_AreaTreeNewDesign/src/java/org/apache/fop/render/pcl/PCLDocumentHandler.java
Date Mon, 16 Feb 2009 10:26:34 GMT
On 12.02.2009 13:35:57 Vincent Hennebert wrote:
> 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;

I'm so completely lost. I have absolutely no idea what you want to say
here. I don't see any optimization potential here at all. But I'm sure
you could do it better.

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

Fixed. Thanks for pointing out the oversight.

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

Two questions actually. But I don't know how/what to repond to that.


Jeremias Maerki


Mime
View raw message