Re: RFR: 8223717: javafx printing: Support Specifying Print to File in the API [v3]
On Fri, 25 Jun 2021 04:08:30 GMT, Prasanta Sadhukhan
wrote:
>> Phil Race has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8223717: javafx printing: Support Specifying Print to File in the API
>
> modules/javafx.graphics/src/main/java/com/sun/prism/j2d/print/J2DPrinterJob.java
> line 352:
>
>> 350: try {
>> 351:
>> settings.setOutputFile(dest.getURI().toURL().toString());
>> 352: } catch (MalformedURLException e) {
>
> I guess in 2D RasterPrinterJob, if there is any exception, we try to form a
> default file "out.prn"
> [https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/sun/print/RasterPrinterJob.java#L1137]
> and try to print there. Can't we do it here too?
I am going to just change to support file so this becomes a non-issue
> modules/javafx.graphics/src/main/java/com/sun/prism/j2d/print/J2DPrinterJob.java
> line 356:
>
>> 354: } else {
>> 355: settings.setOutputFile("");
>> 356: }
>
> Actually in 2D , it seems if dest is null, we redirect printing to actual
> printer instead of file. Not sure if that is what we need to do here too!!
> [https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/sun/print/RasterPrinterJob.java#L1148]
that is what we are doing ..
> modules/javafx.graphics/src/main/java/javafx/print/JobSettings.java line 485:
>
>> 483: *
>> 484: * If the application does not have permission to write to the
>> specified
>> 485: * file, a {@code SecurityException} may be thrown when printing.
>
> As I see in 2D atleast in Win32PrintService, if there is a SecurityException
> for creating a Destination object from a URI, it retries again by creating a
> new URL with default file "file:out.prn"
> Is it not needed here?
> [https://github.com/openjdk/jdk/blob/master/src/java.desktop/windows/classes/sun/print/Win32PrintService.java#L1181]
No. We are doing our own checking if a SM is running and disallow it.
-
PR: https://git.openjdk.java.net/jfx/pull/543
Re: RFR: 8223717: javafx printing: Support Specifying Print to File in the API [v3]
On Fri, 25 Jun 2021 21:13:31 GMT, Kevin Rushforth wrote:
>> Phil Race has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8223717: javafx printing: Support Specifying Print to File in the API
>
> modules/javafx.graphics/src/main/java/javafx/print/JobSettings.java line 492:
>
>> 490: * a writable file it may be ignored, or a SecurityException may be
>> thrown.
>> 491: * The default value is an empty string, which is interpreted as
>> unset,
>> 492: * which means output is sent to the printer.
>
> Can you add a `* @defaultValue empty string` javadoc tag?
will do ..
> tests/manual/printing/PrintToFileTest.java line 134:
>
>> 132: job.printPage(printNode);
>> 133: job.endJob();
>> 134: if (f.exists()) {
>
> I have only tried this on Windows. This check is failing for me when I run
> the test, and yet the file is created correctly. If I add a short sleep, it
> works (so the printing is likely being done on a different thread).
interesting. I guess I will have to add a sleep as you did.
> tests/manual/printing/PrintToFileTest.java line 137:
>
>> 135: System.out.println("created file");
>> 136: passed = true;
>> 137: f.delete();
>
> Maybe leave the file? I found it useful to verify its contents.
ok
-
PR: https://git.openjdk.java.net/jfx/pull/543
Re: RFR: 8223717: javafx printing: Support Specifying Print to File in the API [v3]
On Fri, 25 Jun 2021 21:20:53 GMT, Kevin Rushforth wrote:
>> modules/javafx.graphics/src/main/java/com/sun/prism/j2d/print/J2DPrinterJob.java
>> line 839:
>>
>>> 837: security.checkPrintJobAccess();
>>> 838: String file = settings.getOutputFile();
>>> 839: if (!file.isEmpty()) {
>>
>> Don't we need to check for file!= null?
>
> The default value for the property is the empty string. But it does bring up
> a good point that we should either check and throw NPE if `setOutputFile` is
> called with `null` or we should map null to the empty string.
we do remap null to the empty string .. the code in
JobSettings.outputFileProperty() does it.
So this will never be null
-
PR: https://git.openjdk.java.net/jfx/pull/543
Re: RFR: 8223717: javafx printing: Support Specifying Print to File in the API [v3]
Yes .. we still need to deal with it until it is actually removed. Its going to be here for all the life of JDK 17 LTS which I expect FX will want to support for all that time. -phil. On 6/25/21 6:42 PM, Eric Bresie wrote: security manager That’s not the same security manager being discussed as being deprecated for Java 17 and beyond is it? Eric Bresie ebre...@gmail.com (mailto:ebre...@gmail.com) On June 25, 2021 at 4:20:03 PM CDT, Kevin Rushforth mailto:k...@openjdk.java.net)> wrote: On Thu, 24 Jun 2021 23:00:42 GMT, Phil Race mailto:p...@openjdk.org)> wrote: Overall the new API and functionality looks good, and fills a void that was discussed [in this thread](https://mail.openjdk.java.net/pipermail/openjfx-dev/2019-May/023292.html) a couple years ago. General comments: The first, which is the one we had been discussing, is where the URL is a valid file URL, but the program can't write to it, either because we are running with a security manager and the application doesn't have permission, or because we can't write to the output file (e.g., the path doesn't exist or is read-only). PR: https://git.openjdk.java.net/jfx/pull/543
Re: Re: RFR: 8223717: javafx printing: Support Specifying Print to File in the API [v3]
> security manager That’s not the same security manager being discussed as being deprecated for Java 17 and beyond is it? Eric Bresie ebre...@gmail.com (mailto:ebre...@gmail.com) > On June 25, 2021 at 4:20:03 PM CDT, Kevin Rushforth (mailto:k...@openjdk.java.net)> wrote: > On Thu, 24 Jun 2021 23:00:42 GMT, Phil Race (mailto:p...@openjdk.org)> wrote: > > Overall the new API and functionality looks good, and fills a void that was > discussed [in this > thread](https://mail.openjdk.java.net/pipermail/openjfx-dev/2019-May/023292.html) > a couple years ago. > > General comments: > > The first, which is the one we had been discussing, is where the URL is a > valid file URL, but the program can't write to it, either because we are > running with a security manager and the application doesn't have permission, > or because we can't write to the output file (e.g., the path doesn't exist or > is read-only). > > PR: https://git.openjdk.java.net/jfx/pull/543
Re: RFR: 8223717: javafx printing: Support Specifying Print to File in the API [v3]
On Thu, 24 Jun 2021 23:00:42 GMT, Phil Race wrote:
>> This enhancement adds the String property outputFileProperty() to the
>> JobSettings class.
>> The value should be a string that references a local file encoded as a URL.
>> If this is non-null and set to a location that the user has permission to
>> write to,
>> then the printer output will be spooled there instead of the printer, so
>> long as the platform printing system supports this.
>> The user can of course also set a print-to-file destination in the platform
>> printer dialogs which may over-ride what the application set. But now the
>> application can also see what it was set to, and cancel or alter it if
>> necessary.
>>
>> A simple manual test is provided, manual mainly because the few real
>> printing functional tests are all manual as they are only useful if run with
>> a printer configured.
>
> Phil Race has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8223717: javafx printing: Support Specifying Print to File in the API
I left a comment on the implementation and a couple on the test inline.
tests/manual/printing/PrintToFileTest.java line 134:
> 132: job.printPage(printNode);
> 133: job.endJob();
> 134: if (f.exists()) {
I have only tried this on Windows. This check is failing for me when I run the
test, and yet the file is created correctly. If I add a short sleep, it works
(so the printing is likely being done on a different thread).
tests/manual/printing/PrintToFileTest.java line 137:
> 135: System.out.println("created file");
> 136: passed = true;
> 137: f.delete();
Maybe leave the file? I found it useful to verify its contents.
-
PR: https://git.openjdk.java.net/jfx/pull/543
Re: RFR: 8223717: javafx printing: Support Specifying Print to File in the API [v3]
On Fri, 25 Jun 2021 03:44:56 GMT, Prasanta Sadhukhan
wrote:
>> Phil Race has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8223717: javafx printing: Support Specifying Print to File in the API
>
> modules/javafx.graphics/src/main/java/com/sun/prism/j2d/print/J2DPrinterJob.java
> line 839:
>
>> 837: security.checkPrintJobAccess();
>> 838: String file = settings.getOutputFile();
>> 839: if (!file.isEmpty()) {
>
> Don't we need to check for file!= null?
The default value for the property is the empty string. But it does bring up a
good point that we should either check and throw NPE if `setOutputFile` is
called with `null` or we should map null to the empty string.
-
PR: https://git.openjdk.java.net/jfx/pull/543
Re: RFR: 8223717: javafx printing: Support Specifying Print to File in the API [v3]
On Fri, 25 Jun 2021 07:18:50 GMT, Johan Vos wrote:
>> Consistency with other APIs would be the main reason. We could do something
>> similar to what we do for `Image`, and treat a string without a protocol as
>> a file (although not relative to the classpath as in the case of images),
>> turning it into a `file:` URL internally where needed.
>>
>> @prrace what do you think?
>
> I agree with @kevinrushforth that if a String without protocol is passed, it
> should be treated as a file (absolute or relative to ?)
> I'm also not sure that the URL should be exposed here. I understand it's
> needed in the lower-level print API but you already do the conversion in the
> `syncOutputFile` method. Hence, since only the file protocol is supported, it
> might be easier for API users to pass the location of the file instead of a
> URL.
> In case later other URL protocols are supported, a property `outputURL` might
> be introduced?
I like the flexibility and consistency of defining it as a URL, as long as we
also interpret a url without a scheme as a file name. Borrowing language from
the Image docs, perhaps something like this?
The URL string can either be a URL with a "file:" protocol that can be resolved
by @link java.net.URL} or a file path that can be resolved by {@link
java.io.File}.
-
PR: https://git.openjdk.java.net/jfx/pull/543
Re: RFR: 8223717: javafx printing: Support Specifying Print to File in the API [v3]
On Thu, 24 Jun 2021 23:00:42 GMT, Phil Race wrote: >> This enhancement adds the String property outputFileProperty() to the >> JobSettings class. >> The value should be a string that references a local file encoded as a URL. >> If this is non-null and set to a location that the user has permission to >> write to, >> then the printer output will be spooled there instead of the printer, so >> long as the platform printing system supports this. >> The user can of course also set a print-to-file destination in the platform >> printer dialogs which may over-ride what the application set. But now the >> application can also see what it was set to, and cancel or alter it if >> necessary. >> >> A simple manual test is provided, manual mainly because the few real >> printing functional tests are all manual as they are only useful if run with >> a printer configured. > > Phil Race has updated the pull request incrementally with one additional > commit since the last revision: > > 8223717: javafx printing: Support Specifying Print to File in the API Overall the new API and functionality looks good, and fills a void that was discussed [in this thread](https://mail.openjdk.java.net/pipermail/openjfx-dev/2019-May/023292.html) a couple years ago. General comments: * If the user selects "print to file" from the native dialog, it will write to the `outputFile` property. This should be documented. * On the question of error handling. There are two types of errors we could consider. The first, which is the one we had been discussing, is where the URL is a valid file URL, but the program can't write to it, either because we are running with a security manager and the application doesn't have permission, or because we can't write to the output file (e.g., the path doesn't exist or is read-only). The second is a bad URL. If the URL is malformed or uses an unsupported protocol (anything other than `file:`), we could throw IllegalArgumentException, either when the invalid URL is set or when the print job is run. We might want to do this regardless of what we do about the first type of error. This could be done as simple as adding `@throws IllegalArgumentException if the url uses a protocol other than "file:"`. What do you think? For files that can't be written (the first type of error), we have three choices: 1. Define it loosely such that using a file that cannot be written to will either throw an exception when the print job is run or it will be ignored. 2. Define it to always throw an exception. We would need to add validation logic in PrintJob before calling the implementation methods, but we wouldn't necessarily be able to catch all errors (e.g., an IOEXception that only happens after you start writing to the file). This seems like overkill. 3. Define it to never throw an exception. The implementation would need to wrap the call to print in a try / catch, probably logging the result. I would probably lean towards 1 (which is what you are suggesting), but 3 could work as well. I left a couple inline comments on the API. modules/javafx.graphics/src/main/java/javafx/print/JobSettings.java line 492: > 490: * a writable file it may be ignored, or a SecurityException may be > thrown. > 491: * The default value is an empty string, which is interpreted as > unset, > 492: * which means output is sent to the printer. Can you add a `* @defaultValue empty string` javadoc tag? - PR: https://git.openjdk.java.net/jfx/pull/543
Re: RFR: 8223717: javafx printing: Support Specifying Print to File in the API [v3]
On Thu, 24 Jun 2021 23:00:42 GMT, Phil Race wrote:
>> This enhancement adds the String property outputFileProperty() to the
>> JobSettings class.
>> The value should be a string that references a local file encoded as a URL.
>> If this is non-null and set to a location that the user has permission to
>> write to,
>> then the printer output will be spooled there instead of the printer, so
>> long as the platform printing system supports this.
>> The user can of course also set a print-to-file destination in the platform
>> printer dialogs which may over-ride what the application set. But now the
>> application can also see what it was set to, and cancel or alter it if
>> necessary.
>>
>> A simple manual test is provided, manual mainly because the few real
>> printing functional tests are all manual as they are only useful if run with
>> a printer configured.
>
> Phil Race has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8223717: javafx printing: Support Specifying Print to File in the API
modules/javafx.graphics/src/main/java/com/sun/prism/j2d/print/J2DPrinterJob.java
line 352:
> 350: try {
> 351: settings.setOutputFile(dest.getURI().toURL().toString());
> 352: } catch (MalformedURLException e) {
I guess in 2D RasterPrinterJob, if there is any exception, we try to form a
default file "out.prn"
[https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/sun/print/RasterPrinterJob.java#L1137]
and try to print there. Can't we do it here too?
modules/javafx.graphics/src/main/java/com/sun/prism/j2d/print/J2DPrinterJob.java
line 356:
> 354: } else {
> 355: settings.setOutputFile("");
> 356: }
Actually in 2D , it seems if dest is null, we redirect printing to actual
printer instead of file. Not sure if that is what we need to do here too!!
[https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/sun/print/RasterPrinterJob.java#L1148]
modules/javafx.graphics/src/main/java/com/sun/prism/j2d/print/J2DPrinterJob.java
line 839:
> 837: security.checkPrintJobAccess();
> 838: String file = settings.getOutputFile();
> 839: if (!file.isEmpty()) {
Don't we need to check for file!= null?
modules/javafx.graphics/src/main/java/javafx/print/JobSettings.java line 485:
> 483: *
> 484: * If the application does not have permission to write to the
> specified
> 485: * file, a {@code SecurityException} may be thrown when printing.
As I see in 2D atleast in Win32PrintService, if there is a SecurityException
for creating a Destination object from a URI, it retries again by creating a
new URL with default file "file:out.prn"
Is it not needed here?
[https://github.com/openjdk/jdk/blob/master/src/java.desktop/windows/classes/sun/print/Win32PrintService.java#L1181]
-
PR: https://git.openjdk.java.net/jfx/pull/543
Re: RFR: 8223717: javafx printing: Support Specifying Print to File in the API [v3]
On Thu, 24 Jun 2021 23:19:22 GMT, Kevin Rushforth wrote: >> We aren't ignoring it .. its just handed on, so its going to be the same as >> the other non-writable case .. file could not be created .. printing error >> .. >> we could do some up-front validation if that seems like the way we want to >> go. This could I guess also check if the file is writeable .. but since the >> user can select a non-writable file location in the dialog I don't think we >> should do that part. In which case I wonder about the URL validation too .. > > Consistency with other APIs would be the main reason. We could do something > similar to what we do for `Image`, and treat a string without a protocol as a > file (although not relative to the classpath as in the case of images), > turning it into a `file:` URL internally where needed. > > @prrace what do you think? I agree with @kevinrushforth that if a String without protocol is passed, it should be treated as a file (absolute or relative to ?) I'm also not sure that the URL should be exposed here. I understand it's needed in the lower-level print API but you already do the conversion in the `syncOutputFile` method. Hence, since only the file protocol is supported, it might be easier for API users to pass the location of the file instead of a URL. In case later other URL protocols are supported, a property `outputURL` might be introduced? - PR: https://git.openjdk.java.net/jfx/pull/543
Re: RFR: 8223717: javafx printing: Support Specifying Print to File in the API [v3]
On Thu, 24 Jun 2021 23:03:29 GMT, Phil Race wrote: >> As a potential user of this API I have to ask, why would this be a URL >> instead of a File or Path ? Particularly if only "file:" URLs will be >> valid. I can't think of a scenario where it won't always be an extra >> inconvenience to convert the path I have to a URL. > > We aren't ignoring it .. its just handed on, so its going to be the same as > the other non-writable case .. file could not be created .. printing error .. > we could do some up-front validation if that seems like the way we want to > go. This could I guess also check if the file is writeable .. but since the > user can select a non-writable file location in the dialog I don't think we > should do that part. In which case I wonder about the URL validation too .. Consistency with other APIs would be the main reason. We could do something similar to what we do for `Image`, and treat a string without a protocol as a file (although not relative to the classpath as in the case of images), turning it into a `file:` URL internally where needed. @prrace what do you think? - PR: https://git.openjdk.java.net/jfx/pull/543
Re: RFR: 8223717: javafx printing: Support Specifying Print to File in the API [v3]
On Thu, 24 Jun 2021 23:19:22 GMT, Kevin Rushforth wrote: >> We aren't ignoring it .. its just handed on, so its going to be the same as >> the other non-writable case .. file could not be created .. printing error >> .. >> we could do some up-front validation if that seems like the way we want to >> go. This could I guess also check if the file is writeable .. but since the >> user can select a non-writable file location in the dialog I don't think we >> should do that part. In which case I wonder about the URL validation too .. > > Consistency with other APIs would be the main reason. We could do something > similar to what we do for `Image`, and treat a string without a protocol as a > file (although not relative to the classpath as in the case of images), > turning it into a `file:` URL internally where needed. > > @prrace what do you think? > As a potential user of this API I have to ask, why would this be a URL > instead of a File or Path ? Particularly if only "file:" URLs will be valid. > I can't think of a scenario where it won't always be an extra inconvenience > to convert the path I have to a URL. I will think about it but not promising yet. We only forsee (for now) a file .. We use URL a lot and also the javax.print API in 2D uses URL .. and so 2D hands us a file URL as well as expecting one .. and FX printing depends heavily on that right now so there's no escaping URLs somewhere. - PR: https://git.openjdk.java.net/jfx/pull/543
Re: RFR: 8223717: javafx printing: Support Specifying Print to File in the API [v3]
On Thu, 24 Jun 2021 22:53:57 GMT, Scott Palmer wrote: >> modules/javafx.graphics/src/main/java/javafx/print/JobSettings.java line 474: >> >>> 472: * encoded name of a filesystem file, to which the platform printer >>> 473: * driver should spool the rendered print data. >>> 474: * >> >> Can you add a sentence indicating that the URL needs to be a `file:` URL? >> And indicate what happens if it isn't (I presume it is ignored)? > > As a potential user of this API I have to ask, why would this be a URL > instead of a File or Path ? Particularly if only "file:" URLs will be valid. > I can't think of a scenario where it won't always be an extra inconvenience > to convert the path I have to a URL. We aren't ignoring it .. its just handed on, so its going to be the same as the other non-writable case .. file could not be created .. printing error .. we could do some up-front validation if that seems like the way we want to go. This could I guess also check if the file is writeable .. but since the user can select a non-writable file location in the dialog I don't think we should do that part. In which case I wonder about the URL validation too .. - PR: https://git.openjdk.java.net/jfx/pull/543
Re: RFR: 8223717: javafx printing: Support Specifying Print to File in the API [v3]
> This enhancement adds the String property outputFileProperty() to the > JobSettings class. > The value should be a string that references a local file encoded as a URL. > If this is non-null and set to a location that the user has permission to > write to, > then the printer output will be spooled there instead of the printer, so long > as the platform printing system supports this. > The user can of course also set a print-to-file destination in the platform > printer dialogs which may over-ride what the application set. But now the > application can also see what it was set to, and cancel or alter it if > necessary. > > A simple manual test is provided, manual mainly because the few real printing > functional tests are all manual as they are only useful if run with a printer > configured. Phil Race has updated the pull request incrementally with one additional commit since the last revision: 8223717: javafx printing: Support Specifying Print to File in the API - Changes: - all: https://git.openjdk.java.net/jfx/pull/543/files - new: https://git.openjdk.java.net/jfx/pull/543/files/1c21e10f..8050d5f9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=543=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx=543=01-02 Stats: 4 lines in 1 file changed: 1 ins; 1 del; 2 mod Patch: https://git.openjdk.java.net/jfx/pull/543.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/543/head:pull/543 PR: https://git.openjdk.java.net/jfx/pull/543
Re: RFR: 8223717: javafx printing: Support Specifying Print to File in the API [v3]
On Thu, 24 Jun 2021 22:38:40 GMT, Kevin Rushforth wrote: >> Phil Race has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8223717: javafx printing: Support Specifying Print to File in the API > > modules/javafx.graphics/src/main/java/javafx/print/JobSettings.java line 490: > >> 488: * setting will be ignored. >> 489: * If the URL specifies a non-existent path, or does not specify >> 490: * a writable file it may be ignored, or a SecurityException may be >> thrown. > > I want to look at what we do for other properties like this. I'm not sure how > feasible it is to throw an exception back to the application. And I wouldn't > want to single out a security exception (what about a read-only file > system?). I think that whatever the reason, if the file can't be written to, > we should treat it consistently. Probably by ignoring the request. I presume > that's what will happen if the print driver doesn't support print-to-file? Not easy to specify exactly. We are handing it off to 2D We aren't doing any up-front verification here, but I don't want to copy 2D behaviour into the docs, even if I could be sure about that .. I may just want to be more waffly about what happens. Could be printed to the printer, could be a failed job, could be an exception of some kind if some nice piece of Java code recognises a problem up-front - PR: https://git.openjdk.java.net/jfx/pull/543
Re: RFR: 8223717: javafx printing: Support Specifying Print to File in the API [v3]
On Thu, 24 Jun 2021 22:25:53 GMT, Kevin Rushforth wrote: >> Phil Race has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8223717: javafx printing: Support Specifying Print to File in the API > > modules/javafx.graphics/src/main/java/javafx/print/JobSettings.java line 474: > >> 472: * encoded name of a filesystem file, to which the platform printer >> 473: * driver should spool the rendered print data. >> 474: * > > Can you add a sentence indicating that the URL needs to be a `file:` URL? And > indicate what happens if it isn't (I presume it is ignored)? As a potential user of this API I have to ask, why would this be a URL instead of a File or Path ? Particularly if only "file:" URLs will be valid. I can't think of a scenario where it won't always be an extra inconvenience to convert the path I have to a URL. - PR: https://git.openjdk.java.net/jfx/pull/543
