Some thoughts about the selection implementation

[joliss]

Hey guys,

I don't really have a pull request to send, but there's an architectural issue we encountered with the current RowSelectionMixin implementation. We implemented our own simple selection handling, so I just wanted to share with you what we did.

We needed to fire events when the user clicks a row. In our case, we wanted to transition the router.

"Oh", you say, "you can just observe selection to transition the router." That's what we tried at first, but it turns out to be problematic: When the router restores the state from the URL, it needs to set the selection too. But then the observer fires and sends an event back to the router. This circularity seems like an anti-pattern. We really want to react only when the user clicks, but not when the router sets a selection. That's what events are for, and we shouldn't be using an observer. But there's no way to get the click (or mousedown) event at the moment.

There's another issue in the code: isSelected is set imperatively through an observer, rather than being defined declaratively as a property. We encountered problems with rows staying selected even though selection had been reset. You could probably pinpoint a bug in the code, but I think it's better to fix the fundamental approach.

So what we did to fix these issues was extremely simple:

1. Instead of using RowSelectionMixin, we handle mouseDown ourselves and fire off an event to the router, which in turns sets tableController.selection upon entering the route. (We didn't even want de-selecting.)
2. Define isSelected declaratively to highlight rows. We define it on the row view, not the row object.

App.TableRowView = Ember.Table.TableRow.extend
  # Upstream uses row.isSelected. That's bad, because `row` doesn't have a
  # controller to compare the current selection. Let's have the isSelected
  # property on the view instead.
  classNameBindings: ['row.isActive:active', 'isSelected:selected']

  isSelected: (->
    record = @get('content.content')
    record? and record == @get('controller.selection')
  ).property('content.content', 'controller.selection')

  mouseDown: ->
    if record = @get('content.content')
      @get('controller.target').send('doStuff', record)

In our TableController subclass, we put tableRowViewClass: 'App.TableRowView'. And that (plus router code) is all we needed to get selections working.

So I suspect we might be able to simplify the RowSelectionMixin a lot.

I also want to suggest that isSelected conceptually really does belong on the view: The currently-selected records are canonically stored in tableController.selection -- I think that's an excellent choice. But then we'd really like to avoid duplicating that state into the rows. Rather, we want to have the view decide whether it should be visually highlighted (implemented as the isSelected property), so that's purely a view concern.

By the same token, isActive might better live in the view as well, since it's purely about visual highlighting. (I'm not sure if the Row objects are used for anything else at all -- maybe they can go away then.)

Anyways, just wanted to share this. Hope it's useful!

[ppong]

@joliss thanks for the suggestion. It seems like certain part of the RowSelectionMixin is problematic. We will see how to best refactor that piece of code. Let me know if you have any further suggestions.

[billy-addepar]

This version of Ember table is no longer supported. If you want to continue discussion, you can open the issue on Ember Table Legacy