Scott, A few points.
First, I said only that initializing with construction is more robust then separating the two, and only that. That's plainly true. Why you want to turn that around to ad hominem generalizations is beyond me and not productive. Second, it certainly is a good point that you now might disable the configurator "too late", which could easily lead to inconsistencies That could be caught in a couple of ways - a good one would be warning the developer on the next request. GlobalConfiguratorImpl could easily handle this on its own, and both make sure that "if you start it, it finishes", and warn (and ignore) on the next configurator invocation if someone has disabled too late. Disabling the configurator is a rare enough requirement (in the number of places that'll want to do it) that this is a sufficient improvement to deal with that legitimate issue - all we need to do is catch anyone that gets it wrong (and avoid misbehaviors when it does happen). Third, you really need to stop imagining introspection is a horribly slow operation. That was true in JDK 1.2, but hardly the case at all now. And introspection *once during initialization*, not even once per request, isn't even remotely a performance issue, and describing it as such is just plain wrong. Fourth, referring to GlobalConfiguratorImpl by name from the API is awful. (We do this for the RequestContextFactoryImpl, and I've always disliked that decision). It totally breaks API-IMPL separation and dependency order. We're talking lesser of two evils here, and it's a judgement call. That said, I'm happy with moving Configurator.getInstance() down to impl (into GlobalConfiguratorImpl), as you're right that there isn't an obvious reason why it would need to be exposed at this point. That gets rid of the need to use any introspection API (Services or otherwise) to access GlobalConfiguratorImpl. Finally, I had little choice *but* to check in code on that branch. It's version controlled, so if it breaks portlets, then svn revert is trivial. But when you repeatedly misrepresent the technical aspects of what I'm proposing, claiming everything I propose is horrific and slow and ugly and will break some ill-defined API that is still to come, how else can I show you what I want??? Now, let's work together to go forwards. My checkin isn't end-of-story, no more than yours was. FWIW, I can't believe that JSR-301 is going to somehow make this API unusable. It will obviously be a different API, but I can't believe that it'll be so different that it'll be completely impossible to use that API such that it can invoke whatever it is that we come up with. -- Adam On 12/22/06, Scott O'Bryan <[EMAIL PROTECTED]> wrote:
Adam, You I must have a different definition of robust. Two of the MANY examples that illustrate my point is as follows:
1. There is no error checking on disableConfiguratorServices method and,
the implementation as it stand now, could force us to break our contract with the configurators. Furthermore, it's not always obvious that something wrong was done. For instance, the following code will not generate an error: FacesContectFactory.getFacesContext(context, request, response, lifecycle); ... Configurator.disableConfiguratorServices(); This will not generate an error yet cleanup (endRequest) will not occur even though beginRequest WAS called (when the FacesContextFactory was created OR when the filter is run). getExternalContext MAY or MAY NOT be wrapped depending on whether or not an external context was retrieved before the disable. The basically leaves the status of externalContext in an unknown state and will not execute the end Request. You eliminated the "reflection" in my latest patch by getting rid of this check. In a public "rhobust" API, this really needs to be enforced. It USED to call an IllegalStateException so it didn't necessarily have to be caught because it should be fairly obvious at the time someone writes their code. 2. You got rid of reflection in the public Configurator class by using Trinidad's Service Loader. This class not ONLY uses reflection, but it also reads a bunch of files and returns an "array" of initialized services. Do we really need to open some file objects and initialize an extra array for no reason? Now it's an interesting idea to be able to replace the global configurator, but first off that wasn't a requirement that I was coding for and second off, we cannot guarantee the "order" that the services files are loaded in. Again this leaves us in an unknown state if we are trying to override the default functionality and is simply using a sledge hammer to drive a nail if we aren't. You were able to eliminate a lot of the casting by not exposing the is initialized method on the global configurator by moving the initialization code inside of "getInstance" method. I kept this separate so that I could adjust to JSR-301 as I need to. Butt the API is now locked in to perform initialization at that stage. This is something that very well could become a problem. And I haven't even taken a look at how you handled the isInRequest functionality that I need to maintain the proper flexibility in the portal case. Still, I really don't have time to argue these points. I'll just have to generate Jira tickets as problems come up and we'll have to change the API's if we need to change them. All I really have to say about the subject is allowing the GlobalConfigurator implementation handles all these issues for free. And using my latest checkins handles most of them, just not as cleanly. Certainly. though, a lot more cleanly then using the "Service Loader". You said that you didn't know if these changes worked in a portal environment, and I'm not real sure I can confirm that within the next week or so. But checking in changes to a "portal compatibility" branch without being able to check them in a Proof of Concept (POC) is really damaging to the project being that Portal compatibility is what this project is all about. I was able to test my stuff as a POC using the Oracle Portal and Bridge, and I'm also aware of the changes that need to be made in the MyFaces Bridge to support these cases. Many of these issues ARE going to be addressed in JSR-301 but my hope it to have the myfaces bridge up to par before that time. My final, and I mean final, comment on this is that it is important to me to have error checking on disableConfiguratorServices. That's from a robust API perspective. From a Portal standpoint, it is far more important for me to have control over the initialization logic and to error out of getInstance when an instance is not available (or return null, I don't care which) then it is for me to have an exposed "getInstance()" in the API and for the thing to return a dummy configurator. Exposing the getInstance made sense when we had the ability to tack additional non-static API's onto the GlobalConfigurator. Since we don't, I can't see ANY reason why someone would need this API outside of Trinidad. The object is simply "not useful" outside of this codebase anymore. Plus it keeps us flexible to add an API later if we need one whereas this current implementation LOCKS us in to providing a plain run-of-the-mill configurator. Lastly, for GOD sake, don't use the service loading system to load this class. The Service loading system is great for making extensions to Trinidad, but it has limitations which stem from the fact that order within these services is not guaranteed. For now I say we simply have the GlobalConfiguratorImpl (in the Impl package) be referenced directly if we need to, and if we have the need to add additional globalConfigurators in the future, we can come up with an API that makes sense. This one makes no sense at all. Essentially I outlined for you the things in my last checkin. You're latest checkin' will lock us into unwanted implementations, implementations that make NO sense what so ever, and implementations that are not robust. As I stated before, though, I don't really have time to argue with you anymore. I have some other projects that I need to work on at the moment. So if you're happy with the API, call a vote and let's get this moved in so I can move forward and we can hopefully have trinidad working inside of a MyFaces Portal environment sometime this century. Scott O'Bryan Adam Winer wrote: > On 12/21/06, Scott O'Bryan <[EMAIL PROTECTED]> 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. > > > Well, I don't get that claim, as I didn't add a single cast in my > patch at all, and removed references to the impl class, so I can't > imagine > how I've made things worse. And I didn't really see any casts that were > already there. > > 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. > > > A create + initialize construct is more robust than a create-and-hope- > it-gets-initialized-by-someone-else construct. > > -- Adam > > > > 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 >> > >> >> >
