[REVIEW REQUEST] RT-33442: isSelected in TableViewSelectionModel is called too many times

[Jeff Martin]

I'm late to this discussion, and I don't see anything wrong with the selectRange() API, but perhaps another option worth considering would be to add a DisableSelectionEvents or CoalesceSelectionEvents attribute.

The only advantage to that I can think of is that it could allow bulk selection of non-contiguous cells which might otherwise also exhibit the slowdown.

jeff


On Oct 21, 2013, at 8:35 AM, Stephen F Northover <steve.x.northover at oracle.com> wrote:

> Hi Johnathan,
> 
> 1) Is it possible to do the optimization without adding API (ugly but safe)?
> 2) Another alternative (ugly) is to add the API but make it return a boolean indicating whether it happened or not.
> 3) It seem really weird to me that you can't convert from Column to index and back again.  What about getColumns()?
> 
> Steve
> 
> On 2013-10-21 5:04 AM, Jonathan Giles wrote:
>> The only reason why we tended to prefer TableColumn* in the API over ints is that the TableColumn approach more clearly defined what columns were meant to be selected (both because you could read directly back from the argument list and because there was never any chance that the int positions could become corrupted if the column order were to ever change in the middle of an operation and the int values would no longer be valid).
>> 
>> In general it is a relatively weak argument (but an argument nonetheless), so I'm not massively concerned one way or the other, except from an API consistency point of view, where the TableColumn approach definitely is more consistent. I figure I'm about to call it a night here, so I'll wait and see what feedback is received over night and will hopefully have the go-ahead from the community to finish this changeset off and push tomorrow :-)
>> -- Jonathan
>> On 21/10/2013 9:53 p.m., Johan Vos wrote:
>>> I would prefer to have the selectRange (int, int, int, int) call. I know TableColumn* is used heavily in the API, but it doesn't sound right to me to use a primitive (int) for a row-selector and an Object for a column-selector.
>>> Having a no-op on the abstract call sounds ok, everybody happy.
>>> 
>>> Leaving the API as it is now is not really an option IMHO, as performance indeed becomes a problem for large tables.
>>> 
>>> - Johan
>>> 
>>> 
>>> 2013/10/20 Jonathan Giles <jonathan.giles at oracle.com <mailto:jonathan.giles at oracle.com>>
>>> 
>>>    On 19/10/2013 8:24 a.m., Jonathan Giles wrote:
>>>    > On 19/10/2013 2:13 a.m., Stephen F Northover wrote:
>>>    >> If it is a noop, will it not break people when the system
>>>    calls it and
>>>    >> it does nothing?
>>>    > Only the abstract TableSelectionModel is a no-op, the
>>>    implementations
>>>    > (one each for TableView and TreeTableView) will both override and
>>>    > provide the implementation that I already have in the patch for
>>>    > TableView. I would rather have the implementation in the base
>>>    class but
>>>    > this is not feasible due to requirements in looking up column
>>>    indices.
>>>    I looked into providing a default implementation in
>>>    TableSelectionModel,
>>>    but the problem is is that there just isn't enough data at this
>>>    level of
>>>    the API.
>>> 
>>>    In short, the API could take two forms:
>>>    selectRange(int minRow, int minColumn, int maxRow, int maxColumn)
>>>    selectRange(int minRow, TableColumnBase minColumn, int maxRow,
>>>    TableColumnBase maxColumn)
>>> 
>>>    Either implementation is possible (although at present I've
>>>    implemented
>>>    the first approach, but for consistency it is probably better we
>>>    use the
>>>    second approach as all other API uses TableColumn* to represent
>>>    columns,
>>>    not ints).
>>> 
>>>    The problem is is that neither approach gives me what I want for a
>>>    default implementation, which would take the following form:
>>>    for (int row = minRow; row <= maxRow; row++) {
>>>        for (int col = minColumn; col <= maxColumn; col++) {
>>>            TableColumnBase column = getColumn(col);
>>>            select(row, column);
>>>        }
>>>    }
>>> 
>>>    With the first API option (all ints), I am lacking the getColumn(..)
>>>    method to convert column indices to TableColumn instances.
>>>    With the second API option, I have no way of converting the two
>>>    TableColumnBase instances back into ints, and then I have the same
>>>    problem as with the first API.
>>> 
>>>    Therefore, my options as far as I can see (and I'm happy to be wrong)
>>>    are to either not introduce this API and keep the performance
>>>    poor (for
>>>    now - hopefully we'll come up with a better solution in time) or to
>>>    introduce one of the two methods above and then have a no-op
>>>    implementation clearly documented in TableSelectionModel (with
>>>    the two
>>>    existing implementations in JavaFX 8.0 both implementing a far
>>>    more high
>>>    performance approach than we have currently).
>>> 
>>>    Thoughts?
>>>    -- Jonathan
>>> 
>>> 
>>> 
>> 
>