documentcloud/backbone — pull request #1562 by tgriesser (2012-08-20 04:06:20+00:00)
Title: Fix for bug #1478, unexpected change events when silent:true

There are several pieces that are causing the issues mentioned [in the bug here](https://github.com/documentcloud/backbone/issues/1478), which boil down to the fact that not all of the change flags are properly removed when `{silent:true}`, and also that `this._previousAttributes` is not accurate when inside of a nested change.

The rules of `change` events order aren't entirely defined, but generally speaking when a change is triggered, the changes for the individual items are triggered first, (i.e. `change:x`, `change:y`), followed by the overall `change` event. During these change blocks, `this._silent` flags may build up which are inaccurate when checking the current value against `this._previousAttributes`, rather than the value of the newly changed item.

I'm thinking that adding the following lines will help with this for the majority of cases, without much overhead:

1. `if (!changing) delete this._silent[attr];` - the silent attribute needs to be deleted if it was previously set (e.g.) 'A' ---> 'B' (silent) ---> 'C' (silent) ----> 'A' should not trigger a change. Special cases for silent apply when the set takes place within a nested loop #3.

2. `delete changes[attr];` - similar to the above, because `changes` is potentially a reference to `options.changes`, it should also be removed in all cases if there isn't a change to trigger for that item.

3. `if (changing != void 0 && _.isEqual(now[attr], changing[attr])) delete this._silent[attr];`

This one is a little trickier. Because `this._previousAttributes` isn't a cloned copy of `this.attributes` until after the full main loop takes place, we need some way of knowing whether the set item is new, or whether it would have the same value of what will eventually be set as the model's attributes. For this, in the `change:` function, `this._changing` is changed from a boolean to either `{}` (during a change) or `undefined` when a change isn't taking place, and for each item the set value is assigned to `this._changing`, so it can be checked against `now[attr]`.

This probably doesn't account for every single edge case of nested changes inside changes. However, it does fix the bug in question, pass for all of the current tests, and adds a few other test cases which are logical but would not pass with the current implementation. It might also be good to determine a clear guide as to what can be expected to be true during event callbacks, or what the limitations are for nested callbacks (particularly nested loud callbacks, which IMO should be discouraged, as their behavior is undefined and potentially lead to infinite loops).