Re: [PORTAL] Changes and API review?

[Scott O'Bryan]

BTW- There are some API inconsistencies that need to be documented.  The 
beginRequest and endRequest methods on the Global configurator object 
are designed to be run at the beginning and end of each request phase 
(In servlet there is one and in Portlet there are two) where as the 
Configurator services are designed to be run at the beginning and end of 
the "physical" request.  The global configurator handles this scenario 
but the Javadocs are writtern from an implementors standpoint.

Do you want me to submit a patch to fix the Javadocs or do you want to 
do it?

Scott

Scott O'Bryan wrote:
Adam,

Well, you basically implemented one of the solutions I said I didn't 
like earlier, but oh well.  And there are a number of places you need 
to cast.  So the concerns are still valid.

The one question I do have is why does getInstance take in an 
ExternalContext?  I'm assuming it's because your executing the init 
inside of the getInstance object and I'm not sure this is the correct 
place for this.  My hope in all of this was to be able to explicitly 
handle initialization of this object.  Plus, nine times out of ten, 
the ExternalContext object is ignored in the call to this method.

Either way, don't see as I really have the time to argue.  So is it 
acceptable to everyone?

Also, there is no


Adam Winer wrote:
Scott,

OK, well, I just went ahead and implemented what I was trying
to say, to see if I'd run into the problems you're describing.  I 
didn't...
(It's possible I've broken something in portlet land - I only tested
the changes in a servlet environment.)

On 12/21/06, Scott O'Bryan <[EMAIL PROTECTED]> wrote:
Adding getInstance() to the configurator will either force us to 
cast in
a bunch of different places

No, it doesn't.  No casts were required at all, much
less in a bunch of places, and most of the code now
doesn't even have references to the impl class at all.

or to expose the GlobalConfiguratorImpl's
api to the rest of the world (which I don't want to do because they are
applicable ONLY to global configurator.  And it won't lock us into an
API we may have to expand later.

As for simply putting the Boolean on the request map, either I'll have
to make a protected constant on the Configurator interface (which 
has no
bearing on any of the configurators except the global configurator 
so it
shouldn't go into the ancestor) or I add a porotected (isDisabled)
method (which, again, is only applicable to the GlobalConfiguratorImpl
and therfore shouldn't do into it's Ancestor).

See the code checked in, which does not suffer these probems.

I've never been one to include a feature into an interface or a class
that is applicable in only one instance of that class because it
violated basics OO design principals.  The only other option I see here
is to define the constant used to store the boolean in BOTH classes and
hope they remain in sync, but I've never been a big fan of that either.

Again, see the code checked in.

-- Adam