jfx
jfx copied to clipboard
openjdk
8235491: Tree/TableView: implementation of isSelected(int) violates contract
- reword SelectionModel.isSelected(int) javadoc, removing incorrect statement "Is functionally equivalent to calling
getSelectedIndices().contains(index)." - reimplement TableView.TableViewSelectionModel.isSelected(int) method to return true when at least one cell in any column is selected on the given row (was: all columns)
- change selectRowWhenInSingleCellSelectionMode() and selectRowWhenInSingleCellSelectionMode2() in TableViewSelectionModelImplTest to reflect new reality.
NOTE: proposed change alters semantics of isSelected(int) method (in the right direction, in my opinion).
Progress
- [x] Change must not contain extraneous whitespace
- [x] Commit message must refer to an issue
- [ ] Change requires a CSR request to be approved
- [ ] Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)
Issues
- JDK-8235491: Tree/TableView: implementation of isSelected(int) violates contract
- JDK-8290741: Tree/TableView: implementation of isSelected(int) violates contract (CSR)
Reviewing
Using git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx pull/839/head:pull/839
$ git checkout pull/839
Update a local copy of the PR:
$ git checkout pull/839
$ git pull https://git.openjdk.org/jfx pull/839/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 839
View PR using the GUI difftool:
$ git pr show -t 839
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/839.diff
:wave: Welcome back angorya! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.
![bridgekeeper[bot] avatar](https://avatars.githubusercontent.com/u/41768318?v=4 "Published by a pub.dev verified publisher"){kind=small}
Webrevs
- 10: Full - Incremental (9ad93526)
- 09: Full - Incremental (35247bc6)
- 08: Full - Incremental (76466935)
- 07: Full - Incremental (9de5858b)
- 06: Full - Incremental (7fa25883)
- 05: Full - Incremental (ad3c70b9)
- 04: Full - Incremental (6267a0e4)
- 03: Full - Incremental (be4825ab)
- 02: Full - Incremental (fcdf223d)
- 01: Full - Incremental (2b76eddb)
- 00: Full (82f0db72)
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 /csr
@kevinrushforth The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).
@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.
Kevin: Proposed CSR - JDK-8290741 what would be the "correct fix version"?
Kevin: Proposed CSR - JDK-8290741 what would be the "correct fix version"?
openjfx20
- Would you suggest possible clarification please?
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 ...ontract - looks like some typo.
One minor issue - the description of this PR starts with
...ontract- looks like some typo.
github truncated the title and placed the remainder into PR description, I just left it there. fixed.
forgot: will not be near my IDE until next Monday - then I'll run the tests and see for myself :)
@kleopatra : Various scenarios are pretty well covered by the existing tests. Just in case, I've added tests to exercise the model with cell selection enabled.
The doc changes look fine to me, since I agree that further clarification of MultipleSelectionModel.isSelected can be done in a follow-up.
a bit confused about the csr - shouldn't that be focused entirely on SelectionModel.isSelected)? That's where we clarify the contract - all changes to TableXXSelectionModels are implementation changes, fixing their contract violation. So I would expect these not to be mentioned at all in the csr.
BTW: isSelected is not a convenience method - that's already changed here, all mentions of its being so should be removed from the csr as well, IMO :)
@kleopatra : Various scenarios are pretty well covered by the existing tests.
good - we seem to be lucky :)
a bit confused about the csr - shouldn't that be focused entirely on SelectionModel.isSelected)? That's where we clarify the contract - all changes to TableXXSelectionModels are implementation changes, fixing their contract violation. So I would expect these not to be mentioned at all in the csr.
The specification section of the CSR should (and does) list only the spec change to SelectionModel.isSelected. Since there could be an impact to applications that rely on the current behavior, it also seems reasonable to list those in the Solution section (as well as the Compatibility concerns section). I think that the Summary should start by listing the spec change to SelectionModel.isSelected, but it seems useful to also say that the implementation of TableXXSelectionModel is changing as a result.
BTW: isSelected is not a convenience method - that's already changed here, all mentions of its being so should be removed from the csr as well, IMO :)
Agreed.
The specification section of the CSR should (and does) list only the spec change to
SelectionModel.isSelected. Since there could be an impact to applications that rely on the current behavior, it also seems reasonable to list those in the Solution section (as well as the Compatibility concerns section). I think that the Summary should start by listing the spec change toSelectionModel.isSelected, but it seems useful to also say that the implementation ofTableXXSelectionModelis changing as a result.
makes sense - thanks for the explanation :)
the last line fails for cell selection enabled with this implementation and passes when delegating to super (or not override isSelected(int) at all)
I think you are right, @kleopatra ! Took me a while, but I see your point now. This method should indeed delegate to super(), or not be overridden at all.
Thank you for creating the tests - they deserve to be integrated into TableViewSelectionModelImplTest, with your permission.
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).
Personally, I feel that any manipulations with the columns structure should clear the existing selection. I would imagine it's such a rare operation, and the "fix" is so easy (clear selection) that it's not worth even creating an issue for - but I could be wrong.
Don't remember if we have it covered in JBS?
I did not find a close match. There is JDK-8095010; there is ticket you might be familiar with JDK-8093855; and one possibly related JDK-8091191.
I would lean towards creating a new issue, unless one of the three you listed above is a natural fit.
I think you are right, @kleopatra ! Took me a while, but I see your point now. This method should indeed delegate to super(), or not be overridden at all.
I would prefer removing it (reducing clutter :) As it's a private class, there will be no publicly visible change.
Thank you for creating the tests - they deserve to be integrated into TableViewSelectionModelImplTest, with your permission.
sure, everything here is open source and we don't want to duplicate efforts :)
Probably need to do the same fix and test in TreeTableView selection?
Personally, I feel that any manipulations with the columns structure should clear the existing selection. I would imagine it's such a rare operation, and the "fix" is so easy (clear selection) that it's not worth even creating an issue for - but I could be wrong.
haha .. are you prepared to get lynched by furious users 😜 Just imagine you selected several cells across the table, then decide to hide an unrelated column (which is not as rare as you assume :) and all your previous selection work is destroyed.
Don't remember if we have it covered in JBS?
I did not find a close match. There is JDK-8095010; there is ticket you might be familiar with JDK-8093855; and one possibly related JDK-8091191.
none of those seem to be related to modification of columns, so tend to agree with Kevin that we should file a new issue
@kleopatra : good point! I've tested with swing / JTable, and removing a column seem to deselect the cells in the removed column. I guess this should be the expected behavior - to clear selection in the hidden column(s)? If the app requirements call for preserving selection, this can be done on the app level (store the hidden cell selection and restore once the cells come to view)?
Created a separate ticket (JDK-8292143).
the tests - they deserve to be integrated into TableViewSelectionModelImplTest, with your permission.
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 :)
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.
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?
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 https://github.com/openjdk/jfx/pull/858 so there is no need to cherry pick, please?
you are right... I am so sorry. missed this one while cherry picking from another branch. will fix it soon.
no problem, shit happens :)