8235491: Tree/TableView: implementation of isSelected(int) violates contract #839
Conversation
|
👋 Welcome back angorya! A progress list of the required criteria for merging this PR into |
openjdk
bot
changed the title
Webrevs
|
|
Because there is a spec change, and a behavioral change, this needs a CSR. What are the implications of this change on applications? /reviewers 2 |
|
@kevinrushforth |
|
@kevinrushforth has indicated that a compatibility and specification (CSR) request is needed for this pull request. @andy-goryachev-oracle please create a CSR request for issue JDK-8235491 with the correct fix version. This pull request cannot be integrated until the CSR request is approved. |
|
I think the behavior of utility method SelectionModel.isSelected(int) was poorly defined in the context of TableView, and/or incorrect in case of cell selection enabled. People who used TableView.isSelected(int, TableColumn|TableColumnBase) will be ok, but those who assumed (arguable incorrect) implementation may need to revisit their code, and only in the case when cell selection is enabled. |
|
OK. When you create the CSR, you can add a note about possible compatibility impact. Seems it should be low. |
|
@kleopatra Would you have time / interest to review this? |
|
Kevin: |
|
There was a problem hiding this comment.
- I see that you have made below change for cell selection mode -
-
Before :
isSelected(int)method used to returnTrueonly if all of the cells in that row were selected. -
After :
isSelected(int)method returnsTrueif any of the cells in that row are selected.Now, if someone wants to know - while in Cell selection mode, whether all of the cells in a particular row are selected? then they must use another already existing API
isSelected(int row, TableColumn<S,?> column)withcolumnparameter asnull.
This is important and should be captured somewhere as recommendation.
- Similar change needs to be made for
TreeTableViewas well - I have listed a minor comment on coding style
| @Override | ||
| public boolean isSelected(int index) { | ||
| final boolean isCellSelectionEnabled = isCellSelectionEnabled(); | ||
| if (isCellSelectionEnabled) { |
There was a problem hiding this comment.
Code Style : Why to create a local variable with the same name as method it gets its value from?
In this case changing to if (isCellSelectionEnabled()) condition check looks simple enough.
There was a problem hiding this comment.
thank you for your comments, Ajit! below are responses, please let me know if you agree or not:
-
javadoc for TableSelectionModel.isSelected(int, TablecolumnBase) already describes the logic:
`
/**- Convenience function which tests whether the given row and column index
- is currently selected in this table instance. If the table control is in its
- 'cell selection' mode (where individual cells can be selected, rather than
- entire rows), and if the column argument is null, this method should return
- true only if all cells in the given row are selected.
- @param row the row
- @param column the column
- @return true if the given row and column index is currently selected in
- this table instance
*/
public abstract boolean isSelected(int row, TableColumnBase<T,?> column);
`
-
TreeTableView.TreeTableViewSelectionModel extends TableSelectionModel, so the changes affect both.
-
good point, fixed.
There was a problem hiding this comment.
1. javadoc for TableSelectionModel.isSelected(int, TablecolumnBase) already describes the logic:
Yes. I am aware of this documentation. I was thinking of making it clear for the users if they see their application breaks due to this change. I see that you have captured this in "Compatibility Impact" section of the CSR.
2. TreeTableView.TreeTableViewSelectionModel extends TableSelectionModel, so the changes affect both.
I see that there are overridden methods public boolean isSelected(int index) and public boolean isSelected(int row, TableColumnBase<TreeItem<S>,?> column) in class TreeTableViewArrayListSelectionModel present in TreeTableView.java. Hence, I think that the change that you have made in TableView.java - isSelected(int index) method should also be made in TreeTableView.java - isSelected(int index) as well.
3. good point, fixed.
Thanks!
There was a problem hiding this comment.
- Would you suggest possible clarification please? The javadoc for SelectionModel.isSelected(int) is changed to say
Convenience method to inform if the given index is currently selected
* in this SelectionModel. It will return true when at least one cell
* is selected in the specified row index.
- you are right! fixed.
There was a problem hiding this comment.
Yes. I am aware of this documentation. I was thinking of making it clear for the users if they see their application breaks due to this change.
If any application breaks, it's its own fault: they coded against the implementation of TableXXSelectionModels which was (incorrectly) isSelected(int) == isSelected(int, null).
That might be due to the specification mess, though:
- the old spec on
SelectionModel.isSelected(int)was incorrectly arguing with api on MultipleSelectionModel, that is Is functionally equivalent to calling getSelectedIndices().contains(index) - that invariant actually belongs into
MultipleSelectionModel.isSelected(int)which has no specialized spec - the method is first implemented in the (unfortunately hidden)
MultipleSelectionModelBase.isSelected(int)fulfilling the constraint but also without spec.
To clean this up completely, we could also change the of MultipleSelectionModel and move the old isSelected(int) == getSelectedIndices().contains(int) into the spec of its isSelected. Probably could be done in a follow-up issue (or added to one of the open doc errors around selection models).
What do you think?
There was a problem hiding this comment.
To clean this up completely, we could also change the of MultipleSelectionModel and move the old
isSelected(int) == getSelectedIndices().contains(int)into the spec of its isSelected. Probably could be done in a follow-up issue (or added to one of the open doc errors around selection models).What do you think?
I like this idea. Either filing a new bug or fold into an existing one seems fine.
As you have mentioned the impact of this change & using alternate API in "Compatibility Impact" section of the CSR - we are good! No additional change is required. One minor issue - the description of this PR starts with |
github truncated the title and placed the remainder into PR description, I just left it there. fixed. |
modules/javafx.controls/src/main/java/javafx/scene/control/SelectionModel.java
Outdated
Show resolved
Hide resolved
| @Override | ||
| public boolean isSelected(int index) { | ||
| if (isCellSelectionEnabled()) { | ||
| int columnCount = tableView.getVisibleLeafColumns().size(); | ||
| for (int col = 0; col < columnCount; col++) { | ||
| if (selectedCellsMap.isSelected(index, col)) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| } else { | ||
| return selectedCellsMap.isSelected(index, -1); | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.smith@example.com"),
new Person("Isabella", "Johnson", "isabella.johnson@example.com"),
new Person("Ethan", "Williams", "ethan.williams@example.com"),
new Person("Emma", "Jones", "emma.jones@example.com"),
new Person("Michael", "Brown", "michael.brown@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
| return false; | ||
| } else { | ||
| return selectedCellsMap.isSelected(index, -1); | ||
| } | ||
| } |
There was a problem hiding this comment.
same comment as tableView - just to not forget :)
| assertFalse(model.isSelected(3)); | ||
| assertTrue(model.isSelected(3)); | ||
| assertEquals(1, model.getSelectedCells().size()); | ||
| } | ||
|
|
There was a problem hiding this comment.
do we have tests isSelected(index) for all combinations of single/multiple/cellSelection? If not, now might be a good time to add them :)
There was a problem hiding this comment.
a very good point!
should definitely cover all the scenarios.
| assertFalse(cells(model), model.isSelected(3, null)); | ||
| assertFalse(model.isSelected(3)); | ||
| assertTrue(model.isSelected(3)); | ||
| assertEquals(1, model.getSelectedCells().size()); | ||
| } |
There was a problem hiding this comment.
same comment as TableView :)
|
forgot: will not be near my IDE until next Monday - then I'll run the tests and see for myself :) |
modules/javafx.controls/src/main/java/javafx/scene/control/TreeTableRow.java
Show resolved
Hide resolved
similar tests (along with that in the bug report) adapted to treeTable should be added to the treeTable test - we should keep them as sync'ed as possible, to not make future maintainers wonder why they aren't included :) |
The tests were added to both TableViewSelectionModelImplTest and TreeTableViewSelectionModelImplTest. |
hmm .. in TreeTableViewSelectionModelImplTest, I don't see any of the testRowSelectionAfterSelectAndHideLastColumnXX nor testSelectRowWhenInSingleCellSelectionModeIsSelected, what am I missing? |
you are right... I am so sorry. missed this one while cherry picking from another branch. will fix it soon. would it be possible to review #858 so there is no need to cherry pick, please? |
no problem, shit happens :) |
|
I see a very good progress on this PR. Thanks @andy-goryachev-oracle and @kleopatra ! |
|
updated PR description. thank you, @aghaisas |
There was a problem hiding this comment.
This all looks good to me now. I left one comment on the CSR, and I'll review that once that is done.
This will give @aghaisas a chance to review as well.
|
@andy-goryachev-oracle This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 10 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@aghaisas, @kleopatra, @kevinrushforth) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
|
hmm .. isn't this ready for integration? CSR being approved was the last step, or not? |
|
/integrate |
|
@andy-goryachev-oracle |
|
/sponsor |
|
Going to push as commit 7cb8d67.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@kevinrushforth @andy-goryachev-oracle Pushed as commit 7cb8d67. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
getSelectedIndices().contains(index)."NOTE: proposed change alters semantics of isSelected(int) method (in the right direction, in my opinion).
Progress
Issues
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx pull/839/head:pull/839$ git checkout pull/839Update a local copy of the PR:
$ git checkout pull/839$ git pull https://git.openjdk.org/jfx pull/839/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 839View PR using the GUI difftool:
$ git pr show -t 839Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/839.diff