Thanks Jesse, that was really just an initial commit to see whether you
really dislike the approach. And I think, I got your point now. I have
really no problem with that.
2007/7/1, Jesse Kuhnert <[EMAIL PROTECTED]>:
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
.
will do.
-) 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.
I did it for invocation.arguments. But as you explain below, there's a much
better way. So, it'll go away.
-) 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.
Yep, I'll do it your way
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...
very good pointer. I really start to look forward getting into JS/Dojo a bit
more ...
-) 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.
Yes, sorry to make you repeat that point, it's on my list. I am using
FireBug, and be assured: Once we agree on the basic impl, which, I think,
we're very close to, I do have the ambitions to make it as fast as possible.
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.
no problem with that, of course.
I hope my input doesn't seem too anal / etc.. I'm sure you don't and it's
it's barely anal enough! No, seriously. I do this because I enjoy
discussions like this, even and especially, when I'm the one learning most
of it.
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
--
Marcus Schulte
http://marcus-schulte.blogspot.com
