jfx
jfx copied to clipboard
openjdk
8292353: TableRow vs. TreeTableRow: inconsistent visuals in cell selection mode
The issue is caused by TreeTableRow incorrectly selected when cell selection mode is enabled.
Changes:
- modified TreeTableRow.updateSelection()
Progress
- [x] Change must not contain extraneous whitespace
- [x] Commit message must refer to an issue
- [ ] Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)
Issue
- JDK-8292353: TableRow vs. TreeTableRow: inconsistent visuals in cell selection mode
Reviewers
- Ajit Ghaisas (@aghaisas - Reviewer)
Reviewing
Using git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx pull/875/head:pull/875
$ git checkout pull/875
Update a local copy of the PR:
$ git checkout pull/875
$ git pull https://git.openjdk.org/jfx pull/875/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 875
View PR using the GUI difftool:
$ git pr show -t 875
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/875.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
- 12: Full - Incremental (2a9b9417)
- 11: Full - Incremental (324828ec)
- 10: Full - Incremental (13ee82f4)
- 09: Full - Incremental (2c7130d9)
- 08: Full (d5ad5605)
- 07: Full (94bb6515)
- 06: Full - Incremental (d785e3d7)
- 05: Full - Incremental (3ced43d0)
- 04: Full - Incremental (326fcce4)
- 03: Full - Incremental (419e76c1)
- 02: Full - Incremental (51275937)
- 01: Full - Incremental (0b9aadb7)
- 00: Full (ee910150)
@andy-goryachev-oracle this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:
git checkout 8292353.visuals
git fetch https://git.openjdk.org/jfx master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push
Dear @kleopatra : thank you for insightful comments and suggestions!
I do agree that, in general, it might be beneficial to have one test case per condition or failure, but it should not be a requirement. I believe it is perfectly fine to have a test that executes a complex scenario. In this particular case, the failure is known, it is described in JDK-8292353, and the fact that there are multiple places where test fails (before the fix) is not important. What is important is that the failing paths are exercised and pass as a result of the fix.
I do disagree with the requirement to write a highly artificial tests like testRowSelectionStateOnCellSelectionEnabledMultiple() because it makes too many assumptions on the internal structure of the code being tested. I would rather have a test that simulates as much as possible the actual user interaction. I am not saying we have to forbid simple tests, but rather we need both. After all, we have all these tests and yet plenty of bugs avoided detection for years. It took me 10 minutes to write and test a simple example and identify a number of issues (though many of them already in JBS) with Tree/TableView.
I'd say let's allow some diversity in our testing approaches. Unless there is a very good reason to refactor, I'd rather leave TreeAndTableViewTest as is and not touch other files, as they are too big and I don't want to replicate code.
What do you think?
I do agree that, in general, it might be beneficial to have one test case per condition or failure, but it should not be a requirement.
well, this project follows (should, at least, there are legacy areas ;) established best practices for unit testing. If you want that changed, the best place to discuss such a change in strategy is the mailinglist.
I believe it is perfectly fine to have a test that executes a complex scenario.
we certainly can have tests for complex scenarios - but only if testing such a complex scenario. That's not the case here.
I do disagree with the requirement to write a highly artificial tests like testRowSelectionStateOnCellSelectionEnabledMultiple() because it makes too many assumptions on the internal structure of the code being tested.
with all due respect: that sentence is completely wrong.
- there is nothing artificial to test a class in the most minimal context possible, in fact that's the whole point of unit testing
- the amount of assumptions on internal structure is zero, they use public api to follow the three A (Arrange: create the cell, configure it at a fixed location inside a virtualized control) A (Act: change the selection of the control) A (Assert: verify the cell has updated its own state accordingly)
After all, we have all these tests and yet plenty of bugs avoided detection for years.
that's mostly because the coverage is .. suboptimal ;) As this issue demonstrates: there are no simple tests to guarantee a treeTableRow updates itself reliably.
I would rather have a test that simulates as much as possible the actual user interaction.
grabbing a tableRow from a temporary scenegraph (note: its scene is null once you get it), adding its tableCells to a different scenegraph (note: after mouseFirer constructor tableCell.getParent() != tableRow) and fire a mouseEvent onto that isolated tableCell is near .. actual user interaction? That's not case ;)
Unless there is a very good reason
The very good reason is that your test setup is suboptimal (apart from changing the recommended Attach-Act-Assert into Attach-Act-Assert-ActAgain-AssertAgain-... ) for the issue here ;)
We have a very clear functional path to test: a fully configured TableRow must sync its own selection state to the TableView's selection state at the location of the row.
So all we need is a TableView (with columns, as we want cell selection) and a TableRow configured to represent a location in that TableView. Changing table's selection at the location of the row must change (or not, depending on selection context) the selection state of the row. The most simple means to change table's selection is programatically using public api on the table's selectionModel with a direct mapping of
selectionModel.select -> tableRow.updateSelection
With yours:
stageloaderA -> createAndConfigure tableRows/Cells
-> tableRow = lookup row at index
-> stageloaderA.dispose
// at this point we are already far from any"real" context - no user interaction possible
tableCell = lookup column for tableColumn
// nevertheless with rather normal micro-cosmos:
assertEquals(tableCell.getParent(), tableCell.getTableRow());
mouseFirer -> stageLoaderB -> insert targetNode into its own root
// at this point we have an isolated tableCell in a slightly weird state, now failing the above assert:
assertEquals(tableCell.getParent(), tableCell.getTableRow());
// now fire a mouseEvent onto that isolated tableCell
// (which nevertheless is fully configured with its tableView and location)
mouseFirer.doFirer -> tableCell.behaviour ->
__selectionModel.select -> tableRow.updateSelection__
and here at the very end we are again testing the same as in the simple case, after going on a detour which might (or not) have side-effects or bugs of their own. We might go that tour if we are specifically interested in that path, that is when we are really want to test the behaviour (mouse and keyboard input) - and then we must be certain that this very last expectation is fulfilled.
What do you think?
Same as yesterday:
- missing direct tests
- unrelated, overly complex (and potentially testing the wrong thingy) TreeAndTableViewTest
Dear @kleopatra :
Thank you so much for the wisdom. Let me try to refactor the test cases.
A few questions:
-
StageLoader - since it is being disposed of by MouseFirer, would it make sense to explain how to use it properly in its javadoc? Perhaps a StageLoader needs to be created and explicitly disposed of at the beginning and end of each test?
-
I noticed that despite MouseFirer's firing MOUSE_PRESSED followed by MOUSE_RELEASED, the cell's pseudostyles contain "pressed" which seems to be incorrect. Perhaps it is due to multiple stages being created?
-
Why do I need to call VirtualFlowTestUtils.getCell(table, 0) for the cells to be created? Do you know what is missing here?
thank you.
addressed feedback from @kleopatra :
- implemented direct tests: TreeTableRowTest, TableRowTest
- properly using StageLoader
- tree table tests fail with the current master, pass with this PR. also, tree/table behavior is ok with a standalone tester (not checked in)
Lessons learned:
- StageLoader is required in this case to properly initialize the cells. No more calls to VirtualFlowTestUtils
I do want to keep my TreeAndTableViewTest as it exercises different paths, which is beneficial. There will be more test cases applicable to both Tree- and TableView.
thank you.
I haven't looked at these specific tests in detail, but as a general practice I see value in having both types of tests: unit tests that test a specific thing, and tests of various scenarios.
@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).
Never thought that I would ever feel on the verge of arguing against adding tests ;)
I haven't looked at these specific tests in detail, but as a general practice I see value in having both types of tests: unit tests that test a specific thing, and tests of various scenarios.
Yeah, we could do additional testing of broader scenarios - assuming the tightly bounded specific tests are done. We should answer two questions:
- what exactly do we expect to gain, that is which additional safety do they give?
- what is the boundary of those scenarios, why add one and not another?
In this particular case we have a tighly bounded problem: the row updates itself incorrectly based on the state of the selection model. We know that all other potential collaborators talk to the selection model directly. Given we fixed the problem (done), and have failing/passing tests before/after the fix involving an isolated, fully configured cell (not yet done), my answers:
- nothing I can see (might be missing something, of course :)
- the choice of scenarios (scene only, simulating mouse input) feels arbitrary, why not key input (or touch, accessiblity .. )? They all are at the same level of unrelatedness, IMO - we are not testing the effectiveness of this bug fix, but something else (for the mouse input f.i. tableCell - behaviour)
@kleopatra Only the author (@andy-goryachev-oracle) is allowed to issue the reviewer command.
/reviewer remove kleopatra
just noticed that this self removal request didn't work (a bit unexpected ;) - @kevinrushforth please remove me as reviewer for this PR
/reviewer remove kleopatra
just noticed that this self removal request didn't work (a bit unexpected ;) - @kevinrushforth please remove me as reviewer for this PR
There's nothing to remove, at least from a Skara point of view. You aren't listed as a Reviewer of this PR.
If you mean that you are tagged as a potential reviewer from GitHub's point of view, yes, since you left review comments. It doesn't translate into your having reviewed it, though. I can try to remove you as a potential reviewer, but that's under control of GitHub not Skara.
I give up - removing TreeAndTableViewTest.
Thank you @kevinrushforth for insights and suggestions!
@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:
8292353: TableRow vs. TreeTableRow: inconsistent visuals in cell selection mode
Reviewed-by: aghaisas, kcr
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 22 new commits pushed to the master branch:
- 337c78183be84a3420691f615096b54a68ac4300: 8293444: Creating ScrollPane with same content component causes memory leak
- cc00c8d5c7a73b9d9bc7c292ad51f8af9e63ff78: 8293795: [Accessibility] [Win] [Narrator] Exceptions when deleting text with continous key press in TextArea and TextField
- 35675c8d27d54a26059b182614e18152794dbcec: 8293971: Loading new Media from resources can sometimes fail when loading from FXML
- 03569b719b9d838d643dc4514971b06617275e86: 8293883: Add tests/.classpath (Eclipse)
- 82db6cc342e7de4391f14c70877e77b0e52fbfff: 8289541: Update ICU4C to 71.1
- 7c6a54d41945a21d2a4f0eceae56b5ba3df9f39f: 8293839: Documentation memory consistency effects of runLater
- a27840e8fd7e79b1692488e4de7f6f5fdb930cf4: 8293368: GitHub Workflows security hardening
- 5e4552db7389d93829e6ecfc61936ccad75c56c2: 8089280: horizontal scrollbar should never become visible in TableView with constrained resize policy
- cef583eef41aff3db55542d5e90423eec5601f41: 8293214: Add support for Linux/LoongArch64
- 27f1905077cbc475fbce1b0f8d950d014dbb07a4: 8279640: ListView with null SelectionModel/FocusModel throws NPE
- ... and 12 more: https://git.openjdk.org/jfx/compare/6d6726c3d69379559c4d66681726adc4e794899c...master
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.
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 (@kleopatra, @aghaisas, @kevinrushforth) but any other Committer may sponsor as well.
➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).
@andy-goryachev-oracle Your change (at version 2a9b9417a03e5e29683be43cbdfad345fbd9db63) is now ready to be sponsored by a Committer.
Going to push as commit 9768b5e42391e2d48aefffa3a3f6b1de6c30ef9d.
Since your change was applied there have been 22 commits pushed to the master branch:
- 337c78183be84a3420691f615096b54a68ac4300: 8293444: Creating ScrollPane with same content component causes memory leak
- cc00c8d5c7a73b9d9bc7c292ad51f8af9e63ff78: 8293795: [Accessibility] [Win] [Narrator] Exceptions when deleting text with continous key press in TextArea and TextField
- 35675c8d27d54a26059b182614e18152794dbcec: 8293971: Loading new Media from resources can sometimes fail when loading from FXML
- 03569b719b9d838d643dc4514971b06617275e86: 8293883: Add tests/.classpath (Eclipse)
- 82db6cc342e7de4391f14c70877e77b0e52fbfff: 8289541: Update ICU4C to 71.1
- 7c6a54d41945a21d2a4f0eceae56b5ba3df9f39f: 8293839: Documentation memory consistency effects of runLater
- a27840e8fd7e79b1692488e4de7f6f5fdb930cf4: 8293368: GitHub Workflows security hardening
- 5e4552db7389d93829e6ecfc61936ccad75c56c2: 8089280: horizontal scrollbar should never become visible in TableView with constrained resize policy
- cef583eef41aff3db55542d5e90423eec5601f41: 8293214: Add support for Linux/LoongArch64
- 27f1905077cbc475fbce1b0f8d950d014dbb07a4: 8279640: ListView with null SelectionModel/FocusModel throws NPE
- ... and 12 more: https://git.openjdk.org/jfx/compare/6d6726c3d69379559c4d66681726adc4e794899c...master
Your commit was automatically rebased without conflicts.
@kevinrushforth @andy-goryachev-oracle Pushed as commit 9768b5e42391e2d48aefffa3a3f6b1de6c30ef9d.
:bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.