Adjusting add/remove event handlers to not re-render entire collectionView

[elliottjohnson]

I've refactored the code that creates and adds a model's view to the collectionView into the private function _renderModelView.

I've also added the method to look up a model's view in the viewManager and remove it.

These two methods are used by the listeners on collection.add and collection.remove to update the collectionView without a full re-render.

This should hopefully address the issue discussed here: https://github.com/rotundasoftware/backbone.collectionView/issues/32

I've tested against sorted collectionView's and the collections are indeed sorted properly.

If you have any questions or suggestions I'm all ears.

-Elliott

[elliottjohnson]

I've adjusted the code to properly index based upon options provided to Backbone.collection.add() as well as handling the addition of an array of models, but I've noticed what I'd consider a bug with how the add event passes arguments to the listeners.

It seems that when the event handler gets the added model(s), it always receives a single model. In the case of wanting to insert an array of models into the Collection at a certain point, the event handler does not receive the array of models, but instead fires once for each model as they're added (I assume as they are added).

In the case of inserting them into the view, I'd think we'd want to insert them in the order received at the position provided by the options. The current behavior is that each model "pushes" into the position, so the order ends up being reversed.

I'm interested to know what you think about this behavior and what you suggest I do?

[dgbeck]

Sounds like a tough problem to work around. I wonder can you find any discussion in backbone repo or on stack overflow, etc. regarding the issue?

Seems like it must be a problem in other frameworks as well like Marionette. It looks like same thing would happen, from a quick glance at the collection view code.

Marionette.CollectionView = Marionette.View.extend({
  // used as the prefix for item view events
  // that are forwarded through the collectionview
  itemViewEventPrefix: "itemview",

  // constructor
  constructor: function(options){
    this._initChildViewStorage();

    Marionette.View.prototype.constructor.apply(this, slice(arguments));

    this._initialEvents();
  },

  // Configured the initial events that the collection view
  // binds to. Override this method to prevent the initial
  // events, or to add your own initial events.
  _initialEvents: function(){
    if (this.collection){
      this.listenTo(this.collection, "add", this.addChildView, this);
      this.listenTo(this.collection, "remove", this.removeItemView, this);
      this.listenTo(this.collection, "reset", this.render, this);
    }
  },

  // Handle a child item added to the collection
  addChildView: function(item, collection, options){
    this.closeEmptyView();
    var ItemView = this.getItemView(item);
    var index = this.collection.indexOf(item);
    this.addItemView(item, ItemView, index);
  },

  // Override from `Marionette.View` to guarantee the `onShow` method
  // of child views is called.
  onShowCalled: function(){
    this.children.each(function(child){
      Marionette.triggerMethod.call(child, "show");
    });
  },

  // Internal method to trigger the before render callbacks
  // and events
  triggerBeforeRender: function(){
    this.triggerMethod("before:render", this);
    this.triggerMethod("collection:before:render", this);
  },

  // Internal method to trigger the rendered callbacks and
  // events
  triggerRendered: function(){
    this.triggerMethod("render", this);
    this.triggerMethod("collection:rendered", this);
  },

  // Render the collection of items. Override this method to
  // provide your own implementation of a render function for
  // the collection view.
  render: function(){
    this.isClosed = false;
    this.triggerBeforeRender();
    this._renderChildren();
    this.triggerRendered();
    return this;
  },

  // Internal method. Separated so that CompositeView can have
  // more control over events being triggered, around the rendering
  // process
  _renderChildren: function(){
    this.closeEmptyView();
    this.closeChildren();

    if (this.collection && this.collection.length > 0) {
      this.showCollection();
    } else {
      this.showEmptyView();
    }
  },

  // Internal method to loop through each item in the
  // collection view and show it
  showCollection: function(){
    var ItemView;
    this.collection.each(function(item, index){
      ItemView = this.getItemView(item);
      this.addItemView(item, ItemView, index);
    }, this);
  },

  // Internal method to show an empty view in place of
  // a collection of item views, when the collection is
  // empty
  showEmptyView: function(){
    var EmptyView = Marionette.getOption(this, "emptyView");

    if (EmptyView && !this._showingEmptyView){
      this._showingEmptyView = true;
      var model = new Backbone.Model();
      this.addItemView(model, EmptyView, 0);
    }
  },

  // Internal method to close an existing emptyView instance
  // if one exists. Called when a collection view has been
  // rendered empty, and then an item is added to the collection.
  closeEmptyView: function(){
    if (this._showingEmptyView){
      this.closeChildren();
      delete this._showingEmptyView;
    }
  },

  // Retrieve the itemView type, either from `this.options.itemView`
  // or from the `itemView` in the object definition. The "options"
  // takes precedence.
  getItemView: function(item){
    var itemView = Marionette.getOption(this, "itemView");

    if (!itemView){
      throwError("An `itemView` must be specified", "NoItemViewError");
    }

    return itemView;
  },

  // Render the child item's view and add it to the
  // HTML for the collection view.
  addItemView: function(item, ItemView, index){
    // get the itemViewOptions if any were specified
    var itemViewOptions = Marionette.getOption(this, "itemViewOptions");
    if (_.isFunction(itemViewOptions)){
      itemViewOptions = itemViewOptions.call(this, item, index);
    }

    // build the view 
    var view = this.buildItemView(item, ItemView, itemViewOptions);

    // set up the child view event forwarding
    this.addChildViewEventForwarding(view);

    // this view is about to be added
    this.triggerMethod("before:item:added", view);

    // Store the child view itself so we can properly
    // remove and/or close it later
    this.children.add(view);

    // Render it and show it
    this.renderItemView(view, index);

    // call the "show" method if the collection view
    // has already been shown
    if (this._isShown){
      Marionette.triggerMethod.call(view, "show");
    }

    // this view was added
    this.triggerMethod("after:item:added", view);
  },

  // Set up the child view event forwarding. Uses an "itemview:"
  // prefix in front of all forwarded events.
  addChildViewEventForwarding: function(view){
    var prefix = Marionette.getOption(this, "itemViewEventPrefix");

    // Forward all child item view events through the parent,
    // prepending "itemview:" to the event name
    this.listenTo(view, "all", function(){
      var args = slice(arguments);
      args[0] = prefix + ":" + args[0];
      args.splice(1, 0, view);

      Marionette.triggerMethod.apply(this, args);
    }, this);
  },

  // render the item view
  renderItemView: function(view, index) {
    view.render();
    this.appendHtml(this, view, index);
  },

  // Build an `itemView` for every model in the collection.
  buildItemView: function(item, ItemViewType, itemViewOptions){
    var options = _.extend({model: item}, itemViewOptions);
    return new ItemViewType(options);
  },

  // get the child view by item it holds, and remove it
  removeItemView: function(item){
    var view = this.children.findByModel(item);
    this.removeChildView(view);
    this.checkEmpty();
  },

  // Remove the child view and close it
  removeChildView: function(view){

    // shut down the child view properly,
    // including events that the collection has from it
    if (view){
      this.stopListening(view);

      // call 'close' or 'remove', depending on which is found
      if (view.close) { view.close(); }
      else if (view.remove) { view.remove(); }

      this.children.remove(view);
    }

    this.triggerMethod("item:removed", view);
  },

  // helper to show the empty view if the collection is empty
  checkEmpty: function() {
    // check if we're empty now, and if we are, show the
    // empty view
    if (!this.collection || this.collection.length === 0){
      this.showEmptyView();
    }
  },

  // Append the HTML to the collection's `el`.
  // Override this method to do something other
  // then `.append`.
  appendHtml: function(collectionView, itemView, index){
    collectionView.$el.append(itemView.el);
  },

  // Internal method to set up the `children` object for
  // storing all of the child views
  _initChildViewStorage: function(){
    this.children = new Backbone.ChildViewContainer();
  },

  // Handle cleanup and other closing needs for
  // the collection of views.
  close: function(){
    if (this.isClosed){ return; }

    this.triggerMethod("collection:before:close");
    this.closeChildren();
    this.triggerMethod("collection:closed");

    Marionette.View.prototype.close.apply(this, slice(arguments));
  },

  // Close the child views that this collection view
  // is holding on to, if any
  closeChildren: function(){
    this.children.each(function(child){
      this.removeChildView(child);
    }, this);
    this.checkEmpty();
  }
});

[elliottjohnson]

I see. I've done a bit more research on the topic and have seen _.debounce suggested on stackoverflow and https://github.com/jashkenas/backbone/issues/2617 . In our case we don't mind how things are rendered, but actually do care about tracking and incrementing the option.at argument across listen events.

I've also noticed some regressions when running the qunit tests, so I'll also spend some time getting all tests to pass and maybe add some more if needed.

I'm going to really busy in the next few days, so I think Thursday might be the next day I have time to come back to this. In the mean time I'll read the backbone source for the 'add' event and try to think up a clean way to fix the above issues.

[elliottjohnson]

While doing other things the idea of looking to the collection for positioning came to me. Pretty obvious and looking now the Marionette code you posted above does the same thing.

I sacrificed some study time to get it done and tested... works as expected. I also corrected the re-rendering by sorted collections.

Thursday I'm going to put time towards getting all the qunit tests functioning.

[dgbeck]

Awesome!

[dgbeck]

Also like we can abstract out a getContainerEl method, since we use this several times.. and I think we gotta use it in the add event handler as well

var containerEl = (this._isRenderedAsTable()) ? this.$el.find( "> tbody" ) : this.$el;

[elliottjohnson]

I think I've covered all the bases in the above comments. Tomorrow I'll continue to follow up with working on qunit tests.

[elliottjohnson]

I've made a few adjustments to get things to function with the test suite.

I'm really impressed at how complete it is. Found a few edge cases I outlined in the commit message and had to really think about how to deal with the situation of removing a selected model.

I'm through the thick of it, getting tests 1 - 38 working, but it sticks at 39 and it's late, so I'll look more in the morning. Hopefully it's something trivial :)

[elliottjohnson]

I've addressed the last four tests, fixed how empty-list-captions are potentially removed during an add event, did a minor revision bump since it's a feature with backwards compatibility, and added a quick changelog entry.

Looked over the documentation and don't believe there is anything to change.

Unless there is something else, I think this is close to being finished.

[elliottjohnson]

Closing to reopen against rotundasoftware's dev branch.