Re: RFR: 8272870: Add convenience factory methods for border and background [v2]
On Tue, 7 Sep 2021 06:31:18 GMT, Ambarish Rapte wrote:
>> Since the `Background` constructors take a list of `Paint` objects, I think
>> saying a "single" `{@code Paint}` is helpful. I can see how adding "for
>> `BackgroundFill`" (or maybe "as a `BackgroundFill`"?) might make it clearer.
>
> Sound good to me, It would read like `A convenience factory method for
> creating a Background by specifying a single {@code Paint} as a
> BackgroundFill`
Does mentioning `BackgroundFill` in the description contribute something? The
`@implSpec` tag gives that info. In my eyes, if the docs say the background
will use a single `Paint` then I know exactly what I'm going to get regardless
of what it uses in the implementation.
-
PR: https://git.openjdk.java.net/jfx/pull/610
Re: RFR: 8272870: Add convenience factory methods for border and background [v2]
On Fri, 3 Sep 2021 21:08:27 GMT, Kevin Rushforth wrote:
>> modules/javafx.graphics/src/main/java/javafx/scene/layout/Background.java
>> line 366:
>>
>>> 364: */
>>> 365: public static Background fill(Paint fill) {
>>> 366: return new Background(new BackgroundFill(fill, null, null));
>>
>> 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**
>
> 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.
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 `null`s 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.
-
PR: https://git.openjdk.java.net/jfx/pull/610
Re: RFR: 8272870: Add convenience factory methods for border and background [v2]
On Tue, 7 Sep 2021 08:37:31 GMT, Ambarish Rapte wrote: >> Good idea to indicate what happens on `null`, although that might be better >> in the description? > > I think that should be fine too. > How should we describe the @param in that case. > `@param fill Any Paint as BackgroundFill` -> Does this sound good ? I disliked the docs of `BackgroundFill` in that regard. If the type of a parameter `t` is `T`, saying in the docs `@param t any T` is useless. - PR: https://git.openjdk.java.net/jfx/pull/610
Re: RFR: 8272870: Add convenience factory methods for border and background [v2]
On Fri, 3 Sep 2021 21:06:38 GMT, Kevin Rushforth wrote:
>> modules/javafx.graphics/src/main/java/javafx/scene/layout/Background.java
>> line 361:
>>
>>> 359: *
>>> 360: * @implSpec This call is equivalent to {@code new Background(new
>>> BackgroundFill(fill, null, null));}.
>>> 361: * @param fill the fill of the background
>>
>> `@param fill Any Paint. If null, the value Color.TRANSPARENT is used.`
>>
>> Picked from BackgroundFill constructor's doc
>
> Good idea to indicate what happens on `null`, although that might be better
> in the description?
I think that should be fine too.
How should we describe the @param in that case.
`@param fill Any Paint as BackgroundFill` -> Does this sound good ?
-
PR: https://git.openjdk.java.net/jfx/pull/610
Re: RFR: 8272870: Add convenience factory methods for border and background [v2]
On Fri, 3 Sep 2021 21:04:13 GMT, Kevin Rushforth wrote:
>> modules/javafx.graphics/src/main/java/javafx/scene/layout/Background.java
>> line 357:
>>
>>> 355:
>>> 356: /**
>>> 357: * A convenience factory method for creating a background with a
>>> single {@code Paint}. The background will use the
>>
>> Suggestion to rephrase as:
>> `A convenience factory method for creating a Background by specifying only
>> the {@code Paint} for BackgroundFill`
>
> Since the `Background` constructors take a list of `Paint` objects, I think
> saying a "single" `{@code Paint}` is helpful. I can see how adding "for
> `BackgroundFill`" (or maybe "as a `BackgroundFill`"?) might make it clearer.
Sound good to me, It would read like `A convenience factory method for creating
a Background by specifying a single {@code Paint} as a BackgroundFill`
-
PR: https://git.openjdk.java.net/jfx/pull/610
Re: RFR: 8272870: Add convenience factory methods for border and background [v2]
On Fri, 3 Sep 2021 13:23:39 GMT, Marius Hanl wrote:
>> Nir Lisker has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Corrected comment tags
>
> modules/javafx.graphics/src/main/java/javafx/scene/layout/Background.java
> line 366:
>
>> 364: */
>> 365: public static Background fill(Paint fill) {
>> 366: return new Background(new BackgroundFill(fill, null, null));
>
> 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**
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.
-
PR: https://git.openjdk.java.net/jfx/pull/610
Re: RFR: 8272870: Add convenience factory methods for border and background [v2]
On Thu, 2 Sep 2021 12:14:58 GMT, Ambarish Rapte wrote:
>> Nir Lisker has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Corrected comment tags
>
> modules/javafx.graphics/src/main/java/javafx/scene/layout/Background.java
> line 357:
>
>> 355:
>> 356: /**
>> 357: * A convenience factory method for creating a background with a
>> single {@code Paint}. The background will use the
>
> Suggestion to rephrase as:
> `A convenience factory method for creating a Background by specifying only
> the {@code Paint} for BackgroundFill`
Since the `Background` constructors take a list of `Paint` objects, I think
saying a "single" `{@code Paint}` is helpful. I can see how adding "for
`BackgroundFill`" (or maybe "as a `BackgroundFill`"?) might make it clearer.
> modules/javafx.graphics/src/main/java/javafx/scene/layout/Background.java
> line 361:
>
>> 359: *
>> 360: * @implSpec This call is equivalent to {@code new Background(new
>> BackgroundFill(fill, null, null));}.
>> 361: * @param fill the fill of the background
>
> `@param fill Any Paint. If null, the value Color.TRANSPARENT is used.`
>
> Picked from BackgroundFill constructor's doc
Good idea to indicate what happens on `null`, although that might be better in
the description?
-
PR: https://git.openjdk.java.net/jfx/pull/610
Re: RFR: 8272870: Add convenience factory methods for border and background [v2]
On Fri, 27 Aug 2021 17:40:48 GMT, Nir Lisker wrote:
>> Added convenience factory factory methods for Background and Border.
>
> Nir Lisker has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Corrected comment tags
modules/javafx.graphics/src/main/java/javafx/scene/layout/Background.java line
366:
> 364: */
> 365: public static Background fill(Paint fill) {
> 366: return new Background(new BackgroundFill(fill, null, null));
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**
-
PR: https://git.openjdk.java.net/jfx/pull/610
Re: RFR: 8272870: Add convenience factory methods for border and background [v2]
On Fri, 27 Aug 2021 17:40:48 GMT, Nir Lisker wrote:
>> Added convenience factory factory methods for Background and Border.
>
> Nir Lisker has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Corrected comment tags
Provided few suggestions for doc.
modules/javafx.graphics/src/main/java/javafx/scene/layout/Background.java line
357:
> 355:
> 356: /**
> 357: * A convenience factory method for creating a background with a
> single {@code Paint}. The background will use the
Suggestion to rephrase as:
`A convenience factory method for creating a Background by specifying only the
{@code Paint} for BackgroundFill`
modules/javafx.graphics/src/main/java/javafx/scene/layout/Background.java line
361:
> 359: *
> 360: * @implSpec This call is equivalent to {@code new Background(new
> BackgroundFill(fill, null, null));}.
> 361: * @param fill the fill of the background
`@param fill Any Paint. If null, the value Color.TRANSPARENT is used.`
Picked from BackgroundFill constructor's doc
modules/javafx.graphics/src/main/java/javafx/scene/layout/Border.java line 393:
> 391:
> 392: /**
> 393: * A convenience factory method for creating a solid border with a
> single {@code Paint}. The border will use the
Suggestion to rephrase as:
`A convenience factory method for creating a solid Border by specifying only
the {@code Paint} for BorderStroke.`
modules/javafx.graphics/src/main/java/javafx/scene/layout/Border.java line 398:
> 396: * @implSpec
> 397: * This call is equivalent to {@code new Border(new
> BorderStroke(stroke, BorderStrokeStyle.SOLID, null, null));}.
> 398: * @param stroke the stroke of the border
`@param stroke The stroke to use for all sides. If {@code null}, defaults to
{@code Color.BLACK}.`
Picked from BorderStroke doc
-
Changes requested by arapte (Reviewer).
PR: https://git.openjdk.java.net/jfx/pull/610
Re: RFR: 8272870: Add convenience factory methods for border and background [v2]
On Fri, 27 Aug 2021 14:13:01 GMT, Kevin Rushforth wrote:
>> Nir Lisker has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Corrected comment tags
>
> modules/javafx.graphics/src/main/java/javafx/scene/layout/Background.java
> line 364:
>
>> 362: * {@code new Background(new BackgroundFill(fill, null, null));}
>> 363: * @param fill the fill of the background
>> 364: * @return a new background of the given fill
>
> Need to add `@since 18`
`@return a new background of the given fill` -> `@return a new Background with
specified fill`
> modules/javafx.graphics/src/main/java/javafx/scene/layout/Border.java line
> 400:
>
>> 398: * {@code new Border(new BorderStroke(stroke,
>> BorderStrokeStyle.SOLID, null, null));}
>> 399: * @param stroke the stroke of the border
>> 400: * @return a new border of the given stroke
>
> Same comments about the javadoc as above.
`@return a new border of the given stroke` -> `@return a new Border with the
specified stroke`
-
PR: https://git.openjdk.java.net/jfx/pull/610
Re: RFR: 8272870: Add convenience factory methods for border and background [v2]
On Fri, 27 Aug 2021 14:15:06 GMT, Kevin Rushforth wrote: > I presume you will add some unit tests? Yes, when the implementation is settled upon. - PR: https://git.openjdk.java.net/jfx/pull/610
Re: RFR: 8272870: Add convenience factory methods for border and background [v2]
> Added convenience factory factory methods for Background and Border. Nir Lisker has updated the pull request incrementally with one additional commit since the last revision: Corrected comment tags - Changes: - all: https://git.openjdk.java.net/jfx/pull/610/files - new: https://git.openjdk.java.net/jfx/pull/610/files/33ef548a..bb54859e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=610=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=610=00-01 Stats: 8 lines in 2 files changed: 2 ins; 3 del; 3 mod Patch: https://git.openjdk.java.net/jfx/pull/610.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/610/head:pull/610 PR: https://git.openjdk.java.net/jfx/pull/610
