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

Conversation

@nlisker
Copy link
Collaborator

@nlisker nlisker commented Aug 24, 2021

Added convenience factory factory methods for Background and Border.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8272870: Add convenience factory methods for Border and Background

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/610/head:pull/610
$ git checkout pull/610

Update a local copy of the PR:
$ git checkout pull/610
$ git pull https://git.openjdk.java.net/jfx pull/610/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 610

View PR using the GUI difftool:
$ git pr show -t 610

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/610.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Aug 24, 2021

👋 Welcome back nlisker! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@nlisker nlisker marked this pull request as ready for review Aug 24, 2021
@openjdk openjdk bot added the rfr label Aug 24, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Aug 24, 2021

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Aug 24, 2021

/reviewers 2
/csr

@openjdk
Copy link

@openjdk openjdk bot commented Aug 24, 2021

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@openjdk openjdk bot added the csr label Aug 24, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Aug 24, 2021

@kevinrushforth has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@nlisker please create a CSR request for issue JDK-8272870. This pull request cannot be integrated until the CSR request is approved.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

The API looks good. I left a couple comments on the javadoc.

I presume you will add some unit tests?

@nlisker
Copy link
Collaborator Author

@nlisker nlisker commented Aug 27, 2021

I presume you will add some unit tests?

Yes, when the implementation is settled upon.

@kevinrushforth kevinrushforth self-requested a review Aug 27, 2021
Copy link
Member

@arapte arapte left a comment

Provided few suggestions for doc.

@@ -353,6 +353,19 @@ public Background(final @NamedArg("fills") BackgroundFill[] fills, final @NamedA
hash = result;
}

/**
* A convenience factory method for creating a background with a single {@code Paint}. The background will use the
Copy link
Member

@arapte arapte Sep 2, 2021

Choose a reason for hiding this comment

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

Suggestion to rephrase as:
A convenience factory method for creating a Background by specifying only the {@code Paint} for BackgroundFill

Copy link
Member

@kevinrushforth kevinrushforth Sep 3, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Member

@arapte arapte Sep 7, 2021

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@nlisker nlisker Sep 9, 2021

Choose a reason for hiding this comment

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

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.

* default {@code CornerRadii} and the default {@code Insets} of {@link BackgroundFill}.
*
* @implSpec This call is equivalent to {@code new Background(new BackgroundFill(fill, null, null));}.
* @param fill the fill of the background
Copy link
Member

@arapte arapte Sep 2, 2021

Choose a reason for hiding this comment

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

@param fill Any Paint. If null, the value Color.TRANSPARENT is used.

Picked from BackgroundFill constructor's doc

Copy link
Member

@kevinrushforth kevinrushforth Sep 3, 2021

Choose a reason for hiding this comment

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

Good idea to indicate what happens on null, although that might be better in the description?

Copy link
Member

@arapte arapte Sep 7, 2021

Choose a reason for hiding this comment

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

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 ?

Copy link
Collaborator Author

@nlisker nlisker Sep 7, 2021

Choose a reason for hiding this comment

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

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.

*
* @implSpec
* This call is equivalent to {@code new Border(new BorderStroke(stroke, BorderStrokeStyle.SOLID, null, null));}.
* @param stroke the stroke of the border
Copy link
Member

@arapte arapte Sep 2, 2021

Choose a reason for hiding this comment

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

@param stroke The stroke to use for all sides. If {@code null}, defaults to {@code Color.BLACK}.

Picked from BorderStroke doc

@@ -389,6 +389,20 @@ public Border(@NamedArg("strokes") BorderStroke[] strokes, @NamedArg("images") B
hash = result;
}

/**
* A convenience factory method for creating a solid border with a single {@code Paint}. The border will use the
Copy link
Member

@arapte arapte Sep 2, 2021

Choose a reason for hiding this comment

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

Suggestion to rephrase as:
A convenience factory method for creating a solid Border by specifying only the {@code Paint} for BorderStroke.

@openjdk openjdk bot removed the rfr label Sep 9, 2021
@openjdk openjdk bot added the rfr label Sep 9, 2021
@nlisker
Copy link
Collaborator Author

@nlisker nlisker commented Sep 9, 2021

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?

Copy link
Member

@arapte arapte left a comment

Provided few minor comments, overall looks good to me.

@@ -353,6 +353,20 @@ public Background(final @NamedArg("fills") BackgroundFill[] fills, final @NamedA
hash = result;
}

/**
* A convenience factory method for creating a background with a single {@code Paint}.
Copy link
Member

@arapte arapte Sep 15, 2021

Choose a reason for hiding this comment

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

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

Copy link
Member

@arapte arapte Sep 15, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@nlisker nlisker Sep 19, 2021

Choose a reason for hiding this comment

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

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.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Sep 15, 2021

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?

@arapte
Copy link
Member

@arapte arapte commented Sep 15, 2021

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.

@Maran23
Copy link
Member

@Maran23 Maran23 commented Sep 16, 2021

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. :)

@nlisker
Copy link
Collaborator Author

@nlisker nlisker commented Sep 16, 2021

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.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Sep 16, 2021

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).

@nlisker
Copy link
Collaborator Author

@nlisker nlisker commented Sep 16, 2021

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.

@Maran23
Copy link
Member

@Maran23 Maran23 commented Sep 19, 2021

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.

@nlisker
Copy link
Collaborator Author

@nlisker nlisker commented Sep 20, 2021

@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.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Looks good. I'll also review the CSR, and then you can Finalize.

arapte
arapte approved these changes Oct 5, 2021
@openjdk openjdk bot removed the csr label Oct 7, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Oct 7, 2021

@nlisker This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8272870: Add convenience factory methods for border and background

Reviewed-by: kcr, arapte

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 26 new commits pushed to the master branch:

  • 2c86e0f: 8274433: All Cells: misbehavior of startEdit
  • 64aa926: 8188026: TextFieldXXCell: NPE on calling startEdit
  • f3c72b9: 8214158: Implement HostServices.showDocument on macOS without calling AWT
  • 30f5606: 8231644: TreeTableView Regression: Indentation wrong using Label as column content type
  • 55faac4: 8271474: Tree-/TableCell: inconsistent edit event firing pattern
  • 478512b: 8274107: Cherry pick GTK WebKit 2.32.4 changes
  • 4b9cb21: 8273969: Memory Leak on the Runnable provided to Platform.startup
  • 338b999: 8273946: Move clearQuad method to BaseShaderGraphics superclass
  • 5c355fa: 8090158: Wrong implementation of adjustValue in scrollBars
  • f59a057: 8090547: Allow for transparent backgrounds in WebView
  • ... and 16 more: https://git.openjdk.java.net/jfx/compare/fdc88341f1df8fb9c99356ada54b25124b77ea6e...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Oct 7, 2021
@nlisker
Copy link
Collaborator Author

@nlisker nlisker commented Oct 7, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Oct 7, 2021

Going to push as commit bb73d43.
Since your change was applied there have been 26 commits pushed to the master branch:

  • 2c86e0f: 8274433: All Cells: misbehavior of startEdit
  • 64aa926: 8188026: TextFieldXXCell: NPE on calling startEdit
  • f3c72b9: 8214158: Implement HostServices.showDocument on macOS without calling AWT
  • 30f5606: 8231644: TreeTableView Regression: Indentation wrong using Label as column content type
  • 55faac4: 8271474: Tree-/TableCell: inconsistent edit event firing pattern
  • 478512b: 8274107: Cherry pick GTK WebKit 2.32.4 changes
  • 4b9cb21: 8273969: Memory Leak on the Runnable provided to Platform.startup
  • 338b999: 8273946: Move clearQuad method to BaseShaderGraphics superclass
  • 5c355fa: 8090158: Wrong implementation of adjustValue in scrollBars
  • f59a057: 8090547: Allow for transparent backgrounds in WebView
  • ... and 16 more: https://git.openjdk.java.net/jfx/compare/fdc88341f1df8fb9c99356ada54b25124b77ea6e...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Oct 7, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Oct 7, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Oct 7, 2021

@nlisker Pushed as commit bb73d43.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@nlisker nlisker deleted the 8272870_Add_convenience_factory_methods_for_Border_and_Background branch Oct 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants