Model change events causing a remove/add on collection

[jeffthink]

I'm not actually sure if there's something I'm doing wrong, but for all my Obscura collections, when a change event is fired on the superset collection (e.g. a model's properties have been changed), the model being changed moves to the bottom of the collection.

Backbone.Obscura calls the following code:

function onChange(model) {
  if (this.contains(model)) {
    this._collection.remove(model);
    onAdd.call(this, model);
  }
}

which means that the model is removed, then re-added to the list. Though it looks like it's index is being retained in the onAdd method, so I'm not sure what is going on.

If a model's property changes, does the collection need to remove/add it? Shouldn't the model already have the change?

Any thoughts?

[jmorrell]

Obscura tries to keep everything in the same order unless a sort direction is defined, so if it's working as you describe that's certainly a bug. It seems like there might be some assumptions I'm making in the sorted collection that could be improved.

However, I can't reproduce this behavior. Here's a simple example:

var _ = require('underscore');
var Backbone = require('backbone');
var Obscura = require('backbone.obscura');

var m1 = new Backbone.Model({ id: 1 });
var m2 = new Backbone.Model({ id: 2 });
var m3 = new Backbone.Model({ id: 3 });
var m4 = new Backbone.Model({ id: 4 });
var m5 = new Backbone.Model({ id: 5 });

var collection = new Backbone.Collection([ m1, m2, m3, m4, m5 ]);
var proxy = new Obscura(collection);

console.log(proxy.pluck('id')); // [1, 2, 3, 4, 5]
m1.set({ id: 100 });
console.log(proxy.pluck('id')); // [100, 2, 3, 4, 5]

If you have an example you can give me, that would help immensely in tracking this down. If you can't publish the code publicly feel free to email me.

[mcordingley]

Just want to say that I'm hitting the same issue, though it's more of a breaking issue for me.

I have a Marionette CollectionView that I'm passing an Obscura to. The ItemViews within that CollectionView have some event handlers that alter the model state. They're as simple as clicking a button to toggle a boolean on the model. I also have the view listening to changes on the model to update the view state in response to these. The UI bindings hash on the Marionette ItemView was mysteriously coming back unbound. These never get unbound, so it was especially perplexing, right up until I noticed the view's CID jumping forward.

I can email you the relevant files if you want to see the context on what's happening.

[mcordingley]

Is it possible for the Obscura to trigger a sort without a reset or removing and then re-adding the model? I think we're getting some nasty ripple effects from those operations. Either that, or somehow be able to suppress the remove, add, and reset events, prior to triggering a sort event (where appropriate.)

[mcordingley]

I just sent a pull request to your SortedCollection repository to only remove and add on changes that would alter the model's index in the proxy collection. It's probably only a band-aid on the issue in that there has to be a better way to maintain order in the proxy collection. (I just don't know what that would be.)

[jmorrell]

Sorry for the late response, was out of town without internet.

If you have files that can demonstrate the issue that would be really helpful. Please send it to my email (on my profile). I should have some time tonight to check out your pull request and see about resolving this issue.

[jmorrell]

This should be fixed by: https://github.com/jmorrell/backbone-sorted-collection/pull/3

I've updated Obscura with the changes. Let me know if this doesn't fix your issues.

As I mentioned in an email to @mcordingley, the "correct" way to do this would likely be to not fire any add or remove events, rather just a sort event when things actually move. A smart renderer could then just rearrange DOM nodes on sort for performance.

This issue with this is that Marionette doesn't support sorted collections (https://github.com/marionettejs/backbone.marionette/wiki/Adding-support-for-sorted-collections). Which means that in the common case people would have to do a lot more work to make this work. The performance benefits are minor (very few people are going to have apps so sensitive to two DOM updates instead of moving a DOM node), so I've decided to leave it as it is, at least for now.

If you still see issues, or have another way of solving this, please let me know, but I'm closing this for now :)