Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8292353: TableRow vs. TreeTableRow: inconsistent visuals in cell selection mode #875

Conversation

andy-goryachev-oracle
Copy link
Contributor

@andy-goryachev-oracle andy-goryachev-oracle commented Aug 17, 2022

The issue is caused by TreeTableRow incorrectly selected when cell selection mode is enabled.

Changes:

  • modified TreeTableRow.updateSelection()

Progress

  • Change must not contain extraneous whitespace
  • 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

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

@bridgekeeper
Copy link

bridgekeeper bot commented Aug 17, 2022

👋 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.

@andy-goryachev-oracle andy-goryachev-oracle marked this pull request as ready for review August 17, 2022 20:17
@openjdk openjdk bot added the rfr Ready for review label Aug 17, 2022
@mlbridge
Copy link

mlbridge bot commented Aug 17, 2022

@openjdk openjdk bot removed the rfr Ready for review label Aug 19, 2022
@openjdk openjdk bot added the rfr Ready for review label Aug 19, 2022
Copy link
Collaborator

@kleopatra kleopatra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix looks good (left a minor inline comment which you might follow or not :)

The tests, though, ... need some love:

As a general rule, it's preferred to assert a single state per test method instead of changing the state repeatedly inside the same method. We can see that something is wrong when having code comments like:

line 240:         assertFalse(row.isSelected()); // JDK-8292353 failure

line 249:         assertFalse(row.isSelected()); // JDK-8292353 failure

running the test before the fix and the second failure is never reached. We want to see the failures, all of them :)

Write a test as near to the cause as possible: here the cause is an incorrect selection state of the TreeTableRow under certain conditions. That's unrelated to any user interaction, so a setup faking such an interaction is a detour which might (or not) bring in their own trouble (aside: I do see the css parser warnings when running TreeAndTableViewTest inside Eclipse - that is JDK-8291853 which we don't yet understand, except that something fishy might be going on ;) Best to use the most simple setup to test that exact state, here that would be an isolated Tree/TableRow thats configured with a Tree/TableView at some index and setup the view's selection state as needed, for examples see TreeTableRowTest or something like the following

@Test
public void testRowSelectionStateOnCellSelectionEnabledMultiple() {
    // configure view
    addColumns();
    tree.getSelectionModel().setCellSelectionEnabled(true);
    tree.getSelectionModel().setSelectionMode(SelectionMode.MULTIPLE);
    // configure tableRow        
    int selected = 1;
    cell.updateTreeTableView(tree);
    cell.updateIndex(selected);

    // select all columns if possible
    tree.getSelectionModel().select(selected, null);
    assertEquals("sanity: all columns selected", tree.getColumns().size(),
            tree.getSelectionModel().getSelectedCells().size());
    assertFalse("row must not be selected in cell selection mode", cell.isSelected());
}

and one for the second failure before the fix plus (for coverage) the other combinations of cell selection enablement/selection mode :) These should reside in TreeTableRowTest, IMO, to be "near" the other row state tests.

For coverage, we should also have the corresponding tests for TableRow (also isolated, not wrapped into faked user interaction). They should reside in TableRowTest. Which we don't have yet, time to add it :)

My suggestions:

  • convert the monolithic test methods into separate tests, each without changing the state you want to test
  • configure and test the selection state of an isolated row (without faking user interaction, similar to the example above)
  • for TreeTableRow, add the tests to TreeTableRowTest
  • for TableRow, add TableRowTest and add analogue methods as those you added for TreeTableRow
  • remove TreeAndTableViewTest

@openjdk
Copy link

openjdk bot commented Aug 23, 2022

@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

@openjdk openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Aug 23, 2022
@openjdk openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Aug 23, 2022
@andy-goryachev-oracle
Copy link
Contributor Author

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?

@kleopatra
Copy link
Collaborator

kleopatra commented Aug 24, 2022

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

@andy-goryachev-oracle
Copy link
Contributor Author

Dear @kleopatra :

Thank you so much for the wisdom. Let me try to refactor the test cases.

A few questions:

  1. 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?

  2. 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?

  3. 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.

@openjdk openjdk bot removed the rfr Ready for review label Aug 24, 2022
Comment on lines 148 to 157
public static TableRow getTableRow(TableView t, int row) {
for (Node n: t.lookupAll(".table-row-cell")) {
if (n instanceof TableRow c) {
if (row == c.getIndex()) {
return c;
}
}
}
throw new Error("TableRow not found at " + row);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no review, just a couple of comments:

Generally, should prefer api over lookup - we have shims to access package/protected api.

In this particular case, the lookup is brittle: it assumes that

  • either there is a single row configured with the given index
  • or that it's the first when iterating over the set

With the first being incorrect (corner case, of course :)

@Test
public void testLookupLastNotEmptyCell() {
    stageLoader = new StageLoader(table);
    int index = table.getItems().size()- 1;
    Set<Node> tableRows = table.lookupAll(".table-row-cell").stream()
            .filter(n -> n instanceof TableRow<?> && ((TableRow<?>) n).getIndex() == index)
            .collect(Collectors.toSet());
    assertEquals(1, tableRows.size());
}

we fall back to the second assumption, which is implementation dependent (and unspecified) - it's accidental that we actually do get the row we want.

Having done a bit of digging into VirtualFlowTestUtils (see JDK-8293356) I now think it's safe enough to use it (even though it's on the oldish side, not using recently added api, probably needs some polish) - as long as we keep the scenegraph stable during the test, f.i. by manually adding the table to a stage (either via StageLoader or showing in our own).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to have two or more rows with the same row index? i would imagine that will break a lot of things.

I am not sure why you think the lookup is brittle - after all, VirtualFlowTestUtils uses lookup to get a pointer to the VirtualFlow, is it not (line 333)? Is it because the lookup may not return a cell if it is outside of the current view port area and therefore has not been created (while the .getCell() guarantees to create one)?

Is this the only objection to the changes in this PR? I suppose i could change the tests to use VirtualFlowTestUtils, though my preference would be still to test close to the real world scenario, using public APIs (as opposed to getting into internals using some internal test utils), given the fact that I do expect these cells to be visible given the dimensions of the table and columns.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to have two or more rows with the same row index? i would imagine that will break a lot of things.

don't imagine, verify ;) What happens when you run the test? What happens when you look at the scenegraph (f.i. with ScenicView)? Don't know why they are there but they are - well outside the viewport.

Is this the only objection to the changes in this PR?

I honestly cannot understand how you possibly could have reached the conclusion that this is the only objection given my comments ..

Anyway, I'm off here - don't know how to abort a review, but not going to waste more time and nerves on who doesn't seem to listen (that's my personal feeling, of course ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, what I really meant is - are the code changes ok and we are only discussing updating tests?

my goal here is to fix as many bugs as I possibly can, while listening to the feedback of more experienced team members and addressing their concerns. it is not always clear when an exchange of ideas ends and marching orders begin, especially in situations where several possibilities exist. perhaps a clearly annunciated request for specific changes would work?

I feel really uncomfortable with thousands of defects logged against openjfx, and all I want is to help.

@kleopatra
Copy link
Collaborator

/reviewer remove kleopatra

@openjdk
Copy link

openjdk bot commented Sep 14, 2022

@kleopatra Only the author (@andy-goryachev-oracle) is allowed to issue the reviewer command.

@kleopatra
Copy link
Collaborator

kleopatra commented Sep 23, 2022

/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

@kevinrushforth
Copy link
Member

/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.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix looks good.

As for the tests, I somewhat agree with the comments left by @kleopatra, especially the comment about the suitability of the tests in TreeAndTableViewTest. Having said that, they aren't harmful, so I will leave it up to you as to whether to remove that test class or leave it in.

I left a few specific comments on the tests inline. You can either update this PR (probably best) or file a follow-up test bug, which can be done during an upcoming test sprint.

@andy-goryachev-oracle
Copy link
Contributor Author

I give up - removing TreeAndTableViewTest.

Thank you @kevinrushforth for insights and suggestions!

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@openjdk
Copy link

openjdk bot commented Oct 7, 2022

@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:

  • 337c781: 8293444: Creating ScrollPane with same content component causes memory leak
  • cc00c8d: 8293795: [Accessibility] [Win] [Narrator] Exceptions when deleting text with continous key press in TextArea and TextField
  • 35675c8: 8293971: Loading new Media from resources can sometimes fail when loading from FXML
  • 03569b7: 8293883: Add tests/.classpath (Eclipse)
  • 82db6cc: 8289541: Update ICU4C to 71.1
  • 7c6a54d: 8293839: Documentation memory consistency effects of runLater
  • a27840e: 8293368: GitHub Workflows security hardening
  • 5e4552d: 8089280: horizontal scrollbar should never become visible in TableView with constrained resize policy
  • cef583e: 8293214: Add support for Linux/LoongArch64
  • 27f1905: 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).

@openjdk openjdk bot added the ready Ready to be integrated label Oct 7, 2022
@andy-goryachev-oracle
Copy link
Contributor Author

/integrate

@openjdk openjdk bot added the sponsor Ready to sponsor label Oct 7, 2022
@openjdk
Copy link

openjdk bot commented Oct 7, 2022

@andy-goryachev-oracle
Your change (at version 2a9b941) is now ready to be sponsored by a Committer.

@kevinrushforth
Copy link
Member

/sponsor

@openjdk
Copy link

openjdk bot commented Oct 7, 2022

Going to push as commit 9768b5e.
Since your change was applied there have been 22 commits pushed to the master branch:

  • 337c781: 8293444: Creating ScrollPane with same content component causes memory leak
  • cc00c8d: 8293795: [Accessibility] [Win] [Narrator] Exceptions when deleting text with continous key press in TextArea and TextField
  • 35675c8: 8293971: Loading new Media from resources can sometimes fail when loading from FXML
  • 03569b7: 8293883: Add tests/.classpath (Eclipse)
  • 82db6cc: 8289541: Update ICU4C to 71.1
  • 7c6a54d: 8293839: Documentation memory consistency effects of runLater
  • a27840e: 8293368: GitHub Workflows security hardening
  • 5e4552d: 8089280: horizontal scrollbar should never become visible in TableView with constrained resize policy
  • cef583e: 8293214: Add support for Linux/LoongArch64
  • 27f1905: 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.

@openjdk openjdk bot added the integrated Pull request has been integrated label Oct 7, 2022
@openjdk openjdk bot closed this Oct 7, 2022
@openjdk openjdk bot removed ready Ready to be integrated rfr Ready for review sponsor Ready to sponsor labels Oct 7, 2022
@openjdk
Copy link

openjdk bot commented Oct 7, 2022

@kevinrushforth @andy-goryachev-oracle Pushed as commit 9768b5e.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@andy-goryachev-oracle andy-goryachev-oracle deleted the 8292353.visuals branch October 7, 2022 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated
5 participants