On Wed, Sep 3, 2008 at 3:39 AM, Simon Kitching <[EMAIL PROTECTED]> wrote:
> Hi Leonardo, > > Leonardo Uribe schrieb: > >> >> >> On Wed, Sep 3, 2008 at 2:38 AM, Simon Kitching <[EMAIL PROTECTED]<mailto: >> [EMAIL PROTECTED]>> wrote: >> >> Sorry, but I must vote -1 on this. >> >> Most of the issues I raised earlier about the ExtensionsFilter >> refactoring have still not been addressed. >> >> One change has been made: the ExtensionsFilter now does actually >> do the work, and the FacesContextFactory does not wrap the object >> in that case. >> >> >> I believe that you didn't see the latest code. Please check it. >> > > Sorry, I don't understand what you mean. > > To rephrase what I said above: > * I suggested a change to re-enable the ExtensionsFilter > * The change was made > * This is good > > >> >> But the rest of the concerns I had still exist. In particular, >> there is a large amount of new code that exists to parse the >> web.xml file looking for config properties - and I don't see why >> any of this code should exist. I had thought that an earlier email >> by you (Leonardo) meant that you agreed and would change this to >> just looking for servlet init parameters via the normal APIs. >> >> >> Check the code please. >> > I did, right before writing the email :-) > >> >> >> In particular, I think we should delete the ExtensionsFilterConfig >> class completely. >> >> >> I'm open to suggestions, but no alternative has been put on the table. >> > > I did make a suggestion. And I thought you agreed. > > The suggestion was for the TomahawkFacesContext* classes to simply use > ServletContext.getInitParameter -- for servlet config > PortletContext.getInitParameter -- for portlet config > > As I pointed out earlier, we *do* need to be backwards-compatible for code > that still uses ExtensionsFilter. But because the old filter code has been > restored, this continues to work as before. > > For new code, the config params can just be configured as global options > rather than as options within a filter-mapping. Because the ability to use > tomahawk "extensions" functionality without a filter-mapping is new, there > is no backwards-compatibility requirement. > Ok, I understand the missing point. There are some config init params for use TomahawkFacesContextWrapper or not (this was discussed and solved). But it could be good to have config params for set the values used by TomahawkFacesContextWrapper for handle the multipart request case (what you are proposing in a difficult to understand way). One problem is do any cast to PortletContext to get the init param (any cast to PortletContext or any portlet class creates a requeriment portlet api when loading this class, so introduce ClassNotFoundExceptions on servlet environments). I believe it is possible to do this using reflection (or create a separate class that has the portlet api code, so it is only called when the app is on portlet environment). Suggestions, patches and any help are welcome (if you want it do it yourself!). > > > >> >> Minor points: >> * Why is new class AbstractAttributeMap in the "servlet" package? >> * Should AbstractAttributeMap be named "_AbstractAttributeMap" >> instead, ie is it part of tomahawk's "stable public api" or just >> an internal implementation detail? >> * New classes should have @since annotations >> * etc >> >> Any of this points has been proposed before. The problem is if the points >> are blocker or not for a release. >> > > Another interesting question would be: what other new classes have been > added? > A clirr report would be useful here... If you can do a list of things to do for release tomahawk it could be good (and please be more accurate. It is difficult to guess ;) ). Then we can discuss what it is blocker or not. regards Leonardo Uribe > > Regards, > Simon > >
