geronimo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jakob Korherr <jakob.korh...@gmail.com>
Subject Re: ordering tck failures in geronimo
Date Fri, 10 Dec 2010 15:54:30 GMT
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 <lu4242@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.korherr@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 <lu4242@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.korherr@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.korherr@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 <xhhsld@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 <lu4242@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

Mime
View raw message