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

8289357: (Tree)TableView is null in (Tree)TableRowSkin during autosize #805

Closed

Conversation

Maran23
Copy link
Member

@Maran23 Maran23 commented Jun 28, 2022

Initialize the (Tree)TableView when creating the measure row.
This will guarantee, that we can access the (Tree)TableView in the (Tree)TableRowSkin, which is currently only null during the autosizing (It is always set otherwise).

With this change, a NPE is happening as the (Tree)TableRow currently assumes, that there must be a VirtualFlow somewhere in the scene (parent). I guard against this now.
I remembered, that there is a ticket for the above behaviour here: https://bugs.openjdk.org/browse/JDK-8274065

Finally, the (Tree)TableRow must be removed after the autosizing and the index must be set to -1 (as for the cell) so that e.g. cancelEdit() is not triggered. Some tests catched that (see test_rt_31015). This did not happen before as the table row setup was not complete, but now the row does know the table and therefore installs some listener on it in order to fire corresponding edit events.


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)

Issues

  • JDK-8289357: (Tree)TableView is null in (Tree)TableRowSkin during autosize
  • JDK-8292009: Wrong text artifacts in table header

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 805

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

Using diff file

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

鈥. Also prevent a NPE as we don't have a VirtualFlow in the context of autosizing
@bridgekeeper
Copy link

bridgekeeper bot commented Jun 28, 2022

馃憢 Welcome back mhanl! 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 Jun 28, 2022
@mlbridge
Copy link

mlbridge bot commented Jun 28, 2022

Webrevs

@kevinrushforth
Copy link
Member

/reviewers 2

@openjdk
Copy link

openjdk bot commented Jul 6, 2022

@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 kevinrushforth self-requested a review July 8, 2022 14:57
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 code change looks quite reasonable to me, but it would be good to get additional eyes on this. @aghaisas will also review it.

@kleopatra do you have any thoughts on this?

@kevinrushforth kevinrushforth self-requested a review July 22, 2022 16:03
@kevinrushforth
Copy link
Member

I just noticed comments in JDK-8292009 indicating that this PR fixes that bug as well. It would be good to get the review for this fix done.

Since JDK-8292009 described a different problem that is also fixed as a result, it seems good to add that issue to this PR.

@kevinrushforth
Copy link
Member

@Maran23 GitHub is reporting a merge conflict in two of the test files. Can you merge the latest upstream master into your branch?

鈥able-view-null-in-table-row-skin

锟 Conflicts:
锟	modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableRowSkinTest.java
锟	modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TreeTableRowSkinTest.java
@Maran23
Copy link
Member Author

Maran23 commented Aug 10, 2022

@Maran23 GitHub is reporting a merge conflict in two of the test files. Can you merge the latest upstream master into your branch?

Didn't noticed, thanks for the ping.
For some reason the macOS build can not download ant:

+ mkdir -p /Users/runner/build-tools
+ wget -O /Users/runner/build-tools/apache-ant-1.10.5.tar.gz https://archive.apache.org/dist/ant/binaries/apache-ant-1.10.5-bin.tar.gz
--2022-08-10 09:37:54--  https://archive.apache.org/dist/ant/binaries/apache-ant-1.10.5-bin.tar.gz
Resolving archive.apache.org (archive.apache.org)... 138.[20](https://github.com/Maran23/jfx/runs/7763778214?check_suite_focus=true#step:3:21)1.131.134
Connecting to archive.apache.org (archive.apache.org)|138.201.131.134|:443... failed: Operation timed out.
Retrying.

I will trigger a rebuild soon.

@kevinrushforth
Copy link
Member

For some reason the macOS build can not download ant:
...
I will trigger a rebuild soon.

@andy-goryachev-oracle and I have both seen this recently on some of our GHA runs. It's an intermittent failure, so a rebuild usually works.

@Maran23
Copy link
Member Author

Maran23 commented Aug 10, 2022

/issue add JDK-8292009

@openjdk
Copy link

openjdk bot commented Aug 10, 2022

@Maran23
Adding additional issue to issue list: 8292009: Wrong text artifacts in table header.

Copy link
Collaborator

@aghaisas aghaisas 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 to me!

@Maran23
Copy link
Member Author

Maran23 commented Dec 6, 2022

Can anyone else check this out soon? :)

@kevinrushforth
Copy link
Member

Can anyone else check this out soon? :)

Sorry about the delay. I do want to see this go in soon, so yes, I can take a look. Perhaps @andy-goryachev-oracle can too?

@andy-goryachev-oracle
Copy link
Contributor

andy-goryachev-oracle commented Dec 6, 2022

we also have #931 in the pipeline.
thankfully, the changes seem to be orthogonal, even though I expect a few merge conflicts.

@Maran23
Copy link
Member Author

Maran23 commented Dec 8, 2022

Just merged in the latest master to be safe. :)

Copy link
Collaborator

@aghaisas aghaisas 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 to me!

@openjdk openjdk bot added the ready Ready to be integrated label Dec 8, 2022
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 changes look good, please resolve the conflict - my #931 got integrated first, sorry.

@openjdk
Copy link

openjdk bot commented Dec 8, 2022

@Maran23 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 8289357-table-view-null-in-table-row-skin
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 merge-conflict Pull request has merge conflict with target branch and removed ready Ready to be integrated labels Dec 8, 2022
鈥able-view-null-in-table-row-skin

锟 Conflicts:
锟	modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableRowSkin.java
@openjdk openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Dec 8, 2022
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.

lgtm

@openjdk openjdk bot added the ready Ready to be integrated label Dec 8, 2022
@Maran23
Copy link
Member Author

Maran23 commented Dec 8, 2022

/integrate

@openjdk
Copy link

openjdk bot commented Dec 8, 2022

Going to push as commit c900a00.

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

openjdk bot commented Dec 8, 2022

@Maran23 Pushed as commit c900a00.

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

@Maran23 Maran23 deleted the 8289357-table-view-null-in-table-row-skin branch August 4, 2023 18:00
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
4 participants