On Thu, 28 Jul 2022 16:37:33 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> modules/javafx.controls/src/main/java/javafx/scene/control/TableView.java
>> line 2829:
>>
>>> 2827: return selectedCellsMap.isSelected(index, -1);
>>> 2828: }
>>> 2829: }
>>
>> wondering why you distinguish between not/ cellSelection? This method is all
>> about row selection (that is the dimension available in base/multiple
>> selection) - shouldn't it behave the same way, independent of whether the
>> second - cell - dimension is turned on? If so, we could simply delegate to
>> super - or not override it at all.
>
> Good question!
> The implementation is lifted from its isSelected(int,Tree/TableColumn)
> sibling with the logic altered for the case of enabled cell selection.
> Let me check if the logic can be delegated to superclass.
Think that I found a case where this implementation violates the invariant
`getSelectedIndices().contains(row) == isSelected(row)` - happens in cell
selection mode when we select the last column and then hide that column:
sm.select(row, column);
assertTrue("sanity: row " + row + "contained in selectedIndices",
sm.getSelectedIndices().contains(row));
assertTrue("sanity: row must be selected" , sm.isSelected(row));
column.setVisible(false);
assertTrue("after hiding column: row " + row + "contained in
selectedIndices", sm.getSelectedIndices().contains(row));
assertTrue("after hiding column: row must be selected" ,
sm.isSelected(row));
the last line fails for cell selection enabled with this implementation and
passes when delegating to super (or not override `isSelected(int)` at all)
Aside: there is a general issue with cell selection not updated on columns
modification (the selection visual is kept on the same column index without
changing selectedCells accordingly - technically due to looping across the
visibleLeafCells vs. all leaf columns. Might or might not be what ux requires,
but the selectedCells must be in sync with the visuals always). Don't remember
if we have it covered in JBS?
The complete test set:
@Test
public void
testRowSelectionAfterSelectAndHideLastColumnMultipleCellEnabled() {
assertRowSelectionAfterSelectAndHideLastColumn(SelectionMode.MULTIPLE,
true);
}
@Test
public void
testRowSelectionAfterSelectAndHideLastColumnMultipleNotCellEnabled() {
assertRowSelectionAfterSelectAndHideLastColumn(SelectionMode.MULTIPLE,
false);
}
@Test
public void testRowSelectionAfterSelectAndHideLastColumnSingleCellEnabled()
{
assertRowSelectionAfterSelectAndHideLastColumn(SelectionMode.SINGLE,
true);
}
@Test
public void
testRowSelectionAfterSelectAndHideLastColumnSingleNotCellEnabled() {
assertRowSelectionAfterSelectAndHideLastColumn(SelectionMode.SINGLE,
false);
}
public void assertRowSelectionAfterSelectAndHideLastColumn(SelectionMode
mode, boolean cellEnabled) {
TableView<Person> table = createPersonTableView();
TableView.TableViewSelectionModel<Person> sm =
table.getSelectionModel();
sm.setCellSelectionEnabled(cellEnabled);
sm.setSelectionMode(mode);
int row = 1;
int col = table.getColumns().size() - 1;
assertRowSelectionAfterSelectAndHideColumn(table, row, col);
}
private void assertRowSelectionAfterSelectAndHideColumn(TableView<Person>
table, int row, int col) {
TableViewSelectionModel<Person> sm = table.getSelectionModel();
TableColumn<Person, ?> column = table.getColumns().get(col);
TablePosition<Person, ?> pos = new TablePosition<>(table, row, column);
sm.select(row, column);
assertTrue("sanity: row " + row + "contained in selectedIndices",
sm.getSelectedIndices().contains(row));
assertTrue("sanity: row must be selected" , sm.isSelected(row));
column.setVisible(false);
assertTrue("after hiding column: row " + row + "contained in
selectedIndices", sm.getSelectedIndices().contains(row));
assertTrue("after hiding column: row must be selected" ,
sm.isSelected(row));
}
/**
* Creates and returns a TableView with Persons and columns for all their
properties.
*/
private TableView<Person> createPersonTableView() {
final ObservableList<Person> data =
FXCollections.observableArrayList(
new Person("Jacob", "Smith", "jacob.sm...@example.com"),
new Person("Isabella", "Johnson",
"isabella.john...@example.com"),
new Person("Ethan", "Williams",
"ethan.willi...@example.com"),
new Person("Emma", "Jones", "emma.jo...@example.com"),
new Person("Michael", "Brown",
"michael.br...@example.com"));
TableView<Person> table = new TableView<>();
table.setItems(data);
TableColumn<Person, String> firstNameCol = new TableColumn("First
Name");
firstNameCol.setCellValueFactory(new PropertyValueFactory<Person,
String>("firstName"));
TableColumn<Person, String> lastNameCol = new TableColumn("Last Name");
lastNameCol.setCellValueFactory(new PropertyValueFactory<Person,
String>("lastName"));
TableColumn<Person, String> emailCol = new TableColumn("Email");
emailCol.setCellValueFactory(new PropertyValueFactory<Person,
String>("email"));
table.getColumns().addAll(firstNameCol, lastNameCol, emailCol);
return table;
}
arrgg .. this site is killing me again - no idea why this comment is duplicated
.. sry
-------------
PR: https://git.openjdk.org/jfx/pull/839
