Hi Good to know that, thanks. In these moments I'm running the necessary steps for send the vote, so I hope to release these artifacts this week.
regards, Leonardo Uribe 2010/12/13 Ivan <xhh...@gmail.com> > Thanks, Jakob and Leonardo, the new changes work perfect ;-) > By the way, I have run the TCK on my local environment, although there are > some failure cases, they should be caused by webbeans integration. > > 2010/12/11 Jakob Korherr <jakob.korh...@gmail.com> > > Hi guys, >> >> As discussed, I just committed a modified version of the proposed patch. >> >> Furthermore I added the custom SPI impl that I used for testing to the >> issue [1] which may help for the real Geronimo impl. >> >> It would be great if you guys could check if this solution works for >> you, and if so, we can close this issue and start the 2.0.3 release! >> >> Regards, >> Jakob >> >> [1] >> https://issues.apache.org/jira/secure/attachment/12466010/GeronimoFacesConfigurationMerger.java >> >> 2010/12/10 Jakob Korherr <jakob.korh...@gmail.com>: >> > 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 <lu4...@gmail.com>: >> >> 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 <jakob.korh...@gmail.com> >> >>> >> >>> 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 <lu4...@gmail.com>: >> >>> > 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 <jakob.korh...@gmail.com> >> >>> >> >> >>> >> 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 <jakob.korh...@gmail.com>: >> >>> >> > 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 <xhh...@gmail.com>: >> >>> >> >> 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 <lu4...@gmail.com> >> >>> >> >>> >> >>> >> >>> 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 >> > >> >> >> >> -- >> Jakob Korherr >> >> blog: http://www.jakobk.com >> twitter: http://twitter.com/jakobkorherr >> work: http://www.irian.at >> > > > > -- > Ivan >
