Use objectAt to fetch rowValues (cause meta alloc)

[mixonic]

A WIP to solve #726

Although the selection set contains raw values only, some other code (like the summing of selected counts for group selection state in collapse-tree.js) expect that all items in the selection set have a meta allocated.

The non-meta-allocated rows were added where the bare children property of a rowValue was being iterated. Instead with this change the tree is referenced for pulling out the children, meaning the meta cache system is exercised and metas allocated for any selected row.

Open questions:

• Need a few tests. I think @bantic and I have some reasonable ideas about how to test this now.
• What is the performance implication?
• The old code was adding only children of a selected group to the explicitly selected group. It did so as it ascended the parent meta tree.
  • This implementation ends up iterating over already processed nodes (disregarded because the depth is unexpected, but still processed).
  • The old implementation I believe would be incorrect if the data is { name: 0, children: [{ name: 1, children: [{ name: 2 }, { name: 3 }] }, { name: 4, children: [{ name: 5 }] }] }, 0 is selected, and you toggle 2 then 5 will never be explicitly selected.
  • A better implementation IMO would be to walk the parents until you find the final isSelected node in the parent tree, then walk tree nodes until the depth becomes >= the depth of that parent. That would then include all children of children.
• We should review if there are any other uses of "raw" row values without going through the tree. Not every case is wrong, but they are all possibly not allocating a meta where one is needed.
• Final hairbrained idea: The code that expects this meta to be allocated is itself running only a few lines below this. Is there an alternative bookkeeping which could be internal to this function and is somehow faster/better than allocating the meta? Probably not, but maybe.
  • Regardless selection the Set is actually internal, and there is no reason we can't store an array in that with the meta for each value (or have a maintained-in-parallel Set), and that would avoid the several hits to the meta cache in this function.

[bantic]

This looks good to me. I am working up a failing test case that we can use to ensure this fixes the specific scenario I described in #726 (comment)

Regardless selection the Set is actually internal, and there is no reason we can't store an array in that with the meta for each value (or have a maintained-in-parallel Set), and that would avoid the several hits to the meta cache in this function.

I believe this is still covered, but it's worth keeping in mind that, in addition to a user interactively selecting rows, part of the ET API is that the selection property that's passed-in to the component can be modified (without user interaction) and ET needs to react appropriately to times when the selection is changed that way.

[mixonic]

@bantic what I mean is that the actual variable, the Set of selection in this function, is cast to an array before it is passed outside the function, so we can consider the data in that set 100% internal. We simply need to ensure we process it into an array before the function passes it out.

[bantic]

Added a failing test in #731 and I've confirmed that rebasing it onto this branch fixes that test.

I'm also going to take a stab at a test to cover this behavior that we think was buggy before:

The old implementation I believe would be incorrect if the data is { name: 0, children: [{ name: 1, children: [{ name: 2 }, { name: 3 }] }, { name: 4, children: [{ name: 5 }] }] }, 0 is selected, and you toggle 2 then 5 will never be explicitly selected.

[bantic]

Looks good! @mixonic and I reviewed carefully in person and we believe this fixes the known issue.

[mixonic]

@bantic and I reviewed each of the checked off items above to solidify our understanding here. As followup he intends to:

• Document how selection values are passed in to onSelect. We believe the behavior is now consistent but it is a bit odd.
• Close the other issues attempting to resolve unhandled exception for computing width in some applications #726, and cut a beta/next
• Land Test that unrendered (occluded) row selection can be managed #731, which has a test for the issue resolved in this PR.

There is also low-hanging performance work that could be done after this change. Our goal in this patch was just to make it work, but the implementation is a bit naive.