You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@xmlgraphics.apache.org by je...@apache.org on 2010/02/19 17:37:29 UTC

svn commit: r911864 - /xmlgraphics/commons/trunk/src/java/org/apache/xmlgraphics/image/loader/pipeline/PipelineFactory.java

Author: jeremias
Date: Fri Feb 19 16:37:29 2010
New Revision: 911864

URL: http://svn.apache.org/viewvc?rev=911864&view=rev
Log:
Bugfix: Use a List and a sort instead of a SortedSet to avoid problems with the equals() semantics. The result really depended on the entry order into the set rather than the order implied by the penalty value. This could lead to the wrong pipeline being used.

Modified:
    xmlgraphics/commons/trunk/src/java/org/apache/xmlgraphics/image/loader/pipeline/PipelineFactory.java

Modified: xmlgraphics/commons/trunk/src/java/org/apache/xmlgraphics/image/loader/pipeline/PipelineFactory.java
URL: http://svn.apache.org/viewvc/xmlgraphics/commons/trunk/src/java/org/apache/xmlgraphics/image/loader/pipeline/PipelineFactory.java?rev=911864&r1=911863&r2=911864&view=diff
==============================================================================
--- xmlgraphics/commons/trunk/src/java/org/apache/xmlgraphics/image/loader/pipeline/PipelineFactory.java (original)
+++ xmlgraphics/commons/trunk/src/java/org/apache/xmlgraphics/image/loader/pipeline/PipelineFactory.java Fri Feb 19 16:37:29 2010
@@ -20,10 +20,11 @@
 package org.apache.xmlgraphics.image.loader.pipeline;
 
 import java.util.Collection;
+import java.util.Collections;
 import java.util.Comparator;
 import java.util.Iterator;
 import java.util.LinkedList;
-import java.util.SortedSet;
+import java.util.List;
 
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
@@ -143,7 +144,8 @@
             // --> List of resulting flavors, possibly multiple loaders
             loaderFactories = registry.getImageLoaderFactories(originalMime);
             if (loaderFactories != null) {
-                SortedSet candidates = new java.util.TreeSet(new PipelineComparator());
+                List candidates = new java.util.ArrayList();
+
                 //Find best pipeline -> best loader
                 for (int i = 0, ci = loaderFactories.length; i < ci; i++) {
                     ImageLoaderFactory loaderFactory = loaderFactories[i];
@@ -158,9 +160,10 @@
                     }
                 }
 
+                Collections.sort(candidates, new PipelineComparator());
                 //Build final pipeline
                 if (candidates.size() > 0) {
-                    pipeline = (ImageProviderPipeline)candidates.first();
+                    pipeline = (ImageProviderPipeline)candidates.get(0);
                 }
             }
         }



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@xmlgraphics.apache.org
For additional commands, e-mail: commits-help@xmlgraphics.apache.org


Re: svn commit: r911864 - /xmlgraphics/commons/trunk/src/java/org/apache/xmlgraphics/image/loader/pipeline/PipelineFactory.java

Posted by Jeremias Maerki <de...@jeremias-maerki.ch>.
Hi Vincent

On 22.02.2010 13:41:01 Vincent Hennebert wrote:
<snip/>
> Why do you store all the pipelines in a collection if in the end you
> need only the best one?

The original idea was to have a prioritized list of pipelines that can
be tried if the first n pipelines fail to load an image. But then I just
switched to using a single one for now which may explain that.

> Also, do you have a test case to reproduce the issue? So that any
> further change does not introduce a regression.

With my recent additions there are now tests that should cover that.


Jeremias Maerki


---------------------------------------------------------------------
To unsubscribe, e-mail: general-unsubscribe@xmlgraphics.apache.org
For additional commands, e-mail: general-help@xmlgraphics.apache.org


Re: svn commit: r911864 - /xmlgraphics/commons/trunk/src/java/org/apache/xmlgraphics/image/loader/pipeline/PipelineFactory.java

Posted by Vincent Hennebert <vh...@gmail.com>.
Hi Jeremias,

> Author: jeremias
> Date: Fri Feb 19 16:37:29 2010
> New Revision: 911864
> 
> URL: http://svn.apache.org/viewvc?rev=911864&view=rev
> Log:
> Bugfix: Use a List and a sort instead of a SortedSet to avoid problems with the equals() semantics. The result really depended on the entry order into the set rather than the order implied by the penalty value. This could lead to the wrong pipeline being used.
> 
> Modified:
>     xmlgraphics/commons/trunk/src/java/org/apache/xmlgraphics/image/loader/pipeline/PipelineFactory.java
> 
> Modified: xmlgraphics/commons/trunk/src/java/org/apache/xmlgraphics/image/loader/pipeline/PipelineFactory.java
> URL: http://svn.apache.org/viewvc/xmlgraphics/commons/trunk/src/java/org/apache/xmlgraphics/image/loader/pipeline/PipelineFactory.java?rev=911864&r1=911863&r2=911864&view=diff
> ==============================================================================
> --- xmlgraphics/commons/trunk/src/java/org/apache/xmlgraphics/image/loader/pipeline/PipelineFactory.java (original)
> +++ xmlgraphics/commons/trunk/src/java/org/apache/xmlgraphics/image/loader/pipeline/PipelineFactory.java Fri Feb 19 16:37:29 2010
> @@ -20,10 +20,11 @@
> @@ -143,7 +144,8 @@
>              // --> List of resulting flavors, possibly multiple loaders
>              loaderFactories = registry.getImageLoaderFactories(originalMime);
>              if (loaderFactories != null) {
> -                SortedSet candidates = new java.util.TreeSet(new PipelineComparator());
> +                List candidates = new java.util.ArrayList();
> +
>                  //Find best pipeline -> best loader
>                  for (int i = 0, ci = loaderFactories.length; i < ci; i++) {
>                      ImageLoaderFactory loaderFactory = loaderFactories[i];
> @@ -158,9 +160,10 @@
>                      }
>                  }
>  
> +                Collections.sort(candidates, new PipelineComparator());
>                  //Build final pipeline
>                  if (candidates.size() > 0) {
> -                    pipeline = (ImageProviderPipeline)candidates.first();
> +                    pipeline = (ImageProviderPipeline)candidates.get(0);
>                  }
>              }
>          }

Why do you store all the pipelines in a collection if in the end you
need only the best one?

Also, do you have a test case to reproduce the issue? So that any
further change does not introduce a regression.

Thanks,
Vincent

---------------------------------------------------------------------
To unsubscribe, e-mail: general-unsubscribe@xmlgraphics.apache.org
For additional commands, e-mail: general-help@xmlgraphics.apache.org