[Prototype-core] Re: More Event Handling Propositions
>
> 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
element.addObserver(this.cache,'click',function(){}).
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
Event.removeEventListener?
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.
Thanks,
Colin
--~--~-~--~~~---~--~~
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
http://groups.google.com/group/prototype-core?hl=en
-~--~~~~--~~--~--~---
[Prototype-core] Re: More Event Handling Propositions
On 2/9/07, Colin Mollenhour <[EMAIL PROTECTED]> wrote: > > > I think this patch has become quite refined and mature by now, I'd love to > get some more feedback from the Core guys... > 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. Colin, we still haven't discussed having your featureset in core, but that doesn't mean it won't happen. 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). Second, I don't like breaking chaining of observe(). Third, it has too much "private" properties (the underscore ones) and I can't stop thinking this featureset should take way *less* code. 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. -- [1] http://dev.rubyonrails.org/ticket/5786#comment:9 [2] http://dev.rubyonrails.org/ticket/7435#comment:1 --~--~-~--~~~---~--~~ 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 http://groups.google.com/group/prototype-core?hl=en -~--~~~~--~~--~--~---
[Prototype-core] Re: More Event Handling Propositions
> I've been doing a lot of research on the subject of Event registration. > It made me question: do we really need to detach listeners in IE on > unload? Unless anyone knows for sure, I plan to test. According to what I've read primarily here: http://javascript.crockford.com/memory/leak.html it is pretty well established that it is necessary. Also, I think we can remove the try-catch blocks > on lines 116 and 125 of your patch because exceptions should never be > generated I put them in there because I thought IE's methods (or at least detachEvent) might throw an exception, but I honestly don't know for sure. Also, if an old browser doesn't support add/removeEventListener or attach/detachEvent then trying to call those functions would throw an exception, but that is a very minor point. About the Event.element patch, I've never used Safari as I don't have access to a Mac so I wouldn't know, but if you've read my recent posts you know how I feel about Safari... ;) It does look like it would make sense to do that if that is the case. I think this patch has become quite refined and mature by now, I'd love to get some more feedback from the Core guys... Thanks, Colin --~--~-~--~~~---~--~~ 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 http://groups.google.com/group/prototype-core?hl=en -~--~~~~--~~--~--~---
[Prototype-core] Re: More Event Handling Propositions
I should point out that made the return value a simple int rather than
an object with a stopObserving function.
It would be quite easy to add, simply change (in EventCache.observe):
return this.cacheIndex;
to:
return {stopObserving: EventCache.stopObserving.bind(this,
this.cacheIndex)};
But, I don't think the overhead of a bind call and the storage of an
object with a function for EACH event observed is justifiable, just to
have the convenience of:
handle.stopObserving();
rather than:
Event.stopObserving(handle);
I seriously deal with cases that use thousands of event observers,
sometimes created in a short period of time, so this API needs to be
as efficient as possible.
As an aside, I just realized that this reply box in Google Groups has
a textarea that is resizable by dragging! Cool..
Colin
--~--~-~--~~~---~--~~
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
http://groups.google.com/group/prototype-core?hl=en
-~--~~~~--~~--~--~---
[Prototype-core] Re: More Event Handling Propositions
> Colin, thanks for pointing out that my observer array will slow things > down in many cases. I really like your implementation, especially the > way you keep a different EventCache object. One thing I wonder, though, > if like you said your function returns an index--an array index could be > problematic, but why not use an object as a cache? Consider the draft > below. I like it! I like it so much, I blended all of your features with my own! See the patch here (updated): http://dev.rubyonrails.org/ticket/7435 I added the "return value is a cache key" feature, added your fixes to make UA checking happen only once (very nice!), and made Event and the caching model completely separate. All of this with complete backwards compatibility. I didn't include your "stopObservingElementEvent" and "stopObservingElement" functions, instead their functionality is included in stopObserving. Colin --~--~-~--~~~---~--~~ 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 http://groups.google.com/group/prototype-core?hl=en -~--~~~~--~~--~--~---
[Prototype-core] Re: More Event Handling Propositions
Ken Snyder wrote:
> Along with all this Event handling discussion, I'd like to throw in my
> ideas for a rewrite of Event.observe et al before I test and submit as a
> patch. My proposed changes allow the traditional Event.stopObserving()
> and simplify some of the current messaging ideas. Please provide feedback.
>
Colin, thanks for pointing out that my observer array will slow things
down in many cases. I really like your implementation, especially the
way you keep a different EventCache object. One thing I wonder, though,
if like you said your function returns an index--an array index could be
problematic, but why not use an object as a cache? Consider the draft
below.
--Ken
Object.extend(Event, {
//...
// the cache of observers
observers: {},
// the current position in our cache
cacheIdx: 0,
// added return of cacheIdx to allow Event.stopObserving(idx)
observe: function( element, event, observer, useCapture ) {
element = $(element);
useCapture = useCapture || false;
element[this._addFnName](this._eventName(event), observer, useCapture);
this.cacheIdx++;
this.observers[this.cacheIdx] = [element, event, observer, useCapture];
return cacheIdx;
},
stopObserving: function( element, event, observer, useCapture ) {
if( typeof arguments[0]=='number' ) {
this._cancel(arguments[0]);
} else {
element = $(element);
useCapture = useCapture || false;
element[this._removeFnName](this._eventName(event), observer,
useCapture);
//
// The current implementation seems not to clear the cache like this
//
for( idx in this.observers ) {
if( this.observers[idx][0]==element &&
this.observers[idx][1]==event &&
this.observers[idx][2]==observer &&
this.observers[idx][3]==useCapture) {
delete this.observers[idx];
break;
}
}
//
//
//
}
},
unloadCache: function() {
for( idx in this.observers ) {
this._cancel(idx);
}
},
//
// New methods
//
// function that actually detaches the event and clears from cache
_cancel: function(idx) {
var handle = this.observers[idx];
try {
handle.element[this._removeFnName](this._eventName(handle.event),
handle.observer, handle.useCapture);
delete this.observers[idx];
} catch(e) {}
},
// New method: cancel all the observers associated with a particular
element for a particular event
stopObservingElementEvent: function( element, event ) {
element = $(element);
for( idx in this.observers ) {
if( this.observers[idx][0]==element && this.observers[idx][1]==event )
this._cancel[idx];
}
},
// New method: cancel all the observers associated with a particular
element
stopObservingElement: function( element ) {
element = $(element);
for( idx in this.observers ) {
if( this.observers[idx][0]==element )
this._cancel[idx];
}
}
};
// cross-browser function and event naming
if( window.addEventListener ) {
Event._addFnName = 'addEventListener';
Event._removeFnName = 'removeEventListener';
if( navigator.appVersion.match(/Konqueror|Safari|KHTML/) )
Event._eventName = function(name) { return name=='keypress' ?
'keydown' : name; };
} else {
Event._eventName = function(name) { return name; };
}
} else if( window.attachEvent ) {
Event._addFnName = 'attachEvent';
Event._removeFnName = 'detachEvent';
Event._eventName = function(name) { return 'on'+name; };
// garbage collect for IE
window.attachEvent('onunload', Event.unloadCache);
}
Using an indexed object as a cache even opens up the possibility of
using my original API suggestion:
...
observe: function( element, event, observer, useCapture ) {
element = $(element);
useCapture = useCapture || false;
element[this._addFnName](this._eventName(event), observer, useCapture);
this.cacheIdx++;
this.observers[this.cacheIdx] = [element, event, observer, useCapture];
return { cancel: function() { Event._cancel(cacheIdx); } };
}
...
stopObserving: function( element, event, observer, useCapture ) {
if( typeof arguments[0]=='object' && arguments[0].cancel ) {
arguments[0].cancel();
} else {
...
--~--~-~--~~~---~--~~
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
http://groups.google.com/group/prototype-core?hl=en
-~--~~~~--~~--~--~---
