> First of all, for all you joining late to the event discussion from
> several threads back; there's an Event patch [1] sitting in Trac for some
> time now that handles a lot. This and Colin's patch [2] don't overlap in
> functionality as much as in code; some clever merging will be required to
> take the best out of both patches. We had internal discussion about creating
> an event SVN branch in which these changes would be made - it may happen
> soon, depending on the people in charge of the repository.

Currently, your patch doesn't feel exactly "Prototypish".

First of all, I don't like splitting what we have into 2 things -- a
> namespace and a class -- when one can reside in another (eg. an event
> related class should reside in the Event namespace).

I broke it after Tobias' suggestion on the original thread, but it could be
merged back into the Event namespace easily enough using something like
Event.Group. Personally, I like the idea of the cache being a separate class
since the Event class seems to deal almost entirely with making things
cross-browser compatible and caching is somewhat of an add-on.

> Second, I don't like breaking chaining of observe().

I does *not* break Element.Methods.observe. I think this is the chaining you
are concerned about and it is completely intact. The chaining my latest
patch *does* break is just the chaining that I added in a previous patch
where each function returned a reference to the EventCache object so you
could chain multiple observes with different arguments. No functionality
from the original prototype is being lost here. However, there are some new
capabilities that can't be utilized when using the
Element.Methods.observechaining, specifically the return value as a
cache key, and the ability to
add the observer to a different cache. The latter could be remedied by
adding a new function like
This isn't a big deal to me but it's your call.

Third, it has too much "private" properties (the underscore ones) and I
> can't stop thinking this featureset should take way *less* code.

 All of the private properties are there for good reasons, either for
performance or to avoid redundancy. The only one that could be removed
without loss of functionality is the _observe one, but I put this in there
to achieve the separation of event handling and caching. I.e. if you wanted
to create your own caching model or not use caching at all you could use the
_observe and _stopObserving methods. Maybe for that reason these should be
private, but instead be named after the W3C standard? Event.addEventListenerand

This doesn't mean I want you to "fix" your patch. No, it should stay the way
> it is now, but the ticket could stand tests. No, not unit tests (we're still
> figuring out how to unit-test events), but *functional* tests (the next best
> thing). Let the HTML document in [1] serve as the base for your tests. Keep
> em simple; there doesn't have to be much, just showing off the functionality
> and that it really works.

I'll get to running and writing tests as soon as I can (which may not be
very soon). In my unofficial tests (real world usage), they've been working
wonderfully. I could actually "feel" a speed boost in my app that builds and
destroys large trees of complex nodes, particularly when destroying. If the
patch were accepted and the S.A.U widgets were patched to take advantage of
the new cache model, the performance improvement would be even greater. It
mainly comes from not having to do a *linear search* on a huge array for the
cached records on stopObserving calls.

I'll read through your patch code again and see what can be merged between
the two as soon as the Trac system lets me download files... I have been
getting errors like crazy.


You received this message because you are subscribed to the Google Groups 
"Prototype: Core" group.
To post to this group, send email to prototype-core@googlegroups.com
To unsubscribe from this group, send email to [EMAIL PROTECTED]
For more options, visit this group at 

Reply via email to