On Wed, 30 Jun 2021 22:30:47 GMT, Kevin Rushforth <k...@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
