Hi Leo, OK great. That's fine with me.
I will apply the appropriate changes and commit the patch! Regards, Jakob 2010/12/10 Leonardo Uribe <[email protected]>: > Hi Jakob > > I think it is better do not include it as parameter and instead add some > comment on the javadoc, > saying the information to take into account is retrieved from > FacesConfigurationProvider. The fact > of use FacesConfigurationProvider is more an implementation detail than a > requeriment, so if > somebody wants to do not use it is valid usage. > > regards, > > Leonardo Uribe > > 2010/12/10 Jakob Korherr <[email protected]> >> >> Hi, >> >> OK, great! I added another comment to MYFACES-2945 explaining why I >> did it this way. >> >> Please tell me your final opinion after reading this comment and I'll >> commit an appropriate version of the proposed patch! >> >> Regards, >> Jakob >> >> 2010/12/9 Leonardo Uribe <[email protected]>: >> > Hi >> > >> > I think it is not necessary to pass FacesConfigurationProvider as param >> > for >> > getFacesConfigData, because in theory we don't do anything with it >> > before >> > pass it and wrappers will not do anything with it later. I think it >> > looks >> > good to load it using >> > >> > FacesConfigurationProviderFactory.getFacesConfigurationProvider(ExternalContext). >> > >> > The other parts of the patch looks good. >> > >> > regards, >> > >> > Leonardo Uribe >> > >> > 2010/12/9 Jakob Korherr <[email protected]> >> >> >> >> Hi guys, >> >> >> >> I just uploaded a patch for a FacesConfigurationMerger SPI: >> >> MYFACES-2945-FacesConfigurationMerger.patch >> >> >> >> Furthermore I added a quick code sample as comment on the MYFACES-2945 >> >> issue about how Geronimo can use this new SPI. >> >> >> >> Please take a look at the patch and if there are no objections, I will >> >> commit it soon! >> >> >> >> Regards, >> >> Jakob >> >> >> >> 2010/12/9 Jakob Korherr <[email protected]>: >> >> > Hi, >> >> > >> >> > I called it ugly, because of its implementation code in >> >> > DefaultFacesConfigurationProvider: The method is already inside of a >> >> > FacesConfigurationProvider, but it does this: >> >> > >> >> > FacesConfigurationProvider provider = >> >> > FacesConfigurationProviderFactory. >> >> > getFacesConfigurationProviderFactory(_externalContext). >> >> > getFacesConfigurationProvider(_externalContext); >> >> > >> >> > ...and then calls all the other methods of FacesConfigurationProvider >> >> > on this provider instance. >> >> > >> >> > As I said, I know this is this way, because >> >> > FacesConfigurationProvider >> >> > can be wrapped, but IMHO it is really ugly. >> >> > >> >> > >> >> > The method used on MYFACES-2998 was a first approach to solve this >> >> > problem in a better way. However, I really like those two of your >> >> > suggestions: >> >> > >> >> > 1) Leo #2: Create another SPI interface for getFacesConfigData() >> >> > (please suggest a name for it, maybe >> >> > FacesConfigurationMergerProvider?) and remove this method form >> >> > FacesConfigurationProvider. >> >> > 2) Ivan: In a few words: let getFacesConfigData() on >> >> > FacesConfigurationProvider, but provide an >> >> > AbstractFacesConfigurationProvider which implements the merging and >> >> > sorting to let custom SPI impls take advantage of it. >> >> > >> >> > At first, I really liked Ivan's proposal, but after thinking more >> >> > about it, it is not consistent with what we have right now (we do not >> >> > provide any other Abstract-SPI impl) and we would have the huge >> >> > merging and sorting code all in the SPI(-api) package, but IMO it >> >> > should really go into o.a.m.config. >> >> > >> >> > Thus I think that a new FacesConfigurationMergerProvider-SPI as Leo >> >> > proposed is the best choice here. Note that for this SPI it is good >> >> > practice to use other SPI impls. >> >> > >> >> > I will provide a patch for it soon and then we can discuss it >> >> > further! >> >> > >> >> > Regards, >> >> > Jakob >> >> > >> >> > 2010/12/9 Ivan <[email protected]>: >> >> >> my 2 cents, it seems for me less urgly ... >> >> >> a. For the FacesConfigurationProvider , it is better to have only >> >> >> one >> >> >> method >> >> >> getFacesConfigData() >> >> >> b. Create another abstract class AbstractFacesConfigurationProvider >> >> >> which >> >> >> extends FacesConfigurationProvider, and define those proctected >> >> >> methods >> >> >> of >> >> >> get***, also place those sorting/merging codes there. >> >> >> c. In the DefaultFacesConfigurationProvider, it only implements >> >> >> those >> >> >> get*** >> >> >> methods. >> >> >> thanks. >> >> >> >> >> >> 2010/12/9 Leonardo Uribe <[email protected]> >> >> >>> >> >> >>> Hi >> >> >>> >> >> >>> I agree with Jakob about faces-config merging and ordering >> >> >>> algorithm >> >> >>> should not be exposed by MyFaces. Why is it not enough?. At this >> >> >>> point >> >> >>> it is >> >> >>> not clear the reasons. Note in this moment ordering and sorting >> >> >>> algoritm it >> >> >>> is not being exposed by FacesConfigurationProvider interface. >> >> >>> >> >> >>> Other different thing is >> >> >>> FacesConfigurationProvider.getFacesConfigData(). >> >> >>> The intention of this method is provide a way to get a Serializable >> >> >>> object >> >> >>> that represents all config information required to initialize >> >> >>> MyFaces >> >> >>> and >> >> >>> allow container to store it temporally somewere. In this way it is >> >> >>> possible >> >> >>> to deploy and undeploy an application more quickly, because if >> >> >>> "nothing >> >> >>> changes"(no added dependencies, no update from faces-config.xml >> >> >>> files >> >> >>> or >> >> >>> classes) this information is always the same. >> >> >>> >> >> >>> On MYFACES-2945 and previous discussions it was proposed two >> >> >>> different >> >> >>> options: >> >> >>> >> >> >>> 1. Add getFacesConfigData() >> >> >>> 2. Add a set of methods to retrieve FacesConfig objects instead. >> >> >>> >> >> >>> public abstract FacesConfig >> >> >>> getStandardFacesConfig(ExternalContext >> >> >>> ectx); >> >> >>> public abstract FacesConfig >> >> >>> getMetaInfServicesFacesConfig(ExternalContext ectx); >> >> >>> public abstract FacesConfig >> >> >>> getAnnotationsFacesConfig(ExternalContext >> >> >>> ectx, boolean metadataComplete); >> >> >>> public abstract List<FacesConfig> >> >> >>> getClassloaderFacesConfig(ExternalContext ectx); >> >> >>> public abstract List<FacesConfig> >> >> >>> getContextSpecifiedFacesConfig(ExternalContext ectx); >> >> >>> public abstract FacesConfig >> >> >>> getWebAppFacesConfig(ExternalContext >> >> >>> ectx); >> >> >>> >> >> >>> The first option has the advantage that it fill the initial >> >> >>> requeriment >> >> >>> without add more complexity to the problem. The second one seems to >> >> >>> be >> >> >>> more >> >> >>> structured and opens the possibility to do other things like how >> >> >>> override >> >> >>> MyFaces parsing for faces-config.xml files like it is being >> >> >>> discussed. >> >> >>> If >> >> >>> the container parse faces-config.xml files, with the previous >> >> >>> methods >> >> >>> it is >> >> >>> possible to prevent parse the same files twice. >> >> >>> >> >> >>> My first intention, as is shown on MYFACES-2945 was that >> >> >>> FacesConfigurationProvider does not included getFacesConfigData(), >> >> >>> because >> >> >>> it is possible to fill the initial objective with these methods, >> >> >>> but >> >> >>> finally >> >> >>> it was agreed the first option looks better, right? >> >> >>> >> >> >>> Now I see that on MYFACES-2998 that fact is questioned: >> >> >>> >> >> >>> JK>> Unfortunately it also provides a method that should combine >> >> >>> all >> >> >>> these >> >> >>> data: getFacesConfigData(). >> >> >>> JK>> This method is here due to refactorings, but IMHO this is the >> >> >>> last >> >> >>> place where it should be put, >> >> >>> JK>> because it gets "its own implementation" via its Factory and >> >> >>> then >> >> >>> calls all of the above methods on >> >> >>> JK>> it. I know this was introduced to support wrapping the default >> >> >>> impl, >> >> >>> but it really is very, very ugly. >> >> >>> >> >> >>> In few words, why does it looks ugly? or with other words, what can >> >> >>> we >> >> >>> do >> >> >>> to make it cleaner? remove it? or just provide another SPI >> >> >>> interface >> >> >>> and put >> >> >>> that method there? In practice, getFacesConfigData() merges all >> >> >>> FacesConfig >> >> >>> information, and "on the way" it does order applicationFacesConfig >> >> >>> files >> >> >>> (the ones obtained from getClassloaderFacesConfig() and >> >> >>> getContextSpecifiedFacesConfig() ) . To do that it requires to call >> >> >>> all six >> >> >>> methods from FacesConfigurationProvider, there is no other way, so >> >> >>> I >> >> >>> don't >> >> >>> see why do that is considered ugly. >> >> >>> >> >> >>> At this moment we have the following courses of action: >> >> >>> >> >> >>> 1. Remove FacesConfigurationResource interface partially, because >> >> >>> it >> >> >>> is >> >> >>> still too inmature and let it for myfaces core 2.0.4. >> >> >>> 2. Create another SPI interface for getFacesConfigData() (please >> >> >>> suggest a >> >> >>> name for it, maybe FacesConfigurationMergerProvider?) and remove >> >> >>> this >> >> >>> method >> >> >>> form FacesConfigurationResource. Apply the patch on MYFACES-2998 >> >> >>> seems >> >> >>> to be >> >> >>> in this direction, but forget the reason why it is wanted to expose >> >> >>> getFacesConfigData() to the container. >> >> >>> 3. Apply something like MYFACES-2998 patch, and refactor this one >> >> >>> later in >> >> >>> myfaces core 2.0.4 >> >> >>> >> >> >>> Suggestions are welcome. >> >> >>> >> >> >>> regards, >> >> >>> >> >> >>> Leonardo Uribe >> >> >>> >> >> >>> >> >> >>> >> >> >>> >> >> >> >> >> >> >> >> >> >> >> >> -- >> >> >> Ivan >> >> >> >> >> > >> >> > >> >> > >> >> > -- >> >> > Jakob Korherr >> >> > >> >> > blog: http://www.jakobk.com >> >> > twitter: http://twitter.com/jakobkorherr >> >> > work: http://www.irian.at >> >> > >> >> >> >> >> >> >> >> -- >> >> Jakob Korherr >> >> >> >> blog: http://www.jakobk.com >> >> twitter: http://twitter.com/jakobkorherr >> >> work: http://www.irian.at >> > >> > >> >> >> >> -- >> Jakob Korherr >> >> blog: http://www.jakobk.com >> twitter: http://twitter.com/jakobkorherr >> work: http://www.irian.at > > -- Jakob Korherr blog: http://www.jakobk.com twitter: http://twitter.com/jakobkorherr work: http://www.irian.at
