8269638: Property methods, setters, and getters in printing API should be final

[prrace]

• Make various setters and getters and properties final as needed
• Move documentation to the property so the setters and getters inherit it, with an exception for the special case of JobSettings.setPageRanges()
• Override toString() on the properties in JobSettings so it doesn't delegate to the JobSettings class.
• Add a manual test program just so you can see what toString() does. No pass or fail, just informative.

This will need a CSR but I won't create that until the review is done.

---

Progress

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

Issue

• JDK-8269638: Property methods, setters, and getters in printing API should be final

Reviewers

• Kevin Rushforth (@kevinrushforth - Reviewer)
• Prasanta Sadhukhan (@prsadhuk - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 574

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

Using diff file

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

[prrace]

/csr

[bridgekeeper]

👋 Welcome back prr! 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.

[openjdk]

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

[mlbridge]

Webrevs

• 03: Full - Incremental (3da5e4df)
• 02: Full - Incremental (6a84d1d9)
• 01: Full - Incremental (380ad63f)
• 00: Full (4fad1a0d)

[kevinrushforth]

/reviewers 2

[openjdk]

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

[kevinrushforth]

Two quick comments, otherwise looks good (I'll do a more detailed review of the added docs as well we double-check that nothing else was missed).

1. PrinterJob::setPrinter is not final (and probably some / all of the docs could move to the property).
2. There are two new warnings about empty <p> tags:

jfx/modules/javafx.graphics/src/main/java/javafx/print/JobSettings.java:968: warning: empty <p> tag
     * <p>
       ^
jfx/modules/javafx.graphics/src/main/java/javafx/print/JobSettings.java:1049: warning: empty <p> tag
     * <p>
       ^

[prrace]

fixed all the above and uploaded new commit

[kevinrushforth]

Looks good.

[kevinrushforth]

@prsadhuk Can you be the second reviewer?

[prrace]

The CSR has been created : https://bugs.openjdk.java.net/browse/JDK-8270381

[prsadhuk]

ok

[prrace]

uploaded new commit

[prsadhuk]

looks good..
However I have a different question...I was looking at printerProperty and I saw In l182, we are not checking for getDefaultPrinter() returns null or not but in l120, we do...Is it not required in l182?

[kevinrushforth]

looks good..
However I have a different question...I was looking at printerProperty and I saw In l182, we are not checking for getDefaultPrinter() returns null or not but in l120, we do...Is it not required in l182?

It might be a good follow-on bug to skip lines 185-6 if getDefaultPrinter() is null (else it will NPE), but it's completely unrelated to this fix so wouldn't be done as part of this PR.

[openjdk]

@prrace 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:

8269638: Property methods, setters, and getters in printing API should be final

Reviewed-by: kcr, psadhukhan

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 4 new commits pushed to the master branch:

• 948df32: 8268849: Update to 612.1 version of WebKit
• a34928f: 8269968: [REDO] Bump minimum version of macOS for x64 to 10.12
• 00b353e: 8269967: JavaFX should fail fast on macOS below minimum version
• 0c98d96: 8268718: [macos] Video stops, but audio continues to play when stopTime is reached

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

[prrace]

/integrate

[openjdk]

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

• 1d97808: Merge
• 8501416: 8270246: Deprecate for removal implementation methods in Scene
• 948df32: 8268849: Update to 612.1 version of WebKit
• a34928f: 8269968: [REDO] Bump minimum version of macOS for x64 to 10.12
• 00b353e: 8269967: JavaFX should fail fast on macOS below minimum version
• 0c98d96: 8268718: [macos] Video stops, but audio continues to play when stopTime is reached

Your commit was automatically rebased without conflicts.

[openjdk]

@prrace Pushed as commit 8b8cea2.

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

[prrace]

looks good..
However I have a different question...I was looking at printerProperty and I saw In l182, we are not checking for getDefaultPrinter() returns null or not but in l120, we do...Is it not required in l182?

It might be a good follow-on bug to skip lines 185-6 if getDefaultPrinter() is null (else it will NPE), but it's completely unrelated to this fix so wouldn't be done as part of this PR.

I don't think there's a problem here.
Else we'd have had a thousand bug reports by now.
Printer can never be null. Its instaniated by the private PrinterJob constructor.
So perhaps more to the point is are the lines right before that necessary ?

              if (value == null) {
                    value = Printer.getDefaultPrinter();
                }

The public no-args factory method returns null if it can't find a printer and the one
where the app supplies the printer to the factory method that takes one it NPEs out in the internal constuctor
if you pass in the illegal value NULL as the printer.

There's a small issue there as the doc doesn't actually say that but anyone
who has tried it will have found out pretty quickly !

[kevinrushforth]

Printer can never be null. Its instaniated by the private PrinterJob constructor.

If Printer.getDefaultPrinter() can never return null, then you are right that there is no bug here.

So perhaps more to the point is are the lines right before that necessary ?

          if (value == null) {
                value = Printer.getDefaultPrinter();
            }

Yes, this null check is needed, since we specify that setting the printer property to null will use the default printer.

[prrace]

Yes, this null check is needed, since we specify that setting the printer property to null will use the default printer.

oh yeah. So nothing to do here.

The only question is about the behaviour of
public static final PrinterJob createPrinterJob(Printer printer) {}
when called with null.

• Document the NPE
• Don't do anything
• Substitute the default printer and create the job and document this
• return null if there isn't one and document this.

[kevinrushforth]

I don't have a strong opinion on this one. I might lean toward the 3rd option, since it is consistent with calling PrinterJob::setPrinter(null) on an existing PrinterJob. It would also mean that createPrinterJob(null) and createPrinterJob() would be equivalent. Not sure how useful this equivalence would be.