openjdk
8289357: (Tree)TableView is null in (Tree)TableRowSkin during autosize
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
- [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)
Issues
- JDK-8289357: (Tree)TableView is null in (Tree)TableRowSkin during autosize
- JDK-8292009: Wrong text artifacts in table header
Reviewers
- Ajit Ghaisas (@aghaisas - Reviewer)
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
:wave: 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.
![bridgekeeper[bot] avatar](https://avatars.githubusercontent.com/u/41768318?v=4 "Published by a pub.dev verified publisher"){kind=small}
Webrevs
- 07: Full (54f24a7f)
- 06: Full (4b0820b7)
- 05: Full (1ee61b2c)
- 04: Full - Incremental (4839de00)
- 03: Full - Incremental (eb95e91d)
- 02: Full - Incremental (b6dcccdb)
- 01: Full - Incremental (e3a9545e)
- 00: Full (1c23a030)
@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).
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.
@Maran23 GitHub is reporting a merge conflict in two of the test files. Can you merge the latest upstream master into your branch?
@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.
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
Adding additional issue to issue list: 8292009: Wrong text artifacts in table header.
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?
we also have https://github.com/openjdk/jfx/pull/931 in the pipeline. thankfully, the changes seem to be orthogonal, even though I expect a few merge conflicts.
@Maran23 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:
8289357: (Tree)TableView is null in (Tree)TableRowSkin during autosize
8292009: Wrong text artifacts in table header
Reviewed-by: kcr, angorya
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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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.
@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
@Maran23 Pushed as commit c900a00c7527f290e8047792fef4b45002930892.
:bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.