Passing _scope to a callback as argument. Updated test suit. #51

Merged
merged 1 commit into from Aug 14, 2012

Conversation

Projects
None yet
3 participants
Contributor

okonet commented Aug 14, 2012

Removed setTimeout from setScope since it's unreliable and breaks tests. Instead passing _scope to a callback as argument to ensure it remains the same by the time of execution. Fixes #48

@okonet okonet Removed setTimeout from setScope since it's unreliable and breaks tes…
…ts. Instead passing _scope to a callback as argument to ensure it remains the same by the time of execution. Fixes #48
6f4f667

@madrobby madrobby added a commit that referenced this pull request Aug 14, 2012

@madrobby madrobby Merge pull request #51 from okonet/master
Passing _scope to a callback as argument. Updated test suit.
35ce98c

@madrobby madrobby merged commit 35ce98c into madrobby:master Aug 14, 2012

Owner

madrobby commented Aug 14, 2012

Nice, that looks like a better solution! :)

Thomas
http://script.aculo.us/thomas
http://slash7.com/company

On Aug 14, 2012, at 5:38 AM, Andrey Okonetchnikov notifications@github.com wrote:

Removed setTimeout from setScope since it's unreliable and breaks tests. Instead passing _scope to a callback as argument to ensure it remains the same by the time of execution. Fixes #48

You can merge this Pull Request by running:

git pull https://github.com/okonet/keymaster master
Or view, comment on, or merge it at:

#51

Commit Summary

Removed setTimeout from setScope since it's unreliable and breaks tes…
File Changes

M keymaster.js (8)
M test/keymaster.html (125)
Patch Links

https://github.com/madrobby/keymaster/pull/51.patch
https://github.com/madrobby/keymaster/pull/51.diff

Reply to this email directly or view it on GitHub.

Contributor

nwinkler commented Sep 3, 2013

Sorry to get back to this after such a long time, but this change makes it impossible to change the scope for the current call from the filter() function. Since the _scope is passed as an argument to the dispatch() function, calling setScope() from within the filter() function doesn't have any effect on the current event. To make things worse, it changes the scope for the next event.

See #96 for a proposed fix, although it seems to pretty much revert your change.

The documentation states that you can change the scope from within the filter() function, but this seems to be broken with this PR.

Should we provide a way to allow both? In your change, you're basically preventing a call to setScope() to have any effect on the current call - but for the use case described in #95 you actually want to change the scope for the current call.

I'll try to create a pull request to fix this.

Owner

madrobby commented Sep 3, 2013

I've merged #96 which seems a good solution (locks the scope in before handlers lock in, but you can change it in filter).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment