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 2 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
@@ -354,11 +354,12 @@ public Background(final @NamedArg("fills") BackgroundFill[] fills, final @NamedA
}

/**
* A convenience factory method for creating a background with a single {@code Paint}. The background will use the
* default {@code CornerRadii} and the default {@code Insets} of {@link BackgroundFill}.
* A convenience factory method for creating a background with a single {@code Paint}.

This comment has been minimized.

@arapte

arapte Sep 15, 2021
Member Outdated

Minor: typo: background -> Background
Let's use the Background in all places where we refer the class name. Two other places which need similar change are in @param and @return

A similar change is needed for Border.stroke method also: border -> Border

This comment has been minimized.

@arapte

arapte Sep 15, 2021
Member Outdated

Another suggestion for the description, inspired from the constructors of this class:
A convenience factory method to create a new Background by supplying a single {@code Paint}.

I am good with the current statement too. Above one will just be uniform with existing doc. I leave it to your choice.

This comment has been minimized.

@nlisker

nlisker Sep 19, 2021
Author Collaborator Outdated

Above one will just be uniform with existing doc.

In this case it doesn't matter much, but in general I don't like many places of the Background and Border docs as they stand :)
I already looked at them as a starting point when creating the initial commit.

*
* @implSpec This call is equivalent to {@code new Background(new BackgroundFill(fill, null, null));}.
* @param fill the fill of the background
* @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
*/
@@ -390,12 +390,12 @@ public Border(@NamedArg("strokes") BorderStroke[] strokes, @NamedArg("images") B
}

/**
* A convenience factory method for creating a solid border with a single {@code Paint}. The border will use the
* default {@code CornerRadii} and the default {@code BorderWidths} of {@link BorderStroke}.
* A convenience factory method for creating a solid border with a single {@code Paint}.
*
* @implSpec
* This call is equivalent to {@code new Border(new BorderStroke(stroke, BorderStrokeStyle.SOLID, null, null));}.
* @param stroke the stroke of the border
* 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
*/
@@ -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,13 @@ 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);
}

// 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,11 @@ 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);
}
}