RFR: 8223717: javafx printing: Support Specifying Print to File in the API [v5]

[Phil Race]

On Wed, 30 Jun 2021 22:30:47 GMT, Kevin Rushforth <kcr at openjdk.org> wrote:

>> Phil Race has updated the pull request incrementally with three additional commits since the last revision:
>> 
>>  - 8223717: javafx printing: Support Specifying Print to File in the API
>>  - 8223717: javafx printing: Support Specifying Print to File in the API
>>  - 8223717: javafx printing: Support Specifying Print to File in the API
>
> modules/javafx.graphics/src/main/java/javafx/print/JobSettings.java line 481:
> 
>> 479:      * such as Postscript or PDF, and the application intends to distribute
>> 480:      * the result instead of printing it, or for some other reason the
>> 481:      * application does not want physical media (paper) emitted by the printer.
> 
> Very minor: maybe consider combining the first three paragraphs into a single paragraph?

well .. I usually like a short paragraph that succinctly says what it does as the first paragraph

Anyway I've re-read all this and I prefer it as it is.

> modules/javafx.graphics/src/main/java/javafx/print/JobSettings.java line 486:
> 
>> 484:      * equivalent to null, which means output is sent to the printer.
>> 485:      * So in order to reset to print to the printer, clear the value of
>> 486:      * this property by setting it to null or an empty string.
> 
> This doesn't flow as well as it could. I think you only need to mention once that `null` is the same as an empty string, and then you can just say "empty string". Maybe something like this?
> 
> 
> The default value is an empty string, which means output is sent to the printer. So in order to reset
> to print to the printer, clear the value of this property by setting it to an empty string. A value
> of {@code null} is treated as an empty string.

But I don't say it twice. I say
>  The default value is an empty string, which is interpreted as unset, equivalent to null,

and 
>  clear the value of this property by setting it to null or an empty string.

which is somewhat different and makes i t very clear that either will work .. 

So I think this is all fine and flows as intended.

> modules/javafx.graphics/src/main/java/javafx/print/JobSettings.java line 489:
> 
>> 487:      * <p>
>> 488:      * Additionally if the application displays a printer dialog which allows
>> 489:      * the user to specify a file destination including altering an application
> 
> Minor: I think there should be a comma between `destination` and `including`.

yep

> modules/javafx.graphics/src/main/java/javafx/print/JobSettings.java line 491:
> 
>> 489:      * the user to specify a file destination including altering an application
>> 490:      * specified file destination, the value of this property will reflect that
>> 491:      * user-specified choice, including clearing it to re-set to print to
> 
> `reset` is one word (no need to hyphenate).

it is reset elsewhere not sure why a hyphen is here.

> modules/javafx.graphics/src/main/java/javafx/print/JobSettings.java line 500:
> 
>> 498:      * a user writable file, when printing the results are platform-dependent, including
>> 499:      * replacement with a default output file location, printing to the printer instead,
>> 500:      * or a platform printing error.
> 
> This sentence is a bit awkward and hard to parse. Maybe break it into two sentences? Perhaps something like this:
> 
> 
> If the specified name specifies a non-existent path, or does not specify a user writable file, the
> results when printing are platform-dependent. Possible behavior might include replacement with
> a default output file location, printing to the printer instead, or a platform printing error.

ok split it

-------------

PR: https://git.openjdk.java.net/jfx/pull/543