Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8346227: Seal Paint and Material #1665

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

8346227: Seal Paint and Material #1665

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5c4b42c
Seal Paint and Material
mstr2 Dec 13, 2024
f0a39b1
use exhaustive switch pattern
mstr2 Dec 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 6 additions & 17 deletions modules/javafx.graphics/src/main/java/com/sun/javafx/tk/Toolkit.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -594,23 +594,12 @@ private Object getPaint(RadialGradient paint) {
}

public Object getPaint(Paint paint) {
if (paint instanceof Color) {
return createColorPaint((Color) paint);
}

if (paint instanceof LinearGradient) {
return getPaint((LinearGradient) paint);
}

if (paint instanceof RadialGradient) {
return getPaint((RadialGradient) paint);
}

if (paint instanceof ImagePattern) {
return createImagePatternPaint((ImagePattern) paint);
}

return null;
return switch (paint) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()));

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

case Color color -> createColorPaint(color);
case LinearGradient gradient -> getPaint(gradient);
case RadialGradient gradient -> getPaint(gradient);
case ImagePattern pattern -> createImagePatternPaint(pattern);
};
}

protected static final double clampStopOffset(double offset) {
Expand Down
1 change: 0 additions & 1 deletion modules/javafx.graphics/src/main/java/javafx/scene/paint/ImagePattern.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -383,7 +383,6 @@ void acc_removeListener(AbstractNotifyListener platformChangeListener) {
@Override Object acc_getPlatformPaint() {
if (acc_isMutable() || platformPaint == null) {
platformPaint = Toolkit.getToolkit().getPaint(this);
assert platformPaint != null;
}
return platformPaint;
}
Expand Down
2 changes: 1 addition & 1 deletion modules/javafx.graphics/src/main/java/javafx/scene/paint/Material.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
*
* @since JavaFX 8.0
*/
public abstract class Material {
public abstract sealed class Material permits PhongMaterial {

static {
// This is used by classes in different packages to get access to
Expand Down
3 changes: 2 additions & 1 deletion modules/javafx.graphics/src/main/java/javafx/scene/paint/Paint.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@
*
* @since JavaFX 2.0
*/
public abstract class Paint implements Interpolatable<Paint> {
public abstract sealed class Paint implements Interpolatable<Paint>
permits Color, LinearGradient, RadialGradient, ImagePattern {

static {
Toolkit.setPaintAccessor(new Toolkit.PaintAccessor() {
Expand Down
2 changes: 1 addition & 1 deletion modules/javafx.graphics/src/main/java/javafx/scene/paint/PhongMaterial.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -431,7 +431,7 @@
* @see Shape3D
* @since JavaFX 8.0
*/
public class PhongMaterial extends Material {
public non-sealed class PhongMaterial extends Material {

private boolean diffuseColorDirty = true;
private boolean specularColorDirty = true;
Expand Down