Re: [REVIEW REQUEST] RT-33442: isSelected in TableViewSelectionModel is called too many times
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.northo...@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.gi...@oracle.com
mailto:jonathan.gi...@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
Re: [REVIEW REQUEST] RT-33442: isSelected in TableViewSelectionModel is called too many times
I've added an updated patch to RT-33442. From the jira comment included with the patch: 1) selectRange method in TableSelectionModel is now abstract (given that TableSelectionModel is new in 8.0 this is fine). 2) selectRange argument list is now (int, TableColumnBase, int, TableColumnBase) 3) TableViewSelectionModel API is now unfortunately the same (i.e. I can't make the type TableColumn, which is more specific, and requires casting inside the method) 4) I have ported the patch to also apply to TreeTableView. 5) The same problem in 3) also exists for TreeTableView. I'll get a implementation review done by a controls team member, but a +1 on the API choice would be appreciated. -- Jonathan On 22/10/2013 9:43 a.m., Stephen F Northover wrote: TableSelectionModel is new for JDK8 (at least according to the Javadoc). If so, add the method as abstract and put implementations in the concrete subclasses, no? Steve On 2013-10-21 3:15 PM, Jonathan Giles wrote: On 22/10/2013 2:35 a.m., Stephen F Northover wrote: 1) Is it possible to do the optimization without adding API (ugly but safe)? There are aspects of the optimisation that can be done without adding API. However, in my testing this API change is the most important of the changes, so without it there would be some, but much less, benefit of including the other changes. 2) Another alternative (ugly) is to add the API but make it return a boolean indicating whether it happened or not. Sure, I'm fine with this if this is the desired approach. 3) It seem really weird to me that you can't convert from Column to index and back again. What about getColumns()? The reason why this is the case is that TableSelectionModel is control independent, so it does not have the TableView or TreeTableView available to it. If it did these translations would be trivial (and are done all the time). This is why TableSelectionModel is currently a no-op, whereas the actual TableSelectionModel implementations (hidden within TableView and TreeTableView) can do precisely this. -- Jonathan
Re: [REVIEW REQUEST] RT-33442: isSelected in TableViewSelectionModel is called too many times
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. Further, what about discontinuous selections (ie. one item, then an unselected item, then a million selected items in a block). You can do this on native Windows using Shift+Ctrl+Select but I see that this is not supported in the FX table. I haven't included discontinuous selection in this round of performance research, but in general the changes in the RT-33442 changeset will have the same drastic improvement there as well. However, as you note, there is not a full implementation of discontinuous selection implemented in TableView yet, so the point is a little moot at this stage. -- Jonathan
[REVIEW REQUEST] RT-33442: isSelected in TableViewSelectionModel is called too many times
Hi all, I know we're very late in the release, but I'm proposing the addition of one new method to the TableSelectionModel abstract class, which will have implementations provided by TableView and TreeTableView. The method added into TableSelectionModel will be a no-op method by default (to hopefully not break anyone), and I propose it should have the following method signature: public void selectRange(int minRow, int minColumn, int maxRow, int maxColumn) This method will be used internally, and by any interested end user, to signal to the TableView/TreeTableView control to select all cells within the range of (minRow, minColumn) - (maxRow, maxColumn), inclusive. The reason why I want to introduce this is summed up well in RT-33442 and RT-33619. In short, given a big enough table, a shift+click can essentially cause the TableView to hang whilst it selects all the cells within the range. This is basically due to all the selection events overloading the system. The benefit of introducing this method is that I can make the selection essentially an atomic operation, which will result in drastically less event noise. To better understand what I mean, there is a patch attached to RT-33442 (rt33442_2.patch). This patch introduces this change, as well as a number of other implementation changes. In doing so the selection performance in this use case goes from crashing my test application (presumably it would complete, but I give up after five minutes of waiting) to taking under 1 second. There is still more testing to be done, but after applying this patch all controls unit tests continue to pass. Also, as noted in the jira issue, this patch is incomplete as I have not ported the patch to also cover TreeTableView. So, after all that, should we add selectRange(minRow, minColumn, maxRow, maxColumn) to JavaFX 8.0? If so, does anyone have an improved method signature they can recommend? I will of course be writing unit tests to cover this method and testing to ensure there are no regressions as best I can. Thanks, -- Jonathan
