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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

8285700: [TreeTableView] graphic property of TreeItem is still visible after collapsing tree #1172

Closed

Conversation

karthikpandelu
Copy link
Member

@karthikpandelu karthikpandelu commented Jul 7, 2023

In TreeTableRowSkin, graphic was not updated along with tree item update.

Made changes to update graphics of TreeTableView row in updateTreeItem() method.

Added options in monkey tester to add graphics and subnodes to TreeTableView rows.


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-8285700: [TreeTableView] graphic property of TreeItem is still visible after collapsing tree (Bug - P4)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1172/head:pull/1172
$ git checkout pull/1172

Update a local copy of the PR:
$ git checkout pull/1172
$ git pull https://git.openjdk.org/jfx.git pull/1172/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1172

View PR using the GUI difftool:
$ git pr show -t 1172

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1172.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Jul 7, 2023

馃憢 Welcome back kpk! 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.

@openjdk openjdk bot added the rfr Ready for review label Jul 7, 2023
@mlbridge
Copy link

mlbridge bot commented Jul 7, 2023

Webrevs

Copy link
Contributor

@andy-goryachev-oracle andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

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

tested using the modified MonkeyTester, works with the fix and fails in the master branch.
good job!

@openjdk
Copy link

openjdk bot commented Jul 7, 2023

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

8285700: [TreeTableView] graphic property of TreeItem is still visible after collapsing tree

Reviewed-by: angorya, mhanl

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 15 new commits pushed to the master branch:

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.

鉃★笍 To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Ready to be integrated label Jul 7, 2023
@Maran23
Copy link
Member

Maran23 commented Jul 8, 2023

Is it possible to write a Unit test as well?

@johanvos johanvos self-requested a review July 9, 2023 07:57
@johanvos
Copy link
Collaborator

johanvos commented Jul 9, 2023

+1 on @Maran23 to add a unit test.
For these kinds of changes, I think it is important to have a performance check as well (e.g. count the number of invocations to updateItem etc).

@kevinrushforth
Copy link
Member

/reviewers 2

@openjdk
Copy link

openjdk bot commented Jul 11, 2023

@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).

@openjdk openjdk bot removed the ready Ready to be integrated label Jul 11, 2023
@karthikpandelu
Copy link
Member Author

Is it possible to write a Unit test as well?

I tried to write unit test but since it should be checked if the graphic is cleared while collapsing the tree and added back while expanding, I couldn't find out a way to do this. I tried to use VirtualFlowTestUtils.assertGraphicIsVisible() method similar to how it is used in TreeTableViewTest to do the same but it did not help in this case. Please let me know if you have any suggestions.

For these kinds of changes, I think it is important to have a performance check as well (e.g. count the number of invocations to updateItem etc).

I have checked my changes using the program given in JDK-8143266. I did not find performance degradation as a result of my changes. Do you suggest writing separate test for this? Please let me know your suggestion.

@kleopatra
Copy link
Collaborator

kleopatra commented Jul 11, 2023

Is it possible to write a Unit test as well?

I tried to write unit test but since it should be checked if the graphic is cleared while collapsing the tree and added back while expanding, I couldn't find out a way to do this.

I would try to test the state of the graphic of the concrete item - keep track of it on setup and before showing the treetable - after showing, it should/not be part of the visible hierarchy (as appropriate) after expanding/collapsing. When it should be visible also verify that it is at the expected location.

@karthikpandelu
Copy link
Member Author

Added unit test as suggested above.

Copy link
Contributor

@andy-goryachev-oracle andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

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

the test passes with the fix, fails in the master branch.

one minor suggestion, unrelated, but probably worth addressing in to this PR.

assertTrue(treeItem.isExpanded());
assertEquals(treeItemSubNodeAfterExpand, treeItemSubNode.getGraphic().localToScene(treeItemSubNode.getGraphic().getBoundsInLocal()).getMinY());
}

@AfterEach
public void after() {
Copy link
Contributor

Choose a reason for hiding this comment

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

unrelated to the current PR, but could this method be more defensive, to avoid any problems with the invocation order?

    @AfterEach
    public void after() {
        if (stageLoader != null) {
            stageLoader.dispose();
            stageLoader = null;
        }
    }

Copy link
Member Author

Choose a reason for hiding this comment

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

This looks like a good check to have.
Made above suggested code changes.

@andy-goryachev-oracle
Copy link
Contributor

github says it fails pre-submit check - probably some kind of spurious failure, local build and headful tests (minus web tests) are ok on macOS.

@karthikpandelu
Copy link
Member Author

github says it fails pre-submit check - probably some kind of spurious failure, local build and headful tests (minus web tests) are ok on macOS.

Ran the pre-submit checks again. All checks are passing now

Copy link
Contributor

@andy-goryachev-oracle andy-goryachev-oracle 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, re-approving the latest changes.

@karthikpandelu
Copy link
Member Author

@johanvos could you please review the latest updates?

Copy link
Collaborator

@johanvos johanvos left a comment

Choose a reason for hiding this comment

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

Looks ok now. Thanks @kleopatra for the guidance on the test.

@openjdk openjdk bot added the ready Ready to be integrated label Jul 19, 2023
tree.getRoot().setExpanded(true);

int itemNodeSize = tree.getRoot().getChildren().size();
TreeItem treeItem = (TreeItem)tree.getRoot().getChildren().get(itemNodeSize - 2);
Copy link
Member

Choose a reason for hiding this comment

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

Cast is not needed here and below

tree.setRoot(new TreeItem("Root"));

for (int i = 0; i < 4; i++) {
TreeItem parent = new TreeItem("item - " + i, new Rectangle(10, 10));
Copy link
Member

Choose a reason for hiding this comment

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

Generics should be added here

@openjdk openjdk bot removed the ready Ready to be integrated label Jul 20, 2023
@karthikpandelu
Copy link
Member Author

Made changes according to @Maran23's review comments.
Please re-approve the changes.

Copy link
Member

@Maran23 Maran23 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. Can confirm that the test fails before.
Thanks!

Copy link
Contributor

@andy-goryachev-oracle andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

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

re-approving latest changes.

@openjdk openjdk bot added the ready Ready to be integrated label Jul 20, 2023
@karthikpandelu
Copy link
Member Author

Thanks @andy-goryachev-oracle , @johanvos and @Maran23 for the review and thanks @kleopatra for the guidance on the the test.

@karthikpandelu
Copy link
Member Author

/integrate

@openjdk
Copy link

openjdk bot commented Jul 20, 2023

Going to push as commit 600cee7.
Since your change was applied there have been 15 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Jul 20, 2023
@openjdk openjdk bot closed this Jul 20, 2023
@openjdk openjdk bot removed ready Ready to be integrated rfr Ready for review labels Jul 20, 2023
@openjdk
Copy link

openjdk bot commented Jul 20, 2023

@karthikpandelu Pushed as commit 600cee7.

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

@karthikpandelu karthikpandelu deleted the treetableview_fix branch July 21, 2023 06:32
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
6 participants