8277000: Tree-/TableRowSkin: replace listener to fixedCellSize by live lookup #1645

Maran23 · 2024-11-22T20:31:08Z

This PR improves the Tree-/TableRowSkin code by doing a normal live lookup for the fixedCellSize instead of adding listener just to update variables(fixedCellSizeEnabled and fixedCellSize) which can otherwise be also just lookup'd without the hassle of listeners.

While this is mostly a cleanup, it does improve the state of the Tree-/TableRow, as when the TableRowSkinBase constructor is called, the variables are not yet set.

It is also consistent with the other cells, see also JDK-8246745.
Helps a bit with JDK-8185887 (#1644), but as written above, not required as there is no (visible) effect.

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-8277000: Tree-/TableRowSkin: replace listener to fixedCellSize by live lookup (Bug - P4)

Reviewers

Andy Goryachev (@andy-goryachev-oracle - Reviewer)
Michael Strauß (@mstr2 - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1645

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

Using diff file

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

Using Webrev

Link to Webrev Comment

…e lookup

bridgekeeper · 2024-11-22T20:31:18Z

👋 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 · 2024-11-22T20:31:45Z

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

8277000: Tree-/TableRowSkin: replace listener to fixedCellSize by live lookup

Reviewed-by: angorya, mstrauss

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

9ac707d: 8353845: com.sun.javafx.css.BitSet.equals(null) throws NPE
3dc975b: 8234153: [TEST_BUG] Rewrite Popup_parentWindow_Test
dc115d5: 8353916: Unexpected event type for DOM mutation events with WebKit 620.1
... and 57 more: https://git.openjdk.org/jfx/compare/d615fdc717a55251225823ae5eeefb766e900e99...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.

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

Maran23 · 2024-11-22T20:32:14Z

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableRowSkin.java


        ListenerHelper lh = ListenerHelper.get(this);

-        lh.addChangeListener(control.indexProperty(), (ev) -> {


Since #1635 got merged, this is not needed as TableRowSkinBase does that already:
registerChangeListener(control.indexProperty(), e -> requestCellUpdate());. So this is basically a noop.

mlbridge · 2024-11-22T20:35:45Z

Webrevs

kevinrushforth · 2024-11-22T20:41:46Z

/reviewers 2

openjdk · 2024-11-22T20:42:05Z

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

andy-goryachev-oracle

Tested with the Monkey Tester, both TreeTableView and TableView - looks good, responds to fixed cell size changes.

Left a few minor comments.

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableRowSkinBase.java

…tree-table-row-skin-live-lookup

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableRowSkinBase.java

bridgekeeper · 2025-01-16T18:09:40Z

@Maran23 This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

…tree-table-row-skin-live-lookup # Conflicts: # modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableRowSkin.java # modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableRowSkinBase.java # modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableRowSkin.java

bridgekeeper · 2025-02-13T21:32:03Z

@Maran23 This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

andy-goryachev-oracle · 2025-02-14T15:35:17Z

Now that #1644 has been merged, should this PR be re-opened?

Maran23 · 2025-02-17T22:47:50Z

/open

Now that #1644 has been merged, should this PR be re-opened?

Yes! I wanted to check everything first before I reopen it. Should be good now.

openjdk · 2025-02-17T22:48:14Z

@Maran23 This pull request is now open

andy-goryachev-oracle

A good improvement!
The testing on macOS 15.3.1 M1 looks good.

kevinrushforth · 2025-03-06T21:59:33Z

@arapte Can you be the second reviewer?

bridgekeeper · 2025-04-04T03:37:23Z

@Maran23 This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

mstr2

Looks good and works as expected.

Maran23 · 2025-04-11T17:37:11Z

/integrate

openjdk · 2025-04-11T17:38:52Z

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

8a61dd2: 8354337: GHA: Windows build fails with chmod permission error
dfe8dbb: 8335547: Support multi-line prompt text for TextArea
9ac707d: 8353845: com.sun.javafx.css.BitSet.equals(null) throws NPE
... and 59 more: https://git.openjdk.org/jfx/compare/d615fdc717a55251225823ae5eeefb766e900e99...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-04-11T17:39:05Z

@Maran23 Pushed as commit fcdccd9.

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

8277000: Tree-/TableRowSkin: replace listener to fixedCellSize by liv…

8206f82

…e lookup

openjdk bot added the rfr Ready for review label Nov 22, 2024

Maran23 commented Nov 22, 2024

View reviewed changes

andy-goryachev-oracle reviewed Dec 4, 2024

View reviewed changes

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableRowSkinBase.java Outdated Show resolved Hide resolved

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableRowSkinBase.java Show resolved Hide resolved

andy-goryachev-oracle mentioned this pull request Dec 4, 2024

8185887: TableRowSkinBase fails to correctly virtualize cells in horizontal direction #1644

Closed

3 tasks

Maran23 added 2 commits December 10, 2024 00:51

Call getFixedCellSize() once

63119d9

Merge branch 'master' of https://github.com/openjdk/jfx into 8277000-…

5d5c0c3

…tree-table-row-skin-live-lookup

andy-goryachev-oracle reviewed Dec 10, 2024

View reviewed changes

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableRowSkinBase.java Show resolved Hide resolved

Maran23 added 2 commits February 6, 2025 13:26

Merge and adjust logic to master

e6595b8

bridgekeeper bot closed this Feb 13, 2025

Use virtualFlow directly

9b12fbf

openjdk bot reopened this Feb 17, 2025

Add as javadoc

bb79066

andy-goryachev-oracle approved these changes Feb 20, 2025

View reviewed changes

mstr2 approved these changes Apr 11, 2025

View reviewed changes

openjdk bot added the ready Ready to be integrated label Apr 11, 2025

openjdk bot added the integrated Pull request has been integrated label Apr 11, 2025

openjdk bot closed this Apr 11, 2025

openjdk bot removed ready Ready to be integrated rfr Ready for review labels Apr 11, 2025

Maran23 deleted the 8277000-tree-table-row-skin-live-lookup branch April 30, 2025 19:10


		ListenerHelper lh = ListenerHelper.get(this);

		lh.addChangeListener(control.indexProperty(), (ev) -> {

8277000: Tree-/TableRowSkin: replace listener to fixedCellSize by live lookup #1645

8277000: Tree-/TableRowSkin: replace listener to fixedCellSize by live lookup #1645

Uh oh!

Conversation

Maran23 commented Nov 22, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Nov 22, 2024

Uh oh!

openjdk bot commented Nov 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Maran23 Nov 22, 2024

Choose a reason for hiding this comment

Uh oh!

mlbridge bot commented Nov 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

kevinrushforth commented Nov 22, 2024

Uh oh!

openjdk bot commented Nov 22, 2024

Uh oh!

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bridgekeeper bot commented Jan 16, 2025

Uh oh!

bridgekeeper bot commented Feb 13, 2025

Uh oh!

andy-goryachev-oracle commented Feb 14, 2025

Uh oh!

Maran23 commented Feb 17, 2025

Uh oh!

openjdk bot commented Feb 17, 2025

Uh oh!

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

Uh oh!

kevinrushforth commented Mar 6, 2025

Uh oh!

bridgekeeper bot commented Apr 4, 2025

Uh oh!

mstr2 left a comment

Choose a reason for hiding this comment

Uh oh!

Maran23 commented Apr 11, 2025

Uh oh!

openjdk bot commented Apr 11, 2025

Uh oh!

openjdk bot commented Apr 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

Maran23 commented Nov 22, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Nov 22, 2024 •

edited

Loading

mlbridge bot commented Nov 22, 2024 •

edited

Loading