Thanks for doing this up as a proposal, it really helps with discussion. I did not think that MarkFactory or ExternalGraphicFactory needed to be aware of priority as this is handled by FactoryRegistry; but you are correct none of the existing implementations extend AbstractFactory.
Your proposal is on MarkFactory; do you need a similar change for ExternalGraphicFactory? I am not quite sure how MarkFactoriesProcessor is intended to work: - processFactories( Iterator<MarkFactory> factories ): Iterator<MarkFactory> Processing an iterator on the fly is good, it amounts to doing pair wise ordering, or a comparator, which is already supported by FactoryRegistry. - priority(): int What is the int for? Is it to order between competing instances of MarkFactoriesProcessor ? My hesitation is seeing more than one way to do things, especially if it is a one-off for a specific factory. So I would like to see what the limitation you are running into with the existing setup, and if your proposal is an improvement we could do it for all FactoryFinders. We can look at other factory finders for examples of helper methods used to address common challenges (factory iterator providers for injecting instances, establishing pairwise ordering <https://github.com/geotools/geotools/blob/06d379a2fdbfdbf94641c16813c37ef02aa7f63c/modules/library/referencing/src/main/java/org/geotools/referencing/ReferencingFactoryFinder.java#L546>, ...). I know that establishing pairwise ordering is a pain, and there is already a helper method that takes a comparator <https://github.com/geotools/geotools/blob/main/modules/library/metadata/src/main/java/org/geotools/util/factory/FactoryRegistry.java#L1265> to make this easier. Checking your proposal you reference propose changing the code here <https://github.com/geotools/geotools/blob/2c0f22f0a5a13885aa1d85faa6f715ea87f3c90e/modules/library/render/src/main/java/org/geotools/renderer/style/SLDStyleFactory.java#L1362> : *Iterator<MarkFactory> it = DynamicSymbolFactoryFinder.getMarkFactories();* I would ask instead that you change DynamicSymbolFactoryFinder directly; that interface is already responsible for SPI discovery, Here is a proposed alternative approach: 1. Leave StyleFactory alone (it already has too much logic) and trust DynamicSymbolFactoryFinder.getMarkFactories() to respect your ordering, especially since you are trying to work around how factory discovery is based on classpath order. 2. Create a method DynamicSymbolFactoryFinder setMarkFactoryOrder( Comparator<MarkFactory> comparator ): public static synchronized Iterator<MarkFactory> setMarkFactoryOrder( Comparator<MarkFactory> comparator ) { getServiceRegistry().setOrdering( MarkFactory.class, comparator ); } 4. This calls FactoryRegister.setOrdering( category, comparator ) <https://github.com/geotools/geotools/blob/main/modules/library/metadata/src/main/java/org/geotools/util/factory/FactoryRegistry.java#L1265> which should do the trick. -- Jody Garnett
_______________________________________________ GeoTools-Devel mailing list GeoTools-Devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/geotools-devel
