8346227: Seal Paint and Material #1665

mstr2 · 2024-12-13T23:33:18Z

The Paint and Material classes can't be extended by user code, because their implementations require special support in internal JavaFX code. The classes should be sealed.

/csr
/reviewers 2

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change requires CSR request JDK-8346461 to be approved
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issues

JDK-8346227: Seal Paint and Material (Enhancement - P4)
JDK-8346461: Seal Paint and Material (CSR)

Reviewers

Nir Lisker (@nlisker - Reviewer)
Andy Goryachev (@andy-goryachev-oracle - Reviewer)
Kevin Rushforth (@kevinrushforth - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1665

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2024-12-13T23:34:00Z

👋 Welcome back mstrauss! 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-12-13T23:34:37Z

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

8346227: Seal Paint and Material

Reviewed-by: nlisker, angorya, kcr

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

5b074c4: 8342703: CSS transition is not started when initial value was not specified
6ec588c: 8315873: [GHA] Update checkout and cache action to use v4
76d5e1a: 8343398: Add reducedData preference
98916fe: 8342233: Regression: TextInputControl selection is backwards in RTL mode
f06b15b: 8346693: Update copyright header for files modified in 2024
e30d0d5: 8345136: Update JDK_DOCS property to point to JDK 23 docs
5279be6: 8345127: Add --sun-misc-unsafe-memory-access=allow when running tests until JDK-8334137 is fixed
ec8c032: 8218745: TableView: visual glitch at borders on horizontal scrolling
0c9d4ee: 8345937: Update JUnit to 5.11.3
56175f4: 8346222: SwingNodePlatformExitCrashTest fails with JUnit 5.11.3

Please see this link for an up-to-date comparison between the source branch of this pull request and 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 · 2024-12-13T23:35:01Z

@mstr2 has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@mstr2 please create a CSR request for issue JDK-8346227 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

openjdk · 2024-12-13T23:35:17Z

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

mlbridge · 2024-12-13T23:37:50Z

Webrevs

01: Full - Incremental (f0a39b16)
00: Full (5c4b42c8)

andy-goryachev-oracle · 2024-12-17T18:01:57Z

modules/javafx.graphics/src/main/java/com/sun/javafx/tk/Toolkit.java

-        }
-
-        return null;
+        return switch (paint) {


standard warning: not equivalent change (in the case of null paint).
it's ok in this case, since it looks like this method is never called with a null argument thanks to guards like this replicated across the code base:

peer.setFillPaint(getFill() == null ? null : tk.getPaint(getFill()));

would it have been easier to allow null argument and return null from getPaint() instead?

null is not a valid argument for this method, and has never been. acc_getPlatformPaint() previously asserted that the return value is not null.

this is not what I am asking though: in more than one place we have callers like this

peer.setFillPaint(getFill() == null ? null : tk.getPaint(getFill()));

it could be changed to

peer.setFillPaint(tk.getPaint(getFill()));

Yes, that would make the callsite a bit easier. However, from an API perspective, I think it's not the best approach to handle nulls in the toolkit layer. A system that channels nulls thorugh various layers tends to become harder to reason about over time.

I disagree: it makes it more resilient by doing what the caller has to do anyway.
However, the answer is acceptable.

Btw, even if we did decide to change it in the future, we wouldn't do it as part of this PR.

kevinrushforth

Looks good.

kevinrushforth · 2024-12-18T18:04:13Z

modules/javafx.graphics/src/main/java/com/sun/javafx/tk/Toolkit.java

-        }
-
-        return null;
+        return switch (paint) {


Btw, even if we did decide to change it in the future, we wouldn't do it as part of this PR.

mstr2 · 2025-01-08T19:17:25Z

/integrate

openjdk · 2025-01-08T19:17:59Z

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

5b074c4: 8342703: CSS transition is not started when initial value was not specified
6ec588c: 8315873: [GHA] Update checkout and cache action to use v4
76d5e1a: 8343398: Add reducedData preference
98916fe: 8342233: Regression: TextInputControl selection is backwards in RTL mode
e30d0d5: 8345136: Update JDK_DOCS property to point to JDK 23 docs
5279be6: 8345127: Add --sun-misc-unsafe-memory-access=allow when running tests until JDK-8334137 is fixed
ec8c032: 8218745: TableView: visual glitch at borders on horizontal scrolling
0c9d4ee: 8345937: Update JUnit to 5.11.3
56175f4: 8346222: SwingNodePlatformExitCrashTest fails with JUnit 5.11.3

Your commit was automatically rebased without conflicts.

openjdk · 2025-01-08T19:18:04Z

@mstr2 Pushed as commit 3cfd317.

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

Seal Paint and Material

5c4b42c

openjdk bot added rfr Ready for review csr Need approved CSR to integrate pull request labels Dec 13, 2024

use exhaustive switch pattern

f0a39b1

nlisker approved these changes Dec 17, 2024

View reviewed changes

andy-goryachev-oracle reviewed Dec 17, 2024

View reviewed changes

kevinrushforth self-requested a review December 18, 2024 17:43

andy-goryachev-oracle approved these changes Dec 18, 2024

View reviewed changes

kevinrushforth approved these changes Dec 20, 2024

View reviewed changes

openjdk bot added ready Ready to be integrated and removed csr Need approved CSR to integrate pull request labels Jan 8, 2025

openjdk bot added the integrated Pull request has been integrated label Jan 8, 2025

openjdk bot closed this Jan 8, 2025

openjdk bot removed ready Ready to be integrated rfr Ready for review labels Jan 8, 2025

mstr2 deleted the feature/sealed-paint branch June 1, 2025 22:02

8346227: Seal Paint and Material #1665

8346227: Seal Paint and Material #1665

Uh oh!

Conversation

mstr2 commented Dec 13, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Dec 13, 2024

Uh oh!

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

Uh oh!

Uh oh!

openjdk bot commented Dec 13, 2024

Uh oh!

openjdk bot commented Dec 13, 2024

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

andy-goryachev-oracle Dec 17, 2024

Choose a reason for hiding this comment

Uh oh!

mstr2 Dec 17, 2024

Choose a reason for hiding this comment

Uh oh!

andy-goryachev-oracle Dec 17, 2024

Choose a reason for hiding this comment

Uh oh!

mstr2 Dec 18, 2024

Choose a reason for hiding this comment

Uh oh!

andy-goryachev-oracle Dec 18, 2024

Choose a reason for hiding this comment

Uh oh!

kevinrushforth Dec 18, 2024

Choose a reason for hiding this comment

Uh oh!

kevinrushforth left a comment

Choose a reason for hiding this comment

Uh oh!

kevinrushforth Dec 18, 2024

Choose a reason for hiding this comment

Uh oh!

mstr2 commented Jan 8, 2025

Uh oh!

openjdk bot commented Jan 8, 2025

Uh oh!

openjdk bot commented Jan 8, 2025

Uh oh!

Uh oh!

mstr2 commented Dec 13, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Dec 13, 2024 •

edited

Loading

mlbridge bot commented Dec 13, 2024 •

edited

Loading