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

[Jonathan Giles]

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