8172095: Let Node.managed become CSS-styleable

[abhinayagarwal]

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8172095: Let Node.managed become CSS-styleable

Reviewers

• @Dlittler949 (no known github.com user name / role) 🔄 Re-review required (review applies to 56b8f24)
• Kevin Rushforth (@kevinrushforth - Reviewer)
• Ambarish Rapte (@arapte - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/602/head:pull/602
$ git checkout pull/602

Update a local copy of the PR:
$ git checkout pull/602
$ git pull https://git.openjdk.java.net/jfx pull/602/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 602

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/602.diff

[bridgekeeper]

👋 Welcome back abhinayagarwal! 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.

[kevinrushforth]

This will need at least a brief discussion on the openjfx-dev mailing list.

/csr
/reviewers 2

[openjdk]

@kevinrushforth has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@abhinayagarwal please create a CSR request for issue JDK-8172095. This pull request cannot be integrated until the CSR request is approved.

[openjdk]

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

[abhinayagarwal]

Past OpenJFX mailing list discussion: http://mail.openjdk.java.net/pipermail/openjfx-dev/2015-July/017597.html

[mlbridge]

Webrevs

• 01: Full - Incremental (394a47e3)
• 00: Full (56b8f24b)

[kevinrushforth]

Please start a new thread and reference that one. That thread asks for a new property, but I think what you propose, which is to make the existing managed property CSS styleable, is better. Given how simple a request yours is, I expect the discussion will be brief.

[abhinayagarwal]

Thanks for the feedback. I will create a CSR and start a thread on the mailing list.

[abhinayagarwal]

CSR: https://bugs.openjdk.java.net/browse/JDK-8272386

[abhinayagarwal]

I started a discussion on the mailing list. Is there anything else pending to move this PR forward?

[kevinrushforth]

Is there anything else pending to move this PR forward?

Nothing else needed for now. The review can proceed.

[kevinrushforth]

The new API looks fine. I left one comment on the cssref docs, and that's otherwise ready to go (so you can get the CSR ready).

I noticed that there are no existing tests in NodeTest for the default value or for setting / getting the property, which there are for other properties. Can you add these missing tests, to verify that there are no problems?

[arapte]

Changes look good to me.
The tests, managedSetFromCSS() and Node_cssMethods_Test fail before and pass after. Other tests do pass both before and after, which is expected behaviour.

[kevinrushforth]

Looks good.

[kevinrushforth]

This is ready to be integrated once the CSR is approved.

[openjdk]

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

8172095: Let Node.managed become CSS-styleable

Reviewed-by: kcr, arapte

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

• b2ecfac: 8271398: GTK3 drag view image swaps red and blue color channels
• 7329279: 8273754: Re-introduce Automatic-Module-Name in empty jars
• 51265c0: 8270107: Open source FXMediaPlayer test app
• f987b18: 8089398: [ChoiceBox, ComboBox] throws NPE on setting value on null selectionModel
• a272c4f: 8273324: IllegalArgumentException: fromIndex(0) > toIndex(-1) after clear and select TableCell
• 26d6438: 8273138: BidirectionalBinding fails to observe changes of invalid properties
• 5552213: Merge
• 24d0600: 8273343: Create release notes for JavaFX 17
• 78ae4a8: 8269871: CellEditEvent: must not throw NPE in accessors
• 2267b11: 8273071: SeparatorSkin: must remove child on dispose
• ... and 9 more: https://git.openjdk.java.net/jfx/compare/852d2875248040ea5eff2b41bd23a4fde02486a9...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 (@kevinrushforth, @arapte) 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).

[abhinayagarwal]

/integrate

[openjdk]

@abhinayagarwal
Your change (at version 394a47e) is now ready to be sponsored by a Committer.

[kevinrushforth]

/sponsor

[openjdk]

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

• b2ecfac: 8271398: GTK3 drag view image swaps red and blue color channels
• 7329279: 8273754: Re-introduce Automatic-Module-Name in empty jars
• 51265c0: 8270107: Open source FXMediaPlayer test app
• f987b18: 8089398: [ChoiceBox, ComboBox] throws NPE on setting value on null selectionModel
• a272c4f: 8273324: IllegalArgumentException: fromIndex(0) > toIndex(-1) after clear and select TableCell
• 26d6438: 8273138: BidirectionalBinding fails to observe changes of invalid properties
• 5552213: Merge
• 24d0600: 8273343: Create release notes for JavaFX 17
• 78ae4a8: 8269871: CellEditEvent: must not throw NPE in accessors
• 2267b11: 8273071: SeparatorSkin: must remove child on dispose
• ... and 9 more: https://git.openjdk.java.net/jfx/compare/852d2875248040ea5eff2b41bd23a4fde02486a9...master

Your commit was automatically rebased without conflicts.

[openjdk]

@kevinrushforth @abhinayagarwal Pushed as commit d356340.

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