Hi Vincent,

Thanks a lot for taking the time to comb through my large patch :).

Vincent Hennebert wrote:
Hi Adrian,

Nice job! Two nits:

Author: acumiskey
Date: Tue May  6 09:14:09 2008
New Revision: 653826
<snip/>
Modified:
xmlgraphics/fop/trunk/src/java/org/apache/fop/apps/FopFactory.java
URL: 
http://svn.apache.org/viewvc/xmlgraphics/fop/trunk/src/java/org/apache/fop/apps/FopFactory.java?rev=653826&r1=653825&r2=653826&view=diff
==============================================================================
--- xmlgraphics/fop/trunk/src/java/org/apache/fop/apps/FopFactory.java 
(original)
+++ xmlgraphics/fop/trunk/src/java/org/apache/fop/apps/FopFactory.java Tue May  
6 09:14:09 2008
+    /**
+     * Returns the font manager
+     * @return the font manager
+     */
+    public FontManager getFontManager() {
+        if (fontManager == null) {
+            this.fontManager = new FontManager(this);
+        }
+        return this.fontManager;
     }

Shouldn’t this method be made synchronized? IIC it might be called by
several threads.

Yes it could well have been declared as synchronized, but then again there are other methods in FopFactory to which this could apply. I have now moved this instantiation to the constructor along with all the other component objects.



Modified:
xmlgraphics/fop/trunk/src/java/org/apache/fop/render/PrintRenderer.java
URL: 
http://svn.apache.org/viewvc/xmlgraphics/fop/trunk/src/java/org/apache/fop/render/PrintRenderer.java?rev=653826&r1=653825&r2=653826&view=diff
==============================================================================
--- xmlgraphics/fop/trunk/src/java/org/apache/fop/render/PrintRenderer.java 
(original)
+++ xmlgraphics/fop/trunk/src/java/org/apache/fop/render/PrintRenderer.java Tue 
May  6 09:14:09 2008
     /**
-     * adds a font list to current list of fonts
-     * @param fontInfoList font list
+     * Adds a font list to current list of fonts
+     * @param fontList a font info list
      */
-    public void addFontList(List fontInfoList) {
-        if (this.fontList == null) {
-            setFontList(fontInfoList);
+    public void addFontList(List/*<EmbedFontInfo>*/ fontList) {
+        if (embedFontInfoList == null) {
+            setFontList(fontList);
         } else {
-            this.fontList.addAll(fontInfoList);
+            fontList.addAll(fontList);

I have a slight doubt here. It’s not that concatenating a list to itself
doesn’t have any interest, but... ;-) Shouldn’t it be embedFontInfoList instead?

Good spot, have committed a fix for this now :)

Thanks again,

Adrian.

Reply via email to