New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update the binding count correctly after calling removeEvent #1527

Merged
merged 1 commit into from Mar 17, 2015

Conversation

Projects
None yet
3 participants
@akagomez
Contributor

akagomez commented Mar 17, 2015

Closes #1490.

daffl added a commit that referenced this pull request Mar 17, 2015

Merge pull request #1527 from bitovi/unbinding-bindings-count-1490
Update the binding count correctly after calling removeEvent

@daffl daffl merged commit 6387aaf into master Mar 17, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@daffl daffl deleted the unbinding-bindings-count-1490 branch Mar 17, 2015

this._bindings--;
// Subtract the difference in the number of handlers bound to this
// event before/after removeEvent
this._bindings = this._bindings - (handlerCount - handlers.length);
}

This comment has been minimized.

@justinbmeyer

justinbmeyer Mar 18, 2015

Contributor

maybe I'm missing something, but what if someone passes a handler? Not all events for that handler will be removed.

@justinbmeyer

justinbmeyer Mar 18, 2015

Contributor

maybe I'm missing something, but what if someone passes a handler? Not all events for that handler will be removed.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Mar 18, 2015

Contributor

@daffl @akagomez I think there's a bug here. But maybe I'm misunderstanding. What happens if I do:

        var map = new can.Map({
            test: 1
        });

        map.on('test', can.noop);
                map.on('test', function(){});
        equal(map._bindings, 2, 'The number of bindings is correct');

        map.off('test', can.noop);
        equal(map._bindings, 1, 'The number of bindings is still correct');
Contributor

justinbmeyer commented Mar 18, 2015

@daffl @akagomez I think there's a bug here. But maybe I'm misunderstanding. What happens if I do:

        var map = new can.Map({
            test: 1
        });

        map.on('test', can.noop);
                map.on('test', function(){});
        equal(map._bindings, 2, 'The number of bindings is correct');

        map.off('test', can.noop);
        equal(map._bindings, 1, 'The number of bindings is still correct');
@akagomez

This comment has been minimized.

Show comment
Hide comment
@akagomez

akagomez Mar 18, 2015

Contributor

@justinbmeyer The test you provided passes: 1917e59

If I remember correctly, this._bindEvents is organized like this:

{ 
  firstName: [
    {
      name: 'firstName', 
      handler: fn1
    }
  ], 
  lastName: [
    {
      name: 'lastName',
      handler: fn2
    },
    {
      name: 'lastName',
      handler: fn3
    }
  ]
}

Let's assume map.off('lastName', fn3); is called.

By getting the length of this._bindEvents.lastName before and after can.removeEvent we know how many event handler were removed for that eventName, and handler/validate method combo.

Unless I'm wrong in assuming that this._bindings is meant to cache the number of eventName/handler pairs there are in this._bindEvents, I can't see how this PR introduces a bug.

Contributor

akagomez commented Mar 18, 2015

@justinbmeyer The test you provided passes: 1917e59

If I remember correctly, this._bindEvents is organized like this:

{ 
  firstName: [
    {
      name: 'firstName', 
      handler: fn1
    }
  ], 
  lastName: [
    {
      name: 'lastName',
      handler: fn2
    },
    {
      name: 'lastName',
      handler: fn3
    }
  ]
}

Let's assume map.off('lastName', fn3); is called.

By getting the length of this._bindEvents.lastName before and after can.removeEvent we know how many event handler were removed for that eventName, and handler/validate method combo.

Unless I'm wrong in assuming that this._bindings is meant to cache the number of eventName/handler pairs there are in this._bindEvents, I can't see how this PR introduces a bug.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Mar 18, 2015

Contributor

yeah, you're right. nice work.

Contributor

justinbmeyer commented Mar 18, 2015

yeah, you're right. nice work.

@daffl daffl added this to the 2.2.1 milestone Mar 24, 2015

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