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.

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

8272870: Add convenience factory methods for border and background #610

Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -353,6 +353,20 @@ public Background(final @NamedArg("fills") BackgroundFill[] fills, final @NamedA
hash = result;
}

/**
* A convenience factory method for creating a {@code Background} with a single {@code Paint}.
*
* @implSpec
* This call is equivalent to {@link BackgroundFill#BackgroundFill(Paint, CornerRadii, Insets)
* new Background(new BackgroundFill(fill, null, null));}.
* @param fill the fill of the background. If {@code null}, {@code Color.TRANSPARENT} will be used.
* @return a new background of the given fill
This conversation was marked as resolved by nlisker

This comment has been minimized.

@kevinrushforth

kevinrushforth Aug 27, 2021
Member

Need to add @since 18

This comment has been minimized.

@arapte

arapte Sep 2, 2021
Member

@return a new background of the given fill -> @return a new Background with specified fill

* @since 18
*/
public static Background fill(Paint fill) {
return new Background(new BackgroundFill(fill, null, null));
This conversation was marked as resolved by nlisker

This comment has been minimized.

@Maran23

Maran23 Sep 3, 2021
Member

null CornerRaddii and null Insets will use CornerRadii.EMPTY and Insets.EMPTY. Maybe we should use those here instead so it's more clear for anyone having a look in the source code? I also always use those instead of null.
Same for BorderStroke

This comment has been minimized.

@kevinrushforth

kevinrushforth Sep 3, 2021
Member

Are you talking about the implementation or the code? I guess both, since the @implSepc indicates what this call is equivalent to. I don't have a strong opinion on this one.

This comment has been minimized.

@nlisker

nlisker Sep 9, 2021
Author Collaborator

You don't need to look at the source code, I can link to the delegated constructor and the docs will show those. I prefer using nulls because the point of these is that you want a border/background with 1 color and you don't care about things like widths, insets and corners - they default to whatever the default is. If you care what they are, this method is probably not what you're looking for. It's also shorter and less to read through when the extra info is defaults.

}

/**
* Gets whether the fill of this Background is based on percentages (that is, relative to the
* size of the region being styled). Specifically, this returns true if any of the CornerRadii
@@ -389,6 +389,20 @@ public Border(@NamedArg("strokes") BorderStroke[] strokes, @NamedArg("images") B
hash = result;
}

/**
* A convenience factory method for creating a solid {@code Border} with a single {@code Paint}.
*
* @implSpec
* This call is equivalent to {@link BorderStroke#BorderStroke(Paint, BorderStrokeStyle, CornerRadii, BorderWidths)
* new Border(new BorderStroke(stroke, BorderStrokeStyle.SOLID, null, null));}.
* @param stroke the stroke of the border (for all sides). If {@code null}, {@code Color.BLACK} will be used.
* @return a new border of the given stroke
This conversation was marked as resolved by nlisker

This comment has been minimized.

@kevinrushforth

kevinrushforth Aug 27, 2021
Member

Same comments about the javadoc as above.

This comment has been minimized.

@arapte

arapte Sep 2, 2021
Member

@return a new border of the given stroke -> @return a new Border with the specified stroke

* @since 18
*/
public static Border stroke(Paint stroke) {
return new Border(new BorderStroke(stroke, BorderStrokeStyle.SOLID, null, null));
}

/**
* {@inheritDoc}
*/
@@ -33,6 +33,9 @@
import javafx.scene.layout.BackgroundImage;
import javafx.scene.layout.BackgroundPosition;
import javafx.scene.layout.BackgroundShim;
import javafx.scene.layout.Border;
import javafx.scene.layout.BorderStroke;
import javafx.scene.layout.BorderStrokeStyle;
import javafx.scene.layout.CornerRadii;
import javafx.scene.paint.Color;
import org.junit.Test;
@@ -714,6 +717,20 @@ public void imagesIsUnmodifiable() {
assertFalse(b.isFillPercentageBased());
}

@Test
public void testSingleFill() {
var background1 = Background.fill(Color.BEIGE);
This conversation was marked as resolved by nlisker

This comment has been minimized.

@arapte

arapte Sep 15, 2021
Member

I think we should have a test for null argument too.
Similar for the test BorderTest.testSingleStroke()

This comment has been minimized.

@nlisker

nlisker Sep 15, 2021
Author Collaborator

Yes, I wanted to add these but forgot.

var background2 = new Background(new BackgroundFill(Color.BEIGE, null, null));
assertEquals("The factory method should give the same result as the constructor", background1, background2);
}

@Test
public void testSingleFillWithNullPaint() {
var background1 = Background.fill(null);
var background2 = new Background(new BackgroundFill(null, null, null));
assertEquals("The factory method should give the same result as the constructor", background1, background2);
}

// TODO: What happens if the corner radii become so big that we would end up with a negative opaque
// inset in one dimension?

@@ -561,4 +561,18 @@ public void imagesIsUnmodifiable() {
Border a = new Border((BorderStroke[])null, null);
assertFalse(a.equals("Some random String"));
}

@Test
public void testSingleStroke() {
var border1 = Border.stroke(Color.BEIGE);
var border2 = new Border(new BorderStroke(Color.BEIGE, BorderStrokeStyle.SOLID, null, null));
assertEquals("The factory method should give the same result as the constructor", border2, border1);
}

@Test
public void testSingleStrokeWithNullPaint() {
var border1 = Border.stroke(null);
var border2 = new Border(new BorderStroke(null, BorderStrokeStyle.SOLID, null, null));
assertEquals("The factory method should give the same result as the constructor", border2, border1);
}
}