Sorry it took so long to respond. I've looked things over and have some thoughts on the changes.
-) Where are the unit tests? :) I don't always do this properly and there are certainly cases where a feature is covered by integration tests but this is probably one of those situations where it makes sense to have them. See this thread for more: http://www.nabble.com/TAPESTRY-1370%3A-Where-are-the-tests-tf3887096.html#a11019346. -) Why the around advice event connection? I don't see any benefit / use of the functionality this feature provides so am curious as to why it was chosen. Esp. since it costs much more performance wise than normal connections. -) Still am very much against these values going in as listener parameters. Hard coding the sp=value stuff in to the client side is definitely a big no no. More reasons for not doing this are the unexpected results from calling IRequestCycle.getListenerParameters() which will now include these client side values. I can dig around some more but am fairly certain this is not the described behavior of this functionality. It will also result in the ListenerMethodInvoker service doing a lot of unnecessary work for every listener method invocation resulting from an event in the system - regardless of whether people want the extra values or not.. I can go on and on about the possible bad side effects but hopefully this is enough to support my arguments. I'm not saying you can't make them be separate arguments from the BrowserEvent object, I just really think they need to be treated exactly like IRequestCycle/BrowserEvent as far as how ListenerMethodInvokerImpl handles them as optional arguments to any method. Putting the values in BrowserEvent may be easiest to do as far as not adding undo overhead/complication in the ListenerMethodInvokerImpl logic. ...This I'm not as sure about, so maybe you have some more ideas on it. If you can find a way to add listener method arguments as separate objects consisting of these json arguments that won't be unintuitive / conflict with normal listener method arguments (esp when we add the parameters ognl expression to @EventListener) then I'm open to ideas.. -) ComponentEvent.script is a good start - but there is also ElementEvent.script/WidgetEvent.script as well as any/all possible points in the system where a listener method may be invoked asynchronously. You'd have to probably look at various Link components as well. (not to mention the form event connection logic missing in ComponentEvent.script) This may be just you checking stuff in so we can sync so if that's the case then sorry in advance for being anal. There may be some more refactoring I want to do to the javascript code in this part as well once we get closer. The native "arguments" object that should be referencable from any javascript function in any browser would be the preferred way to get at those arguments. If you do decide to stick these extra arguments in the BrowserEvent object then we should probably just make tapestry.buildEventProperties work solely off of the arguments array object. One cool thing about this object is that you can write your connection function like so: function handleEvent(e){ for (var i=0; i < arguments.length; i++){} } And it won't matter how many argument variables like"e" we have defined in our function, they will always exist either way because dojo will really be calling something like <yourfunction>.apply(arguments) - which makes this magically possible. Not a huge deal, I can help here of course... -) buildMethodInterceptionProperties - This is a good start, but I would probably change a few things. I noticed you are using the dojo.json.serialize stuff which is good - but I took a peak at the built dojo.js files we have and don't think we have it in there....So we'lll need to lift that function from dojo and stick it in core + remove the dojo.require for it so that we don't cause any additional IO requests on the client side. This is esp. the case here as IO calls are likely to be included on the majority of pages so this has a big impact on page load times. The rest of the function implementation I think I could help with once we get closer to an agreed impl, it's just small js coding style / shortcut things that aren't super important but I'll probably be anal enough to want to change it at some point. I hope my input doesn't seem too anal / etc.. I'm sure you don't and it's probably redundant to say - but just in case - you always have to have a different frame of mind when working on framework code here and that of normal commercial project code. Usually (but not always) most commercial projects don't allow for the infinite little cycles of perfection and obsession over code but that is not the case here.. We don't have to compromise / work around things for any schedules and such. If we want perfection always this is the easiest place to get it because there is ultimately no one who will tell us what to do but ourselves. There is no such thing as good enough/probably not here. ... Again, please don't take the above the wrong way.. I'll only say it just this once but also have to remind myself of that constantly because I do get lazy here sometimes and must try to stay vigilant and keep things up to standard. There, I hope that's enough initial input. :) On 6/30/07, [EMAIL PROTECTED] <[EMAIL PROTECTED]> wrote:
Author: mschulte Date: Sat Jun 30 14:54:21 2007 New Revision: 552217 URL: http://svn.apache.org/viewvc?view=rev&rev=552217 Log: Tentative fix for TAPESTRY-1202
-- Jesse Kuhnert Tapestry/Dojo team member/developer Open source based consulting work centered around dojo/tapestry/tacos/hivemind. http://blog.opencomponentry.com
