8346227: Seal Paint and Material #1665
Conversation
|
👋 Welcome back mstrauss! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
openjdk
bot
added
rfr
|
@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. |
Webrevs
|
| } | ||
|
|
||
| return null; | ||
| return switch (paint) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
null is not a valid argument for this method, and has never been. acc_getPlatformPaint() previously asserted that the return value is not null.
There was a problem hiding this comment.
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()));
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I disagree: it makes it more resilient by doing what the caller has to do anyway.
However, the answer is acceptable.
There was a problem hiding this comment.
Btw, even if we did decide to change it in the future, we wouldn't do it as part of this PR.
| } | ||
|
|
||
| return null; | ||
| return switch (paint) { |
There was a problem hiding this comment.
Btw, even if we did decide to change it in the future, we wouldn't do it as part of this PR.
The
PaintandMaterialclasses 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
Issues
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1665/head:pull/1665$ git checkout pull/1665Update a local copy of the PR:
$ git checkout pull/1665$ git pull https://git.openjdk.org/jfx.git pull/1665/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1665View PR using the GUI difftool:
$ git pr show -t 1665Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1665.diff
Using Webrev
Link to Webrev Comment