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

[Johan Vos]

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>

> 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
>
>
>