Re: RFR: 8272870: Add convenience factory methods for border and background [v3]
On Sun, 19 Sep 2021 10:53:02 GMT, Marius Hanl wrote:
>>> > ```
>>> > public static Border stroke(Paint stroke, double width) {
>>> > ```
>>> > ...
>>> > But I really want to hear other opinions. This can also be a follow up. :)
>>>
>>> I don't mind adding this variant, but it needs consensus.
>>
>> I agree that it needs consensus, so the question is how many apps would use
>> it from code (as opposed to CSS), and be satisfied with the other defaults.
>> I don't object to adding a 2nd variant that takes a stroke width as long as
>> we stop there. I don't want variants that take style, or corner radii, or
>> insets, etc. (at that point, just use the existing API).
>
>> > > ```
>> > > public static Border stroke(Paint stroke, double width) {
>> > > ```
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > > ...
>> > > But I really want to hear other opinions. This can also be a follow up.
>> > > :)
>> >
>> >
>> > I don't mind adding this variant, but it needs consensus.
>>
>> I agree that it needs consensus, so the question is how many apps would use
>> it from code (as opposed to CSS), and be satisfied with the other defaults.
>> I don't object to adding a 2nd variant that takes a stroke width as long as
>> we stop there. I don't want variants that take style, or corner radii, or
>> insets, etc. (at that point, just use the existing API).
>
> I agree. For me this would be the last useful variant I often needed in daily
> programming. Everything else should use the normal constructor as it is there
> for very specialized background/border.
@Maran23 I think that you should ask on the mailing list if others want that
variant. I didn't see any discussion on this when I proposed these methods.
-
PR: https://git.openjdk.java.net/jfx/pull/610
Re: RFR: 8272870: Add convenience factory methods for border and background [v3]
On Thu, 16 Sep 2021 21:15:20 GMT, Kevin Rushforth wrote:
> > > ```
> > > public static Border stroke(Paint stroke, double width) {
> > > ```
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > > ...
> > > But I really want to hear other opinions. This can also be a follow up. :)
> >
> >
> > I don't mind adding this variant, but it needs consensus.
>
> I agree that it needs consensus, so the question is how many apps would use
> it from code (as opposed to CSS), and be satisfied with the other defaults. I
> don't object to adding a 2nd variant that takes a stroke width as long as we
> stop there. I don't want variants that take style, or corner radii, or
> insets, etc. (at that point, just use the existing API).
I agree. For me this would be the last useful variant I often needed in daily
programming. Everything else should use the normal constructor as it is there
for very specialized background/border.
-
PR: https://git.openjdk.java.net/jfx/pull/610
Re: RFR: 8272870: Add convenience factory methods for border and background [v3]
On Wed, 15 Sep 2021 11:16:39 GMT, Ambarish Rapte wrote: > 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. - PR: https://git.openjdk.java.net/jfx/pull/610
Re: RFR: 8272870: Add convenience factory methods for border and background [v3]
On Thu, 16 Sep 2021 21:15:20 GMT, Kevin Rushforth wrote: > I agree that it needs consensus, so the question is how many apps would use > it from code (as opposed to CSS), and be satisfied with the other defaults. I > don't object to adding a 2nd variant that takes a stroke width as long as we > stop there. I don't want variants that take style, or corner radii, or > insets, etc. (at that point, just use the existing API). My thoughts as well, though if people care about all sorts of combinations, a builder pattern would be the way in my opinion. - PR: https://git.openjdk.java.net/jfx/pull/610
Re: RFR: 8272870: Add convenience factory methods for border and background [v3]
On Thu, 9 Sep 2021 23:43:29 GMT, Nir Lisker wrote:
>> Added convenience factory factory methods for Background and Border.
>
> Nir Lisker has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Removed whitespaces
> - Added tests and doc updates
> > ```
> > public static Border stroke(Paint stroke, double width) {
> > ```
> > ...
> > But I really want to hear other opinions. This can also be a follow up. :)
>
> I don't mind adding this variant, but it needs consensus.
I agree that it needs consensus, so the question is how many apps would use it
from code (as opposed to CSS), and be satisfied with the other defaults. I
don't object to adding a 2nd variant that takes a stroke width as long as we
stop there. I don't want variants that take style, or corner radii, or insets,
etc. (at that point, just use the existing API).
-
PR: https://git.openjdk.java.net/jfx/pull/610
Re: RFR: 8272870: Add convenience factory methods for border and background [v3]
On Thu, 16 Sep 2021 08:36:49 GMT, Marius Hanl wrote:
> One comment from my side: I would find it quite useful if we have another
> border factory method with a double as the second parameter which let us
> specify the border width. So e.g.
>
> ```
> public static Border stroke(Paint stroke, double width) {
> return new Border(new BorderStroke(stroke, BorderStrokeStyle.SOLID, null,
> new BorderWidths(width)));
> }
> ```
>
> But I really want to hear other opinions. This can also be a follow up. :)
I don't mind adding this variant, but it needs consensus.
-
PR: https://git.openjdk.java.net/jfx/pull/610
Re: RFR: 8272870: Add convenience factory methods for border and background [v3]
On Thu, 9 Sep 2021 23:43:29 GMT, Nir Lisker wrote:
>> Added convenience factory factory methods for Background and Border.
>
> Nir Lisker has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Removed whitespaces
> - Added tests and doc updates
One comment from my side: I would find it quite useful if we have another
border factory method with a double as the second parameter which let us
specify the border width. So e.g.
public static Border stroke(Paint stroke, double width) {
return new Border(new BorderStroke(stroke, BorderStrokeStyle.SOLID, null,
new BorderWidths(width)));
}
But I really want to hear other opinions. This can also be a follow up. :)
-
PR: https://git.openjdk.java.net/jfx/pull/610
Re: RFR: 8272870: Add convenience factory methods for border and background [v3]
On Wed, 15 Sep 2021 13:04:52 GMT, Kevin Rushforth wrote: > another possibility is to just capitalize the first occurrence and leave the > others as lower-case? That sounds good too, so only the first occurrence would be changed. - PR: https://git.openjdk.java.net/jfx/pull/610
Re: RFR: 8272870: Add convenience factory methods for border and background [v3]
On Wed, 15 Sep 2021 11:08:04 GMT, Ambarish Rapte wrote:
>> Nir Lisker has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Removed whitespaces
>> - Added tests and doc updates
>
> modules/javafx.graphics/src/test/java/test/javafx/scene/layout/BackgroundTest.java
> line 722:
>
>> 720: @Test
>> 721: public void testSingleFill() {
>> 722: var background1 = Background.fill(Color.BEIGE);
>
> I think we should have a test for null argument too.
> Similar for the test BorderTest.testSingleStroke()
Yes, I wanted to add these but forgot.
-
PR: https://git.openjdk.java.net/jfx/pull/610
Re: RFR: 8272870: Add convenience factory methods for border and background [v3]
On Thu, 9 Sep 2021 23:43:29 GMT, Nir Lisker wrote:
>> Added convenience factory factory methods for Background and Border.
>
> Nir Lisker has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Removed whitespaces
> - Added tests and doc updates
> Let's use the `Background` in all places where we refer the class name.
As a clarifying point, if it is capitalized, indicating an object of that type,
it should use `{@code ... }`. I don't think all uses of `background` need to be
`{@code Background}` (although that would be OK), so another possibility is to
just capitalize the first occurrence and leave the others as lower-case?
-
PR: https://git.openjdk.java.net/jfx/pull/610
Re: RFR: 8272870: Add convenience factory methods for border and background [v3]
On Wed, 15 Sep 2021 11:03:20 GMT, Ambarish Rapte wrote:
>> Nir Lisker has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Removed whitespaces
>> - Added tests and doc updates
>
> 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}.
>
> 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
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.
-
PR: https://git.openjdk.java.net/jfx/pull/610
Re: RFR: 8272870: Add convenience factory methods for border and background [v3]
On Thu, 9 Sep 2021 23:43:29 GMT, Nir Lisker wrote:
>> Added convenience factory factory methods for Background and Border.
>
> Nir Lisker has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Removed whitespaces
> - Added tests and doc updates
Provided few minor comments, overall looks good to me.
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}.
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
modules/javafx.graphics/src/test/java/test/javafx/scene/layout/BackgroundTest.java
line 722:
> 720: @Test
> 721: public void testSingleFill() {
> 722: var background1 = Background.fill(Color.BEIGE);
I think we should have a test for null argument too.
Similar for the test BorderTest.testSingleStroke()
-
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 [v3]
On Thu, 9 Sep 2021 23:43:29 GMT, Nir Lisker wrote: >> Added convenience factory factory methods for Background and Border. > > Nir Lisker has updated the pull request incrementally with two additional > commits since the last revision: > > - Removed whitespaces > - Added tests and doc updates Added a couple of simple tests. My approach to the documentation here is that these are the simplest ways to create a border (background) where you provide only 1 color/gradient and not care about the rest, and what you are going to get is rather obvious. If you want to see what the implementation does and what all the defaults are, the `@implSpec` section has a link that you can follow. I prefer not to start giving these details since they end up taking more attention than those few important details of what the method does. When I tried to expand upon the behavior of the method it quickly became "noise", with details I didn't care for. If people insist I will add them, but think about: is what you get not what you expected to get with the current docs? What surprised you or what were you not sure about? - PR: https://git.openjdk.java.net/jfx/pull/610
Re: RFR: 8272870: Add convenience factory methods for border and background [v3]
> Added convenience factory factory methods for Background and Border. Nir Lisker has updated the pull request incrementally with two additional commits since the last revision: - Removed whitespaces - Added tests and doc updates - Changes: - all: https://git.openjdk.java.net/jfx/pull/610/files - new: https://git.openjdk.java.net/jfx/pull/610/files/bb54859e..345d759f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=610=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx=610=01-02 Stats: 28 lines in 4 files changed: 20 ins; 2 del; 6 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
