Improved selection events and handling for preserving the selection when the last element gets deleted

[timherby]

Thanks for making this project available. Here are 3 small improvements:

1) A small fix to be compatible with IE8 and lower 2) Improved handling for the selectionChanged event to avoid it firing too many times or occasionally not firing at all (when re-rendering) 3) Preserving the selection by offset when the last item is deleted. The selection is already preserved when an item is deleted in the middle, but if the last item is deleted, then the new last item should be selected.

I have included tests for all the changes (except the IE8 one).

Please let me know if you have any questions.

[dgbeck]

Hi @timherby , thanks for the PR.

1. Makes me wonder why we are using "_.bindAll" to begin with.. seems like this is unnecessary. We will do some testing here to se if we can get rid of this totally, or just bind the methods that need to be bound. Thanks!
2. I think this fix is actually in the dev branch already. I see you also changed some _validateSelectionAndRender calls to just render with this commit, but I think we need that there, in case the current selection is no loger valid after the models are removed. (That is, in case the removed models were selected.)
3. Not sure there is a "right" answer here, but I am learning towards leaving this the way it is, with just not selecting anything if the last item is selected and then deleted. Will sleep on it.

In the future if you could isolate the different issues into separate PRs and pull into dev that would be awesome, makes it easier to sort it all out on this end.

Thanks again.

[timherby]

Hi David,

Thanks for getting back so quickly.

1. If you can do it without a _.bindAll" that sounds even better.
2. I ran my new tests against that branch, and it looks like it's still firing extra events when deselecting and reselecting the same items. It's also not not firing any when the selected item is removed by _validateSelectionAndRender. By switching _validateSelectionAndRender to the render, the selection is saved and restored, which takes care of handling selection which is no longer valid. Doing this through render ensures that all the events are fired properly, and that even in the case where setCollection is called to specify a new collection, the right events are still attached (currently that pathway doesn't call _validateSelectionAndRender).
3. I do see UIs that do it either way. For example, the mac Finder selects the next item as long as the deleted item is not the last item, but selects nothing when deleting the last. Outlook selects the previous message when deleting the last message in a list, Facebook Messaging never selects another item either way. I was going for the Outlook paradigm, but obviously there's a bit of user-preference at play here. Would you be amenable to a couple new settings: preserveSelectionOnDelete (true by default) and preserveSelectionOnDeleteLast (false by default for backwards compatibility)? Doing so would enable all 3 style of UIs?

I'd be happy to re-submit the last 2 as separate PRs off the dev branch so that this is integrated with the ChildViewContainer changes. Please let me know if you agree with the approach suggested above, and I'll re-submit.

Tim

[dgbeck]

Hi Tim,

I think I see what you are saying regarding (2).. _restoreSelection calls _setSelectedItems, which already validates the selection by calling _validateSelection. I'm not sure how events are affected, but if you re-submit this as a separate PR on the dev branch, we'll test it out on this end and integrate it, if the behavior looks good. Thank you!

For (3), following backbone's example we want to keep the API as simple as possible. Most people will not even notice the difference between these two behaviors. (You must have a very good attention to detail!) What about just overriding the _restoreSelection in your view that needs this alternate behavior, and then just doing:

_restoreSelection : function() {
    Backbone.CollectionView.prototype._restoreSelection.apply( this, arguments );
    if( this.selectedItems.length === 0 )
        this.setSelectedModel( this.savedSelection.offset - 1, { by : "offset" } );
}

(I think the code in the original PR that uses collection.length will not behave as intended in the case that there are hidden models - offset only counts visible models.)

[timherby]

Thanks for the suggestion on overriding. While I wasn't able to use override _restoreSelection because of the event logic that I added in the new pull request #24, I was able to get the behavior I wanted by overriding setSelectedModel:

    MyCollectionView.prototype.setSelectedModel = function(newSelectedItem, options) {
      if (options && options.by === 'offset' && newSelectedItem >= this.collection.length) {
        newSelectedItem = this.collection.length - 1;
      }
      return MyCollectionView.__super__.setSelectedModel.call(this, newSelectedItem, options);
    };

So I will close this PR.

As far as the IE8 _.bindAll issue, would you like me to file a separate issue to track that?

[go-oleg]

Tim,

The IE8 _.bindAll issue has been fixed in the dev branch (in commit cd14050da54c0fb9d3d0a5e7cc4ae5b4a08d8f83).

Thanks, Oleg

[timherby]

Thanks Oleg. That's great!