Good point about extending factor ( I just assumed because of the name). I expect FactoryListPredicate<Class>, however we can leave the idea for a future release if other factories desire the same level of flexibility. -- Jody Garnett
On Mon, 13 Sept 2021 at 07:18, Fernando Mino < fernando.m...@geosolutionsgroup.com> wrote: > Hi Jody, thanks for your review. > > The problem on making the filter class more generic is MarkFactory > interface does not extend Factory or any other interface, so I need to make > MarFactory as type hierarchy root definition. > Also I tried to move the class to gt-metadata but since MarkFactory is a > gt-render type the compilation fails on that type resolution. > > Aside from that, I updated the PR with the suggested changes. > > > > Regards, > > Fernando Mino > > == > > GeoServer Professional Services from the experts! > > Visit http://bit.ly/gs-services-us for more information. > > == > > Fernando Mino > > Software Engineer > > @fmy2kec > > GeoSolutions Group > phone: +39 0584 962313 > > fax: +39 0584 1660272 > > https://www.geosolutionsgroup.com/ > > http://twitter.com/geosolutions_it > > ------------------------------------------------------- > > Con riferimento alla normativa sul trattamento dei dati personali (Reg. UE > 2016/679 - Regolamento generale sulla protezione dei dati “GDPR”), si > precisa che ogni circostanza inerente alla presente email (il suo > contenuto, gli eventuali allegati, etc.) è un dato la cui conoscenza è > riservata al/i solo/i destinatario/i indicati dallo scrivente. Se il > messaggio Le è giunto per errore, è tenuta/o a cancellarlo, ogni altra > operazione è illecita. Le sarei comunque grato se potesse darmene notizia. > > This email is intended only for the person or entity to which it is > addressed and may contain information that is privileged, confidential or > otherwise protected from disclosure. We remind that - as provided by > European Regulation 2016/679 “GDPR” - copying, dissemination or use of this > e-mail or the information herein by anyone other than the intended > recipient is prohibited. If you have received this email by mistake, please > notify us immediately by telephone or e-mail. > > > On Sun, Sep 12, 2021 at 7:15 PM Jody Garnett <jody.garn...@gmail.com> > wrote: > >> Thanks Fernando, i saw your PR yesterday and it indeed looks good (note >> it is still in draft). >> >> The one feedback I had was to consider changing your implementation from >> gt-render MarkFactoryListPredicate to gt-metadata >> org.geotools.util.factory.FactoryListPredicate<? extends Factory> >> >> The functionality is based on the factory name and is not MarkFactory >> specific. >> -- >> Jody Garnett >> >> >> On Sun, 12 Sept 2021 at 14:14, Fernando Mino < >> fernando.m...@geosolutionsgroup.com> wrote: >> >>> Hi Jody, >>> >>> Thanks one more time for your detailed feedback and the proposal >>> alternative, it is a good one. I made an implementation based on it and it >>> works well, so I updated the GeoServer proposal as well and created the >>> draft PRs for the community evaluation and review. >>> >>> GeoTools PR: >>> https://github.com/geotools/geotools/pull/3634 >>> >>> GeoServer PR: >>> https://github.com/geoserver/geoserver/pull/5279 >>> >>> Regards, >>> >>> Fernando Mino >>> >>> == >>> >>> GeoServer Professional Services from the experts! >>> >>> Visit http://bit.ly/gs-services-us for more information. >>> >>> == >>> >>> Fernando Mino >>> >>> Software Engineer >>> >>> @fmy2kec >>> >>> GeoSolutions Group >>> phone: +39 0584 962313 >>> >>> fax: +39 0584 1660272 >>> >>> https://www.geosolutionsgroup.com/ >>> >>> http://twitter.com/geosolutions_it >>> >>> ------------------------------------------------------- >>> >>> Con riferimento alla normativa sul trattamento dei dati personali (Reg. >>> UE 2016/679 - Regolamento generale sulla protezione dei dati “GDPR”), si >>> precisa che ogni circostanza inerente alla presente email (il suo >>> contenuto, gli eventuali allegati, etc.) è un dato la cui conoscenza è >>> riservata al/i solo/i destinatario/i indicati dallo scrivente. Se il >>> messaggio Le è giunto per errore, è tenuta/o a cancellarlo, ogni altra >>> operazione è illecita. Le sarei comunque grato se potesse darmene notizia. >>> >>> This email is intended only for the person or entity to which it is >>> addressed and may contain information that is privileged, confidential or >>> otherwise protected from disclosure. We remind that - as provided by >>> European Regulation 2016/679 “GDPR” - copying, dissemination or use of this >>> e-mail or the information herein by anyone other than the intended >>> recipient is prohibited. If you have received this email by mistake, please >>> notify us immediately by telephone or e-mail. >>> >>> >>> On Wed, Sep 8, 2021 at 8:34 PM Jody Garnett <jody.garn...@gmail.com> >>> wrote: >>> >>>> Fernando: >>>> >>>> Rather than watching communication go by: >>>> - Moved your proposal to the description section (you had the proposal >>>> outlined as tasks) and wrote in my assumption for MarkFactoryProcessor >>>> interface. >>>> - Wrote up alternate proposal more formally >>>> >>>> -- >>>> Jody Garnett >>>> >>>> >>>> On Thu, 12 Aug 2021 at 14:34, Fernando Mino < >>>> fernando.m...@geosolutionsgroup.com> wrote: >>>> >>>>> Hi Jody, >>>>> >>>>> Thanks for the insight on Factory registry as a better alternative. >>>>> Indeed the DynamicSymbolFactoryFinder.getMarkFactories() -> >>>>> FactoryRegister.setOrdering(..) approach will do the trick for one of the >>>>> scenarios we want to support (I will amend the proposal to add this to >>>>> support the global configuration use case) but we also have the >>>>> per-workspace use case that allows: >>>>> - To have several different MarkFactory ordering and filtering on >>>>> several workspaces. >>>>> - To have several WMS GetMap requests (and rendering threads) in >>>>> parallel, rendering Mark shapes with different MarkFactory implementations >>>>> order and availability profiles. >>>>> >>>>> Probably I'm wrong (and please let me know if indeed there is a way to >>>>> achieve this with DynamicSymbolFactoryFinder.getMarkFactories() -> >>>>> FactoryRegister.setOrdering), but looks like the per-workspace >>>>> configuration + multithreading support is not possible with >>>>> FactoryRegister >>>>> due to its not-thread-safe, globally cached and >>>>> synchronized-static-methods >>>>> caller(DynamicSymbolFactoryFinder). >>>>> >>>>> To use FactoryRegistry we would need to change >>>>> DynamicSymbolFactoryFinder to allow us to store and index different >>>>> FactoryRegistry instances per workspace, but the problem here is GeoTools >>>>> Rendering module is not aware of GeoServer workspaces, and that is why I >>>>> proposed the on-the-fly filtering and ordering interceptor extension >>>>> point, >>>>> to allow us to control this filtering from GeoServer code during request >>>>> runtime. >>>>> >>>>> Naturally any better idea to solve this second use case is welcome. >>>>> >>>>> Regards, >>>>> >>>>> Fernando Mino >>>>> >>>>> == >>>>> >>>>> GeoServer Professional Services from the experts! >>>>> >>>>> Visit http://bit.ly/gs-services-us for more information. >>>>> >>>>> == >>>>> >>>>> Fernando Mino >>>>> >>>>> Software Engineer >>>>> >>>>> @fmy2kec >>>>> >>>>> GeoSolutions Group >>>>> phone: +39 0584 962313 >>>>> >>>>> fax: +39 0584 1660272 >>>>> >>>>> https://www.geosolutionsgroup.com/ >>>>> >>>>> http://twitter.com/geosolutions_it >>>>> >>>>> ------------------------------------------------------- >>>>> >>>>> Con riferimento alla normativa sul trattamento dei dati personali >>>>> (Reg. UE 2016/679 - Regolamento generale sulla protezione dei dati >>>>> “GDPR”), >>>>> si precisa che ogni circostanza inerente alla presente email (il suo >>>>> contenuto, gli eventuali allegati, etc.) è un dato la cui conoscenza è >>>>> riservata al/i solo/i destinatario/i indicati dallo scrivente. Se il >>>>> messaggio Le è giunto per errore, è tenuta/o a cancellarlo, ogni altra >>>>> operazione è illecita. Le sarei comunque grato se potesse darmene notizia. >>>>> >>>>> This email is intended only for the person or entity to which it is >>>>> addressed and may contain information that is privileged, confidential or >>>>> otherwise protected from disclosure. We remind that - as provided by >>>>> European Regulation 2016/679 “GDPR” - copying, dissemination or use of >>>>> this >>>>> e-mail or the information herein by anyone other than the intended >>>>> recipient is prohibited. If you have received this email by mistake, >>>>> please >>>>> notify us immediately by telephone or e-mail. >>>>> >>>>> >>>>> On Wed, Aug 11, 2021 at 8:11 PM Jody Garnett <jody.garn...@gmail.com> >>>>> wrote: >>>>> >>>>>> 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
